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 Apr 9, 2020

Reland #16935
Fix to make sure the merge_threads defaults as false.
Also fixed a DCheck.
Will mark the diffs in inline comments

@auto-assign auto-assign bot requested a review from GaryQian April 9, 2020 18:47
Copy link
Contributor Author

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Below are changes made during this re-land

// Any UIKit related code has to run on main thread.
// When on a non-main thread, we only allow the rest of the method to run if there is no
// Pending UIView operations.
FML_DCHECK([[NSThread currentThread] isMainThread] || !HasPendingViewOperations());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated to use !HasPendingViewOperations() which I think is more accurate in this situation.

}

bool FlutterPlatformViewsController::HasPendingViewOperations() {
if (!views_to_dispose_.empty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consider views_to_dispose also as pending view operation

// Default to `false`.
// If `true`, gpu thread and platform thread should be merged during |EndFrame|.
// Always resets to `false` right after the threads are merged.
bool merge_threads_ = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to defaults to false.

Copy link

Choose a reason for hiding this comment

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

👍

@cyanglaz cyanglaz requested a review from blasten April 9, 2020 18:52
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!

@cyanglaz cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 10, 2020
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 10, 2020
erge branch 'master' into reland_platform_view
@cyanglaz cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Apr 10, 2020
@fluttergithubbot fluttergithubbot merged commit 68fd833 into flutter:master Apr 10, 2020
@cyanglaz cyanglaz deleted the reland_platform_view branch April 10, 2020 20:29
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 10, 2020
@chinmaygarde
Copy link
Member

This commit breaks rendering on iOS with both OpenGL and Metal rendering backends due to the missing transactions on the raster thread. These are now only added when on the main thread. However both CAMetalLayer and CAEAGLLayer set their presentsWithTransaction: property to be true. Caused the regression in flutter/flutter#55784.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants