Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented May 8, 2020

The unobstructed platform view has been implemented. So there is a chance the overlay UIView can change even there is no changes in the platform view itself. Operations in overlay also needs to be run on the platform thread.
Because the current implementation depends on the "paint" traversal to know if an overlay is going to change; and we need to merge the thread before the "paint" traversal. There is no clean way to un-merged the threads when there are platform views.
This PR keeps the threads merged until the platform views are move out of the scene completely.
issue:
flutter/flutter#56474

@cyanglaz cyanglaz requested a review from amirh May 8, 2020 23:24
@auto-assign auto-assign bot requested a review from gw280 May 8, 2020 23:24
@cyanglaz cyanglaz requested review from iskakaushik and blasten and removed request for gw280 May 8, 2020 23:24
// Returns true if there are embedded in the scene at current frame
// Or there will be embedded in the next frame.
// TODO(cyanglaz): https://github.com/flutter/flutter/issues/56474
// Remove this method is the issue above is resloved.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this method when the issue above is resolved?.

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cyanglaz
Copy link
Contributor Author

The Mac Android AOT Engine test was successful here https://chromium-swarm.appspot.com/task?id=4c232e09fb2fd410# in a manually triggered re-run.
Will land this PR even with this task appears red on github.

@cyanglaz cyanglaz merged commit 006dbfc into flutter:master May 13, 2020
@cyanglaz cyanglaz deleted the no_platform_view branch May 13, 2020 15:53
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 13, 2020
iskakaushik pushed a commit to flutter/flutter that referenced this pull request May 14, 2020
* 7b64067 Use 'message' as the parameter name in FlMessageCodec::encode_message (flutter/engine#18253)

* 429beae Roll src/third_party/skia 3d2c41b773f6..3ebadcc98eab (14 commits) (flutter/engine#18333)

* 66ba3a7 Roll src/third_party/dart 2bf325900586..d6fed1f62444 (1 commits) (flutter/engine#18334)

* 2f8495a Completely disable paving the device on Fuchsia (flutter/engine#18340)

* 006dbfc Always keep thread merged when there are platform views. (flutter/engine#18245)

* 21b4d2f [web] Fix paragraph positioning (flutter/engine#18329)

* efdc099 Re-enable Fuchsia tests (flutter/engine#18342)

* ae2222f Revert "Re-enable Fuchsia tests (#18342)" (flutter/engine#18345)
@xster
Copy link
Member

xster commented May 20, 2020

(combing through recent untested PRs)
could we also add a FlutterPlatformViewsTest.m for this?

@cyanglaz
Copy link
Contributor Author

AFAIK, we don't have a way to unit test this with current infra setup. (Maybe my knowledge is outdated) As far as integration tests, we probably can't test this behavior specifically. I will make sure to add an e2e test for the whole thread merging process before actually enabling the thread merging.

wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020
Copy link

@Afrogoddess31 Afrogoddess31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants