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

Android embedding API updates for plugin ecosystem#13280

Merged
matthew-carroll merged 4 commits into
flutter-team-archive:masterfrom
matthew-carroll:plugin-binding-facade_lifecycle-split_save-state-callback
Oct 25, 2019
Merged

Android embedding API updates for plugin ecosystem#13280
matthew-carroll merged 4 commits into
flutter-team-archive:masterfrom
matthew-carroll:plugin-binding-facade_lifecycle-split_save-state-callback

Conversation

@matthew-carroll

@matthew-carroll matthew-carroll commented Oct 22, 2019

Copy link
Copy Markdown
Contributor
  • FlutterPluginBinding is a facade, getFlutterEngine() is deprecated

  • Lifecycle is split between ActivityAware and ServiceAware, FlutterEngine#getLifecycle() is deprecated

  • Activity instance state saving callbacks are forwarded to plugins

…gine, split Lifecycle across ActivityAware and ServiceAware and deprected getLifecycle().
@matthew-carroll

Copy link
Copy Markdown
Contributor Author

@amirh @mklim @cyanglaz @bparrishMines @collinjackson @kroikie Can you all please take a look at FlutterPluginBinding and let me know if you have any thoughts about additions/removals of accessors as a facade for plugins? I went with the few things that seemed obvious and necessary. We should try to reach consensus on that API surface ASAP. Thanks.

@mklim

mklim commented Oct 24, 2019

Copy link
Copy Markdown
Contributor

The facade API LG to me, I didn't notice anything missing. I could very likely be forgetting an edge case though.

@tvolkert

Copy link
Copy Markdown
Contributor

API LGTM. We can always add more to the surface if needed later.

@collinjackson collinjackson self-requested a review October 24, 2019 02:16

@collinjackson collinjackson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm from a testing and Flutterfire perspective.

@matthew-carroll

Copy link
Copy Markdown
Contributor Author

I added JVM tests to ensure that the new plugin facade references are not null when the binding is given to plugins. I also verified that state saving callbacks are forwarded to plugins that register for them.

@matthew-carroll matthew-carroll marked this pull request as ready for review October 24, 2019 04:41
@matthew-carroll

Copy link
Copy Markdown
Contributor Author

@mklim would you mind taking a look at this PR? I'd like to merge this in before beginning work on creating a custom Lifecycle interface.

@mklim mklim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Log.d(TAG, "No preferred FlutterEngine was provided. Creating a new FlutterEngine for"
+ " this FlutterFragment.");
flutterEngine = new FlutterEngine(host.getContext());
flutterEngine = new FlutterEngine(host.getContext(), host.getFlutterShellArgs().toArray());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

getFlutterShellArgs() is non-nullable, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep

cachedEngine.getPlugins().add(mockPlugin);

// Create a fake Host, which is required by the delegate.
FlutterActivityAndFragmentDelegate.Host mockHost = mock(FlutterActivityAndFragmentDelegate.Host.class);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

readability nit: It would probably be better to create a stub than a mock here. Subjective but stubs are generally easier to reason about than a series of when...thenReturn() statements.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. I switched out those mocks for fake implementations.

@kroikie

kroikie commented Oct 24, 2019

Copy link
Copy Markdown

LGTM from FlutterFire end.

* Returns the {@link Lifecycle} associated with the attached {@code Activity}.
*/
@NonNull
Lifecycle getLifecycle();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lifecycle should be fetched through the flutter_android_lifecycle plugin, and not available directly here

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 25, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Oct 26, 2019
git@github.com:flutter/engine.git/compare/7a9c86b8d5e9...89731ae

