Makes android embedding to send full uri #41836

chunhtai · 2023-05-08T23:37:59Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

chunhtai · 2023-05-09T19:14:39Z

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

@@ -838,7 +839,7 @@ public void itDoesNotSendPushRouteInformationMessageWhenOnNewIntentIsNonHierarch
    delegate.onNewIntent(mockIntent);

    // Verify that the navigation channel was not given a push route message.
-    verify(mockFlutterEngine.getNavigationChannel(), times(0))
+    verify(mockFlutterEngine.getNavigationChannel(), times(1))


This test was added #29766

Which seems like a bug to me, it could have send the non hierarchical uri.

The linked patch was trying to avoid a crash. This assertion is probably unnecessary.

However, we should update the comment and have some kind of assertion about what arguments actually got sent through.

reidbaker

Not sure this pr should land. I am not marking it request changes so that i am not blocking the pr. Deferring to @dnfield and @camsim99 for now.

reidbaker · 2023-05-17T19:46:26Z

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

-          }
-          return fullRoute;
-        }
+        return data.toString();


In general you should not rely on the behavior of tostring in production code. https://www.bekk.christmas/post/2019/4/never-use-tostring()-for-behaviour

I can't find a better way to convert uri to a serializable representation. all the suggestions i found is to use to string

I think in this case it's probably ok, since Uri provides both a toString and a parse method that knows how to deserialize from the string produced by toString...

reidbaker · 2023-05-17T19:46:54Z

shell/platform/android/io/flutter/view/FlutterView.java

@@ -360,10 +360,6 @@ public void setInitialRoute(String route) {
    navigationChannel.setInitialRoute(route);
  }

-  public void pushRoute(String route) {


Is this a breaking change?

I am not sure who is using this API, and if there is a way to access this API at all. I can restore it if this is needed though

This is the old embedder v1 API. I think there were ongoing efforts to remove it but the person(s) working on that are no longer on the team.

I would lean towards restoring this. Anyone using the v1 API should migrate to the v2 API, and our test coverage on v1 is pretty poor - there's no need to make it harder than it already is to use v1 API though, until we just outright remove it.

If it's possible, you should put this back, Otherwise LGTM

dnfield · 2023-05-17T20:41:28Z

...atform/android/test/io/flutter/embedding/android/FlutterActivityAndFragmentDelegateTest.java

@@ -308,7 +308,7 @@ public void flutterEngineGroupGetsInitialRouteFromIntent() {
    ArgumentCaptor<FlutterEngineGroup.Options> optionsCaptor =
        ArgumentCaptor.forClass(FlutterEngineGroup.Options.class);
    verify(flutterEngineGroup, times(1)).createAndRunEngine(optionsCaptor.capture());
-    assertEquals("/initial_route", optionsCaptor.getValue().getInitialRoute());
+    assertEquals("foo://example.com/initial_route", optionsCaptor.getValue().getInitialRoute());


When these strings to go the framework, are they handled as dart Uri objects? If so this is probably fine, but if not won't this cause breakages in framework apps?

Even if so, won't this cause breakages to people using toString on the Uri?

yes the framework is treated it as uri object after flutter/flutter#119968

Even if so, won't this cause breakages to people using toString on the Uri?

I am not sure if i understand, can you add more details?

Ahh I see now, thanks for the link ot the framework PR.

dnfield

LGTM modulo avoiding the change to the v1 embedding.

)

…127156) flutter/engine@9039c2d...bca11a4 2023-05-19 [email protected] [web:a11y] support dialogs described by descendants (flutter/engine#42108) 2023-05-18 [email protected] Makes android embedding to send full uri (flutter/engine#41836) 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],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose 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/+doc/main/autoroll/README.md

…lutter#127156) flutter/engine@9039c2d...bca11a4 2023-05-19 [email protected] [web:a11y] support dialogs described by descendants (flutter/engine#42108) 2023-05-18 [email protected] Makes android embedding to send full uri (flutter/engine#41836) 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],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose 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/+doc/main/autoroll/README.md

flutter-dashboard bot added the platform-android label May 8, 2023

chunhtai mentioned this pull request May 8, 2023

Makes android embedding to send full uri #41778

Closed

8 tasks

chunhtai commented May 9, 2023

View reviewed changes

chunhtai requested a review from reidbaker May 9, 2023 19:14

chunhtai marked this pull request as ready for review May 9, 2023 19:15

reidbaker reviewed May 17, 2023

View reviewed changes

reidbaker requested review from dnfield and camsim99 May 17, 2023 19:48

dnfield reviewed May 17, 2023

View reviewed changes

dnfield approved these changes May 18, 2023

View reviewed changes

chunhtai added 3 commits May 18, 2023 16:03

Makes android embedding to send full uri

7d2d514

fix test

480a119

update

67be28d

chunhtai force-pushed the issues/100624-android-2 branch from c21adda to 67be28d Compare May 18, 2023 23:03

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label May 18, 2023

auto-submit bot merged commit be57de6 into flutter:main May 18, 2023

engine-flutter-autoroll mentioned this pull request May 19, 2023

Roll Flutter Engine from 9039c2dfb74c to bca11a423f9c (2 revisions) flutter/flutter#127156

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 19, 2023

be57de6de Makes android embedding to send full uri (flutter/engine#41836

fda3fdf

)

Makes android embedding to send full uri #41836

Makes android embedding to send full uri #41836

Uh oh!

Conversation

chunhtai commented May 8, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidbaker left a comment

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!

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!