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.

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

Android embedding API updates for plugin ecosystem #13280

Conversation

matthew-carroll
Copy link
Contributor

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

  • 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
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
Copy link
Contributor

mklim commented Oct 24, 2019

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
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
Copy link
Contributor

@collinjackson collinjackson left a comment

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

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -223,7 +214,7 @@ private void setupFlutterEngine() {
// FlutterView.
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
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
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
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
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
Copy link

kroikie commented Oct 24, 2019

LGTM from FlutterFire end.

* Returns the {@link Lifecycle} associated with the attached {@code Activity}.
*/
@NonNull
Lifecycle getLifecycle();
Copy link
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
[email protected]:flutter/engine.git/compare/7a9c86b8d5e9...89731ae

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

git log 7a9c86b..89731ae --no-merges --oneline
2019-10-25 [email protected] Intercept SystemSound.play platform message before it's sent. (flutter/engine#13342)
2019-10-25 [email protected] [fuchsia] [packaging] Layout debug symbols for Fuchsia (flutter/engine#13338)
2019-10-25 [email protected] Roll fuchsia/sdk/core/linux-amd64 from Dr9GE... to fHxWy... (flutter/engine#13358)
2019-10-25 [email protected] Roll fuchsia/sdk/core/mac-amd64 from KC8wX... to Tbg2V... (flutter/engine#13357)
2019-10-25 [email protected] Bump dart/language_model to 9fJQZ0TrnAGQKrEtuL3-AXbUfPzYxqpN_OBHr9P4hE4C (flutter/engine#13337)
2019-10-25 [email protected] Roll src/third_party/skia a7f3157ac012..d0a404e84d47 (6 commits) (flutter/engine#13356)
2019-10-25 [email protected] Roll src/third_party/dart d576ce69e1..5ba6fb73ec (3 commits)
2019-10-25 [email protected] Create a separate directory for the intermediate outputs of each Fuchsia archive build action (flutter/engine#13341)
2019-10-25 [email protected] Fix the output filename of the Fuchsia archive build template (flutter/engine#13339)
2019-10-25 [email protected] Roll src/third_party/dart b42c2af535..d576ce69e1 (10 commits)
2019-10-25 [email protected] Roll src/third_party/skia 24a409611f24..a7f3157ac012 (1 commits) (flutter/engine#13353)
2019-10-25 [email protected] Roll src/third_party/skia 28a8f28b3eaf..24a409611f24 (2 commits) (flutter/engine#13352)
2019-10-25 [email protected] Roll fuchsia/sdk/core/linux-amd64 from 9m5ec... to Dr9GE... (flutter/engine#13351)
2019-10-25 [email protected] Roll fuchsia/sdk/core/mac-amd64 from yuA3r... to KC8wX... (flutter/engine#13350)
2019-10-25 [email protected] Roll src/third_party/skia 6f1c20f01fa9..28a8f28b3eaf (2 commits) (flutter/engine#13348)
2019-10-25 [email protected] Expose platform view ID on embedder semantics node (flutter/engine#13345)
2019-10-25 [email protected] Remove TODO on embedder a11y unit tests (flutter/engine#13346)
2019-10-25 [email protected] Roll src/third_party/dart 1bc9fba660..b42c2af535 (12 commits)
2019-10-25 [email protected] Android embedding API updates for plugin ecosystem - plugin facade, split Lifecycle, save state callbacks to plugins (flutter#43241, flutter#43242, flutter#43295) (flutter/engine#13280)
2019-10-24 [email protected] Roll src/third_party/skia 740f85949db2..6f1c20f01fa9 (16 commits) (flutter/engine#13343)
2019-10-24 [email protected] Roll src/third_party/dart 1bd6e20d76..1bc9fba660 (14 commits)
2019-10-24 [email protected] Roll src/third_party/skia 4ab4e641f151..740f85949db2 (12 commits) (flutter/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 [email protected] 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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants