-
Notifications
You must be signed in to change notification settings - Fork 6k
[Windows] Refactor client wrapper to prepare for multi-view #52073
[Windows] Refactor client wrapper to prepare for multi-view #52073
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. |
// Whether the engine has been run. This will be true if Run has been called, | ||
// or if RelinquishEngine has been called (since the view controller will | ||
// run the engine if it hasn't already been run). | ||
bool has_been_run_ = false; |
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've updated the comment and renamed this to run_succeeded_
to reflect its current purpose better.
This comment was incorrect when it was added. See: cd4192d#diff-4d73a4226c049b829de25c009bf79a1d30674d639e9c5c23f7e508690f68d6aaR73
Converted back to draft as this change needs a little work. Apologies for the noise. |
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
df833ab
to
7e04539
Compare
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
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.
test-exempt: code refactor with no semantic change |
…146775) flutter/engine@1a13c7d...84238c4 2024-04-15 [email protected] [scenarios] test disabling surface clear. (flutter/engine#52128) 2024-04-12 [email protected] Roll Dart SDK from 3d13dbfb3284 to ac31be3c8546 (8 revisions) (flutter/engine#52084) 2024-04-12 [email protected] Roll Skia from 761f6f7f6250 to 9e20a146c024 (7 revisions) (flutter/engine#52085) 2024-04-12 [email protected] [Windows] Refactor client wrapper to prepare for multi-view (flutter/engine#52073) 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
…lutter#146775) flutter/engine@1a13c7d...84238c4 2024-04-15 [email protected] [scenarios] test disabling surface clear. (flutter/engine#52128) 2024-04-12 [email protected] Roll Dart SDK from 3d13dbfb3284 to ac31be3c8546 (8 revisions) (flutter/engine#52084) 2024-04-12 [email protected] Roll Skia from 761f6f7f6250 to 9e20a146c024 (7 revisions) (flutter/engine#52085) 2024-04-12 [email protected] [Windows] Refactor client wrapper to prepare for multi-view (flutter/engine#52073) 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
This is a refactoring with no semantic changes. No new public APIs are introduced by this change.
In the future, a new constructor will be added to
FlutterViewController
to allow it to use an existingstd::shared_ptr<FlutterEngine>
to create the view controller. This updatesFlutterViewController
to store astd::shared_ptr
and other minor tweaks.For context, we plan to not land the new multi-window
FlutterViewController
constructor until Window's multi-window support is productionized. Until then, we will maintain a patch for folks that want to experiment with multi-window. This change makes that patch as small as possible.Design doc: flutter.dev/go/desktop-multi-view-runner-apis
Part of flutter/flutter#143767
Part of flutter/flutter#142845
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.