Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968623002:
[WebApk] Fix broken launch when matching an app with verified intent filters. (Closed)
Description[WebApk] Fix broken launch when matching an app with verified intent filters.
The issue was that WebappLauncherActivity queries package manager to see
if "a valid webapk can handle this url" and that "the webapk's package
matches the webapk package I'm going to load". The problem is when a
WebApk's intent scope collides with a native app that has a verified
intent filter, the first part of the query only returns the native app's
package.
This is fixed by forcing intent resolution with the WebApk package name
of the WebApk for which we're trying to render a page and then still
checking that the WebApk is valid.
Note: we still fully respect verified intent filters and does nothing
to change intent dispatch when clicking on a link that could match a
native app with verified intent filters. This simply means that if the
WebApk is launched it'll work and changing intent dispatch is a separate
product question.
BUG=738262
Review-Url: https://codereview.chromium.org/2968623002
Cr-Commit-Position: refs/heads/master@{#483744}
Committed: https://chromium.googlesource.com/chromium/src/+/9f381948a4babe74c82462b7d41f79c370bf09ac
Patch Set 1 #
Total comments: 20
Patch Set 2 : [WebApk] Fix broken launch when matching an app with verified intent filters. #
Depends on Patchset: Messages
Total messages: 19 (11 generated)
Description was changed from ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "that webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Arguably we could remove the check that the url matches the WebApk but it doesn't seem too onerous. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 ========== to ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "the webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Arguably we could remove the check that the url matches the WebApk but it doesn't seem too onerous. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 ==========
Description was changed from ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "the webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Arguably we could remove the check that the url matches the WebApk but it doesn't seem too onerous. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 ========== to ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "the webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 ==========
yfriedman@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:187: * Tests {@link WebApkValidator.webApkCanHandleUrl()} returns null for a non-browsable Intent. s/null/false/ https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:207: * Tests {@link WebApkValidator.webApkCanHandleUrl()} returns false if no WebAPK handles the s/no WebAPK handles/the specified WebApk does not handle/
https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:102: System.out.println("failed to verify"); will remove :/
The CQ bit was checked by yfriedman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % your nits :)
LGTM with nits https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:167: */ s/if it matches a WebAPK but isn't properly signed/the given APK package name is not signed with the WebAPK signature/ https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:188: */ s/non-browsable Intent/non-browsable WebAPK/ https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:240: WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); Can you please change this test to test WebApkValidator#isValidWebApk() (That's the function that is *really* being tested) https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:254: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); Can you please change this test to test WebApkValidator#isValidWebApk() https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:270: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); Can you please change this test to test WebApkValidator#isValidWebApk() https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:286: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); Can you please change this test to test WebApkValidator#isValidWebApk() https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:95: public static boolean webApkCanHandleUrl(Context context, String webApkPackage, String url) { Nit: Rename this function to canWebApkHandleUrl() to make it clearer that this function returns a boolean
thanks https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:167: */ On 2017/06/30 15:59:12, pkotwicz wrote: > s/if it matches a WebAPK but isn't properly signed/the given APK package name is > not signed with the WebAPK signature/ Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:187: * Tests {@link WebApkValidator.webApkCanHandleUrl()} returns null for a non-browsable Intent. On 2017/06/30 00:59:55, Yaron wrote: > s/null/false/ Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:188: */ On 2017/06/30 15:59:12, pkotwicz wrote: > s/non-browsable Intent/non-browsable WebAPK/ Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:207: * Tests {@link WebApkValidator.webApkCanHandleUrl()} returns false if no WebAPK handles the On 2017/06/30 00:59:55, Yaron wrote: > s/no WebAPK handles/the specified WebApk does not handle/ Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:240: WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); On 2017/06/30 15:59:12, pkotwicz wrote: > Can you please change this test to test WebApkValidator#isValidWebApk() (That's > the function that is *really* being tested) Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:254: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); On 2017/06/30 15:59:12, pkotwicz wrote: > Can you please change this test to test WebApkValidator#isValidWebApk() Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:270: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); On 2017/06/30 15:59:12, pkotwicz wrote: > Can you please change this test to test WebApkValidator#isValidWebApk() Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkValidatorTest.java:286: assertNull(WebApkValidator.findWebApkPackage(RuntimeEnvironment.application, infos)); On 2017/06/30 15:59:12, pkotwicz wrote: > Can you please change this test to test WebApkValidator#isValidWebApk() Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:95: public static boolean webApkCanHandleUrl(Context context, String webApkPackage, String url) { On 2017/06/30 15:59:12, pkotwicz wrote: > Nit: Rename this function to canWebApkHandleUrl() to make it clearer that this > function returns a boolean Done. https://codereview.chromium.org/2968623002/diff/1/chrome/android/webapk/libs/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:102: System.out.println("failed to verify"); On 2017/06/30 01:00:55, Yaron wrote: > will remove :/ Done.
The CQ bit was checked by yfriedman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2968623002/#ps20001 (title: "[WebApk] Fix broken launch when matching an app with verified intent filters.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498841558809780,
"parent_rev": "9c70aaafd8d1ed5729a7ed5f95d8eb3aed642931", "commit_rev":
"9f381948a4babe74c82462b7d41f79c370bf09ac"}
Message was sent while issue was closed.
Description was changed from ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "the webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 ========== to ========== [WebApk] Fix broken launch when matching an app with verified intent filters. The issue was that WebappLauncherActivity queries package manager to see if "a valid webapk can handle this url" and that "the webapk's package matches the webapk package I'm going to load". The problem is when a WebApk's intent scope collides with a native app that has a verified intent filter, the first part of the query only returns the native app's package. This is fixed by forcing intent resolution with the WebApk package name of the WebApk for which we're trying to render a page and then still checking that the WebApk is valid. Note: we still fully respect verified intent filters and does nothing to change intent dispatch when clicking on a link that could match a native app with verified intent filters. This simply means that if the WebApk is launched it'll work and changing intent dispatch is a separate product question. BUG=738262 Review-Url: https://codereview.chromium.org/2968623002 Cr-Commit-Position: refs/heads/master@{#483744} Committed: https://chromium.googlesource.com/chromium/src/+/9f381948a4babe74c82462b7d41f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/9f381948a4babe74c82462b7d41f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
