[macOS] Consolidate view management #52254
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| // The controller might be nil, because the engine only stores a weak ref, and | ||
| // this method might have been called from the controller's dealloc. |
There was a problem hiding this comment.
Consider something like:
| // The controller might be nil, because the engine only stores a weak ref, and | |
| // this method might have been called from the controller's dealloc. | |
| // The controller can be nil. The engine stores only a weak ref, this method could have been called from the controller's dealloc. |
I don't feel strongly, feel free to keep as is
| [controller setUpWithEngine:self | ||
| viewIdentifier:viewIdentifier | ||
| threadSynchronizer:_threadSynchronizer]; | ||
| NSAssert(controller.viewIdentifier == viewIdentifier, @"Failed to assign view ID."); | ||
| [_viewControllers setObject:controller forKey:@(viewIdentifier)]; | ||
| NSAssert(controller.attached && controller.engine == self, |
There was a problem hiding this comment.
Should we separate the "attached to an unexpected engine" to a separate assertion? Currently, the assertion message you'll get for this scenario would be confusing.
There was a problem hiding this comment.
I combined them initially because the "suggestion" part is too long. I now moved the "suggestion" part to in-code comment and only left the symptom to the assertion. Let me know if they still feel off.
| [controller setUpWithEngine:self | ||
| viewIdentifier:viewIdentifier | ||
| threadSynchronizer:_threadSynchronizer]; | ||
| NSAssert(controller.viewIdentifier == viewIdentifier, @"Failed to assign view ID."); | ||
| [_viewControllers setObject:controller forKey:@(viewIdentifier)]; | ||
| NSAssert(controller.attached && controller.engine == self, | ||
| @"The FlutterViewController unexpectedly stays unattached after being added. " |
There was a problem hiding this comment.
This assertion message makes sense when you're reading this code, but it might be a little confusing if you see this in the command line without any other context.
Perhaps consider expanding the message to explain this is attaching/adding to the engine. Consider something like:
The FlutterViewController should switch to the attached mode after it is added to a FlutterEngine.
Same comment for the other similar assertions
…147095) flutter/engine@55670b7...3db86b3 2024-04-19 skia-flutter-autoroll@skia.org Roll Dart SDK from 2ff00e4eb759 to d882ce089110 (1 revision) (flutter/engine#52272) 2024-04-19 dkwingsmt@users.noreply.github.com [macOS] Consolidate view management (flutter/engine#52254) 2024-04-19 skia-flutter-autoroll@skia.org Roll Skia from f4a214ecf197 to 1910252ea26a (3 revisions) (flutter/engine#52269) 2024-04-19 jonahwilliams@google.com [Impeller] add non-emulated advanced blend support to experimental canvas (flutter/engine#52245) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#147095) flutter/engine@55670b7...3db86b3 2024-04-19 skia-flutter-autoroll@skia.org Roll Dart SDK from 2ff00e4eb759 to d882ce089110 (1 revision) (flutter/engine#52272) 2024-04-19 dkwingsmt@users.noreply.github.com [macOS] Consolidate view management (flutter/engine#52254) 2024-04-19 skia-flutter-autoroll@skia.org Roll Skia from f4a214ecf197 to 1910252ea26a (3 revisions) (flutter/engine#52269) 2024-04-19 jonahwilliams@google.com [Impeller] add non-emulated advanced blend support to experimental canvas (flutter/engine#52245) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This PR improves view management logic of the macOS
FlutterEngineclass.registerViewController:andderegisterViewControllerForIdentifier:.addViewControllernow directly calls.viewController =on implicit views, so that it matches its verbatim description.addViewControllercorrectly reflects the fact that it doesn't support multiple views yet.Additionally, a useless (for now) member variable is removed.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.