reland support uri launch in android #22363

chunhtai · 2020-11-06T21:17:55Z

Description

The previous PR is reverted due to it being a breaking change. This PR add a flag to turn on and off the new behavior.
To enable deeplinking, just add the following to the android manifest

            <meta-data android:name="flutter_handle_deeplinking" android:value="true" />
            <intent-filter>
              <action android:name="android.intent.action.VIEW" />
              <category android:name="android.intent.category.DEFAULT" />
              <category android:name="android.intent.category.BROWSABLE" />
              <data android:scheme="http" android:host="flutterbooksample.com" android:pathPrefix="/" />
            </intent-filter>

Related Issues

fixes flutter/flutter#69786
fixes flutter/flutter#21255

Tests

I added the following tests:

Replace this with a list of the tests that you added as part of this PR. A
change in behaviour with no test covering it will likely get reverted
accidentally sooner or later. PRs must include tests for all
changed/updated/fixed behaviors. See testing the engine for instructions on
writing and running engine tests.

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 C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

xster · 2020-11-09T17:09:42Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

-    if (host.getInitialRoute() != null) {
-      flutterEngine.getNavigationChannel().setInitialRoute(host.getInitialRoute());
+    if (initialRoute != null) {
+      flutterEngine.getNavigationChannel().setInitialRoute(initialRoute);


I think this has a new semantic now (of the default route being null whereas getInitialRoute used to be non-null). I think it's actually consequential downstream even though it looks like a platform channel

engine/shell/common/engine.cc

Line 322 in 5772dee

initial_route_ = std::move(route->value.GetString());

.

I did a null check before i hand it to the message channel, it will be not null after that point, no?

No not after the call. I meant before. host.getInitialRoute() previously never returns null as far as I can tell. Now there are opportunities to never call flutterEngine.getNavigationChannel().setInitialRoute

ah you are right, I didn't notice the previous if check does not actually do anything.

right. And it's consequential since initial route is "special" in that it plugs to a custom piece of memory in the engine and the dart:ui window API connects to that memory directly. Best not to mess with it since it's so entangled.

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

xster · 2020-11-09T17:14:14Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+     * NavigationChannel.pushRoute}.
+     */
+    @Nullable
+    boolean shouldHandleDeeplinking();


note the classes on flutteractivity/fragment are the user facing docs. This interface is package private.

xster · 2020-11-09T17:15:25Z

shell/platform/android/io/flutter/embedding/android/FlutterFragment.java

+   * See {@link NewEngineFragmentBuilder#shouldHandleDeeplinking()} and {@link
+   * CachedEngineFragmentBuilder#shouldHandleDeeplinking()}.
+   *
+   * <p>Used by this {@code FlutterFragment}'s {@link FlutterActivityAndFragmentDelegate}


don't link to package private classes from a public method's doc

xster · 2020-11-09T17:16:41Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

@@ -779,8 +780,11 @@ public String getDartEntrypointFunctionName() {
   * have control over the incoming {@code Intent}.
   *
   * <p>Subclasses may override this method to directly control the initial route.
+   *
+   * <p>If this method returns null and the {@code shouldHandleDeeplinking()} returns true, the
+   * {@link FlutterActivityAndFragmentDelegate} retrieves the initial route from the {@code Intent}


don't link to package private classes from public methods' docs. The fact that FlutterActivity delegates to a class is an implementation detail and not relevant to the activity's users.

just curious, is there a place like api.flutter.dev that shows these doc string?

not sure if I understood the question correctly. Do you mean these? https://api.flutter.dev/javadoc/io/flutter/embedding/android/FlutterActivity.html

yes, thanks for the link

xster · 2020-11-09T17:16:56Z

shell/platform/android/io/flutter/embedding/android/FlutterFragmentActivity.java

@@ -651,8 +667,11 @@ public String getDartEntrypointFunctionName() {
   * have control over the incoming {@code Intent}.
   *
   * <p>Subclasses may override this method to directly control the initial route.
+   *
+   * <p>If this method returns null and the {@code shouldHandleDeeplinking()} returns true, the
+   * {@link FlutterActivityAndFragmentDelegate} retrieves the initial route from the {@code Intent}


same. Don't mention the package private delegate.

xster · 2020-11-09T17:17:33Z

shell/platform/android/test/io/flutter/embedding/android/FlutterFragmentTest.java

@@ -49,6 +51,7 @@ public void itCreatesNewEngineFragmentWithRequestedSettings() {
    assertEquals("/custom/route", fragment.getInitialRoute());
    assertArrayEquals(new String[] {}, fragment.getFlutterShellArgs().toArray());
    assertFalse(fragment.shouldAttachEngineToActivity());
+    assertTrue(fragment.shouldHandleDeeplinking());
    assertNull(fragment.getCachedEngineId());
    assertTrue(fragment.shouldDestroyEngineWithHost());
    assertEquals(RenderMode.texture, fragment.getRenderMode());


FlutterActivityTest should have a test too

…" (flutter#22298)" This reverts commit f61cbc0.

chunhtai · 2020-11-10T18:15:43Z

shell/platform/android/io/flutter/embedding/android/FlutterActivityAndFragmentDelegate.java

+    if (initialRoute == null) {
+      initialRoute = maybeGetInitialRouteFromIntent(host.getActivity().getIntent());
+      if (initialRoute == null) {
+        initialRoute = DEFAULT_INITIAL_ROUTE;


I made it default to /, it matches the original behavior

chunhtai · 2020-11-10T18:16:32Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+
+  /** Retrieves the meta data specified in the AndroidManifest.xml. */
+  @Nullable
+  protected Bundle getMetaData() throws PackageManager.NameNotFoundException {


I refactored this bundle getter out so i can mock metadata during the test.

chunhtai · 2020-11-10T18:21:49Z

@xster Thanks for reviews! I addressed all comments. This is ready for another look.

xster · 2020-11-11T05:52:04Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

@@ -894,6 +890,25 @@ protected FlutterEngine getFlutterEngine() {
    return delegate.getFlutterEngine();
  }

+  private Bundle cachedMetaData;


sorry, I don't love this. Don't do any of this and just use https://javadoc.io/static/org.mockito/mockito-core/3.2.4/org/mockito/Mockito.html#spy-T-

xster · 2020-11-11T05:52:41Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+
+  /** Mocks the meta data for testing purposes. */
+  @VisibleForTesting
+  public void setMetaData(Bundle metaData) {


If your test is in the same package, you don't need public. Just void setMetaData which implies package private visibility.

But also, don't add this method :)

Just use https://javadoc.io/static/org.mockito/mockito-core/3.2.4/org/mockito/Mockito.html#spy-T- on getMetaData in tests (or just override it in a subclass for the class-under-test).

xster · 2020-11-11T05:54:57Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+
+  /** Retrieves the meta data specified in the AndroidManifest.xml. */
+  @Nullable
+  protected Bundle getMetaData() throws PackageManager.NameNotFoundException {


The throw here is a bit burdensome for the subclasser. I would just re-throw it in a RuntimeException since it should be very unusual.

xster · 2020-11-11T05:55:42Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

+   * Whether to handle the deeplinking from the {@code Intent} automatically if the {@code
+   * getInitialRoute} returns null.
+   *
+   * <p>The default implementation looks for the value of the key `flutter_handle_deeplinking` in


Be explicit about the metadata tag. Just saying key value in an AndroidManifest will be very hard to decipher.

xster · 2020-11-11T05:56:28Z

shell/platform/android/io/flutter/embedding/android/FlutterFragmentActivity.java

@@ -608,6 +628,25 @@ protected String getAppBundlePath() {
    return null;
  }

+  private Bundle cachedMetaData;


Same here. The testing infrastructure could be simpler.

xster · 2020-11-11T18:37:49Z

shell/platform/android/io/flutter/embedding/android/FlutterActivity.java

@@ -894,6 +890,18 @@ protected FlutterEngine getFlutterEngine() {
    return delegate.getFlutterEngine();
  }

+  /** Retrieves the meta data specified in the AndroidManifest.xml. */
+  @Nullable
+  protected Bundle getMetaData() throws RuntimeException {


Ah sorry my bad. I hadn't realized it wasn't public and just noticed now that you changed places where it's reading this and they're already catching PackageManager.NameNotFoundException. In that case, I think we can leave it as it was. With this method throwing PackageManager.NameNotFoundException(and in the fragment activity too) and letting places that call it catch PackageManager.NameNotFoundException and do a reasonable fallback. Sorry I misread.

* Revert "Revert "support uri intent launcher in android (flutter#21275)" (flutter#22298)" This reverts commit f61cbc0. * reland support uri launch for android * refactor * update * fix test * addressing comments * addressing comments * revert throw error

flutter-dashboard bot added the platform-android label Nov 6, 2020

google-cla bot added the cla: yes label Nov 6, 2020

chunhtai requested a review from xster November 7, 2020 01:00

xster reviewed Nov 9, 2020

View reviewed changes

xster mentioned this pull request Nov 9, 2020

Ensure Android architecture works with deep linking flutter/flutter#21255

Closed

chunhtai added 6 commits November 10, 2020 10:02

Revert "Revert "support uri intent launcher in android (flutter#21275)…

cda2c3b

…" (flutter#22298)" This reverts commit f61cbc0.

reland support uri launch for android

5fe12e9

refactor

bce3134

update

6b09ef5

fix test

fe25a89

addressing comments

bc1059c

chunhtai force-pushed the issues/69786-reland branch from d6f42dd to bc1059c Compare November 10, 2020 18:04

chunhtai commented Nov 10, 2020

View reviewed changes

xster reviewed Nov 11, 2020

View reviewed changes

addressing comments

33428d1

xster approved these changes Nov 11, 2020

View reviewed changes

revert throw error

9822d25

chunhtai merged commit d97a81c into flutter:master Nov 11, 2020

engine-flutter-autoroll mentioned this pull request Nov 12, 2020

Roll Engine from c5c7e83b9e53 to d97a81c88983 (6 revisions) flutter/flutter#70333

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 12, 2020

d97a81c reland support uri launch in android (flutter/engine#22363)

9782571

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to d07368169e3b (35 revisions) flutter/flutter#70417

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d97a81c reland support uri launch in android (flutter/engine#22363)

0774c42

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to 1ff237fdca45 (38 revisions) flutter/flutter#70425

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d97a81c reland support uri launch in android (flutter/engine#22363)

d3e6af1

engine-flutter-autoroll mentioned this pull request Nov 13, 2020

Roll Engine from c5c7e83b9e53 to dddb532b5c7b (40 revisions) flutter/flutter#70429

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 13, 2020

d97a81c reland support uri launch in android (flutter/engine#22363)

b4c518b

gaaclarke mentioned this pull request Nov 13, 2020

Dart version check tripping up on copyright notices with local engine flutter/flutter#70498

Closed

reland support uri launch in android #22363

reland support uri launch in android #22363

Uh oh!

Conversation

chunhtai commented Nov 6, 2020 • edited by xster Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

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!

xster Nov 10, 2020 • 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!

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!

xster Nov 9, 2020 • 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!

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!

chunhtai commented Nov 10, 2020

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!

chunhtai commented Nov 6, 2020 •

edited by xster

Loading

xster Nov 10, 2020 •

edited

Loading

xster Nov 9, 2020 •

edited

Loading