git log 7a9c86b..89731ae --no-merges --oneline
2019-10-25 ditman@gmail.com Intercept SystemSound.play platform message before it's sent. (flutter-team-archive/engine#13342)
2019-10-25 iska.kaushik@gmail.com [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutter-team-archive/engine#13338)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Dr9GE... to fHxWy... (flutter-team-archive/engine#13358)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from KC8wX... to Tbg2V... (flutter-team-archive/engine#13357)
2019-10-25 ariaye@google.com Bump dart/language_model to 9fJQZ0TrnAGQKrEtuL3-AXbUfPzYxqpN_OBHr9P4hE4C (flutter-team-archive/engine#13337)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia a7f3157ac012..d0a404e84d47 (6 commits) (flutter-team-archive/engine#13356)
2019-10-25 bkonyi@google.com Roll src/third_party/dart d576ce69e1..5ba6fb73ec (3 commits)
2019-10-25 jason-simmons@users.noreply.github.com Create a separate directory for the intermediate outputs of each Fuchsia archive build action (flutter-team-archive/engine#13341)
2019-10-25 jason-simmons@users.noreply.github.com Fix the output filename of the Fuchsia archive build template (flutter-team-archive/engine#13339)
2019-10-25 bkonyi@google.com Roll src/third_party/dart b42c2af535..d576ce69e1 (10 commits)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 24a409611f24..a7f3157ac012 (1 commits) (flutter-team-archive/engine#13353)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28a8f28b3eaf..24a409611f24 (2 commits) (flutter-team-archive/engine#13352)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 9m5ec... to Dr9GE... (flutter-team-archive/engine#13351)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from yuA3r... to KC8wX... (flutter-team-archive/engine#13350)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6f1c20f01fa9..28a8f28b3eaf (2 commits) (flutter-team-archive/engine#13348)
2019-10-25 chris@bracken.jp Expose platform view ID on embedder semantics node (flutter-team-archive/engine#13345)
2019-10-25 chris@bracken.jp Remove TODO on embedder a11y unit tests (flutter-team-archive/engine#13346)
2019-10-25 bkonyi@google.com Roll src/third_party/dart 1bc9fba660..b42c2af535 (12 commits)
2019-10-25 matthew-carroll@users.noreply.github.com Android embedding API updates for plugin ecosystem - plugin facade, split Lifecycle, save state callbacks to plugins (#43241, #43242, #43295) (flutter-team-archive/engine#13280)
2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 740f85949db2..6f1c20f01fa9 (16 commits) (flutter-team-archive/engine#13343)
2019-10-24 bkonyi@google.com Roll src/third_party/dart 1bd6e20d76..1bc9fba660 (14 commits)
2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4ab4e641f151..740f85949db2 (12 commits) (flutter-team-archive/engine#13336)


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 on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
git@github.com:flutter/engine.git/compare/7a9c86b8d5e9...89731ae

git log 7a9c86b..89731ae --no-merges --oneline
2019-10-25 ditman@gmail.com Intercept SystemSound.play platform message before it's sent. (flutter-team-archive/engine#13342)
2019-10-25 iska.kaushik@gmail.com [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutter-team-archive/engine#13338)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Dr9GE... to fHxWy... (flutter-team-archive/engine#13358)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from KC8wX... to Tbg2V... (flutter-team-archive/engine#13357)
2019-10-25 ariaye@google.com Bump dart/language_model to 9fJQZ0TrnAGQKrEtuL3-AXbUfPzYxqpN_OBHr9P4hE4C (flutter-team-archive/engine#13337)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia a7f3157ac012..d0a404e84d47 (6 commits) (flutter-team-archive/engine#13356)
2019-10-25 bkonyi@google.com Roll src/third_party/dart d576ce69e1..5ba6fb73ec (3 commits)
2019-10-25 jason-simmons@users.noreply.github.com Create a separate directory for the intermediate outputs of each Fuchsia archive build action (flutter-team-archive/engine#13341)
2019-10-25 jason-simmons@users.noreply.github.com Fix the output filename of the Fuchsia archive build template (flutter-team-archive/engine#13339)
2019-10-25 bkonyi@google.com Roll src/third_party/dart b42c2af535..d576ce69e1 (10 commits)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 24a409611f24..a7f3157ac012 (1 commits) (flutter-team-archive/engine#13353)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 28a8f28b3eaf..24a409611f24 (2 commits) (flutter-team-archive/engine#13352)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from 9m5ec... to Dr9GE... (flutter-team-archive/engine#13351)
2019-10-25 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from yuA3r... to KC8wX... (flutter-team-archive/engine#13350)
2019-10-25 skia-flutter-autoroll@skia.org Roll src/third_party/skia 6f1c20f01fa9..28a8f28b3eaf (2 commits) (flutter-team-archive/engine#13348)
2019-10-25 chris@bracken.jp Expose platform view ID on embedder semantics node (flutter-team-archive/engine#13345)
2019-10-25 chris@bracken.jp Remove TODO on embedder a11y unit tests (flutter-team-archive/engine#13346)
2019-10-25 bkonyi@google.com Roll src/third_party/dart 1bc9fba660..b42c2af535 (12 commits)
2019-10-25 matthew-carroll@users.noreply.github.com Android embedding API updates for plugin ecosystem - plugin facade, split Lifecycle, save state callbacks to plugins (flutter#43241, flutter#43242, flutter#43295) (flutter-team-archive/engine#13280)
2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 740f85949db2..6f1c20f01fa9 (16 commits) (flutter-team-archive/engine#13343)
2019-10-24 bkonyi@google.com Roll src/third_party/dart 1bd6e20d76..1bc9fba660 (14 commits)
2019-10-24 skia-flutter-autoroll@skia.org Roll src/third_party/skia 4ab4e641f151..740f85949db2 (12 commits) (flutter-team-archive/engine#13336)


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 on the revert to ensure that a human
is aware of the problem.

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/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

7 participants