|
|
DescriptionLaunch WebAPK if notification opens URL within WebAPK scope
BUG=609122
Committed: https://crrev.com/8c10ae7eb7ac553497d8c24852475b17dd1ffa51
Cr-Commit-Position: refs/heads/master@{#397853}
Patch Set 1 : #
Total comments: 5
Patch Set 2 : #
Messages
Total messages: 24 (9 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
pkotwicz@chromium.org changed reviewers: + hanxi@chromium.org
Xi can you please take a look?
lgtm
pkotwicz@chromium.org changed reviewers: + peter@chromium.org
Peter Beverloo can you please take a look?
Peter Beverloo Ping!
lgtm https://codereview.chromium.org/2010633004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeServiceTabLauncher.java (right): https://codereview.chromium.org/2010633004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeServiceTabLauncher.java:52: NavigationClient.launchWebApk(context, webApkPackageName, url); This is the code path where we launch a *new* tab, not where we focus an existing tab. Will you update the focusing code independently? You could test the focus scenario here: (Make sure you don't kill the app.) https://tests.peter.sh/notification-generator/#action=1
pkotwicz@chromium.org changed reviewers: + rsesek@chromium.org
rsesek@ Can you take a look at this CL from a security perspective? Do we need to check whether a WebAPK is a valid WebAPK prior to launching it from a notification? (Currently this CL does not call WebApkValidator#isValidWebApk()) Peter Beverloo, I have filed a bug for writing a WebAPK version of FullScreenTabWebContentsDelegateAndroid#activateContents() at http://crbug.com/615279
On 2016/05/31 19:09:59, pkotwicz wrote: > rsesek@ Can you take a look at this CL from a security perspective? Do we need > to check whether a WebAPK is a valid WebAPK prior to launching it from a > notification? (Currently this CL does not call WebApkValidator#isValidWebApk()) I think the risk from launching is fairly low. The WebAPK is validated when the service connection for sending notifications/receiving callbacks is bound, right?
pkotwicz@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@ for chrome/android/java OWNERS rsesek@: Yes, the WebAPK is validated when the service connection for sending notifications/receiving callbacks is bound.
https://codereview.chromium.org/2010633004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeServiceTabLauncher.java (right): https://codereview.chromium.org/2010633004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeServiceTabLauncher.java:55: } 1) newline after the brace 2) Don't call them "classic webapps". Say that it'll launch a WebappActivity. https://codereview.chromium.org/2010633004/diff/40001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/NavigationClient.java (right): https://codereview.chromium.org/2010633004/diff/40001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/NavigationClient.java:19: * @param context Fill in the parameters. https://codereview.chromium.org/2010633004/diff/40001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/NavigationClient.java:32: new ComponentName(webApkPackageName, WebApkConstants.WEBAPK_MAIN_ACTIVITY)); Don't add a constant because that can change if the class moves. Use MainActivity.class or MainActivity.class.getName() like we use everywhere else.
On 2016/06/01 23:41:39, pkotwicz wrote: > rsesek@: Yes, the WebAPK is validated when the service connection for sending > notifications/receiving callbacks is bound. LGTM from security
dfalcantara@ can you please take another look? https://codereview.chromium.org/2010633004/diff/40001/chrome/android/webapk/l... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/NavigationClient.java (right): https://codereview.chromium.org/2010633004/diff/40001/chrome/android/webapk/l... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/NavigationClient.java:32: new ComponentName(webApkPackageName, WebApkConstants.WEBAPK_MAIN_ACTIVITY)); I now only set the package name on the intent which is sufficient. I did not use MainClass.class.getName() as you suggested because we do not want chrome_apk to depend on chrome/android/webapk/shell_apk
lgtm
The CQ bit was checked by pkotwicz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, rsesek@chromium.org, hanxi@chromium.org Link to the patchset: https://codereview.chromium.org/2010633004/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2010633004/60001
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Launch WebAPK if notification opens URL within WebAPK scope BUG=609122 ========== to ========== Launch WebAPK if notification opens URL within WebAPK scope BUG=609122 Committed: https://crrev.com/8c10ae7eb7ac553497d8c24852475b17dd1ffa51 Cr-Commit-Position: refs/heads/master@{#397853} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8c10ae7eb7ac553497d8c24852475b17dd1ffa51 Cr-Commit-Position: refs/heads/master@{#397853} |