Alert engine upon registering ServiceBinding #126075

yaakovschectman · 2023-05-04T16:29:54Z

Send a platform message to the engine when the ServiceBinding is registered. Framework side of flutter/engine#41733

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, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.

yaakovschectman · 2023-05-04T16:30:54Z

I think this will need a test, but as it tests the initialization of the binding, I will need to determine exactly how to write it.

gspencergoog

This looks good, but yes, does need a test.

yaakovschectman · 2023-05-04T16:58:51Z

@gspencergoog How would we be able to test that the framework sends a certain platform message to the engine upon initializing the ServiceBinding? Do we have a way to mock engine-side message handling from within framework tests? It looks like we only mock the framework-side.

gspencergoog · 2023-05-04T17:10:54Z

It could be an integration test that has a plugin that registers to receive those platform messages, maybe. That's a lot to set up, though.

Maybe just verifying that the framework tries to send the message is enough: you could modify the TestPlatformDispatcher in the test binding to log the messages sent and verify them , maybe?

flutter/packages/flutter_test/lib/src/window.dart

Line 441 in c2022aa

_platformDispatcher.sendPlatformMessage(name, data, callback);

yaakovschectman · 2023-05-04T17:26:06Z

@gspencergoog I am unclear on what TestPlatformDispatcher you're referring to with regards to this test binding (where ServiceBinding is tested):

flutter/packages/flutter/test/services/binding_test.dart

Line 43 in 2bea177

