-
Notifications
You must be signed in to change notification settings - Fork 6k
[Windows] Allow view controllers to not own the engine #51570
[Windows] Allow view controllers to not own the engine #51570
Conversation
dkwingsmt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| // |FlutterDesktopViewControllerDestroy|. Returns a null pointer in the event of | ||
| // an error. | ||
| // | ||
| // Unlike |FlutterDesktopViewControllerCreate|, this does *not* take ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add a similar paragraph to FlutterDesktopViewControllerCreate as well, so that they can refer to each other. And also we might want to mention that:
// In an app with multiple view controllers, it is highly recommend to use
// `FlutterDesktopEngineCreateViewController` for all view controllers and avoid
// `FlutterDesktopViewControllerCreate`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll definitely want a blurb like this eventually, however, FlutterDesktopEngineCreateViewController isn't in the public header yet. Adding this right now would be confusing to our customers as they can't use this API yet.
I'll make sure to add such a comment when we make this runner API public.
| // using |FlutterDesktopViewControllerCreate| as that takes | ||
| // ownership of the engine. View controllers created using | ||
| // |FlutterDesktopEngineCreateViewController| do not take | ||
| // ownership of the engine and do not set this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add
// If `engine_` is not null, it will be deallocated upon
// the view controller's destruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked the wording, hopefully it's clearer now.
…145611) flutter/engine@09dadce...63ff0de 2024-03-22 [email protected] [Embedder API] Add helper to create viewport metrics (flutter/engine#51562) 2024-03-22 [email protected] [Windows] Allow view controllers to not own the engine (flutter/engine#51570) 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 [email protected],[email protected],[email protected] 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
Adds
FlutterDesktopEngineCreateViewControllerto the Windows embedder's C API. Example usage:This new API creates a view controller that does not own the engine, unlike the existing
FlutterDesktopEngineCreateViewController. In the future, this new API will allow a Windows app to have multiple views that share a single engine.This API is in the
flutter_windows_internal.h, making it "pubternal". This API is not in the public Windows header files, however, one can link against this API for testing purposes. This API will be promoted to the public Windows header files once the Windows embedder is ready to have multiple views.Relevant design documents:
Relevant issues:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.