Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Use hidden window to process flutter messages #24232

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Use hidden window to process flutter messages #24232

merged 1 commit into from
Mar 4, 2021

Conversation

knopp
Copy link
Member

@knopp knopp commented Feb 5, 2021

flutter/flutter#75500

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.
  • The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

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 Hixie on the #hackers channel in Chat.

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.

@google-cla google-cla bot added the cla: yes label Feb 5, 2021
@knopp knopp force-pushed the windows_hidden_window_for_messages branch 3 times, most recently from 6614826 to 2b19d55 Compare February 5, 2021 13:46
@knopp knopp changed the title WIP: Use hidden window to process flutter messages Use hidden window to process flutter messages Feb 5, 2021
@knopp knopp force-pushed the windows_hidden_window_for_messages branch from b0d06a1 to 9d83e7b Compare February 8, 2021 17:08
clarkezone
clarkezone approved these changes Feb 27, 2021
View reviewed changes
Copy link

@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change, LGTM with the caveat of the one comment. Not sure how breaking changes are handled and how those should be timed @cbracken to comment.

shell/platform/windows/flutter_windows_win32.cc
@@ -69,10 +68,7 @@ bool FlutterDesktopViewControllerHandleTopLevelWindowProc(
}

uint64_t FlutterDesktopEngineProcessMessages(FlutterDesktopEngineRef engine) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we even need FlutterDesktopEngineProcessMessages any more with this change. I don't believe we use it in UWP. It would be a breaking change tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary anymore (it a noop that just returns -1). Next step would be to remove this from the Windows template (it would become simple Get/Translate/Dispatch message loop). Returning -1 ensures that existing run-loops work just fine after the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually removing the function would involve writing a migrator into the Flutter tool. (There's some infrastructure in the tooling for it, but it hasn't been used yet on Windows)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuartmorgan, I don't think I'd want to remove the function from API anytime soon, but the template can be significantly simplified. I'll look at the migration, but that should not block this PR, right?

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, both this PR and a template upgrade are non-breaking, so aren't blocked. It's only at the point where it would actually be removed that a migrator would be necessary.

@knopp knopp force-pushed the windows_hidden_window_for_messages branch from f819d0f to 400a006 Compare March 1, 2021 14:46
cbracken
cbracken approved these changes Mar 4, 2021
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM to land.

@cbracken cbracken merged commit 1cd5d01 into flutter:master Mar 4, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 5, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 5, 2021
@engine-flutter-autoroll
1cd5d01 Use hidden window to process flutter messages (flutter/engine…
4588421 
…#24232)
@knopp knopp mentioned this pull request Mar 7, 2021
Closed
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
@knopp @hjfreyer
Use hidden window to process flutter messages (flutter#24232)
019d6f8 
flutter/flutter#75500
chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021
@knopp @chriscraws
Use hidden window to process flutter messages (flutter#24232)
8273c73 
flutter/flutter#75500
@Sunbreak Sunbreak mentioned this pull request Mar 28, 2021
Open
This was referenced Apr 7, 2021
Closed
Closed
Merged
rydmike added a commit to rydmike/flex_color_picker that referenced this pull request Apr 16, 2021
@rydmike
The new Window embedder runloop, may require master channel!
dc08a32 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
rydmike added a commit to rydmike/flex_color_scheme that referenced this pull request Apr 16, 2021
@rydmike
The new Window embedder runloop, may require master channel!
c389684 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
rydmike added a commit to rydmike/flex_color_picker that referenced this pull request Jun 11, 2021
@rydmike
The new Window embedder runloop, may require master channel!
e833ea6 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
rydmike added a commit to rydmike/flex_color_scheme that referenced this pull request Jun 24, 2021
@rydmike
The new Window embedder runloop, may require master channel!
7fbe56f 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
@knopp knopp deleted the windows_hidden_window_for_messages branch July 17, 2021 09:26
rydmike added a commit to rydmike/flex_color_scheme that referenced this pull request Nov 13, 2021
@rydmike
The new Window embedder runloop, may require master channel!
5080902 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
rydmike added a commit to rydmike/flex_color_scheme that referenced this pull request Nov 13, 2021
@rydmike
The new Window embedder runloop, may require master channel!
a30904c 
This new runloop gets rid of extra CPU load on Windows, but may require a version that is not available on all channels yet.
Refer to issue: flutter/flutter#79195
flutter/engine#24232
This should fix it at some point:
flutter/flutter#79969
@loic-sharma loic-sharma mentioned this pull request May 31, 2024
Open
@liff liff mentioned this pull request Dec 5, 2024
Open
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cbracken cbracken cbracken approved these changes

@clarkezone clarkezone clarkezone approved these changes

@stuartmorgan-g stuartmorgan-g Awaiting requested review from stuartmorgan-g

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

@iskakaushik iskakaushik Awaiting requested review from iskakaushik

Assignees
No one assigned
Labels
affects: desktop cla: yes platform-windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@knopp @stuartmorgan-g @cbracken @clarkezone