[Windows] Ensure window is shown #127046
Merged
[Windows] Ensure window is shown #127046
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flutter-dashboard
bot
added
d: examples
loic-sharma
requested review from
cbracken,
christopherfujino and
yaakovschectman
|
|
||
| // Migrate the windows/runner/flutter_window.cpp file. | ||
| final String originalContents = _file.readAsStringSync(); | ||
| final String newContents = replaceFirst( |
Contributor
There was a problem hiding this comment.
Should we check how many matches there are of _before, and do the replace if and only if there is 1 match? Or am I being too paranoid?
Member
Author
There was a problem hiding this comment.
It's a fair concern. The migration shouldn't be too liberal otherwise it might break projects, but also it shouldn't be too conservative otherwise it might not fix projects that are affected by this bug. It's a tricky balance!
To be more conservative I could expand the match template to this:
SetChildContent(flutter_controller_->view()->GetNativeWindow());
flutter_controller_->engine()->SetNextFrameCallback([&]() {
this->Show();
});
return true;
However, this wouldn't have matched some of the example/test projects in the Flutter repo, hence why I picked the current template. I don't feel strongly about this, I'd be happy to make the match more conservative if folks want.
Contributor
There was a problem hiding this comment.
Why didn't this match some of our example projects? Have we changed the code slightly over time?
Member
Author
There was a problem hiding this comment.
Some of our projects were modified to enable integration tests or for adding examples of things like platform channels.
Contributor
There was a problem hiding this comment.
That being the case, are we confident this matcher will work on all the actual user projects we intend to migrate?
Member
Author
There was a problem hiding this comment.
Hm, I checked Google's apps and none of them would match this migration as they've all modified their runners. I'll take an action item to write docs on how to do this migration manually to address this concern.
yaakovschectman
approved these changes
May 18, 2023
Contributor
yaakovschectman
left a comment
yaakovschectman
left a comment
There was a problem hiding this comment.
LGTM, one comment above
loic-sharma
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 21, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 21, 2023
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
May 22, 2023
flutter/flutter@077d644...ab57304 2023-05-20 [email protected] Roll Flutter Engine from 482c99af9c69 to aac09195688d (1 revision) (flutter/flutter#127241) 2023-05-20 [email protected] Reland "[tool] Move Java functions to their own file" (flutter/flutter#126577) 2023-05-20 [email protected] Roll Flutter Engine from f0c02aee69db to 482c99af9c69 (1 revision) (flutter/flutter#127240) 2023-05-19 [email protected] Do not animate `TabBarView` if controller is invalid (flutter/flutter#123442) 2023-05-19 [email protected] Run Mac intel only targets on both intel and arm (flutter/flutter#127230) 2023-05-19 [email protected] [Windows] Ensure window is shown (flutter/flutter#127046) 2023-05-19 [email protected] Roll Flutter Engine from 3267fa29491a to f0c02aee69db (4 revisions) (flutter/flutter#127233) 2023-05-19 [email protected] Roll Flutter Engine from 2b14f8a1f21c to 3267fa29491a (4 revisions) (flutter/flutter#127224) 2023-05-19 [email protected] fixes to anticipate next Dart linter release (flutter/flutter#127211) 2023-05-19 [email protected] Remove deprecated OverscrollIndicatorNotification.disallowGlow (flutter/flutter#127050) 2023-05-19 [email protected] Roll Flutter Engine from f471b37a2146 to 2b14f8a1f21c (1 revision) (flutter/flutter#127221) 2023-05-19 [email protected] [flutter_tools] only try to take a screenshot from flutter drive if the --screenshot flag is passed (flutter/flutter#127150) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/flutter#127218) 2023-05-19 [email protected] Roll Packages from b31a128 to 1e214d7 (3 revisions) (flutter/flutter#127217) 2023-05-19 [email protected] Roll Flutter Engine from a0ea4d2d9ea5 to f471b37a2146 (1 revision) (flutter/flutter#127212) 2023-05-19 [email protected] Revert "Migrate benchmarks to package:web" (flutter/flutter#127207) 2023-05-19 [email protected] [tool] delete xcresult bundle file before each xcode retry. (flutter/flutter#127144) 2023-05-19 [email protected] [iOS] Dispose of log readers and port forwarders if launch fails (flutter/flutter#127140) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages 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 Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers
pushed a commit
to CaseyHillers/flutter
that referenced
this pull request
May 24, 2023
## Background The Windows runner has a race at startup: 1. **Platform thread**: creates a hidden window 2. **Platform thread**: launches the Flutter engine 3. **UI/Raster threads**: renders the first frame 4. **Platform thread**: Registers a callback to show the window once the next frame has been rendered. Steps 3 and 4 happen in parallel and it is possible for step 3 to complete before step 4 starts. In this scenario, the next frame callback is never called and the window is never shown. As a result the `windows_startup_test`'s test, which [verifies that the "show window" callback is called](https://github.com/flutter/flutter/blob/1f09a8662dad3bb1959b24e9124e05e2b9dbff1d/dev/integration_tests/windows_startup_test/windows/runner/flutter_window.cpp#L60-L64), can flake if the first frame is rendered before the show window callback has been registered. ## Solution This change makes the runner schedule a frame after it registers the next frame callback. If step 3 hasn't completed yet, this no-ops as a frame is already scheduled. If step 3 has already completed, a new frame will be rendered, which will call the next frame callback and show the window. Part of flutter#119415 See this thread for alternatives that were considered: flutter/engine#42061 (comment)
This was referenced May 25, 2023
This was referenced May 30, 2023
auto-submit bot
pushed a commit
that referenced
this pull request
May 31, 2023
Address Tong's feedback here: #127695 (comment) Follow-up to: #127046
nploi
pushed a commit
to nploi/packages
that referenced
this pull request
Jul 16, 2023
flutter/flutter@077d644...ab57304 2023-05-20 [email protected] Roll Flutter Engine from 482c99af9c69 to aac09195688d (1 revision) (flutter/flutter#127241) 2023-05-20 [email protected] Reland "[tool] Move Java functions to their own file" (flutter/flutter#126577) 2023-05-20 [email protected] Roll Flutter Engine from f0c02aee69db to 482c99af9c69 (1 revision) (flutter/flutter#127240) 2023-05-19 [email protected] Do not animate `TabBarView` if controller is invalid (flutter/flutter#123442) 2023-05-19 [email protected] Run Mac intel only targets on both intel and arm (flutter/flutter#127230) 2023-05-19 [email protected] [Windows] Ensure window is shown (flutter/flutter#127046) 2023-05-19 [email protected] Roll Flutter Engine from 3267fa29491a to f0c02aee69db (4 revisions) (flutter/flutter#127233) 2023-05-19 [email protected] Roll Flutter Engine from 2b14f8a1f21c to 3267fa29491a (4 revisions) (flutter/flutter#127224) 2023-05-19 [email protected] fixes to anticipate next Dart linter release (flutter/flutter#127211) 2023-05-19 [email protected] Remove deprecated OverscrollIndicatorNotification.disallowGlow (flutter/flutter#127050) 2023-05-19 [email protected] Roll Flutter Engine from f471b37a2146 to 2b14f8a1f21c (1 revision) (flutter/flutter#127221) 2023-05-19 [email protected] [flutter_tools] only try to take a screenshot from flutter drive if the --screenshot flag is passed (flutter/flutter#127150) 2023-05-19 [email protected] Roll goldctl to f808dcff91b221ae313e540c09d79696cd08b8de (flutter/flutter#127218) 2023-05-19 [email protected] Roll Packages from b31a128 to 1e214d7 (3 revisions) (flutter/flutter#127217) 2023-05-19 [email protected] Roll Flutter Engine from a0ea4d2d9ea5 to f471b37a2146 (1 revision) (flutter/flutter#127212) 2023-05-19 [email protected] Revert "Migrate benchmarks to package:web" (flutter/flutter#127207) 2023-05-19 [email protected] [tool] delete xcresult bundle file before each xcode retry. (flutter/flutter#127144) 2023-05-19 [email protected] [iOS] Dispose of log readers and port forwarders if launch fails (flutter/flutter#127140) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages 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 Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 16, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
The Windows runner has a race at startup:
Steps 3 and 4 happen in parallel and it is possible for step 3 to complete before step 4 starts. In this scenario, the next frame callback is never called and the window is never shown.
As a result the
windows_startup_test's test, which verifies that the "show window" callback is called, can flake if the first frame is rendered before the show window callback has been registered.Solution
This change makes the runner schedule a frame after it registers the next frame callback. If step 3 hasn't completed yet, this no-ops as a frame is already scheduled. If step 3 has already completed, a new frame will be rendered, which will call the next frame callback and show the window.
Part of #119415
See this thread for alternatives that were considered: flutter/engine#42061 (comment)
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.