Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2974573002:
Refactor WebApkServiceConnectionManager. (Closed)
DescriptionRefactor 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}
Committed: https://chromium.googlesource.com/chromium/src/+/fb0b8d2f550e397f6c609273d4e4ad9a9e440705
Patch Set 1 #
Total comments: 36
Patch Set 2 : Rebase #
Total comments: 8
Patch Set 3 : Refactoring. #
Total comments: 63
Patch Set 4 #Patch Set 5 : Fix the state changes. #Patch Set 6 : Check bindService()'s return value. #
Total comments: 14
Patch Set 7 : Call the callback on UI thread if bindService() fails. #
Total comments: 35
Patch Set 8 : yfriedman@'s comments. #
Total comments: 19
Patch Set 9 : Clean up #Patch Set 10 : WebApkServiceConnectionManager is no longer a base class. #Patch Set 11 : Clean up. #
Total comments: 7
Patch Set 12 : yfriedman@'s comments. #
Total comments: 52
Patch Set 13 : Try to fix the illegal including in the tests. #
Total comments: 8
Patch Set 14 : pkotwicz@‘s comments. #
Total comments: 20
Patch Set 15 : Update tests and nits. #Patch Set 16 : Rebase. #
Total comments: 14
Patch Set 17 : Split CL as 2/3. #
Total comments: 18
Patch Set 18 : Nits. #Patch Set 19 : Nits. #
Total comments: 2
Patch Set 20 : yfriedman@'s comments. #Messages
Total messages: 108 (72 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== 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 sucessfully and delegate task to the service. In this CL, we add an Identity Service in WebAPK that brwoser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). Currently, different Chrome channels can share the same WebAPK. However, only the real runtime host can connect to the WebApkService sucessfully and delegate task to the service. In this CL, we add an Identity Service in WebAPK that brwoser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. BUG=720077 ==========
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org, yfriedman@chromium.org
Hi Yaron and Peter, We need the Identity Service for different Chrome channels, even if we don't support other browsers. It is better to implement soon than later, since we have no accurate way to know the runtime host for WebAPKs with shell version between 6 and 14(in this CL), but predict which could be wrong. Could you please take a look? Thanks!
https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:83: if (ChromeWebApkHost.isEnabled()) { you need to rebase - this is always true now https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:86: if (webApkPackageName != null) { these else case for this is unhandled. I'd also continue trying to not nest conditions, so so something like: if (webApkPackageName == null) { launchTab; return; } ... https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:106: WebApkValidator.queryWebApkRuntimeHostAsync(webApkPackageName, callback); are we able to do this in all cases where we'd want to use it? or will it require significant refactoring to make it async? https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:87: private String mWebApkPackage; nit: make the local "final" instead. In general, I don't think we should use member variables for transient/temporary operatoins. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:177: if (!TextUtils.equals(runtimeHost, shouldn't need textutils. Just do ContextUtils.getApplicationContext().getPackageName().eqaula(runtimeHost) If our applciation context's package name is null, we're in a bad state https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:24: public class WebApkIdentityServiceConnectionManager { shoudl we just merge this into WebApkServiceConnectionManager? The simplest way would be to use generics, and have one version of this for the chidl service and another for the identity service. Alternatively, you could just have two types for connections and one connection manager. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:98: private static final String TAG = "cr_WebApk"; different tag https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:164: Context context = ContextUtils.getApplicationContext(); is this the right place put this? I would think that you fail to connect from WebApkIdentityServiceConnectionManager.connect and its currently unhandled. How do we get here if there's no identity service? https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:180: WebApkIdentityServiceConnectionManager.getInstance().connect( this is never disconnected https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="android.intent.category.WEBAPK_IDENTITY_SERVICE_API" /> org.chromium.WEBAPK_IDENTITY_SERVICE_API?
Your CL looks mostly good. My main suggestion is putting more of the logic into WebApkIdentityServiceConnectionManager.java https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:161: if (!ChromeWebApkHost.isEnabled()) { ChromeWebApkHost#isEnabled() no longer exists :) https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:73: notifyAndClear(); I don't think that onServiceDisconnected() will be called if onServiceConnected() has not been previously called https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:74: mConnectionManager.mConnections.remove(name.getPackageName()); Can we get rid of the ConnectionState.DISCONNECTED state? https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:87: if (mCallbacks.isEmpty()) return; Nit: The early return is unnecessary given the current implementation https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:120: final ConnectionCallback callback) { The WebApkIdentityService currently only has a single method. Can this method take a Callback<Boolean> with the boolean argument indicating whether the runtime host matches or not Maybe this method can be renamed to checkWebApkRuntimeHost() https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:153: .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); Is the whitespace correct? https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:156: WebApkIdentityServiceConnectionManager() {} Did you mean to make the constructor private? https://codereview.chromium.org/2974573002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:237: scope_url = origin_url; Nit: Can you remove |origin_url| and initialize |scope_url| with notification.origin_url().GetOrigin() https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:80: Can you move the code on lines 85-104 into a separate function called tryLaunchWebApk() https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:100: extraHeaders, postData); We should launch a tab in this case, not a webapp (There might be an entry in WebappDataStorage from when the WebAPK used to use this browser as the host) https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:111: private static void launchTab(int requestId, boolean incognito, final String url, Maybe rename this function to launchTabOrWebapp() https://codereview.chromium.org/2974573002/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkHelper.java (right): https://codereview.chromium.org/2974573002/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkHelper.java:9: import android.os.Bundle; We should not put anything new in webapk/lib/common versioning things in webapk/lib/common is hard to think about (Chrome and the ShellApk may each have a different version of WebApkHelper.java)
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #3 (id:200001) has been deleted
The CQ bit was checked by hanxi@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...
Hi Yaron and Peter, I change the WebApkServiceConnectionManager as a base class, and introduce two subclass: WebApkServiceClient and WebApkIdentityServiceClients that bind to WebAPK service and the identity service respectively. PTAL, thanks! https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:83: if (ChromeWebApkHost.isEnabled()) { On 2017/07/10 18:09:07, Yaron wrote: > you need to rebase - this is always true now Done. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:86: if (webApkPackageName != null) { On 2017/07/10 18:09:07, Yaron wrote: > these else case for this is unhandled. I'd also continue trying to not nest > conditions, so so something like: > > if (webApkPackageName == null) { > launchTab; > return; > } > ... Done. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:106: WebApkValidator.queryWebApkRuntimeHostAsync(webApkPackageName, callback); On 2017/07/10 18:09:07, Yaron wrote: > are we able to do this in all cases where we'd want to use it? or will it > require significant refactoring to make it async? I think the navigation one requires significant refactoring, which is unfortunate:( https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:87: private String mWebApkPackage; On 2017/07/10 18:09:07, Yaron wrote: > nit: make the local "final" instead. In general, I don't think we should use > member variables for transient/temporary operatoins. Good catch! Figure out a way to get rid of this member variable. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:161: if (!ChromeWebApkHost.isEnabled()) { On 2017/07/10 19:06:05, pkotwicz wrote: > ChromeWebApkHost#isEnabled() no longer exists :) Rebased:) https://codereview.chromium.org/2974573002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:177: if (!TextUtils.equals(runtimeHost, On 2017/07/10 18:09:07, Yaron wrote: > shouldn't need textutils. Just do > ContextUtils.getApplicationContext().getPackageName().eqaula(runtimeHost) > > If our applciation context's package name is null, we're in a bad state If the package name is null, won't |ContextUtils.getApplicationContext().getPackageName()| throw no pointer exception? I thought TextUtils.equals() is safer than comparing the two strings directly. Anyway, updated here to avoid a null package name. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:24: public class WebApkIdentityServiceConnectionManager { On 2017/07/10 18:09:07, Yaron wrote: > shoudl we just merge this into WebApkServiceConnectionManager? The simplest way > would be to use generics, and have one version of this for the chidl service and > another for the identity service. Alternatively, you could just have two types > for connections and one connection manager. I would make WebApkServiceConnectionManager be a base class which manages the connections. Besides, introduce WebApkServiceClient and WebApkIdentityServiceClient that are used to connect to two services. Each of the clients are singleton. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:73: notifyAndClear(); On 2017/07/10 19:06:06, pkotwicz wrote: > I don't think that onServiceDisconnected() will be called if > onServiceConnected() has not been previously called The goal here is to remove the "unconnected" connection from the cache, and notify users. Consider the situation that: 1) Chrome bind to an unbounded WebAPK (which hasn't choose a runtime host). The IdentityService will return null. 2) go to app setting, force stop the WebAPK (kill the identity service). 3) re-launch the WebAPK and choose Chrome as runtime host. Next time Chrome connects to the WebAPK's identity service, it will find the existing connection and calls identityService.getRuntimeHostBrowserPackageName() and get a remoteException. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:74: mConnectionManager.mConnections.remove(name.getPackageName()); On 2017/07/10 19:06:05, pkotwicz wrote: > Can we get rid of the ConnectionState.DISCONNECTED state? Done. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:87: if (mCallbacks.isEmpty()) return; On 2017/07/10 19:06:06, pkotwicz wrote: > Nit: The early return is unnecessary given the current implementation Removed. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:98: private static final String TAG = "cr_WebApk"; On 2017/07/10 18:09:07, Yaron wrote: > different tag Done. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:120: final ConnectionCallback callback) { On 2017/07/10 19:06:06, pkotwicz wrote: > The WebApkIdentityService currently only has a single method. Can this method > take a Callback<Boolean> with the boolean argument indicating whether the > runtime host matches or not > > Maybe this method can be renamed to checkWebApkRuntimeHost() Sounds good! Update the callback, and move the logic to WebApkIdentityServiceClient. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:153: .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2017/07/10 19:06:06, pkotwicz wrote: > Is the whitespace correct? "git cl format" is always suggested this. Removed. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java:156: WebApkIdentityServiceConnectionManager() {} On 2017/07/10 19:06:06, pkotwicz wrote: > Did you mean to make the constructor private? Done. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:164: Context context = ContextUtils.getApplicationContext(); On 2017/07/10 18:09:07, Yaron wrote: > is this the right place put this? I would think that you fail to connect from > WebApkIdentityServiceConnectionManager.connect and its currently unhandled. How > do we get here if there's no identity service? Thanks for catching this! If there is no identity service, it should be handled in line 145. https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:180: WebApkIdentityServiceConnectionManager.getInstance().connect( On 2017/07/10 18:09:07, Yaron wrote: > this is never disconnected I assume the disconnect is called after chrome stops. However, I didn't find a good place to call the disconnect(). Even with explicitly called, I did see the IdentityService#unbind() was called when Chrome is closed. Is there any concern of not calling the disconnect()? https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="android.intent.category.WEBAPK_IDENTITY_SERVICE_API" /> On 2017/07/10 18:09:07, Yaron wrote: > org.chromium.WEBAPK_IDENTITY_SERVICE_API? org.webapk.IDENTITY_SERVICE_API? https://codereview.chromium.org/2974573002/diff/100001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2974573002/diff/100001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:237: scope_url = origin_url; On 2017/07/10 19:06:06, pkotwicz wrote: > Nit: Can you remove |origin_url| and initialize |scope_url| with > > notification.origin_url().GetOrigin() Done. https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:80: On 2017/07/10 19:06:06, pkotwicz wrote: > Can you move the code on lines 85-104 into a separate function called > tryLaunchWebApk() Done. https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:100: extraHeaders, postData); On 2017/07/10 19:06:06, pkotwicz wrote: > We should launch a tab in this case, not a webapp (There might be an entry in > WebappDataStorage from when the WebAPK used to use this browser as the host) Done. https://codereview.chromium.org/2974573002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:111: private static void launchTab(int requestId, boolean incognito, final String url, On 2017/07/10 19:06:06, pkotwicz wrote: > Maybe rename this function to launchTabOrWebapp() Done. https://codereview.chromium.org/2974573002/diff/120001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkHelper.java (right): https://codereview.chromium.org/2974573002/diff/120001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkHelper.java:9: import android.os.Bundle; On 2017/07/10 19:06:06, pkotwicz wrote: > We should not put anything new in webapk/lib/common versioning things in > webapk/lib/common is hard to think about (Chrome and the ShellApk may each have > a different version of WebApkHelper.java) I just want to reuse code, but I am fine to revert it back.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:167: WebApkIdentityServiceClient.CheckRuntimeHostCallback callback = per discussion let's move this to validator (we shoudl also clean up that interface but as a separate change) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( it's weird that this isn't in the "NotificationClient". Maybe you delegate this to WebApkServiceConnectionmanager (proguard should inline it) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:204: ContextUtils.getApplicationContext(), getWebApkPackageName()); perhaps we should move this to applicationstatelistener, and also disconnet from identity service when ther eare no visible chrome activities. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:84: String appPackageName, CheckRuntimeHostCallback callback, String runtimeHost) { no need to pass appPackageName through. Can just ContextUtils.getApplicationContext https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:87: mConnections.remove(webApkName); TODO: check whether to kill the corresponding webapkactivity (if the webapk data was cleared/uninstalled)
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:92: Each method should have a method comment https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:156: protected final void onPostExecute(Intent intent) { Nit: Might as well initialize TabDelegate in onPostExecute() https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:168: final TabDelegate tabDelegate = new TabDelegate(incognito); Nit: The TabDelegate does not need to be final https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:792: long nativeNotificationPlatformBridgeAndroid, String runtimeHost, long callbackPointer); runtimeHost should be webApkPackageName? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( I think that we can probably move all of the methods in this class (all two) to WebApkServiceClient I think that WebApkNotificationClient#notifyNotification() is simple enough that it can be moved to WebApkServiceClient https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:24: public class WebApkIdentityServiceClient extends WebApkServiceConnectionManager { Optional: Can "WebApkServiceConnectionManager be a member of WebApkIdentityServiceClient" instead of "WebApkIdentityServiceClient extending WebApkServiceConnectionManager" Yaron mentioned last week that "implementation inheritance" ... that we do everywhere in Clank ... is bad. This class is an easy place to avoid "implementation inheritance". https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:27: void onCheckedRuntimeHost(boolean matchRuntimeHost); Maybe change the parameter to |isValid|. I think that |isValid| is less confusing for people who don't know the details of what WebApkIdentityServiceClient does https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:57: public void onConnected(IBinder service) { Nit: I think that it would be clearer to do the null check here as opposed to on line 60 https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:72: return; Lines 71 and 72 are unnecessary https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:95: private static String maybeUseRuntimeHostFromMetaData(String webApkPackageName) { Maybe rename this function to: extractRuntimeHostFromMetaData() (or alternatively maybeExtractRuntimeHostFromMetaData()) because that's what the function does https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:106: : null; Isn't it notify()'s job to compare the package name? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:110: private static Bundle readMetaData(PackageManager packageManager, String packageName) { Nit: You might as well pass the Context instead of the PackageManager to the function https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:20: * WebAPK has its own "WebAPK service". Maybe change the class comment to: "Each WebAPK has several services. This class manages static global connections betweeen the Chrome application and the "WebAPK services." I think that my suggestion makes it clear that each WebAPK has several services https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:24: * Interface for getting notified once Chrome is connected to the WebAPK Service. Nit: "the WebAPK Service." -> "a WebAPK Service." because the WebAPK now has multiple services https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:61: notifyAndClear(); I don't think that you need to call notifyAndClear() here https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:67: Log.d(TAG, String.format("Got Bind Service: %s", mBinder)); How about: "Got IBinder Service:" https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:85: /** Called when a connection from the WebAPK is disconnected. */ How about: "Called when a WebAPK Service connection is disconnected." In my version I think that it is clearer what has been disconnected https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:94: * @param category The category of the Intent to connect. How about: "The category of the service to connect to." https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:119: appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE); If the IdentityService does not exist, won't Context#bindService() return false? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:155: WebApkServiceConnectionManager() {} Should the category be passed in the constructor? (Because all connections will be for the same category) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); Why the new strict mode checks? I don't think that we enable strict mode checking for WebAPKs
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:92: Each method should have a method comment https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:156: protected final void onPostExecute(Intent intent) { Nit: Might as well initialize TabDelegate in onPostExecute() https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:168: final TabDelegate tabDelegate = new TabDelegate(incognito); Nit: The TabDelegate does not need to be final https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:792: long nativeNotificationPlatformBridgeAndroid, String runtimeHost, long callbackPointer); runtimeHost should be webApkPackageName? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( I think that we can probably move all of the methods in this class (all two) to WebApkServiceClient I think that WebApkNotificationClient#notifyNotification() is simple enough that it can be moved to WebApkServiceClient https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:24: public class WebApkIdentityServiceClient extends WebApkServiceConnectionManager { Optional: Can "WebApkServiceConnectionManager be a member of WebApkIdentityServiceClient" instead of "WebApkIdentityServiceClient extending WebApkServiceConnectionManager" Yaron mentioned last week that "implementation inheritance" ... that we do everywhere in Clank ... is bad. This class is an easy place to avoid "implementation inheritance". https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:27: void onCheckedRuntimeHost(boolean matchRuntimeHost); Maybe change the parameter to |isValid|. I think that |isValid| is less confusing for people who don't know the details of what WebApkIdentityServiceClient does https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:57: public void onConnected(IBinder service) { Nit: I think that it would be clearer to do the null check here as opposed to on line 60 https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:72: return; Lines 71 and 72 are unnecessary https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:95: private static String maybeUseRuntimeHostFromMetaData(String webApkPackageName) { Maybe rename this function to: extractRuntimeHostFromMetaData() (or alternatively maybeExtractRuntimeHostFromMetaData()) because that's what the function does https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:106: : null; Isn't it notify()'s job to compare the package name? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:110: private static Bundle readMetaData(PackageManager packageManager, String packageName) { Nit: You might as well pass the Context instead of the PackageManager to the function https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:20: * WebAPK has its own "WebAPK service". Maybe change the class comment to: "Each WebAPK has several services. This class manages static global connections betweeen the Chrome application and the "WebAPK services." I think that my suggestion makes it clear that each WebAPK has several services https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:24: * Interface for getting notified once Chrome is connected to the WebAPK Service. Nit: "the WebAPK Service." -> "a WebAPK Service." because the WebAPK now has multiple services https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:61: notifyAndClear(); I don't think that you need to call notifyAndClear() here https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:67: Log.d(TAG, String.format("Got Bind Service: %s", mBinder)); How about: "Got IBinder Service:" https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:85: /** Called when a connection from the WebAPK is disconnected. */ How about: "Called when a WebAPK Service connection is disconnected." In my version I think that it is clearer what has been disconnected https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:94: * @param category The category of the Intent to connect. How about: "The category of the service to connect to." https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:119: appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE); If the IdentityService does not exist, won't Context#bindService() return false? https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:155: WebApkServiceConnectionManager() {} Should the category be passed in the constructor? (Because all connections will be for the same category) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); Why the new strict mode checks? I don't think that we enable strict mode checking for WebAPKs
Patchset #4 (id:280001) has been deleted
Patchset #4 (id:300001) has been deleted
The CQ bit was checked by hanxi@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...
Hi Yaron and Peter, PTAL, thanks! https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:92: On 2017/07/12 19:57:58, pkotwicz wrote: > Each method should have a method comment Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:156: protected final void onPostExecute(Intent intent) { On 2017/07/12 19:57:58, pkotwicz wrote: > Nit: Might as well initialize TabDelegate in onPostExecute() Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:168: final TabDelegate tabDelegate = new TabDelegate(incognito); On 2017/07/12 19:57:58, pkotwicz wrote: > Nit: The TabDelegate does not need to be final Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:167: WebApkIdentityServiceClient.CheckRuntimeHostCallback callback = On 2017/07/12 18:14:57, Yaron wrote: > per discussion let's move this to validator (we shoudl also clean up that > interface but as a separate change) Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:792: long nativeNotificationPlatformBridgeAndroid, String runtimeHost, long callbackPointer); On 2017/07/12 19:57:58, pkotwicz wrote: > runtimeHost should be webApkPackageName? Thanks for catching it! https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( On 2017/07/12 19:57:58, pkotwicz wrote: > I think that we can probably move all of the methods in this class (all two) to > WebApkServiceClient > > I think that WebApkNotificationClient#notifyNotification() is simple enough that > it can be moved to WebApkServiceClient I would hope too, but the notifyNotification() has a variable NotificationBuilderBase notificationBuilder which is used in the callback. If moving the constructing of the callback to the WebApkServiceClient, we will introduce dependency of Chrome in the webapk client lib. To consistency, I will just add cancelNotifiction() back in the NotificationClient. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:204: ContextUtils.getApplicationContext(), getWebApkPackageName()); On 2017/07/12 18:14:58, Yaron wrote: > perhaps we should move this to applicationstatelistener, and also disconnet from > identity service when ther eare no visible chrome activities. We need to find something in Chrome to listen to the applicationstatelistener which will introduces a dependency of Chrome. Instead, I make ChromeWebApkHost register a listener for application status change when Chrome is initialized with native. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:24: public class WebApkIdentityServiceClient extends WebApkServiceConnectionManager { On 2017/07/12 19:57:59, pkotwicz wrote: > Optional: Can "WebApkServiceConnectionManager be a member of > WebApkIdentityServiceClient" instead of "WebApkIdentityServiceClient extending > WebApkServiceConnectionManager" > > Yaron mentioned last week that "implementation inheritance" ... that we do > everywhere in Clank ... is bad. This class is an easy place to avoid > "implementation inheritance". Making WebApkServiceConnectionManager as a base class is because I was thinking that each subclass could override WebApkServiceConnectionManager#onServiceDisconnected(). For example, checking whether webapk has uninstalled is only needed in the WebApkService but not in WebApkIdnetityService. Anyway, this is a good suggestion. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:27: void onCheckedRuntimeHost(boolean matchRuntimeHost); On 2017/07/12 19:57:58, pkotwicz wrote: > Maybe change the parameter to |isValid|. I think that |isValid| is less > confusing for people who don't know the details of what > WebApkIdentityServiceClient does Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:57: public void onConnected(IBinder service) { On 2017/07/12 19:57:58, pkotwicz wrote: > Nit: I think that it would be clearer to do the null check here as opposed to on > line 60 Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:72: return; On 2017/07/12 19:57:59, pkotwicz wrote: > Lines 71 and 72 are unnecessary Move the notify to finally{} https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:84: String appPackageName, CheckRuntimeHostCallback callback, String runtimeHost) { On 2017/07/12 18:14:58, Yaron wrote: > no need to pass appPackageName through. Can just > ContextUtils.getApplicationContext ContextUtils introduces dependency of Chrome in the client lib. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:95: private static String maybeUseRuntimeHostFromMetaData(String webApkPackageName) { On 2017/07/12 19:57:59, pkotwicz wrote: > Maybe rename this function to: extractRuntimeHostFromMetaData() (or > alternatively maybeExtractRuntimeHostFromMetaData()) because that's what the > function does maybeExtractRuntimeHostFromMetaData() fits better:) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:106: : null; On 2017/07/12 19:57:59, pkotwicz wrote: > Isn't it notify()'s job to compare the package name? > You are right, the logic is duplicated. Remove it from here. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:110: private static Bundle readMetaData(PackageManager packageManager, String packageName) { On 2017/07/12 19:57:58, pkotwicz wrote: > Nit: You might as well pass the Context instead of the PackageManager to the > function Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:20: * WebAPK has its own "WebAPK service". On 2017/07/12 19:58:00, pkotwicz wrote: > Maybe change the class comment to: > "Each WebAPK has several services. This class manages static global connections > betweeen the Chrome application and the "WebAPK services." > > I think that my suggestion makes it clear that each WebAPK has several services Thanks! https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:24: * Interface for getting notified once Chrome is connected to the WebAPK Service. On 2017/07/12 19:58:00, pkotwicz wrote: > Nit: "the WebAPK Service." -> "a WebAPK Service." > > because the WebAPK now has multiple services Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:61: notifyAndClear(); On 2017/07/12 19:57:59, pkotwicz wrote: > I don't think that you need to call notifyAndClear() here I know it is very unlikely, but in case these is a callback hasn't been called, it is time to call it. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:67: Log.d(TAG, String.format("Got Bind Service: %s", mBinder)); On 2017/07/12 19:57:59, pkotwicz wrote: > How about: "Got IBinder Service:" Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:85: /** Called when a connection from the WebAPK is disconnected. */ On 2017/07/12 19:57:59, pkotwicz wrote: > How about: "Called when a WebAPK Service connection is disconnected." > > In my version I think that it is clearer what has been disconnected Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:87: mConnections.remove(webApkName); On 2017/07/12 18:14:58, Yaron wrote: > TODO: check whether to kill the corresponding webapkactivity (if the webapk data > was cleared/uninstalled) Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:94: * @param category The category of the Intent to connect. On 2017/07/12 19:57:59, pkotwicz wrote: > How about: "The category of the service to connect to." Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:119: appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE); On 2017/07/12 19:58:00, pkotwicz wrote: > If the IdentityService does not exist, won't Context#bindService() return false? I think it will throw SecurityException. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:155: WebApkServiceConnectionManager() {} On 2017/07/12 19:57:59, pkotwicz wrote: > Should the category be passed in the constructor? (Because all connections will > be for the same category) I have thought about this. It could but not necessary, since the only place that the category is needed is connect(). I don't have a strong preference but I am ok to change it. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); On 2017/07/12 19:58:00, pkotwicz wrote: > Why the new strict mode checks? I don't think that we enable strict mode > checking for WebAPKs When webapk isn't running and Chrome calls WebApkUtils#getHostBrowserPackageName() which reads the sharedPref will trigger the strict mode violation. From my understanding, it creates a shared pref (write to disk) if there isn't one, and that is why is disk write violation, not disk read.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). Currently, different Chrome channels can share the same WebAPK. However, only the real runtime host can connect to the WebApkService sucessfully and delegate task to the service. In this CL, we add an Identity Service in WebAPK that brwoser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). Currently, different Chrome channels can share the same WebAPK. However, only the real runtime host can connect to the WebApkService sucessfully and delegate task to the service. In this CL, we add an Identity Service in WebAPK that brwoser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I intoduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ==========
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). Currently, different Chrome channels can share the same WebAPK. However, only the real runtime host can connect to the WebApkService sucessfully and delegate task to the service. In this CL, we add an Identity Service in WebAPK that brwoser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I intoduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we add an Identity Service in WebAPK that browser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I introduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. 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 ==========
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we add an Identity Service in WebAPK that browser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I introduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. 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 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we add an Identity Service in WebAPK that browser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. 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. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I introduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. BUG=720077 ==========
I upload a new patch to fix the notification bug (crbug.com/740614) in the WebApkServiceConnectionManager, PTAL, thanks!
https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:213: ChromeWebApkHost.initializeWithNative(); i think this is doing it too broadly. Why can't we just do this with you initialize a connection to WebApkIdentityService (in a chrome wrapper)? Inside the code that uses the webapk-client-library's identity service, once it establishes the connection it also registers the cleanup? https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:166: WebApkValidator.IsOwnedCheckCallback callback = new WebApkValidator.IsOwnedCheckCallback() { rather than custom callbacks, can the two new callbcsk just be Callback<Boolean> ? https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:64: WebApkIdentityServiceClient.checkWebApkRuntimeHostAsync( Shouldn't this be cancelNotification?
Patchset #8 (id:400001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:213: ChromeWebApkHost.initializeWithNative(); On 2017/07/14 19:52:04, Yaron wrote: > i think this is doing it too broadly. Why can't we just do this with you > initialize a connection to WebApkIdentityService (in a chrome wrapper)? Inside > the code that uses the webapk-client-library's identity service, once it > establishes the connection it also registers the cleanup? Sgtm, updated. https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:166: WebApkValidator.IsOwnedCheckCallback callback = new WebApkValidator.IsOwnedCheckCallback() { On 2017/07/14 19:52:04, Yaron wrote: > rather than custom callbacks, can the two new callbcsk just be Callback<Boolean> > ? We can't, it will introduce dependency on base. https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:64: WebApkIdentityServiceClient.checkWebApkRuntimeHostAsync( On 2017/07/14 19:52:04, Yaron wrote: > Shouldn't this be cancelNotification? Thanks for catching it!
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( In this case we can merge WebApkServiceClient into WebApkNotificationClient. Each host browser has a browser-specific WebApkService (the API is different too). Thus, there is no opportunity to reuse another browser's WebApkServiceClient https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:119: appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE); Context#bindService() returns false if the IdentityService is not declared in the AndroidManifest.xml https://stackoverflow.com/questions/17780049/in-what-case-does-bindservice-re... (I also tested this to make super duper sure) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); Chrome no longer calls WebApkUtils#getHostBrowserPackageName(). My understanding is that Chrome's "super strict" handling of "strict mode violations" does not apply to WebAPKs. You are right that we will trigger a strict mode exception but the strict mode violation should be silent https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( The order of the functions in the .h file should match the order of the functions in the .cc file https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; Shouldn't this be 15? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:73: } finally { Can you move this outside of the finally{} block? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:82: public static void notify( Nits: 1) Can you rename this function to onGotWebApkHostBrowser() The function comment could be: "Called after fetching the WebAPK's host browser." It is weird for the function to be called notify() because the function processes its parameters 2) Can you make |callback| the last parameter? I think that making output parameters the last parameter is a convention (I have not looked it up in the style guide though) 3) Can you rename runtimeHost to webApkRuntimeHostPackageName This makes it clearer: - what the runtime host is (it is a property of the WebAPK) - that both parameters are package names 4) Can you add @param comments for the function parameters? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:90: * - the runtime host specified in the AndroidManifest.xml matches the current app name. Can you please fix the comment? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:92: * - The shell Apk version is less than SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST. Nit: Apk -> APK https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:88: private void notifyAndClear() { You can inline this method in onServiceConnected() given that this function is only called from there https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:141: } finally { Does this need to be in a finally{} block? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:166: if (connection == null || !connection.hasConnected()) continue; - Isn't it impossible for |connection| to be null? - You should call Context#unbindServce() even for services which have not connected yet (otherwise when the service connects it will stay connected as long as Chrome is alive) https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:117: /** Checks whether the current app is the runtime host of the WebAPK asynchronously. */ Nit: "current app" -> "Chrome" https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:118: public static void isOwnedWebApkAsync( 1) Can you please rename this method to checkChromeBacksWebApkAsync() 2) You can rename IsOwnedCheckCallback to CheckChromeBacksWebApkCallback I suggest that CheckChromeBacksWebApkCallback has a method called onChecked(boolean doesChromeBackWebApk) {} 3) I talked to Yaron, and I argued that we can have WebApkValidator#checkChromeBacksWebApkAsync() but that WebApkValidator#checkChromeBacksWebApkAsync() and WebApkIdentifyServiceClient#checkChromeBacksWebApkAsync() should use the same callback class. Yaron agreed with me. 🙌! https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:121: appContext, webApkPackageName, callback); 1) WebApkIdentityServiceClient#checkWebApkRuntimeHostAsync() should be renamed to WebApkIdentityServiceClient#checkChromeBacksWebApkAsync() (Both functions should have the same name to avoid confusion) 2) You can make WebApkIdentityServiceClient#checkChromeBacksWebApkAsync() non-static so that this method "does something" https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl:5: package org.chromium.webapk.lib.common.IdentityService; Based on a code search, package names with upper case components are uncommon. Can the package name be lowercase here too? https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl:7: /** Interface for communicating between WebAPK Identity Service and Chrome. */ If you change the comment in IdentityService.java you should change it here too https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="org.webapk.IDENTITY_SERVICE_API" /> In order to be consistent with CustomTabs we should probably not have a custom category and have a custom action. (Not sure why custom tabs made the change). Custom tabs made the change in https://codereview.chromium.org/1234643002 I know that you are following the pattern in WebApkServiceFactory https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/IdentityService.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/IdentityService.java:13: /** IdentityService allows browsers to commnicate with WebAPK, e.g querying its runtime host. */ Would it be more accurate to say that the "IdentityService allows browsers to query information about the WebAPK."? https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: if (WebApkIdentityServiceClient.isInitialized()) return; My preference would be to: - have a member variable which references the ApplicationStateListener - unegister the listener whenever WebApkIdentityServiceClient#disconnect() is called https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:64: ContextUtils.getApplicationContext()); I think that you can call WebApkServiceClient#disconnect() here too WebApkServiceClient#disconnect() can be static if you want to avoid creating the object if it has not been created yet https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java:56: intent, new ComponentName(WEB_APK_PACKAGE, ""), Mockito.mock(IBinder.class)); Smart use of Mockito! That's more elegant than what I would have done
you may want to add notification folk to look at the notification changes https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: if (WebApkIdentityServiceClient.isInitialized()) return; On 2017/07/14 23:20:05, pkotwicz wrote: > My preference would be to: > - have a member variable which references the ApplicationStateListener > - unegister the listener whenever WebApkIdentityServiceClient#disconnect() is > called Ya I kinda like that because I think it's better to have the state management locally near its use (IMHO what we care about is a single listener instance) https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; nit: can you add a brief explanation of how this worked before/after https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:128: mConnections.put(webApkPackage, newConnection); this is in |doInBackground| which is running on the asynctask threadpool. To avoid locking, can you do this in onPostExecute instead? https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:149: for (Map.Entry<String, Connection> entry : mConnections.entrySet()) { I think this isn't threadsafe. I think the best may way may be to override onPreExecute, grab and clear mconnections and then unbind from everything in doInBackground. That at least ensurse mConnections is only used on UI thread. I don't think there's a race between connect/disconnect at that point because even if we're disconnecting from something while we're connecting, they would be two separate bindings.
I think I have addressed all of the comments. PTAL, thanks! https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( On 2017/07/14 23:20:04, pkotwicz wrote: > In this case we can merge WebApkServiceClient into WebApkNotificationClient. > Each host browser has a browser-specific WebApkService (the API is different > too). Thus, there is no opportunity to reuse another browser's > WebApkServiceClient Well, WebApkSericeClient isn't only for notification, it will have more API than WebApkNotificationClient. I hope there is only one singleton of the WebApkServiceClient, and even later we have more clients needs to be remained in Chrome's code, they can share the same WebApkServiceClient for connections. We will add more APIs, e.g., permissions. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:119: appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE); On 2017/07/14 23:20:04, pkotwicz wrote: > Context#bindService() returns false if the IdentityService is not declared in > the AndroidManifest.xml > https://stackoverflow.com/questions/17780049/in-what-case-does-bindservice-re... > (I also tested this to make super duper sure) Yes, you are right. I have updated here to use the return value in the patch 8:) https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); On 2017/07/14 23:20:04, pkotwicz wrote: > Chrome no longer calls WebApkUtils#getHostBrowserPackageName(). My understanding > is that Chrome's "super strict" handling of "strict mode violations" does not > apply to WebAPKs. You are right that we will trigger a strict mode exception but > the strict mode violation should be silent Sorry, not Chrome, but the IdentityService calls the WebApkUtils#getHostBrowserPackageName() when Chrome binds to it. This strict mode exception only occurs after the introducing the IdentityService. https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( On 2017/07/14 23:20:04, pkotwicz wrote: > The order of the functions in the .h file should match the order of the > functions in the .cc file The following four functions are overriding functions of NotificationPlatformBridge, and it is better to keep them together. After them, are public static functions. So I put OnQueryWebApkPackage() before them. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; On 2017/07/14 23:20:05, pkotwicz wrote: > Shouldn't this be 15? Shell apk number between 6 and 15 is: we introduce the "choose-host-browser" logic but don't have identity service. If a shell apk version is in this range, we don't have a way to know exactly which browser is the runtime host. Before version 6, we probably only have browser-WebAPKs installed. If the browser who is querying the runtime host matches the specified runtime host, we think it is the host browser. Otherwise it is not. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:73: } finally { On 2017/07/14 23:20:05, pkotwicz wrote: > Can you move this outside of the finally{} block? Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:82: public static void notify( On 2017/07/14 23:20:04, pkotwicz wrote: > Nits: > 1) Can you rename this function to onGotWebApkHostBrowser() > The function comment could be: "Called after fetching the WebAPK's host > browser." > It is weird for the function to be called notify() because the function > processes its parameters > 2) Can you make |callback| the last parameter? > I think that making output parameters the last parameter is a convention (I have > not looked it up in the style guide though) > 3) Can you rename runtimeHost to webApkRuntimeHostPackageName > This makes it clearer: > - what the runtime host is (it is a property of the WebAPK) > - that both parameters are package names > 4) Can you add @param comments for the function parameters? Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:90: * - the runtime host specified in the AndroidManifest.xml matches the current app name. On 2017/07/14 23:20:05, pkotwicz wrote: > Can you please fix the comment? Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:92: * - The shell Apk version is less than SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST. On 2017/07/14 23:20:05, pkotwicz wrote: > Nit: Apk -> APK Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:88: private void notifyAndClear() { On 2017/07/14 23:20:05, pkotwicz wrote: > You can inline this method in onServiceConnected() given that this function is > only called from there Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:141: } finally { On 2017/07/14 23:20:05, pkotwicz wrote: > Does this need to be in a finally{} block? Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:166: if (connection == null || !connection.hasConnected()) continue; On 2017/07/14 23:20:05, pkotwicz wrote: > - Isn't it impossible for |connection| to be null? Removed this check. > - You should call Context#unbindServce() even for services which have not > connected yet (otherwise when the service connects it will stay connected as > long as Chrome is alive) In patch set 8, I updated this check to connection.getService() != null. The service is only non-null when it is connected. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:117: /** Checks whether the current app is the runtime host of the WebAPK asynchronously. */ On 2017/07/14 23:20:05, pkotwicz wrote: > Nit: "current app" -> "Chrome" Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:118: public static void isOwnedWebApkAsync( On 2017/07/14 23:20:05, pkotwicz wrote: > 1) Can you please rename this method to checkChromeBacksWebApkAsync() Done. > 2) You can rename IsOwnedCheckCallback to CheckChromeBacksWebApkCallback > I suggest that CheckChromeBacksWebApkCallback has a method called > onChecked(boolean doesChromeBackWebApk) {} Done. > 3) I talked to Yaron, and I argued that we can have > WebApkValidator#checkChromeBacksWebApkAsync() but that > WebApkValidator#checkChromeBacksWebApkAsync() and > WebApkIdentifyServiceClient#checkChromeBacksWebApkAsync() should use the same > callback class. Yaron agreed with me. 🙌! We should avoid browser-specific naming in the WebApkIdentityserviceClient. I just thought it would like wired when you call ChromeWebApkHost.check...() and pass in a WebApkIdentifyServiceClient callback. Anyway, remove the callback defined in the ChromeWebApkHost. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:121: appContext, webApkPackageName, callback); On 2017/07/14 23:20:05, pkotwicz wrote: > 1) WebApkIdentityServiceClient#checkWebApkRuntimeHostAsync() should be renamed > to WebApkIdentityServiceClient#checkChromeBacksWebApkAsync() (Both functions > should have the same name to avoid confusion) WebApkIdentityServiceClient shouldn't have browser-specific naming. > 2) You can make WebApkIdentityServiceClient#checkChromeBacksWebApkAsync() > non-static so that this method "does something" https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl:5: package org.chromium.webapk.lib.common.IdentityService; On 2017/07/14 23:20:05, pkotwicz wrote: > Based on a code search, package names with upper case components are uncommon. > Can the package name be lowercase here too? Thanks for catching it, renamed. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/IdentityService/IIdentityService.aidl:7: /** Interface for communicating between WebAPK Identity Service and Chrome. */ On 2017/07/14 23:20:05, pkotwicz wrote: > If you change the comment in IdentityService.java you should change it here too Done. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="org.webapk.IDENTITY_SERVICE_API" /> On 2017/07/14 23:20:05, pkotwicz wrote: > In order to be consistent with CustomTabs we should probably not have a custom > category and have a custom action. (Not sure why custom tabs made the change). > Custom tabs made the change in https://codereview.chromium.org/1234643002 > > I know that you are following the pattern in WebApkServiceFactory This is follow the pattern to launch the WebappLauncherActivity, there are some discussion when we changed the action there. I don't have a strong preference though. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/IdentityService.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/IdentityService.java:13: /** IdentityService allows browsers to commnicate with WebAPK, e.g querying its runtime host. */ On 2017/07/14 23:20:05, pkotwicz wrote: > Would it be more accurate to say that the "IdentityService allows browsers to > query information about the WebAPK."? Done. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: if (WebApkIdentityServiceClient.isInitialized()) return; On 2017/07/17 14:34:30, Yaron wrote: > On 2017/07/14 23:20:05, pkotwicz wrote: > > My preference would be to: > > - have a member variable which references the ApplicationStateListener > > - unegister the listener whenever WebApkIdentityServiceClient#disconnect() is > > called > > Ya I kinda like that because I think it's better to have the state management > locally near its use (IMHO what we care about is a single listener instance) WebApkIdentityServiceClient can't have dependency on Chrome's code. I did that and reverted the changes after I noticed the dependency issue. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:64: ContextUtils.getApplicationContext()); On 2017/07/14 23:20:05, pkotwicz wrote: > I think that you can call WebApkServiceClient#disconnect() here too I still feel that WebApkServiceClient is mostly used when users are using WebAPKs, we don't need to hold the connection anymore once WebApkActivity is stopped. > WebApkServiceClient#disconnect() can be static if you want to avoid creating the > object if it has not been created yet Good point. I update the WebApkIdentityServiceClient#disconnect() to avoid creating a new object if it has not been created yet. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManagerTest.java:56: intent, new ComponentName(WEB_APK_PACKAGE, ""), Mockito.mock(IBinder.class)); On 2017/07/14 23:20:05, pkotwicz wrote: > Smart use of Mockito! That's more elegant than what I would have done Acknowledged. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; On 2017/07/17 14:34:30, Yaron wrote: > nit: can you add a brief explanation of how this worked before/after Done. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:128: mConnections.put(webApkPackage, newConnection); On 2017/07/17 14:34:30, Yaron wrote: > this is in |doInBackground| which is running on the asynctask threadpool. To > avoid locking, can you do this in onPostExecute instead? Done. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:149: for (Map.Entry<String, Connection> entry : mConnections.entrySet()) { On 2017/07/17 14:34:30, Yaron wrote: > I think this isn't threadsafe. I think the best may way may be to override > onPreExecute, grab and clear mconnections and then unbind from everything in > doInBackground. That at least ensurse mConnections is only used on UI thread. I > don't think there's a race between connect/disconnect at that point because even > if we're disconnecting from something while we're connecting, they would be two > separate bindings. I didn't do that because HashTable is thread safe. But we can do that to eliminate concerns.
Patchset #9 (id:440001) has been deleted
I am now working on making WebApkServiceConnectionManager as a member variable in WebApkIdentityServiceClient and WebApkServiceClient. Please hold off reviewing until I update my patch. Thanks!
Patchset #10 (id:480001) has been deleted
The CQ bit was checked by hanxi@chromium.org to run a CQ dry run
PTAL, thanks!
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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we add an Identity Service in WebAPK that browser can bind to query about the runtime host of the WebAPK. The query of the Identity Service is only enabled for displaying notification purpose for now. 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. Since now each WebAPK has 1) WebApkSerive and 2) WebAPK Identity Service, a refactoring is done to extract the logic that manages the connections to WebAPK services into a generic WebApkConnectionManager. For each kind of service, I introduce: 1) WebApkServiceClient and 2) WebApkIdentityServiceClient. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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 introdce an Identity service in WebAPK that allows a browser to query the WebAPK's runtime host to know whether it owns the WebAPK. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK Serice/WebAPK Identity service respectively. 4) Add runtime host queries when displaying notifications and launching a tab from service worker. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ==========
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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 introdce an Identity service in WebAPK that allows a browser to query the WebAPK's runtime host to know whether it owns the WebAPK. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK Serice/WebAPK Identity service respectively. 4) Add runtime host queries when displaying notifications and launching a tab from service worker. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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 introdce an Identity service in WebAPK that allows a browser to query the WebAPK's runtime host to know whether it owns the WebAPK. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK Serice/WebAPK Identity service respectively. 4) add runtime host queries when displaying notifications and launching a tab from service worker. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ==========
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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 introdce an Identity service in WebAPK that allows a browser to query the WebAPK's runtime host to know whether it owns the WebAPK. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK Serice/WebAPK Identity service respectively. 4) add runtime host queries when displaying notifications and launching a tab from service worker. Usually binding to a service takes 15 - 20 miliseconds. Once bound, a call of the its API takes up to 2 miliseconds. 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 visiable activity of Chrome. BUG=720077 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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 ==========
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( Fair enough. The merged file can be called WebApkServiceClient. However, the merged file should not be in chrome/android/webapk/libs/client The API of the WebApkService for each browser will be completely different https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); This means we should not be calling the Identity service's methods on the UI thread :) https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( You can change the order of the functions in the .cc file to match the order in the .h file as well :) https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:112: const std::string& webapk_package); This function does not seem to be implemented in the .cc file. Remove https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; Good point. I see what you are saying. Please add comments to maybeExtractRuntimeHostFromMetaData() to explain the logic. We should also have JUnit tests for this logic to ensure that the meta data is ignored when it should be https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java:121: appContext, webApkPackageName, callback); Fair point. In this case you can can rename the function to WebApkIdentityServiceClient#checkBrowserBacksWebApkAsync() https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="org.webapk.IDENTITY_SERVICE_API" /> If we make a change we will need code to handle old WebAPKs. I'd rather get this right the first time. I understand that you are following the pattern used to launch WebappLauncherActivity. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: if (WebApkIdentityServiceClient.isInitialized()) return; I was suggesting having a global static variable in ChromeWebApkHost.java https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:64: ContextUtils.getApplicationContext()); I think that in most cases users will stop all Chrome activities so we don't need to worry much about the scenario of "the user stopped using the WebAPK but is still using Chrome."
https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( On 2017/07/18 17:30:46, pkotwicz wrote: > Fair enough. The merged file can be called WebApkServiceClient. However, the > merged file should not be in chrome/android/webapk/libs/client The API of the > WebApkService for each browser will be completely different Ya, I think that's reasonable https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:149: for (Map.Entry<String, Connection> entry : mConnections.entrySet()) { On 2017/07/17 20:45:33, Xi Han wrote: > On 2017/07/17 14:34:30, Yaron wrote: > > I think this isn't threadsafe. I think the best may way may be to override > > onPreExecute, grab and clear mconnections and then unbind from everything in > > doInBackground. That at least ensurse mConnections is only used on UI thread. > I > > don't think there's a race between connect/disconnect at that point because > even > > if we're disconnecting from something while we're connecting, they would be > two > > separate bindings. > > I didn't do that because HashTable is thread safe. But we can do that to > eliminate concerns. Hashtable is threadsafe but I'm pretty sure it's still a race-condition if you're iterating over the entrySet and the set changes due to a later call. So for example you ahve items a,b,c. While you're processing "a", "d" gets connected. If you leave the code this way, you'll also end up clearing "d" (certainly L154 will).
Patchset #11 (id:520001) has been deleted
hanxi@chromium.org changed reviewers: + peter@chromium.org
+peter@chromium.org: Please review changes in *notification* directory. Thanks! https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java (left): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java:78: public static void cancelNotification( On 2017/07/18 17:30:46, pkotwicz wrote: > Fair enough. The merged file can be called WebApkServiceClient. However, the > merged file should not be in chrome/android/webapk/libs/client The API of the > WebApkService for each browser will be completely different Done. https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); On 2017/07/18 17:30:46, pkotwicz wrote: > This means we should not be calling the Identity service's methods on the UI > thread :) The exception happens when Chrome is bind to the service which hasn't launched yet. https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( On 2017/07/18 17:30:46, pkotwicz wrote: > You can change the order of the functions in the .cc file to match the order in > the .h file as well :) In .cc file, the OnQueryWebApkPackage() is called in the Display(), so I feel it is more naturally to have it after Display(). https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:112: const std::string& webapk_package); On 2017/07/18 17:30:46, pkotwicz wrote: > This function does not seem to be implemented in the .cc file. Remove I renamed it to DisplayInternal() in patch set 9, so that is why you didn't see it in the .cc file. https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6; On 2017/07/18 17:30:46, pkotwicz wrote: > Good point. > > I see what you are saying. Please add comments to > maybeExtractRuntimeHostFromMetaData() to explain the logic. We should also have > JUnit tests for this logic to ensure that the meta data is ignored when it > should be I add a link to here in the comments of maybeExtractRuntimeHostFromMetaData(). https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/AndroidManifest.xml (right): https://codereview.chromium.org/2974573002/diff/380001/chrome/android/webapk/... chrome/android/webapk/shell_apk/AndroidManifest.xml:80: <category android:name="org.webapk.IDENTITY_SERVICE_API" /> On 2017/07/18 17:30:47, pkotwicz wrote: > If we make a change we will need code to handle old WebAPKs. I'd rather get this > right the first time. I understand that you are following the pattern used to > launch WebappLauncherActivity. Sorry I misunderstood. I thought you want me to update the text. Sure, update the category to action. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: if (WebApkIdentityServiceClient.isInitialized()) return; On 2017/07/18 17:30:47, pkotwicz wrote: > I was suggesting having a global static variable in ChromeWebApkHost.java I see. https://codereview.chromium.org/2974573002/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:64: ContextUtils.getApplicationContext()); On 2017/07/18 17:30:47, pkotwicz wrote: > I think that in most cases users will stop all Chrome activities so we don't > need to worry much about the scenario of "the user stopped using the WebAPK but > is still using Chrome." Done.
The CQ bit was checked by hanxi@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I upload a new patch to address Yaron's comments about unbinding the connections in AsyncTask. PTAL, thanks! https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/420001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:149: for (Map.Entry<String, Connection> entry : mConnections.entrySet()) { On 2017/07/18 19:25:46, Yaron wrote: > On 2017/07/17 20:45:33, Xi Han wrote: > > On 2017/07/17 14:34:30, Yaron wrote: > > > I think this isn't threadsafe. I think the best may way may be to override > > > onPreExecute, grab and clear mconnections and then unbind from everything in > > > doInBackground. That at least ensurse mConnections is only used on UI > thread. > > I > > > don't think there's a race between connect/disconnect at that point because > > even > > > if we're disconnecting from something while we're connecting, they would be > > two > > > separate bindings. > > > > I didn't do that because HashTable is thread safe. But we can do that to > > eliminate concerns. > > Hashtable is threadsafe but I'm pretty sure it's still a race-condition if > you're iterating over the entrySet and the set changes due to a later call. So > for example you ahve items a,b,c. While you're processing "a", "d" gets > connected. If you leave the code this way, you'll also end up clearing "d" > (certainly L154 will). Got it. I think it is ok to clear the |mConnections| before the async task, and pass the entrySet to the async task to do the unbind.
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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,740614 ==========
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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,740614 ========== to ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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. The refactoring of WebApkServiceConnectionManager also fixes notification issue reported in crbug.com/740614. BUG=720077,740614 ==========
Mostly nits https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... File chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java (right): https://codereview.chromium.org/2974573002/diff/260001/chrome/android/webapk/... chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/WebApkUtils.java:190: StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites(); The strict mode exception occurs here and not elsewhere because Chrome calls ChromeStrictMode#configureStrictMode() I suggest suppressing the StrictMode exception in the IdentityService and not here https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( From the Chromium style guide: "Function declaration order should match function definition order." (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md) https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:93: /** Launches a WebAPK if it is owned by the Chrome. Otherwise, launches a tab for the |url|. */ Nits: "owned by" -> "backed by" "the Chrome" -> "Chrome" https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:100: // Launch WebAPK if one matches the target URL. This comment is no longer valid https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:116: extraHeaders, postData); Sorry for flip flopping. We should call launchTabOrWebapp() here too. This should simplify the logic slightly https://codereview.chromium.org/2974573002/diff/540001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/540001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:67: public static void checkBacksWebApkAsync(final Context context, final String webApkPackageName, Nit: Rename |context| to |browserContext| https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:35: WebApkIdentityServiceClient.CheckBacksWebApkCallback callback) { Does the context have to be passed in? https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:43: */ The comment is out of date. Perhaps: "to disconnect singleton WebApkIdentityserviceClient if it hasn't registered yet" -> "to disconnect all connections to WebAPKs when Chrome is stopped." https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: ContextUtils.getApplicationContext()); I think that disconnecting from both the WebApkIdentityServiceClient and the WebApkServiceClient is a lot nicer. Thank you for making the change! https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:24: * "WebAPK service". Thank you for merging WebApkServiceClient and WebApkNotificationClient. Merging the two classes makes the implementation much cleaner! How about: "that allow browsers communicating with" -> "for browsers to communicate with" Your sentence is an incomplete sentence "Provides APIs that allow browsers communicating with WebAPK services to do something special" is a complete sentence "Provides APIs that allow browsers to do something special" is a complete sentence I can't think of a rule for why it is this way unfortunately. When I wrote a technical report in university, I got in trouble for using "allow" and "enable" interchangeably. "allow" and "enable" should not be used interchangeably as described in this article https://letlucyedit.wordpress.com/2013/04/03/enable-or-allow-why-you-need-to-... https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:28: public abstract static class ApiUseCallback This can now be private? https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:65: */ Can you make this a non-static function? Nit: "hand it over" -> "hands it over" https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:97: String webApkPackage, final String platformTag, final int platformID) { Can you make this a non-static function? https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:110: public static void disconnectWebApkService(Context appContext) { We don't pass the context for notify() so we don't need to pass the context for disconnectWebApkService() either Nits: - Rename this function to disconnectAll(). What you are disconnecting from should be obvious from the class name. - "from WebAPK services" -> "to WebAPK services" (because Chrome initiated the connection to the WebAPKService) https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:35: public void testReturnsSpecifiedRuntimeHostBeforeIntroducingHostBrowserSwitchLogic() { Thank you for adding JUnit tests! I was hoping for tests which called WebApkIdentityServiceClient#checkBacksWebApkAsync(). That way you don't test the implementation details of WebApkIdentityService. I think that testing this in a JUnit test is possible via ShadowApplication#setComponentNameAndServiceForBindServiceForIntent() https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:25: public interface CheckBacksWebApkCallback { void onChecked(boolean isValid); } Nits: - Rename CheckBacksWebApkCallback to CheckBrowserBacksWebApkCallback (The name is lengthier but makes things clearer in my opinion) - Change the parameter name to |doesBrowserBackWebApk| (I think that the parameter name makes things clearer) https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: * introduced a logic to allow user to choose runtime host browser for WebAPKs unbound without Thank you very much for adding the comment! It makes things clearer Nits: - "a logic" -> "logic" - "unbound without any" -> "not bound to any" https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:31: * any browser, and for WebAPKs installed from browsers when the browser isn't installed. Nit: "from browsers when the browser isn't installed" -> "from browsers when the browser is subsequently uninstalled." (I think that my version makes it clearer how this scenario can occur) https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. Maybe omit the part about "clearing the browser's data". Clearing the browser's data does not require choosing the runtime host again. I suspect that you mean WebappDataStorage but this is not obvious. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:36: * querying the runtime host of a WebAPK. Nit: "querying" -> "to query" https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:57: null /* action */, ACTION_WEBAPK_IDENTITY_SERVICE); Shouldn't the second parameter be null? https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:66: */ Nits: - Rename the function to checkBrowserBacksWebApkAsync() - Rename |context| to browserContext (We get the browser's package name from the Context) - Can this method be non static? https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:68: final CheckBacksWebApkCallback callback) { In a separate CL, (not this CL!) we should probably try to avoid calling WebApkIdentityServiceClient#onGotWebApkRuntimeHost() when the service is already connected https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:69: final String packageName = context.getPackageName(); Nits: - Move this computation to within the callback - Rename the variable to browserPackageName (Otherwise it is confusing which package name the variable refers to) https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:98: * @param appPackageName The current app's package name. Nit: Rename appPackageName to browserPackageName https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:101: * current app. Nits: - Rename webApkRuntimeHostPackageName to webApkBackingBrowserPackageName (This is more in line with the wording used elsewhere in thie file) - "The callback to notify the called whether the runtime host matches the current app" -> "The callback to notify whether {@link browserPackageName} backs the WebAPK." https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:116: */ The comment is out of date. This part "the runtime host specified in the AndroidManifest.xml matches the current app name" is incorrect For the comment, perhaps "Extracts the backing browser from the WebAPK's meta data." The case in which the runtime host is not extracted is an implementation detail of the function https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:121: >= SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST) { Nit: Add this comment "The backing browser in the WebAPK's meta data may not be the one which actually backs the WebAPK. The user may have switched the backing browser." https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:141: public static void disconnectWebApkIdentityService(Context appContext) { Nits: - "from" -> "to" (Because the connection was initiated by Chrome) - Rename the function to disconnectAll() I think that what we are disconnecting is obvious from the class name https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:78: /** The category of the service to connect. */ Nit: "to connect" -> "to connect to." https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:81: /** The action of the service to connect. */ Nit: "to connect." -> "to connect to." https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:146: Set<Map.Entry<String, Connection>> entrySet = mConnections.entrySet(); Can you just get mConnections.values() instead? https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:156: appContext.unbindService(connection); You should call Context#unbindServce() even for services which have not connected yet (otherwise when the service connects it will stay connected as long as Chrome is alive)
Patchset #14 (id:600001) has been deleted
https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. Ping on this comment https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:71: * Tests that for WebAPKs with shell APK version equals or higher than the Nit: equals -> equal https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:73: * doesn't have Identity Service, the backs WebAPK check returns null. You are testing that the callback returns false? https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:89: * Tests that for WebAPKs with Identity service, the backs WebAPK check returns true if the Nit: Add '-' to "backs WebAPK check" to improve readability -> "backs-WebAPK-check" (here and elsewhere) https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:121: when(context.getPackageName()).thenReturn(browserPackageName); Can you set the package name via @Config like we do in WebApkUtilsTest.java? You can call registerWebApk() multiple times in a test case instead of calling mockBrowserContext() multiple times https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:134: mShadowApplication.getApplicationContext(), webApkPackageName, callback); - Does WebApkIdentityServiceClient#checkBrowserBacksWebApkAsync() block till the check is done? You can probably fix this by calling Robolectric#runUiThreadTasksIncludingDelayedTasks() and checking that the callback was indeed called. - RuntimeEnvironment.application is the same as ShadowApplication#getApplicationContext() https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:155: when(IIdentityService.Stub.asInterface(service)).thenReturn(identityService); Would it be simpler to have a custom implementation which inherits from IIdentityService.Stub? https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:157: mShadowApplication.setComponentNameAndServiceForBindServiceForIntent( Why did you choose to use the 3 argument version of setComponentNameAndServiceForBindService() instead of the 2 argument version? (This test class cares about the IdentityService and nothing else. It does not matter if it is impossible to connect a service which is not the IdentityService) https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:111: && webApkBackingBrowserPackageName.contentEquals(browserPackageName)); Can you use TextUtils#equals() here? https://codereview.chromium.org/2974573002/diff/620001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:261: void NotificationPlatformBridgeAndroid::DisplayInternal( .cc and .h order applies to DisplayInternal() too
https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:122: if (metadata.getInt(WebApkMetaDataKeys.SHELL_APK_VERSION) In case you haven't done so, you need to add a null check here
Patchset #15 (id:640001) has been deleted
Patchset #15 (id:660001) has been deleted
The CQ bit was checked by hanxi@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: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hanxi@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...
I have fixed the tests issue, PTAL, thanks! https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.h (right): https://codereview.chromium.org/2974573002/diff/360001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.h:67: void OnQueryWebApkPackage( On 2017/07/19 16:09:13, pkotwicz wrote: > From the Chromium style guide: "Function declaration order should match function > definition order." > (https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md) Done. https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java (right): https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:93: /** Launches a WebAPK if it is owned by the Chrome. Otherwise, launches a tab for the |url|. */ On 2017/07/19 16:09:13, pkotwicz wrote: > Nits: > "owned by" -> "backed by" > "the Chrome" -> "Chrome" Done. https://codereview.chromium.org/2974573002/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java:116: extraHeaders, postData); On 2017/07/19 16:09:13, pkotwicz wrote: > Sorry for flip flopping. We should call launchTabOrWebapp() here too. > > This should simplify the logic slightly Acknowledged. https://codereview.chromium.org/2974573002/diff/540001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/540001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:67: public static void checkBacksWebApkAsync(final Context context, final String webApkPackageName, On 2017/07/19 16:09:13, pkotwicz wrote: > Nit: Rename |context| to |browserContext| Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:35: WebApkIdentityServiceClient.CheckBacksWebApkCallback callback) { On 2017/07/19 16:09:13, pkotwicz wrote: > Does the context have to be passed in? Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:43: */ On 2017/07/19 16:09:13, pkotwicz wrote: > The comment is out of date. Perhaps: > "to disconnect singleton WebApkIdentityserviceClient if it hasn't registered > yet" -> "to disconnect all connections to WebAPKs when Chrome is stopped." Thanks! https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:55: ContextUtils.getApplicationContext()); On 2017/07/19 16:09:13, pkotwicz wrote: > I think that disconnecting from both the WebApkIdentityServiceClient and the > WebApkServiceClient is a lot nicer. Thank you for making the change! Acknowledged. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:24: * "WebAPK service". On 2017/07/19 16:09:13, pkotwicz wrote: > Thank you for merging WebApkServiceClient and WebApkNotificationClient. Merging > the two classes makes the implementation much cleaner! > > How about: "that allow browsers communicating with" -> "for browsers to > communicate with" > > Your sentence is an incomplete sentence > "Provides APIs that allow browsers communicating with WebAPK services to do > something special" is a complete sentence > "Provides APIs that allow browsers to do something special" is a complete > sentence > I can't think of a rule for why it is this way unfortunately. > > When I wrote a technical report in university, I got in trouble for using > "allow" and "enable" interchangeably. "allow" and "enable" should not be used > interchangeably as described in this article > https://letlucyedit.wordpress.com/2013/04/03/enable-or-allow-why-you-need-to-... Good article! I haven't thought about this before. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:28: public abstract static class ApiUseCallback On 2017/07/19 16:09:13, pkotwicz wrote: > This can now be private? Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:65: */ On 2017/07/19 16:09:13, pkotwicz wrote: > Can you make this a non-static function? > > Nit: "hand it over" -> "hands it over" Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:97: String webApkPackage, final String platformTag, final int platformID) { On 2017/07/19 16:09:13, pkotwicz wrote: > Can you make this a non-static function? Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java:110: public static void disconnectWebApkService(Context appContext) { On 2017/07/19 16:09:13, pkotwicz wrote: > We don't pass the context for notify() so we don't need to pass the context for > disconnectWebApkService() either > > Nits: > - Rename this function to disconnectAll(). What you are disconnecting from > should be obvious from the class name. > - "from WebAPK services" -> "to WebAPK services" (because Chrome initiated the > connection to the WebAPKService) Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:35: public void testReturnsSpecifiedRuntimeHostBeforeIntroducingHostBrowserSwitchLogic() { On 2017/07/19 16:09:13, pkotwicz wrote: > Thank you for adding JUnit tests! > > I was hoping for tests which called > WebApkIdentityServiceClient#checkBacksWebApkAsync(). That way you don't test the > implementation details of WebApkIdentityService. I think that testing this in a > JUnit test is possible via > ShadowApplication#setComponentNameAndServiceForBindServiceForIntent() Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:25: public interface CheckBacksWebApkCallback { void onChecked(boolean isValid); } On 2017/07/19 16:09:14, pkotwicz wrote: > Nits: > - Rename CheckBacksWebApkCallback to CheckBrowserBacksWebApkCallback (The name > is lengthier but makes things clearer in my opinion) > - Change the parameter name to |doesBrowserBackWebApk| (I think that the > parameter name makes things clearer) Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:30: * introduced a logic to allow user to choose runtime host browser for WebAPKs unbound without On 2017/07/19 16:09:14, pkotwicz wrote: > Thank you very much for adding the comment! It makes things clearer > > Nits: > - "a logic" -> "logic" > - "unbound without any" -> "not bound to any" Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:31: * any browser, and for WebAPKs installed from browsers when the browser isn't installed. On 2017/07/19 16:09:13, pkotwicz wrote: > Nit: "from browsers when the browser isn't installed" -> "from browsers when the > browser is subsequently uninstalled." > > (I think that my version makes it clearer how this scenario can occur) Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. On 2017/07/19 16:09:13, pkotwicz wrote: > Maybe omit the part about "clearing the browser's data". Clearing the browser's > data does not require choosing the runtime host again. I suspect that you mean > WebappDataStorage but this is not obvious. I would prefer to keep it, since it means we can't trust the knowledge from both sides. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. On 2017/07/20 01:32:05, pkotwicz wrote: > Ping on this comment Should publish comments before asking you to review it again:) https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:36: * querying the runtime host of a WebAPK. On 2017/07/19 16:09:14, pkotwicz wrote: > Nit: "querying" -> "to query" Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:57: null /* action */, ACTION_WEBAPK_IDENTITY_SERVICE); On 2017/07/19 16:09:13, pkotwicz wrote: > Shouldn't the second parameter be null? action should be category:( https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:66: */ On 2017/07/19 16:09:14, pkotwicz wrote: > Nits: > - Rename the function to checkBrowserBacksWebApkAsync() > - Rename |context| to browserContext (We get the browser's package name from the > Context) > - Can this method be non static? Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:68: final CheckBacksWebApkCallback callback) { On 2017/07/19 16:09:13, pkotwicz wrote: > In a separate CL, (not this CL!) we should probably try to avoid calling > WebApkIdentityServiceClient#onGotWebApkRuntimeHost() when the service is already > connected Acknowledged. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:69: final String packageName = context.getPackageName(); On 2017/07/19 16:09:14, pkotwicz wrote: > Nits: > - Move this computation to within the callback > - Rename the variable to browserPackageName (Otherwise it is confusing which > package name the variable refers to) Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:98: * @param appPackageName The current app's package name. On 2017/07/19 16:09:13, pkotwicz wrote: > Nit: Rename appPackageName to browserPackageName Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:101: * current app. On 2017/07/19 16:09:14, pkotwicz wrote: > Nits: > - Rename webApkRuntimeHostPackageName to webApkBackingBrowserPackageName (This > is more in line with the wording used elsewhere in thie file) > - "The callback to notify the called whether the runtime host matches the > current app" -> "The callback to notify whether {@link browserPackageName} backs > the WebAPK." > Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:116: */ On 2017/07/19 16:09:14, pkotwicz wrote: > The comment is out of date. This part "the runtime host specified in the > AndroidManifest.xml matches the current app name" is incorrect > > For the comment, perhaps > "Extracts the backing browser from the WebAPK's meta data." > > The case in which the runtime host is not extracted is an implementation detail > of the function Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:121: >= SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST) { On 2017/07/19 16:09:14, pkotwicz wrote: > Nit: Add this comment "The backing browser in the WebAPK's meta data may not be > the one which actually backs the WebAPK. The user may have switched the backing > browser." Done. https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:141: public static void disconnectWebApkIdentityService(Context appContext) { On 2017/07/19 16:09:13, pkotwicz wrote: > Nits: > - "from" -> "to" (Because the connection was initiated by Chrome) > - Rename the function to disconnectAll() I think that what we are disconnecting > is obvious from the class name Done. https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:78: /** The category of the service to connect. */ On 2017/07/19 16:09:14, pkotwicz wrote: > Nit: "to connect" -> "to connect to." Done. https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:81: /** The action of the service to connect. */ On 2017/07/19 16:09:14, pkotwicz wrote: > Nit: "to connect." -> "to connect to." Done. https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:146: Set<Map.Entry<String, Connection>> entrySet = mConnections.entrySet(); On 2017/07/19 16:09:14, pkotwicz wrote: > Can you just get mConnections.values() instead? Done. https://codereview.chromium.org/2974573002/diff/580001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:156: appContext.unbindService(connection); On 2017/07/19 16:09:14, pkotwicz wrote: > You should call Context#unbindServce() even for services which have not > connected yet (otherwise when the service connects it will stay connected as > long as Chrome is alive) Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:71: * Tests that for WebAPKs with shell APK version equals or higher than the On 2017/07/20 01:32:05, pkotwicz wrote: > Nit: equals -> equal Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:73: * doesn't have Identity Service, the backs WebAPK check returns null. On 2017/07/20 01:32:05, pkotwicz wrote: > You are testing that the callback returns false? Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:89: * Tests that for WebAPKs with Identity service, the backs WebAPK check returns true if the On 2017/07/20 01:32:05, pkotwicz wrote: > Nit: Add '-' to "backs WebAPK check" to improve readability -> > "backs-WebAPK-check" > > (here and elsewhere) Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:121: when(context.getPackageName()).thenReturn(browserPackageName); On 2017/07/20 01:32:05, pkotwicz wrote: > Can you set the package name via @Config like we do in WebApkUtilsTest.java? > > You can call registerWebApk() multiple times in a test case instead of calling > mockBrowserContext() multiple times Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:134: mShadowApplication.getApplicationContext(), webApkPackageName, callback); On 2017/07/20 01:32:05, pkotwicz wrote: > - Does WebApkIdentityServiceClient#checkBrowserBacksWebApkAsync() block till the > check is done? > You can probably fix this by calling > Robolectric#runUiThreadTasksIncludingDelayedTasks() and checking that the > callback was indeed called. > - RuntimeEnvironment.application is the same as > ShadowApplication#getApplicationContext() Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:155: when(IIdentityService.Stub.asInterface(service)).thenReturn(identityService); On 2017/07/20 01:32:05, pkotwicz wrote: > Would it be simpler to have a custom implementation which inherits from > IIdentityService.Stub? Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:157: mShadowApplication.setComponentNameAndServiceForBindServiceForIntent( On 2017/07/20 01:32:05, pkotwicz wrote: > Why did you choose to use the 3 argument version of > setComponentNameAndServiceForBindService() instead of the 2 argument version? > (This test class cares about the IdentityService and nothing else. It does not > matter if it is impossible to connect a service which is not the > IdentityService) Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:111: && webApkBackingBrowserPackageName.contentEquals(browserPackageName)); On 2017/07/20 01:32:05, pkotwicz wrote: > Can you use TextUtils#equals() here? Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:122: if (metadata.getInt(WebApkMetaDataKeys.SHELL_APK_VERSION) On 2017/07/21 19:24:27, pkotwicz wrote: > In case you haven't done so, you need to add a null check here Done. https://codereview.chromium.org/2974573002/diff/620001/chrome/browser/notific... File chrome/browser/notifications/notification_platform_bridge_android.cc (right): https://codereview.chromium.org/2974573002/diff/620001/chrome/browser/notific... chrome/browser/notifications/notification_platform_bridge_android.cc:261: void NotificationPlatformBridgeAndroid::DisplayInternal( On 2017/07/20 01:32:05, pkotwicz wrote: > .cc and .h order applies to DisplayInternal() too Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I am splitting this CL, please hold off reviewing. I will make this CL as the one of them to keep the previous comments. But if you already have comments, feel free to publish them, and I will address them in the new CLs. Thanks!
Description was changed from ========== The WebApkService shouldn't be shared between different browsers (via Identity Service). 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. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) introduce WebApkServiceClient and WebApkIdentityServiceClient that manages communication between Chrome and WebAPK service/WebAPK Identity service respectively. 4) add runtime host queries 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. The refactoring of WebApkServiceConnectionManager also fixes notification issue reported in crbug.com/740614. BUG=720077,740614 ========== to ========== Refactor WebApkServiceConnectionManager. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ==========
Description was changed from ========== Refactor WebApkServiceConnectionManager. In this CL, we did the following things: 1) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ========== to ========== 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) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ==========
Description was changed from ========== 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) a refactoring to WebApkServiceConnectionManager to connect any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ========== to ========== 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 any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ==========
Description was changed from ========== 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 any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ========== to ========== 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 be able to connect to any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ==========
Description was changed from ========== 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 be able to connect to any kind of services. 2) introduce IdentityService and its aidl in the shell APK. 3) 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 ========== to ========== 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 IdentityService and its aidl in the shell APK. 3) 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 ==========
Description was changed from ========== 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 IdentityService and its aidl in the shell APK. 3) 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 ========== to ========== 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 ==========
I only keep the refactoring of WebApkConnectionManger part in this CL. PTAL, thanks!
https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. In my view, WebappDataStorage does not represent "owned WebAPKs". There might be WebAPKs which are registered with WebappDataStorage which are no longer backed by Chrome. There might also be WebAPKs not present in WebappDataStorage which are backed by Chrome. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:33: public class WebApkIdentityServiceClientTest { Thank you very much for adding the tests and making them exercise all of WebApkIdentityServiceClient. The tests look good! https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:70: WebApkIdentityServiceClient.disconnectAll(RuntimeEnvironment.application); @After public void tearDown() {} is probably a more logical place to call WebApkIdentityServiceClient#disconnectAll() https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:145: public void testReturnsTrueWhenMatchesRuntimeHostProvidedByIdentityService() { Optional: It is likely that the tests would be easier to read if you inlined the variables (Especially since the tests are so short) When I read registerWebApk() I have to lookup the value of each variable https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:158: private void registerWebApk(String webApkPackageName, String runtimeHost, int shellApkVersion) { It looks like all of the callers to registerWebApk() and checkBacksWebApk() pass in the same |webApkPackageName|. Can you remove the parameters? https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:166: private void checkBacksWebApk(String webApkPackageName, final boolean expectedResult) { - Can this function return the result instead of being passed |expectedResult| ? - Can you rename this function to doesBrowserBackWebApk(). I think that it will make what the function does clearer. With |webApkPackageName| as a parameter it feels like |webApkPackageName| is part of the expectations while it is in fact not part of the expectations https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:110: callback.onChecked(!TextUtils.isEmpty(webApkBackingBrowserPackageName) Now that you are using TextUtils#equals() you can remove the TextUtils#isEmpty() call? https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:144: public void disconnect(final Context appContext) { Nit: You can probably rename this function to disconnectAll() to match calling function name
PTAL, thanks! https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. On 2017/07/24 18:31:31, pkotwicz wrote: > In my view, WebappDataStorage does not represent "owned WebAPKs". There might be > WebAPKs which are registered with WebappDataStorage which are no longer backed > by Chrome. There might also be WebAPKs not present in WebappDataStorage which > are backed by Chrome. Personally I would keep this, since both are the reasons why browser doesn't know whether it is the runtime host of a WebAPK. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:70: WebApkIdentityServiceClient.disconnectAll(RuntimeEnvironment.application); On 2017/07/24 18:31:31, pkotwicz wrote: > @After > public void tearDown() {} > > is probably a more logical place to call > WebApkIdentityServiceClient#disconnectAll() Agreed! https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:145: public void testReturnsTrueWhenMatchesRuntimeHostProvidedByIdentityService() { On 2017/07/24 18:31:31, pkotwicz wrote: > Optional: It is likely that the tests would be easier to read if you inlined the > variables (Especially since the tests are so short) > When I read registerWebApk() I have to lookup the value of each variable I use comments to indicate what are variables for, since I will forget that unless go to the deification of registerWebApk() again. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:158: private void registerWebApk(String webApkPackageName, String runtimeHost, int shellApkVersion) { On 2017/07/24 18:31:31, pkotwicz wrote: > It looks like all of the callers to registerWebApk() and checkBacksWebApk() pass > in the same |webApkPackageName|. Can you remove the parameters? Done. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:166: private void checkBacksWebApk(String webApkPackageName, final boolean expectedResult) { On 2017/07/24 18:31:31, pkotwicz wrote: > - Can this function return the result instead of being passed |expectedResult| ? > > - Can you rename this function to doesBrowserBackWebApk(). > I think that it will make what the function does clearer. With > |webApkPackageName| as a parameter it feels like |webApkPackageName| is part of > the expectations while it is in fact not part of the expectations Done. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:110: callback.onChecked(!TextUtils.isEmpty(webApkBackingBrowserPackageName) On 2017/07/24 18:31:31, pkotwicz wrote: > Now that you are using TextUtils#equals() you can remove the TextUtils#isEmpty() > call? Done. https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:144: public void disconnect(final Context appContext) { On 2017/07/24 18:31:32, pkotwicz wrote: > Nit: You can probably rename this function to disconnectAll() to match calling > function name Done.
LGTM with nits! https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. Maybe change the sentence to: "The browser cannot keep track of WebAPKs which are backed by the browser because when a user changes the browser which backs a WebAPK the browser which previously backed the WebAPK is not notified. A user can switch the browser which backs the WebAPK by clearing the WebAPK's data." https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... File chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java (right): https://codereview.chromium.org/2974573002/diff/700001/chrome/android/webapk/... chrome/android/webapk/libs/client/junit/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClientTest.java:145: public void testReturnsTrueWhenMatchesRuntimeHostProvidedByIdentityService() { Fair enough https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:42: private static final String ACTION_WEBAPK_IDENTITY_SERVICE = "org.webapk.IDENTITY_SERVICE_API"; Have we gotten permission to use the org.webapk namespace? https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:62: public WebApkServiceConnectionManager getConnectionManagerForTesting() { I don't think that your new and improved JUnit tests use this function https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:68: * if the WebAPK either doesn't have an Identity service or hasn't bind to any browser yet. How about: "Checks whether a WebAPK is backed by the browser with {@link browserContext}." It is weird to discuss "the runtime host being null" given that this function does not return the runtime host. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:92: runtimeHost = identityService.getRuntimeHostBrowserPackageName(); If you want, you can add a comment here that |runtimeHost| can be null. I don't think that it is obvious that |runtimeHost| can be null. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:103: * Called after fetching the WebAPK's runtime host browser. Nit: "runtime host" -> "backing" (To match parameter variable name) https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:105: * @param webApkBackingBrowserPackageName The package name of the WebAPK's runtime host browser. Nit: "runtime host" -> "backing" https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:119: public static String maybeExtractRuntimeHostFromMetaData( This function can be private now that it is no longer used in testing https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:84: private Hashtable<String, Connection> mConnections = new Hashtable<String, Connection>(); Now that |mConnections| is only accessed on a single thread, you can probably make this a HashMap.
Hi Yaron, do you have any opinion about using "org.webapk.IDENTITY_SERVICE_API" for the action to bind the IdentityService? Hi Peter B: could you please review changes in: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java. Thanks! https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/560001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:33: * or clear a browser's data that makes the browser loses its knowledge of owned WebAPKs. On 2017/07/24 19:59:48, pkotwicz wrote: > Maybe change the sentence to: > > "The browser cannot keep track of WebAPKs which are backed by the browser > because when a user changes the browser which backs a WebAPK the browser which > previously backed the WebAPK is not notified. A user can switch the browser > which backs the WebAPK by clearing the WebAPK's data."? How about "However, the browser cannot track of WebAPKs which are backed by the browser beacuse the browser is not notificed if a user changes the runtime host of a WebAPK by clearing the WebAPK's data. Besides, the browser loses the knowledge of WebAPKs if a user clears the browser's data." In my version, it includes another factor that user may clears browser's data. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:42: private static final String ACTION_WEBAPK_IDENTITY_SERVICE = "org.webapk.IDENTITY_SERVICE_API"; On 2017/07/24 19:59:48, pkotwicz wrote: > Have we gotten permission to use the org.webapk namespace? Defer this to Yaron. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:62: public WebApkServiceConnectionManager getConnectionManagerForTesting() { On 2017/07/24 19:59:49, pkotwicz wrote: > I don't think that your new and improved JUnit tests use this function Good catch! Removed. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:68: * if the WebAPK either doesn't have an Identity service or hasn't bind to any browser yet. On 2017/07/24 19:59:49, pkotwicz wrote: > How about: "Checks whether a WebAPK is backed by the browser with {@link > browserContext}." > > It is weird to discuss "the runtime host being null" given that this function > does not return the runtime host. Done. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:92: runtimeHost = identityService.getRuntimeHostBrowserPackageName(); On 2017/07/24 19:59:48, pkotwicz wrote: > If you want, you can add a comment here that |runtimeHost| can be null. I don't > think that it is obvious that |runtimeHost| can be null. Done. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:103: * Called after fetching the WebAPK's runtime host browser. On 2017/07/24 19:59:49, pkotwicz wrote: > Nit: "runtime host" -> "backing" (To match parameter variable name) Done. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:105: * @param webApkBackingBrowserPackageName The package name of the WebAPK's runtime host browser. On 2017/07/24 19:59:49, pkotwicz wrote: > Nit: "runtime host" -> "backing" Done. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:119: public static String maybeExtractRuntimeHostFromMetaData( On 2017/07/24 19:59:48, pkotwicz wrote: > This function can be private now that it is no longer used in testing Done. https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:84: private Hashtable<String, Connection> mConnections = new Hashtable<String, Connection>(); On 2017/07/24 19:59:49, pkotwicz wrote: > Now that |mConnections| is only accessed on a single thread, you can probably > make this a HashMap. Done.
https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:42: private static final String ACTION_WEBAPK_IDENTITY_SERVICE = "org.webapk.IDENTITY_SERVICE_API"; On 2017/07/24 20:36:14, Xi Han wrote: > On 2017/07/24 19:59:48, pkotwicz wrote: > > Have we gotten permission to use the org.webapk namespace? > > Defer this to Yaron. Ya, it's a bit strange but we already switched the view action to this namespace: https://codereview.chromium.org/2956993002/ so we're just being consistent https://codereview.chromium.org/2974573002/diff/760001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/760001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:146: mConnections.clear(); Don't you need to have cloned the values or this would empty |values|: https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#values()
Patchset #20 (id:780001) has been deleted
Hi Yaron, PTAL, thanks! https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java (right): https://codereview.chromium.org/2974573002/diff/720001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java:42: private static final String ACTION_WEBAPK_IDENTITY_SERVICE = "org.webapk.IDENTITY_SERVICE_API"; On 2017/07/25 17:00:26, Yaron wrote: > On 2017/07/24 20:36:14, Xi Han wrote: > > On 2017/07/24 19:59:48, pkotwicz wrote: > > > Have we gotten permission to use the org.webapk namespace? > > > > Defer this to Yaron. > > Ya, it's a bit strange but we already switched the view action to this > namespace: https://codereview.chromium.org/2956993002/ so we're just being > consistent Acknowledged. https://codereview.chromium.org/2974573002/diff/760001/chrome/android/webapk/... File chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java (right): https://codereview.chromium.org/2974573002/diff/760001/chrome/android/webapk/... chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java:146: mConnections.clear(); On 2017/07/25 17:00:26, Yaron wrote: > Don't you need to have cloned the values or this would empty |values|: > https://docs.oracle.com/javase/7/docs/api/java/util/HashMap.html#values() Thanks for catching this! I though values() return a copy of the values.
lgtm
Hi Peter B, could you please take a look at file: - chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java Thanks!
The CQ bit was checked by hanxi@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.
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2974573002/#ps800001 (title: "yfriedman@'s comments.")
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": 800001, "attempt_start_ts": 1501019301283830,
"parent_rev": "b31960eae3a13721e8b167b7df7124bcbf60134c", "commit_rev":
"92013be54e9b951d58f1c63366b15344dddc5198"}
CQ is committing da patch.
Bot data: {"patchset_id": 800001, "attempt_start_ts": 1501019301283830,
"parent_rev": "27cc7ed987ea51d2096df60ad5e7a03154c36b2d", "commit_rev":
"fb0b8d2f550e397f6c609273d4e4ad9a9e440705"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/fb0b8d2f550e397f6c609273d4e4... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:800001) as https://chromium.googlesource.com/chromium/src/+/fb0b8d2f550e397f6c609273d4e4... |
