Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Has a binary messenger #9419

Merged
merged 11 commits into from
Jun 28, 2019
Merged

Has a binary messenger #9419

merged 11 commits into from
Jun 28, 2019

Conversation

gaaclarke
Copy link
Member

I made FlutterViewController and FlutterEngine both have binary messengers instead of publicly being binary messengers. This breaks certain retain cycles that we could create. It also minimizes the damage of the current problem we have where channels always leak on iOS. It has a fail-safe mechanism where talking to dead channels just gets ignored.

This won't break plugins, but it will potentially break some add2app scenarios. The client may have been saying:

[FlutterMethodChannel methodChannelWithName:@"foo" binaryMessenger:flutterViewController]

They will now have to say:

[FlutterMethodChannel methodChannelWithName:@"foo" 
                            binaryMessenger:flutterViewController.binaryMessenger]

@gaaclarke gaaclarke requested a review from cbracken June 21, 2019 18:04
@gaaclarke
Copy link
Member Author

https://flutter.dev/docs/development/platform-integration/platform-channels#step-4a-add-an-ios-platform-specific-implementation-using-objective-c

This example code will have to change as a result of this PR.

@gaaclarke
Copy link
Member Author

Relevant issue: flutter/flutter#26007

@gaaclarke
Copy link
Member Author

Email has been sent to flutter-announce: https://groups.google.com/forum/#!topic/flutter-announce/ewwf2Rhm9Uo

xster
xster reviewed Jun 24, 2019
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

This is way better than how it was already. Though since we're going to break API anyway, I'd love it if we made the 2 platforms' APIs more consistent flutter/flutter#33099.

Doesn't have to be all in one PR but we can pre-plan it and keep the component move gradual if we're going to have a DartExecutor which implements a BinaryMessenger.

shell/platform/darwin/ios/framework/Headers/FlutterViewController.h Outdated Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FLETextInputPlugin.mm Show resolved Hide resolved
@xster xster mentioned this pull request Jun 24, 2019
Merged
@gaaclarke
Copy link
Member Author

@xster Sounds good wrt Android. I haven't looked at the Android side much. We probably need a bigger exploration to see how we can unify. I poked around a bit and they are pretty diverged right now so I don't know if its necessary to have to make an android change in response to this change yet.

@gaaclarke gaaclarke requested a review from amirh June 24, 2019 22:56
@xster
Copy link
Member

xster commented Jun 25, 2019

yes, there's unfortunately a bit of divergence currently. Since it's a breaking change, we should ideally plan ahead a bit and not break a second time this quarter. I pinged you a list of other API differences. Let's chat tomorrow to work out something progressive but that we won't break in the future.

xster
xster reviewed Jun 27, 2019
View reviewed changes
Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM.

My general wish is to remove all knowledge of binaryMessengers from the vcs. How we do it in incremental PRs, you can decide as long as we get it all in before the next stable cut.

shell/platform/darwin/ios/framework/Source/FlutterEngine.mm Outdated Show resolved Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Show resolved Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Show resolved Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterViewController.mm Show resolved Hide resolved
shell/platform/darwin/macos/framework/Source/FLEViewController.mm Show resolved Hide resolved
amirh
amirh approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM

@gaaclarke gaaclarke merged commit 50a8e73 into flutter:master Jun 28, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 28, 2019
@engine-flutter-autoroll
50a8e73 Has a binary messenger (flutter/engine#9419)
9a6e334
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 28, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 28, 2019
@engine-flutter-autoroll
50a8e73 Has a binary messenger (flutter/engine#9419)
ab3ed4b
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 28, 2019
Merged
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jun 28, 2019
@engine-flutter-autoroll
Roll engine 185087a..4aaa1a9 (8 commits) (#35251)
bbbd240 
flutter/engine@185087a...4aaa1a9

git log 185087a..4aaa1a9 --no-merges --oneline
4aaa1a9 Roll src/third_party/skia 842e92e29216..b851469b8e96 (6 commits) (flutter/engine#9568)
4e48fc4 Switched preprocessor logic for exporting symbols for testing. (NDEBUG (flutter/engine#9562)
50a8e73 Has a binary messenger (flutter/engine#9419)
7483665 Re-enable embedder_unittests. (flutter/engine#9482)
773cf53 Roll fuchsia/sdk/core/linux-amd64 from SuKWYMSXAMq1uTo9eaIdIOQqBb7Ro-zLyNk01GPOiU8C to N9HpdqEHzWZIcSV_3JN4PNlUNeoK-Pism-mc-a7L-IoC (flutter/engine#9560)
c2fa689 Roll src/third_party/skia d8f79a27b06b..842e92e29216 (108 commits) (flutter/engine#9558)
00c023b Roll fuchsia/sdk/core/mac-amd64 from dcGnduqJ5C8ozmUHeeboHC76nOv7s4XfCKIfefQlGkQC to 6WJ2NCb9uaOzVw20XC4kJqNcD4EP2VxT-PBocGsA6JsC (flutter/engine#9557)
7b9f59e Run benchmarks on try jobs. (flutter/engine#9493)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
@engine-flutter-autoroll @johnsonmh
Roll engine 185087a..4aaa1a9 (8 commits) (flutter#35251)
3c96bb1 
flutter/engine@185087a...4aaa1a9

git log 185087a..4aaa1a9 --no-merges --oneline
4aaa1a9 Roll src/third_party/skia 842e92e29216..b851469b8e96 (6 commits) (flutter/engine#9568)
4e48fc4 Switched preprocessor logic for exporting symbols for testing. (NDEBUG (flutter/engine#9562)
50a8e73 Has a binary messenger (flutter/engine#9419)
7483665 Re-enable embedder_unittests. (flutter/engine#9482)
773cf53 Roll fuchsia/sdk/core/linux-amd64 from SuKWYMSXAMq1uTo9eaIdIOQqBb7Ro-zLyNk01GPOiU8C to N9HpdqEHzWZIcSV_3JN4PNlUNeoK-Pism-mc-a7L-IoC (flutter/engine#9560)
c2fa689 Roll src/third_party/skia d8f79a27b06b..842e92e29216 (108 commits) (flutter/engine#9558)
00c023b Roll fuchsia/sdk/core/mac-amd64 from dcGnduqJ5C8ozmUHeeboHC76nOv7s4XfCKIfefQlGkQC to 6WJ2NCb9uaOzVw20XC4kJqNcD4EP2VxT-PBocGsA6JsC (flutter/engine#9557)
7b9f59e Run benchmarks on try jobs. (flutter/engine#9493)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff ([email protected]), and stop
the roller if necessary.
@gaaclarke gaaclarke mentioned this pull request Sep 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xster xster xster left review comments

@amirh amirh amirh approved these changes

@cbracken cbracken Awaiting requested review from cbracken

Assignees
No one assigned
Labels
cla: yes prod: API break
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gaaclarke @xster @amirh @googlebot