[macOS] Multiview compositor #52253
[macOS] Multiview compositor #52253
Conversation
|
Also cc @loic-sharma |
loic-sharma
left a comment
There was a problem hiding this comment.
This looks good to me, but please get approval from someone familiar with this area
shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm
Outdated
Show resolved
Hide resolved
| return [self viewControllerForIdentifier:kFlutterImplicitViewId]; | ||
| } | ||
|
|
||
| - (FlutterCompositor*)createFlutterCompositor { |
There was a problem hiding this comment.
Is this still the right name for the method? "Create" usually implies allocation of some sort. Here what we're doing is more like initialisation the existing field and returning a pointer to it.
There was a problem hiding this comment.
I think it's still a reasonable name. For whoever uses FlutterEngine, this method creates a embedderAPI/FlutterCompositor object and returns it. How the object is memory-managed is implementation detail.
| FlutterTimeConverter* time_converter, | ||
| FlutterPlatformViewController* platform_views_controller); | ||
| FlutterPlatformViewController* platform_views_controller, | ||
| dispatch_queue_t main_queue); |
There was a problem hiding this comment.
Is there ever a case where this isn't the main queue returned by dispatch_get_main_queue()? If not, I think we should just test dispatch_get_current_queue() == dispatch_get_main_queue() where necessary.
There was a problem hiding this comment.
I'm mostly thinking about testability, although there really aren't many tests for FlutterCompositor. However, the ThreadSynchronizer accepts the queue as a replaceable parameter (which was added when I wrote tests for it). I think it's a clearer design if the same queue is passed in for both classes in production code.
There was a problem hiding this comment.
Ok, ThreadSynchronizer has two initializers, one of which uses the main queue implicitly, which is used in production code. Still, its internal implementation allows a replaceable queue. I'm ok with any suggestion.
cbracken
left a comment
There was a problem hiding this comment.
Mostly lgtm - main question is why we need to pass in the dispatch queue.
flutter/engine@1c23c8f...6534fbf 2024-07-10 58529443+srujzs@users.noreply.github.com Remove casts for toDartObject (flutter/engine#53621) 2024-07-10 dkwingsmt@users.noreply.github.com [macOS] Multiview compositor (flutter/engine#52253) 2024-07-10 skia-flutter-autoroll@skia.org Roll Dart SDK from dba0dfbd06eb to c6053ad84f2b (3 revisions) (flutter/engine#53798) 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 bdero@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 makes the macOS
FlutterCompositorable to handle multiple views by adding methodsAddViewandRemoveView.These two methods must take place on the main queue (platform thread), which is indeed the case, but extra effort is made to assert so. This is important because, due to the constraint of the macOS system that requires rendering to take place on the platform thread (hence
FlutterThreadSynchronizer), no locks are needed to avoid race condition betweenAdd/RemoveViewand presenting.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.