Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2515293004:
Chrome talks to Play to install WebAPKs. (Closed)
DescriptionChrome talks to Play to install WebAPKs.
This CL includes:
- Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's
unpublished PlayInstallService to install WebAPKs.
- WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to
install WebAPKs. The changes are behind the finch flag.
The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/
BUG=662149
Committed: https://crrev.com/c3aaa23c1f5dc55fb7867fd32be50e27a343a069
Cr-Commit-Position: refs/heads/master@{#438001}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Nits. #Patch Set 3 : Add finch flag and don't use InstallerDelegate to monitor Play install. #
Total comments: 1
Patch Set 4 : Make "play_install_webapk" as a param of finch flag. #Patch Set 5 : Rebase. #
Total comments: 2
Patch Set 6 : Move play install-related code out of WebApkInstaller. #
Total comments: 44
Patch Set 7 : Renaming and nits. #Patch Set 8 : Don't use play install in webapk_installer_unittest. #
Total comments: 20
Patch Set 9 : Add a test for play install webapk. #
Total comments: 6
Patch Set 10 : Nits. #Patch Set 11 : Upload the missing GooglePlayWebApkInstallDelegate. #
Total comments: 22
Patch Set 12 : dominickn@'s comments. #Patch Set 13 : Changes due to Play's changes on the install API. #Patch Set 14 : Make GooglePlayWebApkInstallDelegate as a singleton. #Messages
Total messages: 70 (39 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Chrome talks to Play to install WebAPKs. BUG=662149 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the flag "enable-play-install-improved-a2hs". BUG=662149 ==========
Description was changed from ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the flag "enable-play-install-improved-a2hs". BUG=662149 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the flag "enable-play-install-improved-a2hs". The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org
Hi Peter, could you please take a look? 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.
Patchset #1 (id:80001) has been deleted
https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:17: optional string version = 2; If Chrome wants this to be an int, can we change the proto to expect an int? We might need the int version to have a different "proto number" https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:22: // The token to give the Phonesky backend for the rest of the information. How about: "Unique id identifying session with WebAPK server." Maybe Scott has ideas for naming/documenting the proto member. I am not a big fan of the "token" name https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:338: std::atoi(response->version().c_str()), Random thought: Why isn't the version part of the token?
https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:17: optional string version = 2; On 2016/11/23 03:11:28, pkotwicz wrote: > If Chrome wants this to be an int, can we change the proto to expect an int? We > might need the int version to have a different "proto number" Talked to Glenn offline, the change would break all the existing installed WebAPKs, so we have to deprecate this and add a new int "version". I think this is your suggestion too. It seems the simplest way is to keep this and do the conversion in Chrome. https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:22: // The token to give the Phonesky backend for the rest of the information. On 2016/11/23 03:11:28, pkotwicz wrote: > How about: "Unique id identifying session with WebAPK server." Updated. > Maybe Scott has ideas for naming/documenting the proto member. > I am not a big fan of the "token" name The webapk.proto declared on the server side uses "token", and Play also have "token" as a key word in the installOptions. Personally, I would prefer to use "token" to avoid renaming on all these places. https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:338: std::atoi(response->version().c_str()), On 2016/11/23 03:11:28, pkotwicz wrote: > Random thought: Why isn't the version part of the token? Scott confirmed that the token is just a random number and doesn't contain any other information, all the useful information is cached on the server. Personally, I would prefer to keep the version as a separate field like now.
https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:17: optional string version = 2; Fair enough https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:22: // The token to give the Phonesky backend for the rest of the information. Let's rename it in Chrome so that at least Chrome uses a more meaningful name than "token" https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:338: std::atoi(response->version().c_str()), I dislike that the token is a random number. If it's truly random, than there is a small chance of collisions and it is possible for a person to install a different WebAPK than they intended. Can it be the WebAPK id with a random number appended to it? I still don't understand why passing the version to Google Play is important. Is passing the version number a "Google Play requirement" or a "WebAPK server requirement"?
https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:22: // The token to give the Phonesky backend for the rest of the information. On 2016/11/23 15:56:38, pkotwicz wrote: > Let's rename it in Chrome so that at least Chrome uses a more meaningful name > than "token" How about "wam_token" as the one sent to Play? https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:338: std::atoi(response->version().c_str()), On 2016/11/23 15:56:38, pkotwicz wrote: > I dislike that the token is a random number. If it's truly random, than there is > a small chance of collisions and it is possible for a person to install a > different WebAPK than they intended. Can it be the WebAPK id with a random > number appended to it? > > I still don't understand why passing the version to Google Play is important. Is > passing the version number a "Google Play requirement" or a "WebAPK server > requirement"? As discussed offline, the version is required by Play, and we keep it as it is now.
Patchset #4 (id:160001) has been deleted
Patchset #3 (id:140001) has been deleted
I update this CL according to the internal one. For the naming of the token, how about "wam_token"?
The CL looks pretty good I want to push a bit further with finding a descriptive name for the proto field. I will bug Scott offline :) https://codereview.chromium.org/2515293004/diff/180001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/180001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:351: response->token()); Ideally, Google Play installs will work when: - Google Play installs are enabled via variations - The user is not using chrome_public_apk Perhaps we should subclass WebApkInstaller, GooglePlayWebApkInstaller perhaps?
The CL looks pretty good I want to push a bit further with finding a descriptive name for the proto field. I will bug Scott offline :)
Patchset #4 (id:200001) has been deleted
Description was changed from ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the flag "enable-play-install-improved-a2hs". The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ==========
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #4 (id:220001) has been deleted
I change the naming of "token" to "wam_token" and also update it accordingly since the "play_install_webapks" is used as a param of the finch flag. I would prefer to make the refactory to have a PlayWebApkInstaller in a separate CL if needed. PTAL, thanks!
Patchset #5 (id:260001) has been deleted
https://codereview.chromium.org/2515293004/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:437: void WebApkInstaller::InstallWebApkFromPlayStore( We should probably rename this method to InstallOrUpdateWebApkFromPlayStore() unless update is not yet implemented
Patchset #6 (id:300001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2515293004/diff/280001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/280001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:437: void WebApkInstaller::InstallWebApkFromPlayStore( On 2016/12/01 04:18:04, pkotwicz wrote: > We should probably rename this method to InstallOrUpdateWebApkFromPlayStore() > > unless update is not yet implemented Good catch! The update part is added.
Mostly nits! You should probably add a sanity check for the Google Play code path to webapk_installer_unittest.cc https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:11: * WebAPKs. How about: "Defines an interface for installing WebAPKs via Google Play." - Phonesky is the Google-internal name for the Google-Play-Client. We shouldn't use it in external facing code. - Binding to PlayInstallService is an implementation detail https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:12: */ How about: GooglePlayWebApkInstallDelegate ClientDelegate is more complicated than needed. (Yes I know that this class talks to the Google Play client) I remember that someone in the Chrome team in our office really wanted to get some complicated named classes into the code base. This was back when we had a class named RenderWidgetHostViewViews https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:15: * Binds to Phonesky's PlayInstallService and installs WebAPKs asynchronously. How about: "Uses Google Play to install WebAPK asynchronously." https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:19: * @param wamToken The token from WebAPK Minter Server. Nit: wamToken -> token People outside of our team do not know the meaning of the "wam" acronym (Some people might be from the city of Wam in Pakistan though) https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:21: * started. A "true" return value does not guarantee that the install succeeds. How about: True if the install was started. A "true" value does not... https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:29: void reset(); reset() does not seem to be called by any of the open source code. Can we remove it from the interface? https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:38: /** Flag to enable installing WebAPKs from Play Store. */ "Play Store" -> "Google Play" https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:39: private static final String PLAY_INSTALL_WEBAPKS = "play_install_webapks"; Nit: You can name the parameter "play_install". WebAPKs is redundant because this is a parameter for the WebAPK feature https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:53: /** Talks to Play Store to install WebAPKs. */ "Play Store" -> "Google Play" Can you please put the non-final-static-members either before or after the non-final-non-static-members? https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:60: private static boolean sIsWebApkInstallClientDelegateInitialized; I think that you can remove this variable. I am not worried about running canUseGooglePlayToInstallWebApk() each time that ChromeApplication#createWebApkInstallClientDelegate() is called https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:80: /** Return whether installing WebAPKs from Play Store is enabled. */ "from Play Store" -> "using Google Play" WebAPKs are not listed in the Play Store :) https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:81: public static boolean enablePlayInstallWebApk() { How about: canUseGooglePlayToInstallWebApk() Your function name makes it sound like this function "enables installing WebAPKs from Google Play" Should this function be part of ChromeWebApkHost? https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:84: ChromeSwitches.ENABLE_WEBAPK, PLAY_INSTALL_WEBAPKS), "true"); ChromeSwitches.ENABLE_WEBAPK -> ChromeFeatureList.WEBAPKS https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:129: } Nit: Group all of the @CalledByNative functions together https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:132: * Install a WebAPK from Play Store and monitors the installation. Nit: monitors -> monitor https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:136: * @param wamToken The token from WebAPK Minter Server. Nit: We use "WebAPK Server" elsewhere. Might as well keep on doing this. (We only have one WebAPK server so we don't need to specify "which WebAPK server") https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:212: Please add a comment https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:23: optional string wam_token = 6; I like "token" better than "wam_token". People outside our team don't know what the "wam" acronym stands for. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:358: if (IsPlayInstallWebApksEnabled() && Is the IsPlayInstallWebApksEnabled() check necessary? https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:360: InstallOrUpdateWebApkFromPlayStore(response->package_name(), Nit: InstallOrUpdateWebApkFromPlayStore() -> InstallOrUpdateWebApkFromGooglePlay() https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:361: std::atoi(response->version().c_str()), Nit: Use base::StringToInt() https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:147: // or update request goes through Play Store. How about: "and the install or update request should be handled by Google Play."
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
I moved the initialization of the static GooglePlayWebApkInstallDelegate into the DeferredStartupHandler. It avoids multiple initialization of a static member. PTAL, thanks! https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:11: * WebAPKs. On 2016/12/02 02:23:59, pkotwicz wrote: > How about: "Defines an interface for installing WebAPKs via Google Play." > > - Phonesky is the Google-internal name for the Google-Play-Client. We shouldn't > use it in external facing code. > - Binding to PlayInstallService is an implementation detail Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:12: */ On 2016/12/02 02:23:59, pkotwicz wrote: > How about: GooglePlayWebApkInstallDelegate > > ClientDelegate is more complicated than needed. > (Yes I know that this class talks to the Google Play client) > > I remember that someone in the Chrome team in our office really wanted to get > some complicated named classes into the code base. This was back when we had a > class named RenderWidgetHostViewViews Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:15: * Binds to Phonesky's PlayInstallService and installs WebAPKs asynchronously. On 2016/12/02 02:23:59, pkotwicz wrote: > How about: "Uses Google Play to install WebAPK asynchronously." Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:19: * @param wamToken The token from WebAPK Minter Server. On 2016/12/02 02:23:59, pkotwicz wrote: > Nit: wamToken -> token > > People outside of our team do not know the meaning of the "wam" acronym > > (Some people might be from the city of Wam in Pakistan though) Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:21: * started. A "true" return value does not guarantee that the install succeeds. On 2016/12/02 02:23:59, pkotwicz wrote: > How about: True if the install was started. A "true" value does not... Well, I think we can count the "bind to the PlayInstallService" as the install was started. I listed that case here because for the first request, we need to bind to the service first, and then execute an async task to call the install API. So the direct return value means whether we can bind to the service successfully, not call to the install API successfully. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstallClientDelegate.java:29: void reset(); On 2016/12/02 02:23:59, pkotwicz wrote: > reset() does not seem to be called by any of the open source code. Can we remove > it from the interface? Yes, that is what I find when I am polishing the internal CL. Removed. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:38: /** Flag to enable installing WebAPKs from Play Store. */ On 2016/12/02 02:24:00, pkotwicz wrote: > "Play Store" -> "Google Play" Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:39: private static final String PLAY_INSTALL_WEBAPKS = "play_install_webapks"; On 2016/12/02 02:24:00, pkotwicz wrote: > Nit: You can name the parameter "play_install". WebAPKs is redundant because > this is a parameter for the WebAPK feature Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:53: /** Talks to Play Store to install WebAPKs. */ On 2016/12/02 02:24:00, pkotwicz wrote: > "Play Store" -> "Google Play" > > Can you please put the non-final-static-members either before or after the > non-final-non-static-members? Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:60: private static boolean sIsWebApkInstallClientDelegateInitialized; On 2016/12/02 02:24:00, pkotwicz wrote: > I think that you can remove this variable. I am not worried about running > canUseGooglePlayToInstallWebApk() each time that > ChromeApplication#createWebApkInstallClientDelegate() is called Hmmm, it is still wired that we initialize a static variable multiple times. Move the initialization to DeferredStartupHandler. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:80: /** Return whether installing WebAPKs from Play Store is enabled. */ On 2016/12/02 02:24:00, pkotwicz wrote: > "from Play Store" -> "using Google Play" > > WebAPKs are not listed in the Play Store :) Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:81: public static boolean enablePlayInstallWebApk() { On 2016/12/02 02:24:00, pkotwicz wrote: > How about: canUseGooglePlayToInstallWebApk() > > Your function name makes it sound like this function "enables installing WebAPKs > from Google Play" > > Should this function be part of ChromeWebApkHost? Moved. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:84: ChromeSwitches.ENABLE_WEBAPK, PLAY_INSTALL_WEBAPKS), "true"); On 2016/12/02 02:24:00, pkotwicz wrote: > ChromeSwitches.ENABLE_WEBAPK -> ChromeFeatureList.WEBAPKS Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:129: } On 2016/12/02 02:23:59, pkotwicz wrote: > Nit: Group all of the @CalledByNative functions together Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:132: * Install a WebAPK from Play Store and monitors the installation. On 2016/12/02 02:24:00, pkotwicz wrote: > Nit: monitors -> monitor Should be Install->Installs. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:136: * @param wamToken The token from WebAPK Minter Server. On 2016/12/02 02:24:00, pkotwicz wrote: > Nit: We use "WebAPK Server" elsewhere. Might as well keep on doing this. (We > only have one WebAPK server so we don't need to specify "which WebAPK server") Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:212: On 2016/12/02 02:24:00, pkotwicz wrote: > Please add a comment Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk.proto:23: optional string wam_token = 6; On 2016/12/02 02:24:00, pkotwicz wrote: > I like "token" better than "wam_token". People outside our team don't know what > the "wam" acronym stands for. All right, change it back. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:358: if (IsPlayInstallWebApksEnabled() && On 2016/12/02 02:24:00, pkotwicz wrote: > Is the IsPlayInstallWebApksEnabled() check necessary? It isn't any more, removed. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:360: InstallOrUpdateWebApkFromPlayStore(response->package_name(), On 2016/12/02 02:24:00, pkotwicz wrote: > Nit: InstallOrUpdateWebApkFromPlayStore() -> > InstallOrUpdateWebApkFromGooglePlay() > Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:361: std::atoi(response->version().c_str()), On 2016/12/02 02:24:00, pkotwicz wrote: > Nit: Use base::StringToInt() Done. https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2515293004/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:147: // or update request goes through Play Store. On 2016/12/02 02:24:00, pkotwicz wrote: > How about: "and the install or update request should be handled by Google Play." Done.
L-G-T-M once you add a test for the Google Play installation path https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:398: */ How about: "Creates an instance of GooglePlayWebApkInstallDelegate." Mentioning that GooglePlayWebApkInstallDelegate talks to Google Play is redundant https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: if (ChromeWebApkHost.canUseGooglePlayToInstallWebApk()) { Shouldn't this be called prior to WebApkVersionManager#updateWebApksIfNeeded() I know that it does not matter because WebApkVersionManager#updateWebApksIfNeeded() is asynchronous but WebApkVersionManager#updateWebApksIfNeeded() might need to use the GooglePlayWebApkInstallDelegate https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:277: (ChromeApplication) ContextUtils.getApplicationContext(); Shoudn't you use mAppContext? https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:53: public static boolean canUseGooglePlayToInstallWebApk() { Personally I would check isEnabled() too (just to make sure) https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:120: int version, String title, String token) { Shouldn't this method return a boolean? So that the UI is notified if the install fails (because sGooglePlayWebApkInstallDelegate is null) https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:155: String token) { Shouldn't this method return a boolean? So that the UI is notified if the install fails (because sGooglePlayWebApkInstallDelegate is null) https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:7: #include <stdlib.h> Do you need this include? https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:355: response->package_name(), version, response->token()); Once InstallOrUpdateWebApkFromGooglePlay() returns a boolean, we should run OnFailure() if the function returns false https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:338: } Can you add a test testing the Google Play installation path? Make sure that OnComplete() is eventually called
https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:160: sGooglePlayWebApkInstallDelegate.installAsync(packageName, version, title, token, null); You should return the return value of GooglePlayWebApkInstallDelegate#installAsync() here
Patchset #9 (id:420001) has been deleted
I also updated the internal CL, PTAL, thanks! https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:398: */ On 2016/12/02 22:27:04, pkotwicz wrote: > How about: "Creates an instance of GooglePlayWebApkInstallDelegate." > > Mentioning that GooglePlayWebApkInstallDelegate talks to Google Play is > redundant Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: if (ChromeWebApkHost.canUseGooglePlayToInstallWebApk()) { On 2016/12/02 22:27:04, pkotwicz wrote: > Shouldn't this be called prior to WebApkVersionManager#updateWebApksIfNeeded() > > I know that it does not matter because > WebApkVersionManager#updateWebApksIfNeeded() is asynchronous but > WebApkVersionManager#updateWebApksIfNeeded() might need to use the > GooglePlayWebApkInstallDelegate Hmm, I don't think WebApkVersionManager#updateWebApksIfNeeded() needs GooglePlayWebApkInstallDelegate, since it is used to extract runtime dex but not really "update" any WebAPK. I am ok to call it first, but the order doesn't matter. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:277: (ChromeApplication) ContextUtils.getApplicationContext(); On 2016/12/02 22:27:04, pkotwicz wrote: > Shoudn't you use mAppContext? Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:53: public static boolean canUseGooglePlayToInstallWebApk() { On 2016/12/02 22:27:04, pkotwicz wrote: > Personally I would check isEnabled() too (just to make sure) Ok, it did bother me a lot that whether isEnable() should be called here. Add it back. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:120: int version, String title, String token) { On 2016/12/02 22:27:04, pkotwicz wrote: > Shouldn't this method return a boolean? So that the UI is notified if the > install fails (because sGooglePlayWebApkInstallDelegate is null) Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:155: String token) { On 2016/12/02 22:27:04, pkotwicz wrote: > Shouldn't this method return a boolean? So that the UI is notified if the > install fails (because sGooglePlayWebApkInstallDelegate is null) Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:160: sGooglePlayWebApkInstallDelegate.installAsync(packageName, version, title, token, null); On 2016/12/02 22:52:21, pkotwicz wrote: > You should return the return value of > GooglePlayWebApkInstallDelegate#installAsync() here Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:7: #include <stdlib.h> On 2016/12/02 22:27:04, pkotwicz wrote: > Do you need this include? Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:355: response->package_name(), version, response->token()); On 2016/12/02 22:27:04, pkotwicz wrote: > Once InstallOrUpdateWebApkFromGooglePlay() returns a boolean, we should run > OnFailure() if the function returns false Done. https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/400001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:338: } On 2016/12/02 22:27:04, pkotwicz wrote: > Can you add a test testing the Google Play installation path? > Make sure that OnComplete() is eventually called Done.
LGTM! https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: bool WebApkInstaller::InstallOrUpdateWebApkFromGooglePlay( Nit: Reorder this function to match order in .h file https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:116: void setHasGooglePlayWebApkInstallDelegate( Nit: setHasGooglePlayWebApkInstallDelegate() -> SetHasGooglePlayWebApkInstallDelegate() https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:117: bool has_google_play_webapk_install_delegate) { Nit: You can rename the parameter to |has_delegate| the meaning of the variable is obvious from the method name :)
Patchset #10 (id:460001) has been deleted
Description was changed from ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce WebApkInstallClientDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the WebApkInstallClientDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ==========
I address all your comments and upload the "MISSING" GooglePlayWebApkInstallDelegate. PTAL, thanks! https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: bool WebApkInstaller::InstallOrUpdateWebApkFromGooglePlay( On 2016/12/06 16:46:32, pkotwicz wrote: > Nit: Reorder this function to match order in .h file Done. https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:116: void setHasGooglePlayWebApkInstallDelegate( On 2016/12/06 16:46:32, pkotwicz wrote: > Nit: setHasGooglePlayWebApkInstallDelegate() -> > SetHasGooglePlayWebApkInstallDelegate() Sorry for the mix of Java naming. https://codereview.chromium.org/2515293004/diff/440001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:117: bool has_google_play_webapk_install_delegate) { On 2016/12/06 16:46:32, pkotwicz wrote: > Nit: You can rename the parameter to |has_delegate| the meaning of the variable > is obvious from the method name :) Done.
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, this CL + the internal one (https://chrome-internal-review.googlesource.com/#/c/305912/) implements the calling of Play API to install WebAPKs. Could you please take a look? Thanks!
On 2016/12/06 22:41:55, Xi Han wrote: > Hi Dominick, > this CL + the internal one > (https://chrome-internal-review.googlesource.com/#/c/305912/) implements the > calling of Play API to install WebAPKs. Could you please take a look? Thanks! (I'm working through the two reviews. Hopefully have them back to you my time tomorrow).
First pass - possibly some dumb questions. :) https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: ChromeApplication application = (ChromeApplication) mAppContext; mAppContext is just the result of calling ContextUtils.getApplicationContext(), right? If so, maybe you don't need to initialize this here, and you can just fetch the PlayInstaller off the application context in WebAplInstaller when you need it? https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:17: public interface GooglePlayWebApkInstallDelegate { Can this just be the PlayWebApkInstaller? Or some other name that doesn't involve InstallDelegate? WebApkInstaller.java already has an InstallerDelegate to monitor the installation status of an APK, and I found myself getting confused with the two. https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:39: * @param token The token from WebAPK Minter Server. Missing an @param for callback https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:50: private static GooglePlayWebApkInstallDelegate sGooglePlayWebApkInstallDelegate; Is there a reason why you need this to be a static variable on this object? Could we fetch it off ContextUtils.getApplicationContext when needed? https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:73: private boolean hasGooglePlayWebApkInstallDelegate() { canInstallFromPlay? https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:374: int version = 1; Set this to 0 by default, and check the return value of base::StringToInt. You should fail if the conversion didn't succeed. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:109: virtual bool HasGooglePlayWebApkInstallDelegate(); CanInstallFromPlay? https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:100: bool has_google_play_webapk_install_delegate_; Can you just call this variable "allow_install_from_play_"? That makes it more obvious. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:116: void SetHasGooglePlayWebApkInstallDelegate(bool has_delegate) { Can this be SetAllowInstallFromPlay(bool allow)? https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:184: bool has_google_play_webapk_install_delegate_; Again, can you call this "allow_install_from_play_"?
Thanks for all the comments. PTAL, thanks! https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: ChromeApplication application = (ChromeApplication) mAppContext; On 2016/12/08 03:31:26, dominickn wrote: > mAppContext is just the result of calling ContextUtils.getApplicationContext(), > right? If so, maybe you don't need to initialize this here, and you can just > fetch the PlayInstaller off the application context in WebAplInstaller when you > need it? Yes, we could move it back to the WebApkInstaller. The only reason that it is here is: we want to initialize the delegate only once. This delegates binds to the Play install service, and we want to reuse the existing connection whenever it is possible. So we only keep one instance of the delegate in the WebApkInstaller. Additionally, sometimes the delegate is null (e.g. user use chrome_public_apk which doesn't have clank code), so we don't know whether it has been initialized by checking whether the object is null. We could have another static flag set in the WebApkInstaller, and that was what I did before. I am not sure which way is better: whether initialing it here, or in WebApkInstaller with a static flag to indicate whether it is has been initialized or not. Any suggestion? https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:17: public interface GooglePlayWebApkInstallDelegate { On 2016/12/08 03:31:26, dominickn wrote: > Can this just be the PlayWebApkInstaller? Or some other name that doesn't > involve InstallDelegate? WebApkInstaller.java already has an InstallerDelegate > to monitor the installation status of an APK, and I found myself getting > confused with the two. PlayWebApkInstallClientInterface? The delegate is a "singleton" object that the WebApkInstaller can delegate the install/update task to. Ideally it should be a subclass of WebApkInstaller, but the singleton design of this delegate makes it impossible to subclass without big refactoring of WebApkInstaller, because we create new WebApkInstaller object for every install/update request. Therefore, we need to name it something different than Installer. https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:39: * @param token The token from WebAPK Minter Server. On 2016/12/08 03:31:26, dominickn wrote: > Missing an @param for callback Done. https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:50: private static GooglePlayWebApkInstallDelegate sGooglePlayWebApkInstallDelegate; On 2016/12/08 03:31:26, dominickn wrote: > Is there a reason why you need this to be a static variable on this object? > Could we fetch it off ContextUtils.getApplicationContext when needed? This is because this delegate binds to the Play install service, and we want to reuse the connection whenever it is possible. Therefore, we want only one object of the |GooglePlayWebApkInstallDelegate|. https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:73: private boolean hasGooglePlayWebApkInstallDelegate() { On 2016/12/08 03:31:26, dominickn wrote: > canInstallFromPlay? Even with the "play-install-webapk" flag turning on, if user use chrome_public_apk, they still cannot install the WebAPK by Play, since the real code is hidden in the clank repository. So the |hasGooglePlayWebApkInstallDelegate| is slightly different than |canInstallFromPlay|. Updated the function description in webapk_installer.h. Sorry for the confusion. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:374: int version = 1; On 2016/12/08 03:31:26, dominickn wrote: > Set this to 0 by default, and check the return value of base::StringToInt. You > should fail if the conversion didn't succeed. Good catch, but maybe we still want to continue the play install in this case. It is because the version is required by Play and they only need it when showing a notification during installation. It would be fine if the version of the WebAPK is just a default "1", I guess. The notification will disappear soon. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:109: virtual bool HasGooglePlayWebApkInstallDelegate(); On 2016/12/08 03:31:26, dominickn wrote: > CanInstallFromPlay? Update the description here. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:100: bool has_google_play_webapk_install_delegate_; On 2016/12/08 03:31:26, dominickn wrote: > Can you just call this variable "allow_install_from_play_"? That makes it more > obvious. Same here. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:116: void SetHasGooglePlayWebApkInstallDelegate(bool has_delegate) { On 2016/12/08 03:31:26, dominickn wrote: > Can this be SetAllowInstallFromPlay(bool allow)? Same here. https://codereview.chromium.org/2515293004/diff/500001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:184: bool has_google_play_webapk_install_delegate_; On 2016/12/08 03:31:26, dominickn wrote: > Again, can you call this "allow_install_from_play_"? Updated the description too.
https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: ChromeApplication application = (ChromeApplication) mAppContext; On 2016/12/08 18:03:07, Xi Han wrote: > On 2016/12/08 03:31:26, dominickn wrote: > > mAppContext is just the result of calling > ContextUtils.getApplicationContext(), > > right? If so, maybe you don't need to initialize this here, and you can just > > fetch the PlayInstaller off the application context in WebAplInstaller when > you > > need it? > > Yes, we could move it back to the WebApkInstaller. The only reason that it is > here is: we want to initialize the delegate only once. This delegates binds to > the Play install service, and we want to reuse the existing connection whenever > it is possible. So we only keep one instance of the delegate in the > WebApkInstaller. The delegate lives on the application context, so it's only initialised once. > > Additionally, sometimes the delegate is null (e.g. user use chrome_public_apk > which doesn't have clank code), so we don't know whether it has been initialized > by checking whether the object is null. We could have another static flag set in > the WebApkInstaller, and that was what I did before. I am not sure which way is > better: whether initialing it here, or in WebApkInstaller with a static flag to > indicate whether it is has been initialized or not. Any suggestion? > One reason to not do something here is that the deferred startup handler might not run tasks at all. Most of the stuff in here is cache or data clearing that doesn't matter if it doesn't run. However, initialising the Play install delegate must happen reliably. How expensive is it to repeatedly construct the Play install delegate? If you really want to only create it once, then the right thing to do is to make it a singleton. I suggest: * renaming the createGooglePlayWebApkInstallDelegate() method to getGooglePlayWebApkInstallDelegate() * making PlayInstallWebApkClient a singleton using the Holder idiom (see PermissionDialogController for an example) * making getGooglePlayWebApkInstallDelegate() inside ChromeApplicationInternal simply return PlayInstallWebApkClient.getInstance(), which will lazily initialise the singleton (once) if you use the Holder pattern Then getGooglePlayWebApkInstallDelegate() is cheap and initialises the delegate once only. Hence WebApkInstaller can just have the delegate as a member variable and fetch it in its constructor since the singleton-ness is handled where it should be - on the singleton class. Does that make sense?
Patchset #14 (id:560001) has been deleted
I have adopt the singleton pattern you suggested, PTAL. Thanks! https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2515293004/diff/500001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:275: ChromeApplication application = (ChromeApplication) mAppContext; On 2016/12/12 03:15:24, dominickn wrote: > On 2016/12/08 18:03:07, Xi Han wrote: > > On 2016/12/08 03:31:26, dominickn wrote: > > > mAppContext is just the result of calling > > ContextUtils.getApplicationContext(), > > > right? If so, maybe you don't need to initialize this here, and you can just > > > fetch the PlayInstaller off the application context in WebAplInstaller when > > you > > > need it? > > > > Yes, we could move it back to the WebApkInstaller. The only reason that it is > > here is: we want to initialize the delegate only once. This delegates binds to > > the Play install service, and we want to reuse the existing connection > whenever > > it is possible. So we only keep one instance of the delegate in the > > WebApkInstaller. > > The delegate lives on the application context, so it's only initialised once. > > > > > Additionally, sometimes the delegate is null (e.g. user use chrome_public_apk > > which doesn't have clank code), so we don't know whether it has been > initialized > > by checking whether the object is null. We could have another static flag set > in > > the WebApkInstaller, and that was what I did before. I am not sure which way > is > > better: whether initialing it here, or in WebApkInstaller with a static flag > to > > indicate whether it is has been initialized or not. Any suggestion? > > > > One reason to not do something here is that the deferred startup handler might > not run tasks at all. Most of the stuff in here is cache or data clearing that > doesn't matter if it doesn't run. However, initialising the Play install > delegate must happen reliably. > > How expensive is it to repeatedly construct the Play install delegate? If you > really want to only create it once, then the right thing to do is to make it a > singleton. I suggest: > > * renaming the createGooglePlayWebApkInstallDelegate() method to > getGooglePlayWebApkInstallDelegate() > * making PlayInstallWebApkClient a singleton using the Holder idiom (see > PermissionDialogController for an example) > * making getGooglePlayWebApkInstallDelegate() inside ChromeApplicationInternal > simply return PlayInstallWebApkClient.getInstance(), which will lazily > initialise the singleton (once) if you use the Holder pattern > > Then getGooglePlayWebApkInstallDelegate() is cheap and initialises the delegate > once only. Hence WebApkInstaller can just have the delegate as a member variable > and fetch it in its constructor since the singleton-ness is handled where it > should be - on the singleton class. > > Does that make sense? I see, yes, we shouldn't trust DeferredStartupHandler to complete all the required work. Thanks for all the suggestions. I adopted the singleton pattern since it is the right way to do.
lgtm. Thanks for the singleton change, I think it's a lot cleaner now.
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, I need OWNERS review for: - chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java Could you please take a look? 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...
That file LGTM.
The CQ bit was unchecked by hanxi@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/2515293004/#ps580001 (title: "Make GooglePlayWebApkInstallDelegate as a singleton.")
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": 580001, "attempt_start_ts": 1481591082898810,
"parent_rev": "65d75e503e86405121438f7b3e3a1678b5590b60", "commit_rev":
"1809bf3cf96fe983c88b1d39afacdcdbc8a634a6"}
Message was sent while issue was closed.
Description was changed from ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 Review-Url: https://codereview.chromium.org/2515293004 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 Review-Url: https://codereview.chromium.org/2515293004 ========== to ========== Chrome talks to Play to install WebAPKs. This CL includes: - Introduce GooglePlayWebApkInstallDelegate which can bind to Phonesky's unpublished PlayInstallService to install WebAPKs. - WebApkInstaller uses the GooglePlayWebApkInstallDelegate to ask Play to install WebAPKs. The changes are behind the finch flag. The internal CL is: https://chrome-internal-review.googlesource.com/#/c/305912/ BUG=662149 Committed: https://crrev.com/c3aaa23c1f5dc55fb7867fd32be50e27a343a069 Cr-Commit-Position: refs/heads/master@{#438001} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c3aaa23c1f5dc55fb7867fd32be50e27a343a069 Cr-Commit-Position: refs/heads/master@{#438001} |