class TestBinding extends BindingBase with SchedulerBinding, ServicesBinding {

gspencergoog · 2023-05-04T17:31:28Z

Yeah, you're right, it's not available at that layer. Maybe it needs to be a test run with the tests for WidgetsBinding.

yaakovschectman · 2023-05-04T17:36:11Z

Are you referring to

flutter/packages/flutter/test/widgets/binding_test.dart

Line 48 in 2bea177

void main() {

?
What is available at that layer that seems useful to this prospective test?

gspencergoog · 2023-05-04T17:44:31Z

The TestWidgetsFlutterBinding uses a TestPlatformDispatcher, which includes an implementation of sendPlatformMessage that I thought might be used to watch messages sent to the engine.

Unfortunately, I don't think that will work because we don't use the injected instance when we send messages to the engine:

flutter/packages/flutter/lib/src/services/binding.dart

Line 458 in c2022aa

    
           ui.PlatformDispatcher.instance.sendPlatformMessage(channel, message, (ByteData? reply) {

loic-sharma · 2023-05-04T19:02:16Z

packages/flutter/lib/src/services/binding.dart

    SystemChannels.system.setMessageHandler((dynamic message) => handleSystemMessage(message as Object));
    SystemChannels.lifecycle.setMessageHandler(_handleLifecycleMessage);
    SystemChannels.platform.setMethodCallHandler(_handlePlatformMessage);
+    _registerBindingWithEngine();


We may want to leverage this as a generic "the ServicesBinding is fully initialized" message in the future. Should we make this be the last step in initInstances?

On second thought, it seems weird to require an app to send a System.registerBinding if it wants to listen to application lifecycle events. For example, a dart:ui app may want to listen to app lifecycle events without using ServicesBinding. Perhaps we should use a different name like System.enableApplicationLifecycle?

I don't think it's that weird. Having something that happens after the binding is initialized seems like something you might want to use for other things besides enabling application lifecycle events too, so it doesn't seem too odd to have a "we're done initializing the binding" message to start things off. It does increase the boilerplate for such an app, however, which is too bad, but I think anyone building an app at that level is expecting some level of boilerplate.

To clarify, I'm fine with the boilerplate. I disliked the previous message name System.registerBinding for a raw dart:ui app.

How about initializationComplete? Is that really true at the point the message is sent?

I'd be fine with System.initializationComplete too, but what would be the best way to document its implications? (Namely that the app now needs to respond to System.requestAppExit)

Yes, for now that is all it implies: that it's ready to respond to System.requestAppExit requests, but to the embedding, it also represents the point at which the framework's bindings are registered, so it can feel free to communicate with the bindings. I also wonder if there are other parts of the engine that assume the binding will respond to a method call which would benefit from waiting until the binding is ready?

yaakovschectman · 2023-05-04T21:12:05Z

What are your thoughts on adding to the windows_startup_test integration test to ensure that the platform message is sent by binding initialization?

gspencergoog · 2023-05-04T22:40:32Z

What are your thoughts on adding to the windows_startup_test integration test to ensure that the platform message is sent by binding initialization?

That sounds reasonable.

yaakovschectman · 2023-05-05T13:44:02Z

I'm running into some difficulty setting the handler for the platform message on the C++ side, because it has a reference to the standard method codec instance, but not the JSON method codec, which is what the platform channel uses, and I do not currently see a way to ensure the platform channel message is sent without registering a handler that responds to it in some way.

gspencergoog · 2023-05-05T18:48:07Z

packages/flutter/lib/src/services/binding.dart

+  /// Alert the engine that the binding is registered. This instructs the engine to
+  /// register its top level window handler on Windows.
+  Future<void> enableApplicationLifecycle() async {
+    SystemChannels.platform.invokeMethod('System.enableApplicationLifecycle');


Loïc and I discussed making this "System.initializationComplete" so that this wouldn't be so tied to app lifecycle, but could be used as a more generic signal. That's what I currently have in my implementations for macOS and Linux.

yaakovschectman · 2023-05-11T14:37:29Z

I've reworked the test. Instead of using the integration test, we are now using a unit test. initializationComplete is now protected but exposed by TestWidgetsFlutterBinding.

goderbauer · 2023-05-12T07:47:27Z

packages/flutter_test/lib/src/binding.dart

    );
  }

+  /// Expose this method for testing.


This exposes the method for every test (not only ours, but also for all our customers). Since there should never be a reason for anyone to call this, we shouldn't do that. You'll have to create a custom binding subclass that's only used for your test.

As only one binding can be active at a time, to my knowledge, I've put this test into a new file so that the custom binding it uses can be used exclusively by this test.

goderbauer

LGTM

…#126075)

…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.

…#126075)

Send message

2bea177

yaakovschectman added platform-windows Building on or for Windows specifically a: desktop Running on desktop labels May 4, 2023

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

yaakovschectman self-assigned this May 4, 2023

flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label May 4, 2023

gspencergoog reviewed May 4, 2023

View reviewed changes

yaakovschectman marked this pull request as ready for review May 4, 2023 18:51

loic-sharma reviewed May 4, 2023

View reviewed changes

Rename message

ff4672e

gspencergoog mentioned this pull request May 4, 2023

[macOS] Wait for binding to be ready before requesting exits from framework flutter/engine#41753

Merged

yaakovschectman added 2 commits May 5, 2023 13:44

Example for sending message

d361640

Working test

549920a

yaakovschectman requested review from gspencergoog and loic-sharma May 5, 2023 18:43

gspencergoog reviewed May 5, 2023

View reviewed changes

yaakovschectman added 2 commits May 11, 2023 10:30

Typo

08cb49e

Remove integration test changes

2bf2ee0

yaakovschectman requested review from cbracken, goderbauer and loic-sharma May 11, 2023 14:37

Merge branch 'master' into wm_close_toplevel

caf3b0b

goderbauer reviewed May 12, 2023

View reviewed changes

Isolate test using custom binding

97078f4

goderbauer reviewed May 16, 2023

View reviewed changes

Merge branch 'master' into wm_close_toplevel

9c9d8d9

yaakovschectman merged commit 130944e into flutter:master May 16, 2023

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter from a76dbe44b99e to 130944e78593 (76 revisions) flutter/packages#4019

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

01b27eb

…#126075)

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter from a76dbe44b99e to 1ca183d55b24 (77 revisions) flutter/packages#4020

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

e6458c3

…#126075)

engine-flutter-autoroll mentioned this pull request May 16, 2023

Roll Flutter from a76dbe44b99e to d0d1feb6283f (78 revisions) flutter/packages#4021

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 16, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

4270681

…#126075)

This was referenced May 25, 2023

Support minifcation for apps that depend on AGP 8 and integration_test #127628

Merged

i127682 android api 33 test support #127689

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

667d7e4

…#126075)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

640697b

…#126075)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

a543e6f

…#126075)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

130944e Alert engine upon registering ServiceBinding (flutter/flutter…

c4e1a91

…#126075)

JSUYA mentioned this pull request Aug 28, 2023

[engine] Sync Flutter 3.13.1 source code flutter-tizen/embedder#44

Merged

JSUYA mentioned this pull request Sep 8, 2023

Update to SystemChannel flutter-tizen/embedder#47

Closed

4 tasks

Alert engine upon registering ServiceBinding #126075

Alert engine upon registering ServiceBinding #126075

Uh oh!

Conversation

yaakovschectman commented May 4, 2023

Pre-launch Checklist

Uh oh!

yaakovschectman commented May 4, 2023

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented May 4, 2023

Uh oh!

gspencergoog commented May 4, 2023

Uh oh!

yaakovschectman commented May 4, 2023

Uh oh!

gspencergoog commented May 4, 2023

Uh oh!

yaakovschectman commented May 4, 2023

Uh oh!

gspencergoog commented May 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented May 4, 2023

Uh oh!

gspencergoog commented May 4, 2023

Uh oh!

yaakovschectman commented May 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaakovschectman commented May 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants