Moved the default BinaryMessenger instance to ServicesBinding #37489

adazh · 2019-08-02T16:55:43Z

Description

Moved the [default BinaryMessenger] instance (

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

Line 154 in 0cf4033

const BinaryMessenger defaultBinaryMessenger = _DefaultBinaryMessenger._();

) to ServicesBinding.

By this change, we are capable of registering a different default BinaryMessenger under testing environment, by creating a ServicesBinding subclass for testing. With that, we can track # of pending platform messages for synchronization purposes (see the related issue below).

Related Issues

#37409

Tests

Unit/Widget tests updated.
Manually tested on the example platform_channel app.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). https://groups.google.com/d/msg/flutter-announce/sHAL2fBtJ1Y/mGjrKH3dEwAJ
No, this is not a breaking change.

This is a breaking change:

It deprecated the original defaultBinaryMessenger instance. The usage of defaultBinaryMessenger is mostly within the framework, but since it's publicly accessible, so still could possibly impact users.
Bindings need to be properly initialized before using any platform channels.

fluttergithubbot · 2019-08-02T16:55:46Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

adazh · 2019-08-02T16:57:59Z

@Hixie @tvolkert @goderbauer
I haven't done cleanup/fixing tests, but I would like to check in with you guys first to make sure I'm on the right track. PTAL. Thanks!

Hixie · 2019-08-02T17:23:11Z

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

+  }
+
+  /// The messenger which sends the bytes for this channel, not null.
+  BinaryMessenger binaryMessenger;


this should be final

Updated to use the _binaryMessenger getter.

Hixie · 2019-08-02T17:24:45Z

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

+  /// A default [BinaryMessenger] instance will be used if [binaryMessenger] is
+  /// null.
+  PlatformAssetBundle({this.binaryMessenger}) {
+    binaryMessenger ??= ServicesBinding.instance.provideBinaryMessenger();


this can be an initializer, which allows the field to be final.

that said, see later comments for an alternative strategy that would be consistent with what we'll have to do below.

Hixie · 2019-08-02T17:27:15Z

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

+    assert(name != null);
+    assert(codec != null);
+    binaryMessenger ??= ServicesBinding.instance.provideBinaryMessenger();
+  }


we need to keep this class const.

Easiest way to do this, I think, is to have the default value of the binaryMessenger argument be null, have it set a private field _binaryMessenger, and then have BinaryMessenger get binaryMessenger => _binaryMessenger ?? ServicesBinding.instance.defaultBinaryMessenger, where the latter is a getter that returns a precached value created during initInstances.

Thanks! I'll try this approach.

Updated accordingly. PTAL.

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

Hixie · 2019-08-02T17:30:05Z

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

+  BinaryMessenger provideBinaryMessenger() {
+    return defaultBinaryMessenger;
+  }
+


I think you want to flip these around. The getter should be the way to access this, to make it clear that we're not initializing a new value each time. The method should be protected, and should be called during initInstances to configure a field that defaultBinaryMessenger returns.

Updated. I was wrong by treating the binaryMessenger field as private member (which is public accessible), and had this provideBinaryMessenger as the getter for it.

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

Hixie · 2019-08-02T17:45:16Z

packages/flutter_test/lib/src/test_text_input.dart

  final VoidCallback onCleared;

+  /// The messenger which sends the bytes for this channel, not null.
+  BinaryMessenger binaryMessenger;


should be final

or just use the one from the services binding directly.

in fact here and various other places, i think it's better to do ServicesBinding.instance.defaultBinaryMessenger directly than to use a local copy. Alternatively, use a private _binaryMessenger getter that just returns ServicesBinding.instance.defaultBinaryMessenger. This avoids the local copy.

I like the latter approach. It's generally a good practice to keep external dependencies obvious. Also, easier for injecting mocks for testing.

Hixie · 2019-08-02T17:46:51Z

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

-/// This is used to send messages from the application to the platform, and
-/// keeps track of which handlers have been registered on each channel so
-/// it may dispatch incoming messages to the registered handler.
-const BinaryMessenger defaultBinaryMessenger = _DefaultBinaryMessenger._();


rather than removing this (a hard breaking change), let's just deprecate it and turn it into a getter than returns ServicesBinding.instance.defaultBinaryMessenger.

