Implement unobstructed Platform Views on iOS by blasten · Pull Request #17049 · flutter/engine

blasten · 2020-03-10T02:31:41Z

For context about how this is used, refer to https://flutter.dev/go/unobstructed-platform-views

The scenario app has been updated to test the size of the UIViews created by this implementation.

gaaclarke

I didn't review the tests yet. I did just a code review, not a design review. It's a bit big to grok in one reading.

edit: there is quite a few instances in this PR were we are indexing into a collection and there seems like there might be a possibility for out-of-bounds errors. Might be worth being a bit more defensive with those calls.

gaaclarke · 2020-03-17T23:13:48Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

+
+      layer = std::make_shared<FlutterPlatformViewLayer>(
+          std::move(overlay_view), std::move(ios_surface), std::move(surface));
+    } else {


nit: there is a bit of copy-n-paste between these two branches, could be split out to a function.

This is existing code. I just moved it to this method. I can definitely refactor this in a different PR.

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

cyanglaz

LGTM modulo nits. We should probably defer the decision making on the external_view_embedder->FinishFrame(); thingy to @amirh or @chinmaygarde

cyanglaz · 2020-03-19T17:22:27Z

shell/common/rasterizer.cc

-    frame->Submit();
    if (external_view_embedder != nullptr) {
-      external_view_embedder->SubmitFrame(surface_->GetContext());
+      external_view_embedder->SubmitFrame(surface_->GetContext(),


I'm OK with this only considering iOS. I'm not sure what the best approach would be considering external embedder. Probably need more inputs from @amirh or @chinmaygarde

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

gaaclarke

Thanks for responding to last review, mostly looks good. I gave this PR a deeper reading this time and read through the tests and I'm feeling good about it. There are still a few nits. One thing I was thinking, don't you think golden tests would be good for this? I don't think they should be part of the PR but maybe we can throw down an issue to add golden tests.

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.mm

shell/platform/darwin/ios/ios_surface_metal.mm

gaaclarke · 2020-03-20T17:13:22Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

+    sk_sp<RTree> rtree = platform_view_rtrees_[platform_view_id];
+    sk_sp<SkPicture> picture = picture_recorders_[platform_view_id]->finishRecordingAsPicture();


This is one of the places where there were unchecked indexing happening. How are you confident that this won't be a problem? Seems like we should at the minimum have a FML_DCHECK here.

Every entry in composition_order_ always has an entry in picture_recorders_ and platform_view_rtrees_. This is currently tested in the scenario app.

blasten · 2020-03-20T18:32:01Z

@gaaclarke The scenario app has a few golden tests, so that cover the compositor. The ones I added are checking for the exact size of the overlay UIViews.

gaaclarke

+1 from me

blasten · 2020-03-20T19:38:49Z

I'm going to go ahead and merge this PR. @chinmaygarde @amirh Feel free to make any comments and I will address them.

This reverts commit 2627634.

…17049)

clarkezone · 2020-03-29T16:12:02Z

I'm trying to understand this change better and notice that http://go/flutter-hybrid-composition is not publicly accessible.. any chance it can be shared so the community outside google can see?

blasten · 2020-04-02T16:43:18Z

@clarkezone I updated the link in the description.

cyanglaz · 2020-05-05T18:12:34Z

shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm

-    return did_submit;
-  }
-  DetachUnusedLayers();
-  active_composition_order_.clear();


I realized this line was removed in this PR. Which caused active_composition_order_ never be cleaned up.
@blasten Let's fix this.

auto-assign bot requested a review from iskakaushik March 10, 2020 02:31

googlebot added cla: yes labels Mar 10, 2020

blasten mentioned this pull request Mar 10, 2020

Add Flutter RTree #16923

Merged

blasten removed the request for review from iskakaushik March 10, 2020 02:32

blasten added the Work in progress (WIP) Not ready (yet) for review! label Mar 10, 2020

chinmaygarde self-requested a review March 10, 2020 05:02

Emmanuel Garcia added 23 commits March 10, 2020 16:44

Implement unobstructed Platform Views

ad66694

Comments

1fde093

Format gn

9b747a3

Remove log

e8cfa5d

Add FlutterRTree unit test

ae64ac3

Remove blue UIView

e722620

Fix header

bce3c8a

Update licenses_flutter

b1975e1

Update unit tests

4318306

test (delete)

84c3b0f

Fix platform view scenarios tests

3635a70

typo

39f0bad

Update golden platform view

62d8d8d

Fix FlutterRTree bug

11242e9

Clip platform view in the background canvas

fcaa142

Add GetPlatformViewRect to EmbedderExternalViewEmbedder

2b27ed6

Remove matrix check

b7c2bc6

Return empty skrect

71ba25c

Test

7002929

Fix tests

9f7f91c

Update rtree unit test

4cb007a

Minor fixes

326f339

Revert unintended change

6d283a9

blasten requested a review from amirh March 14, 2020 01:33

Missing method

165d9ea

blasten requested a review from gaaclarke March 17, 2020 22:30

gaaclarke reviewed Mar 18, 2020

View reviewed changes

Feedback

41b8ac4

blasten requested a review from gaaclarke March 19, 2020 00:20

Merge remote-tracking branch 'upstream/master' into unobstructed_pv

8488382

cyanglaz approved these changes Mar 19, 2020

View reviewed changes

gaaclarke reviewed Mar 20, 2020

View reviewed changes

Feedback

32f433a

gaaclarke approved these changes Mar 20, 2020

View reviewed changes

blasten merged commit 2627634 into flutter:master Mar 20, 2020

blasten deleted the unobstructed_pv branch March 20, 2020 19:39

blasten pushed a commit that referenced this pull request Mar 20, 2020

Revert "Implement unobstructed Platform Views on iOS (#17049)"

4ae4e67

This reverts commit 2627634.

blasten mentioned this pull request Mar 20, 2020

Revert "Implement unobstructed Platform Views on iOS" #17233

Merged

blasten pushed a commit that referenced this pull request Mar 20, 2020

Revert "Implement unobstructed Platform Views on iOS (#17049)" (#17233)

70f6d18

This reverts commit 2627634.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 20, 2020

2627634 Implement unobstructed Platform Views on iOS (flutter/engine#…

2b423fd

…17049)

engine-flutter-autoroll mentioned this pull request Mar 20, 2020

Roll engine fdf4f7883f67..70f6d18bfa44 (4 commits) flutter/flutter#52985

Closed

blasten mentioned this pull request Mar 20, 2020

Reland: Implement unobstructed Platform Views on iOS #17237

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2020

2627634 Implement unobstructed Platform Views on iOS (flutter/engine#…

577eb9c

…17049)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 24, 2020

2627634 Implement unobstructed Platform Views on iOS (flutter/engine#…

75f1f74

…17049)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2020

2627634 Implement unobstructed Platform Views on iOS (flutter/engine#…

13d14e6

…17049)

blasten mentioned this pull request Mar 26, 2020

Reland unobstructed platform views #17336

Merged

cyanglaz reviewed May 5, 2020

View reviewed changes

cyanglaz mentioned this pull request May 5, 2020

[iOS platform view] fix active_composition_order_ never cleared #18153

Merged

jmagman mentioned this pull request Oct 11, 2022

[Impeller] testOneOverlayAndTwoIntersectingOverlays scenario test fails with impeller turned on, does not show platform view overlays flutter/flutter#113251

Closed

		sk_sp<RTree> rtree = platform_view_rtrees_[platform_view_id];
		sk_sp<SkPicture> picture = picture_recorders_[platform_view_id]->finishRecordingAsPicture();

Conversation

blasten commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

blasten Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

cyanglaz Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gaaclarke Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

blasten Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

blasten commented Mar 20, 2020

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

blasten commented Mar 20, 2020

Uh oh!

clarkezone commented Mar 29, 2020

Uh oh!

blasten commented Apr 2, 2020

Uh oh!

cyanglaz May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

blasten commented Mar 10, 2020 •

edited

Loading

gaaclarke left a comment •

edited

Loading

gaaclarke left a comment •

edited

Loading