Only register top level window message listener upon registering service binding #41733

yaakovschectman · 2023-05-04T16:25:14Z

Move registering the WM_CLOSE message listener in the engine from initialization to in response to a message sent from ServiceBinding upon its initialization so that we do not intercept window messages when there is no binding registered.

Addresses flutter/flutter#126033.

Pre-launch Checklist

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.

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

shell/platform/windows/flutter_windows_engine_unittests.cc

shell/platform/windows/flutter_windows_engine.cc

shell/platform/windows/flutter_windows_engine_unittests.cc

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

shell/platform/windows/flutter_windows_engine_unittests.cc

Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com>

loic-sharma · 2023-05-04T20:38:40Z

shell/platform/windows/flutter_windows_engine.h

@@ -269,6 +269,10 @@ class FlutterWindowsEngine {
  // Called when a WM_DWMCOMPOSITIONCHANGED message is received.
  void OnDwmCompositionChanged();

+  // Called in response to the framework registering a ServiceBindings.
+  // Registers the top level handler for the WM_CLOSE window message.
+  void OnServiceBindingsRegistered();


Don't forget to update these names to reflect the latest framework message name :)

loic-sharma

Looks good!

shell/platform/linux/fl_platform_plugin.cc

cbracken

lgtm!

cbracken · 2023-05-09T19:31:24Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

@@ -1031,6 +1031,10 @@ - (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result {
    result(@{@"value" : @([self clipboardHasStrings])});
  } else if ([call.method isEqualToString:@"System.exitApplication"]) {
    [[self terminationHandler] handleRequestAppExitMethodCall:call.arguments result:result];
+  } else if ([call.method isEqualToString:@"System.initializationComplete"]) {
+    // TODO(chrome-bot): Handle this message to enable exit message listening.


Is chrome-bot something I don't yet know about or should this be gspencergoog?

chrome-bot was from the patch generated by the lint failure. It looks like no one is assigned to the open linux PR, but he authored it, so probably.

robert-ancell

LGTM (reviewed Linux part of this PR)

…ering service binding (flutter/engine#41733)

…sions) (#126455) Manual roll requested by aaclarke@google.com Cannot build log URL because revision "892f88d2513f" is invalid: Luci builds of "Linux Fuchsia FEMU" for 892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED 2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 jonahwilliams@google.com [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 yjbanov@google.com [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS 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 aaclarke@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://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

…ering service binding (flutter/engine#41733)

…126468) flutter/engine@10ac36c...406b354 2023-05-10 mdebbar@google.com [web] Re-enable history tests on Safari (flutter/engine#41901) 2023-05-10 flar@google.com switch from MockCanvas to DisplayListBuilder in layer unit tests (flutter/engine#41889) 2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 jonahwilliams@google.com [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 yjbanov@google.com [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS 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 aaclarke@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://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

Send a platform message to the engine when the `ServiceBinding` is registered. Framework side of flutter/engine#41733 Addresses #126033 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>

…mework (#41782) ## Description Similar to #41733 and #41753 this causes the linux embedding to wait until it hears that the scheduler binding has registered itself before proceeding to send termination requests to the framework. This allows applications that don't use the framework (just use `dart:ui` directly) to exit automatically when the last window is closed. Without this change, the app does not exit when the window is closed. Depends on framework PR flutter/flutter#126075 landing first. ## Related PRs - #41733 - #41753 ## Related Issues - flutter/flutter#126033. ## Tests - Added a test to make sure that it doesn't send a termination request if the binding hasn't notified that it is ready yet.

…mework (#41753) Similar to #41733 and #41782, this causes the macos embedding to wait until it hears that the scheduler binding has registered itself before proceeding to send termination requests to the framework. This allows applications that don't use the framework (just use `dart:ui` directly) to exit automatically when the last window is closed. Without this change, the last window closes, but the app does not exit. Depends on framework PR flutter/flutter#126075 landing first.

…sions) (flutter#126455) Manual roll requested by aaclarke@google.com Cannot build log URL because revision "892f88d2513f" is invalid: Luci builds of "Linux Fuchsia FEMU" for 892f88d2513f4a60ebd6d1496c4ac272e53c55ba was STARTED 2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 jonahwilliams@google.com [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 yjbanov@google.com [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS 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 aaclarke@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://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

…lutter#126468) flutter/engine@10ac36c...406b354 2023-05-10 mdebbar@google.com [web] Re-enable history tests on Safari (flutter/engine#41901) 2023-05-10 flar@google.com switch from MockCanvas to DisplayListBuilder in layer unit tests (flutter/engine#41889) 2023-05-10 zanderso@users.noreply.github.com Revert "Move linux fuchsia engine v2 build to prod." (flutter/engine#41902) 2023-05-10 godofredoc@google.com Migrate mac host clang tidy to engine v2. (flutter/engine#41824) 2023-05-10 jonahwilliams@google.com [Impeller] delete special handling of RRect. (flutter/engine#41872) 2023-05-10 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JiOACcaGrDphuHIql... to a3rrULFaXlwS8mfjW... (flutter/engine#41898) 2023-05-10 109111084+yaakovschectman@users.noreply.github.com Only register top level window message listener upon registering service binding (flutter/engine#41733) 2023-05-10 skia-flutter-autoroll@skia.org Roll Skia from 8c936fb9ba8e to 32f4cfc2460b (27 revisions) (flutter/engine#41891) 2023-05-10 yjbanov@google.com [web] dialog a11y fixes (flutter/engine#41681) 2023-05-10 skia-flutter-autoroll@skia.org Roll Dart SDK from 2cf089614e1c to 7ad028c26344 (2 revisions) (flutter/engine#41882) 2023-05-10 jonahwilliams@google.com [Impeller] Increase minimum size of alpha glyph atlas. (flutter/engine#41880) 2023-05-10 jonahwilliams@google.com [Impeller] Use separate atlases and shaders for color and alpha (flutter/engine#41780) 2023-05-10 godofredoc@google.com Move linux fuchsia engine v2 build to prod. (flutter/engine#41865) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JiOACcaGrDph to a3rrULFaXlwS 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 aaclarke@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://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

Send a platform message to the engine when the `ServiceBinding` is registered. Framework side of flutter/engine#41733 Addresses flutter#126033 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [Features we expect every widget to implement]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat --------- Co-authored-by: Greg Spencer <gspencergoog@users.noreply.github.com>

yaakovschectman added 2 commits May 4, 2023 10:35

Listen for binding register

a9edfd8

Consume message on mac, linux

248630a

yaakovschectman added affects: desktop platform-windows labels May 4, 2023

yaakovschectman requested review from robert-ancell, cbracken, loic-sharma and gspencergoog May 4, 2023 16:25

yaakovschectman self-assigned this May 4, 2023

flutter-dashboard bot added platform-linux platform-macos labels May 4, 2023

yaakovschectman mentioned this pull request May 4, 2023

Alert engine upon registering ServiceBinding flutter/flutter#126075

Merged

8 tasks

Fix typo

4f72ae4

gspencergoog reviewed May 4, 2023

View reviewed changes

shell/platform/windows/flutter_windows_engine_unittests.cc Outdated Show resolved Hide resolved

yaakovschectman added 3 commits May 4, 2023 12:39

Fix typo

d27058c

Test service binding registration

3807362

Formatting

952520b

yaakovschectman marked this pull request as ready for review May 4, 2023 16:59