Added a @deprecated annotation and updated all the tests. PTAL.

…Hixie@

adazh · 2019-08-03T01:51:05Z

@Hixie This app under test reveals a case that MessageChannel could possibly be called before ServicesBinding is initialized. See the app's main.

It can be fixed by switching the two lines in main:
_kReloadChannel.setMessageHandler(run); run(ui.window.defaultRouteName);.
But would like to double check whether this is a legit assumption that calling MessageChannel should usually happen after runApp.

Hixie · 2019-08-04T00:20:12Z

Generally we want people to call the binding initialization logic before they do anything anyway.

Hixie · 2019-08-04T00:21:29Z

(which doesn't have to be runApp, you can also do it directly)

adazh · 2019-08-05T20:40:55Z

@Hixie @tvolkert @goderbauer Tests all fixed. PTAL.

goderbauer

LGTM

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

goderbauer · 2019-08-07T22:25:19Z

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

 /// keeps track of which handlers have been registered on each channel so
 /// it may dispatch incoming messages to the registered handler.
-const BinaryMessenger defaultBinaryMessenger = _DefaultBinaryMessenger._();
+@Deprecated('Use ServicesBinding.instance.defaultBinaryMessenger instead.')


Can you update the doc comment above as well to reflect that this is deprecated and link to the new thing that should be used?

Updated. PTAL.

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

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

tvolkert · 2019-08-08T19:01:42Z

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

+
+  /// Initializes a default [BinaryMessenger] instance that can be used for
+  /// sending platform messages.
+  @protected


Is there a reason this can't be private? Do we expect subclasses to override it to return a different binary messenger than the default?

Ya, exactly. We'll want to set a different binary messenger for the ServicesBinding used in tests.

tvolkert · 2019-08-08T19:02:43Z

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

+  /// This is used to send messages from the application to the platform, and
+  /// keeps track of which handlers have been registered on each channel so
+  /// it may dispatch incoming messages to the registered handler.
+  BinaryMessenger defaultBinaryMessenger;


This is public and mutable. We should probably make it private with a public getter.

Hmm... seems that won't work, since we'll need to set it in subclasses.
Thus, the setter has to be visible in its subclasses, otherwise we don't have a way to modify it in a subclass.

Then what about changing the signature of initBinaryMessenger() to return the messenger rather than setting it? then the instance variable can be private (with a public getter), and the base class' initInstances() method can set the instance variable to the value it gets back from initBinaryMessenger().

Along with this change, maybe initBinaryMessenger() should be renamed createBinaryMessenger() or something similar?

I was dumb... Ya, that would totally work. Updated defaultBinaryMessenger to be a private field. PTAL.

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

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

adazh · 2019-08-09T17:54:11Z

Included this breaking change in the Changelog.

amirh · 2019-08-09T22:51:48Z

This forces plugins to choose whether to work with master or stable (sample breakage.

Can we land this in phases instead?

amirh · 2019-08-09T23:00:34Z

Offline comment by ada - we can call TestWidgetsFlutterBinding.ensureInitialized(); both on stable and master which will fix the breakage.

tvolkert · 2019-08-09T23:00:50Z

@amirh does adding TestWidgetsFlutterBinding.ensureInitialized(); to the tests not fix the problem in both master and stable? Also, this should only affect tests, not actual production code.

mklim · 2019-08-09T23:16:27Z

Discussion offline. This requires either runApp() or WidgetsFlutterBinding.ensureInitialized() to have been called before using any MethodChannels. Plugins broadly rely on MethodChannels and right now there's no general requirements that they be constructed within the runApp call, so the ecosystem breakages from this are broader than originally thought. Reverting this for now.

…#37489)" (#37983) This reverts commit 92ef2b9. This requires either runApp() or WidgetsFlutterBinding.ensureInitialized() to have been called before using any MethodChannels. Plugins broadly rely on MethodChannels and right now there's no general requirements that they be constructed within the runApp call, so the ecosystem breakages from this are broader than originally thought. Reverting for now.

tvolkert · 2019-08-10T16:14:52Z

For when this is re-applied: it also broke the image_list_jit_reported_duration and the image_list_reported_duration devicelab tests.

adazh · 2019-08-12T17:06:43Z

@tvolkert Is there a way I can run these two tests before check-in?

goderbauer · 2019-08-12T17:13:00Z

Instructions for running devicelab tests locally: https://github.com/flutter/flutter/tree/master/dev/devicelab#running-tests-locally

Original PR: (flutter#37489), this PR reverts the revert PR (flutter#37983). This reverts commit 821602a.

Moved the default BinaryMessenger to the ServicesBinding

5cda842

fluttergithubbot added a: tests "flutter test", flutter_test, or one of our tests d: examples Sample code and demos framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 2, 2019

googlebot added the cla: yes label Aug 2, 2019

adazh requested review from Hixie, goderbauer and tvolkert August 2, 2019 16:56

adazh changed the title ~~Moved the default BinaryMessenger to the ServicesBinding~~ Moved the default BinaryMessenger instance to ServicesBinding Aug 2, 2019

Hixie reviewed Aug 2, 2019

View reviewed changes

packages/flutter/lib/src/services/platform_messages.dart Outdated Show resolved Hide resolved

Hixie reviewed Aug 2, 2019

View reviewed changes

packages/flutter/lib/src/services/binding.dart Outdated Show resolved Hide resolved

Hixie reviewed Aug 2, 2019

View reviewed changes

packages/flutter/lib/src/services/binding.dart Outdated Show resolved Hide resolved

Hixie reviewed Aug 2, 2019

View reviewed changes

adazh added 2 commits August 2, 2019 15:39

Keeps const constructors of platform channels; Addressed comments of …

56de41c

…Hixie@

Fixed deprecated defaultBinaryMessenger warnings

8fb78f6

adazh added the c: API break Backwards-incompatible API changes label Aug 2, 2019

Fixed ios_addtoapp integration tests

03f3dea

adazh requested a review from Hixie August 6, 2019 18:37

goderbauer approved these changes Aug 7, 2019

View reviewed changes

fixes

0160753

tvolkert reviewed Aug 8, 2019

View reviewed changes

Addressed Todd's comments

876535a

adazh requested a review from tvolkert August 8, 2019 22:37

adazh added 3 commits August 8, 2019 15:39

Removed unused deps

4f0c362

Updated _defaultBinaryMessenger to be private

504840e

Minor createBinaryMessenger comment update

ce37c8c

tvolkert approved these changes Aug 8, 2019

View reviewed changes

packages/flutter/lib/src/services/binding.dart Outdated Show resolved Hide resolved

Format fixes

98f0808

adazh merged commit 92ef2b9 into flutter:master Aug 9, 2019

adazh deleted the binary_messenger branch August 9, 2019 17:14

mklim mentioned this pull request Aug 9, 2019

Revert "Moved the default BinaryMessenger instance to ServicesBinding… #37983

Merged

9 tasks

adazh added a commit to adazh/flutter that referenced this pull request Aug 13, 2019

Moved the default BinaryMessenger instance to ServicesBinding

74c6863

Original PR: (flutter#37489), this PR reverts the revert PR (flutter#37983). This reverts commit 821602a.

adazh mentioned this pull request Aug 13, 2019

Moved the default BinaryMessenger instance to ServicesBinding #38464

Merged

9 tasks

This was referenced Aug 22, 2019

flutter create --template plugin has failing tests #39077

Closed

Fix plugin template app's tests #39080

Merged

Piinks mentioned this pull request Aug 24, 2019

Fixing default BinaryMessenger Breaking Change flutter/assets-for-api-docs#103

Merged

hellowmq mentioned this pull request Sep 9, 2019

flutter run 启动时卡在闪屏界面，机型华为荣耀 7X BND AL10 和一加五 A5000 luoguoxiong/easyMarketFlutter#1

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021

Moved the default BinaryMessenger instance to ServicesBinding #37489

Moved the default BinaryMessenger instance to ServicesBinding #37489

Uh oh!

Conversation

adazh commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Aug 2, 2019

Uh oh!

adazh commented Aug 2, 2019

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

adazh commented Aug 3, 2019

Uh oh!

Hixie commented Aug 4, 2019

Uh oh!

Hixie commented Aug 4, 2019

Uh oh!

adazh commented Aug 5, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

adazh Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adazh commented Aug 2, 2019 •

edited

Loading

adazh Aug 8, 2019 •

edited

Loading