New issue
Advanced search Search tips

Issue 720077 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Make it possible for the WebApkService to be used by Chrome Canary and Chrome Dev simultaneously

Project Member Reported by pkotw...@chromium.org, May 9 2017

Issue description

Make it possible for the WebApkService to be used by Chrome Canary and Chrome Dev simultaneously

Tentatively assigning to Xi who is working on unbound WebAPK support
 
Repro steps:
1) Install Chrome Dev and Chrome Canary
2) Install WebAPK for tests.peter.sh/notification-generator/ from Chrome Canary
3) Type tests.peter.sh/notification-generator/ into the omnibox in Chrome Dev
4) Try to send a notification

Expected:
Notification is shown
Actual:
Notification is not shown
Sigh. This is cause we just check if there is a webapk, not whether it's bound to you
Labels: -Pri-3 Pri-2
This matters in a few places:
- whether we remote out renderers
- whether the app menu should say "Open"
- notifications
...

We should think this through and ensure there's a canonical way of doing this
Currently, when the service throws an exception in WebApkServiceImpl#onTransact() WebApkNotificationClient#onConnected() does not get an exception.

The exception seems to be sent to the client if the thrown exception is of type UnsupportedOperationException. I assume that any of the exception types listed in Parcel#writeException() would work.
Weird - I would've expected android to propagate it back, wrapped by a remote exception. Looks like we should switch to UnsupportedOperationException
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0301db54bc58b9d5968de88a939574b40c6e82a0

commit 0301db54bc58b9d5968de88a939574b40c6e82a0
Author: Xi Han <hanxi@google.com>
Date: Tue Jul 25 18:17:36 2017

Introduce WebAPK's Identity service.

This is the 1/3 patch of fixing bug "The WebApkService shouldn't be shared
between different browsers".

Currently, different Chrome channels can share the same WebAPK. However, only
the real runtime host can connect to the WebApkService successfully and delegate
task to the service.

Therefore, we introduce an Identity service in WebAPK that allows a browser to
query the WebAPK's runtime host to know whether it owns the WebAPK.

Bug:  720077 
Change-Id: Icd8f76f27f2ab62dfee00808da73f0fcd16e7f48
Reviewed-on: https://chromium-review.googlesource.com/583508
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489375}
[modify] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/libs/common/BUILD.gn
[add] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/identity_service/IIdentityService.aidl
[add] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/identity_service/OWNERS
[add] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/identity_service/common.aidl
[modify] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/shell_apk/AndroidManifest.xml
[modify] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/shell_apk/BUILD.gn
[modify] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/shell_apk/shell_apk_version.gni
[add] https://crrev.com/0301db54bc58b9d5968de88a939574b40c6e82a0/chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/IdentityService.java

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb0b8d2f550e397f6c609273d4e4ad9a9e440705

commit fb0b8d2f550e397f6c609273d4e4ad9a9e440705
Author: hanxi <hanxi@chromium.org>
Date: Tue Jul 25 21:54:11 2017

Refactor WebApkServiceConnectionManager.

This is the 2/3 patch of fixing bug "The WebApkService shouldn't be shared
between different browsers".

In this CL, we did the following things:
1) refactor WebApkServiceConnectionManager to connect to any kind of services.
2) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages
   communication between Chrome and WebAPK service/WebAPK Identity service respectively.

The refactoring of WebApkServiceConnectionManager also fixes notification issue
reported in  crbug.com/740614 .

BUG= 720077 , 740614 
TBR=peter@chromium.org

Review-Url: https://codereview.chromium.org/2974573002
Cr-Commit-Position: refs/heads/master@{#489454}

[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[rename] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/java_sources.gni
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/BUILD.gn
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/junit/DEPS
[add] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java
[add] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java
[modify] https://crrev.com/fb0b8d2f550e397f6c609273d4e4ad9a9e440705/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2875da73830e16380ee4c356f145026c0c65647d

commit 2875da73830e16380ee4c356f145026c0c65647d
Author: Xi Han <hanxi@google.com>
Date: Thu Jul 27 22:51:12 2017

Query WebAPK's Identity Service.

This is the 3/3 patch of fixing bug "The WebApkService shouldn't be shared
between different browsers". See the following CLs for reference.
- https://chromium-review.googlesource.com/c/583508 [1/3]
- https://codereview.chromium.org/2974573002/ [2/3]

In this CL, we add runtime host queries from WebAPKs' Identity services when
displaying notifications and launching a tab from service worker.

Usually binding to a service takes 15 - 20 milliseconds. Once bound, a call of
the its API takes up to 2 milliseconds. Because it is hard to predict when
Chrome may query a WebAPK's Identity service, so we keep the connections until
there isn't any visible activity of Chrome.

Bug:  720077 
Change-Id: Id488fffd708e4c7be7b50c1ff849d94d1288827f
Reviewed-on: https://chromium-review.googlesource.com/583438
Commit-Queue: Xi Han <hanxi@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490039}
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/browser/notifications/notification_platform_bridge_android.cc
[modify] https://crrev.com/2875da73830e16380ee4c356f145026c0c65647d/chrome/browser/notifications/notification_platform_bridge_android.h

Comment 10 by hanxi@chromium.org, Jul 31 2017

Labels: Merge-Request-61
Request merge on patches on comment #7, #8, #9.
We introduce an Identify service for WebAPKs. For the existing WebAPKs with browser-switch-logic introduced in CL(https://codereview.chromium.org/2858563004/), we can't know which browser hosts until they update to the version with this service. So we hope we can make it in M61. 
Project Member

Comment 11 by sheriffbot@chromium.org, Jul 31 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-61 Merge-Rejected-61
I'm sorry, but this looks like we're hooking up a large chunk of code (e.g. developing a feature) post-branch, which puts our release cycle at risk.  See go/chrome-merges for guidelines as to what is acceptable when.  Rejecting for M61, but ping me if this is going to cause us major issues and we can discuss.
Status: Fixed (was: Assigned)
Thanks, Alex. We'll proceed with m62 for this.

Sign in to add a comment