Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2675803004:
Fall back to shortcut A2HS when Phonesky is out of date or unavailable. (Closed)
DescriptionFall back to shortcut A2HS when Phonesky is out of date or unavailable.
If Google Play Install gServices flags aren't turned on, the WebAPK
install will fail when the Google Play Install flow is allowed via
Finch flags.
This CLs adds a check of whether the Google Play install API is
available or not. If not, we will fallback to classic A2HS shortcut
flow.
The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/
Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv5w/edit#
BUG=687222
Review-Url: https://codereview.chromium.org/2675803004
Cr-Commit-Position: refs/heads/master@{#448715}
Committed: https://chromium.googlesource.com/chromium/src/+/9c327a61afee72b188f3e80f6bfb576836b8ce1c
Patch Set 1 #
Total comments: 9
Patch Set 2 : Show "open" button if a WebApk has been installed and the App banner shows. #Patch Set 3 : Update add_to_homescreen_data_fetcher_unittest.cc. #
Total comments: 12
Patch Set 4 : pkotwicz@'s comments. #
Total comments: 3
Patch Set 5 : dominickn@'s comments. #
Total comments: 18
Patch Set 6 : Nits. #
Total comments: 4
Patch Set 7 : Nits. #Patch Set 8 : Add isEnabled() check in ChromeWebApkHost#isGooglePlayInstallEnabledByChromeFeature(). #
Total comments: 2
Patch Set 9 : Clean up. #Patch Set 10 : Rebase. #Messages
Total messages: 74 (50 generated)
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. BUG=687222 ==========
Patchset #1 (id:1) has been deleted
hanxi@google.com changed reviewers: + hanxi@google.com, pkotwicz@chromium.org
Hi Peter, The internal CL is https://chrome-internal-review.googlesource.com/c/323685/. Could you please take a look? Thanks!
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ BUG=687222 ==========
hanxi@chromium.org changed reviewers: - hanxi@google.com
hanxi@google.com changed reviewers: + hanxi@google.com
I added a fix for App banner, please see patch set #2. Only the first three changed files are due to my changes, others are due to rebase. PTAL, thanks!
The CQ bit was checked by hanxi@google.com 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...
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ==========
hanxi@google.com changed reviewers: - hanxi@google.com
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Still looking at the CL. Here are my comments so far https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:86: private void canUseGooglePlayInstallApi() { I prefer the pattern that Gonzalo used (passing a pointer to the native callback) in ShortcutHelper#retrieveWebApks() https://codereview.chromium.org/2629573004/diff/440001/chrome/android/java/sr... https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:93: Should you cache the result? https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:102: delegate.canUseInstallApi("org.chromium.test", 0, "test", "", "", callback); These parameters should an implementation detail of GooglePlayWebApkInstallDelegate https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:107: void onCanUseInstallApi(String packageName, int statusCode); Why is this part of the delegate?
Here's the second part of the comments https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:61: if (is_webapk_enabled) For the sake of simplicity, I am ok with creating a homesreen shortcut if: - WebAPKs are enabled - The play install flow is disabled I would rename |can_use_webapk_install_flow| to |is_webapk_enabled| https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/chrome_webapk_host.h (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/chrome_webapk_host.h:32: // Checks whether Google Play Install API is available. Nit: Document that the callback can be called synchronously https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:431: response->package_name(), version, response->token()); Why this change? If we are ignoring the return value of InstallOrUpdateWebApkFromGooglePlay(), InstallOrUpdateWebApkFromGooglePlay() should not have a return value. https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.cc:92: weak_ptr_factory_.GetWeakPtr(), Is a weak ptr needed? We currently leak AddToHomescreenManager. See crbug.com/688444 https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.cc:198: if (is_webapk_compatible_) { My preference would be to call ChromeWebApkHost::CanUseGooglePlayToInstallWebApk() from here
Patchset #4 (id:80001) has been deleted
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Hi Peter, PTAL, thanks! https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:86: private void canUseGooglePlayInstallApi() { On 2017/02/03 17:04:17, pkotwicz wrote: > I prefer the pattern that Gonzalo used (passing a pointer to the native > callback) in ShortcutHelper#retrieveWebApks() > https://codereview.chromium.org/2629573004/diff/440001/chrome/android/java/sr... Done. https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:93: On 2017/02/03 17:04:17, pkotwicz wrote: > Should you cache the result? I am not sure here. Can we always trust the cached results? https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:102: delegate.canUseInstallApi("org.chromium.test", 0, "test", "", "", callback); On 2017/02/03 17:04:17, pkotwicz wrote: > These parameters should an implementation detail of > GooglePlayWebApkInstallDelegate Done. https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:107: void onCanUseInstallApi(String packageName, int statusCode); On 2017/02/03 17:04:17, pkotwicz wrote: > Why is this part of the delegate? Good catch. Removed. https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:61: if (is_webapk_enabled) On 2017/02/03 18:56:32, pkotwicz wrote: > For the sake of simplicity, I am ok with creating a homesreen shortcut if: > - WebAPKs are enabled > - The play install flow is disabled > > I would rename |can_use_webapk_install_flow| to |is_webapk_enabled| Yes it is complex, but I try to make it less difficult to understand why these logic is added. Personally I would prefer to have the expected behavior for even the corner case like this: if an webapk has installed and the app banner showns due to bypass-engagement-check, it is nice to show the "open" button no matter whether the google play flags is on or not. https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/chrome_webapk_host.h (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/chrome_webapk_host.h:32: // Checks whether Google Play Install API is available. On 2017/02/03 18:56:32, pkotwicz wrote: > Nit: Document that the callback can be called synchronously Done. https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:431: response->package_name(), version, response->token()); On 2017/02/03 18:56:32, pkotwicz wrote: > Why this change? If we are ignoring the return value of > InstallOrUpdateWebApkFromGooglePlay(), InstallOrUpdateWebApkFromGooglePlay() > should not have a return value. This is because ChromeWebApkHost::CanUseGooglePlayInstallApi(callback) always wait for the callback to be called, even for the failure case. However, that API eventually share the same code with InstallOrUpdateWebApkFromGooglePlay(). I will introduce a follow up CL to remove all the return values of the InstallOrUpdateWebApkFromGooglePlay(). https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.cc:92: weak_ptr_factory_.GetWeakPtr(), On 2017/02/03 18:56:32, pkotwicz wrote: > Is a weak ptr needed? We currently leak AddToHomescreenManager. See > crbug.com/688444 Updated to Unretained(this). https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapps/add_to_homescreen_manager.cc:198: if (is_webapk_compatible_) { On 2017/02/03 18:56:32, pkotwicz wrote: > My preference would be to call > ChromeWebApkHost::CanUseGooglePlayToInstallWebApk() from here As discussed offline, we keep the call in the AddToHomescreeManager::Start() to prevent adding more complicated logic in the manager and data fetcher.
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.
https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:93: As long as it is an in memory cache I think so. It would be weird for Google Play to randomly stop being able to install WebAPKs https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:61: if (is_webapk_enabled) Can we instead have the simpler behavior: "Only check whether there is an existing WebAPK when we are about to install a WebAPK" Thus, we will not check whether there is an existing WebAPK when we are about to install a web app shortcut. In particular for the case of: - WebAPK enabled via variations or command line flag - Google Play install enabled via variations - Google Play install disabled via Google Play flags (gservices) We won't check whether there is a WebAPK installed When we are about to create a web app shortcut, we should display an "open" banner only if a webapp is already installed. We should not care at all about WebAPK state. We don't check whether a webapp is already installed because it isn't possible to do this https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/60001/chrome/browser/android/... chrome/browser/android/webapk/webapk_installer.cc:431: response->package_name(), version, response->token()); Please file the bug too :) https://codereview.chromium.org/2675803004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2675803004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:33: Do you still need these constants here? https://codereview.chromium.org/2675803004/diff/100001/chrome/browser/android... File chrome/browser/android/webapk/chrome_webapk_host.cc (right): https://codereview.chromium.org/2675803004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/chrome_webapk_host.cc:8: #include "base/bind.h" Is this include necessary? https://codereview.chromium.org/2675803004/diff/100001/chrome/browser/android... chrome/browser/android/webapk/chrome_webapk_host.cc:10: #include "chrome/browser/android/webapk/webapk_install_service.h" Is this include necessary?
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) 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 Dominick and Peter, I have re-implemented according to your suggestions. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:73: if (!isGooglePlayInstallEnabledByChromeFeature() I don't think that having a separate isGooglePlayInstallEnabledByChromeFeature() function is necessary. You can just check !isEnabled() || !nativeCanUseGooglePlayToInstallWebApk() here https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:66: return mGooglePlayWebApkInstallDelegate != null Can the null check be removed? https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: // |InstallOrUpdateWebApkFromGooglePlay()| which is executed asynchronously. Nits: "Removes" -> "Remove" "|InstallOrUpdateWebApkFromGooglePlay()|" -> "InstallOrUpdateWebApkFromGooglePlay()" "which is" -> "is"
https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:112: return isEnabled() && nativeCanUseGooglePlayToInstallWebApk(); I would move isEnabled() up to canInstallWebApk() https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:125: return isEnabled() && nativeCanInstallFromUnknownSources(); I would move the isEnabled() check up to canInstallWebApk(). https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:129: private static boolean isWebApkInstallFlowAvailable() { Call this canInstallWebApk() to match the native side? Plus, you should just be able to inline isEnabled() || canUseGooglePlayToInstallWebApk() || canInstallFromUnknownSources() in here. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:58: * @return True if the Install API is available. I would make this method return void. It will only return false if there's no need to do an async check, but you still call the callback anyway and ignore the return value. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:60: boolean canUseInstallApi(Callback<Boolean> callback); Just call this canInstallWebApk()?
Patchset #6 (id:180001) has been deleted
Hi Dominick, PTAL, thanks! https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:73: if (!isGooglePlayInstallEnabledByChromeFeature() On 2017/02/05 02:13:54, pkotwicz wrote: > I don't think that having a separate isGooglePlayInstallEnabledByChromeFeature() > function is necessary. You can just check !isEnabled() || > !nativeCanUseGooglePlayToInstallWebApk() here This function is also callded by ChromeApplicationInternal, which is in the internal CL. I keep the |isGooglePlayInstallEnabledByChromeFeature|, but here adopt your suggestions, since it is more clear to call them directly. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:112: return isEnabled() && nativeCanUseGooglePlayToInstallWebApk(); On 2017/02/06 05:22:49, dominickn wrote: > I would move isEnabled() up to canInstallWebApk() Removed. Also, I added additional call in WebApkInstallStatusReceiver (refer to the internal CL). https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:125: return isEnabled() && nativeCanInstallFromUnknownSources(); On 2017/02/06 05:22:49, dominickn wrote: > I would move the isEnabled() check up to canInstallWebApk(). Remove |canInstallFromUnknownSources()|, since it is only called by |canInstallWebApk()|. . https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:129: private static boolean isWebApkInstallFlowAvailable() { On 2017/02/06 05:22:49, dominickn wrote: > Call this canInstallWebApk() to match the native side? Plus, you should just be > able to inline isEnabled() || canUseGooglePlayToInstallWebApk() || > canInstallFromUnknownSources() in here. Done. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:58: * @return True if the Install API is available. On 2017/02/06 05:22:49, dominickn wrote: > I would make this method return void. It will only return false if there's no > need to do an async check, but you still call the callback anyway and ignore the > return value. Done. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/GooglePlayWebApkInstallDelegate.java:60: boolean canUseInstallApi(Callback<Boolean> callback); On 2017/02/06 05:22:49, dominickn wrote: > Just call this canInstallWebApk()? Done. https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:66: return mGooglePlayWebApkInstallDelegate != null On 2017/02/05 02:13:54, pkotwicz wrote: > Can the null check be removed? Done. https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: // |InstallOrUpdateWebApkFromGooglePlay()| which is executed asynchronously. On 2017/02/05 02:13:55, pkotwicz wrote: > Nits: > "Removes" -> "Remove" > "|InstallOrUpdateWebApkFromGooglePlay()|" -> > "InstallOrUpdateWebApkFromGooglePlay()" > "which is" -> "is" How about "Remove the return value of InstallOrUpdateWebApkFromGooglePlay(), since a callback will be called after the install or update is either failed or completed."?
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 #7 (id:220001) has been deleted
Still LGTM https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: // |InstallOrUpdateWebApkFromGooglePlay()| which is executed asynchronously. One small nit: "is either" -> "has either" Otherwise looks good https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:65: * 1) WebAPK is enabled; Nitzs: - "WebAPK is enabled;" -> "WebAPKs are enabled" (alternatively "WebAPK feature is enabled") - Replace ';' with '.' https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:113: */ This function is no longer used?
lgtm Can you add metrics in a follow up to measure how many times: - we fall back to shortcut A2HS because we fail the canInstallWebAPK check - the deferred task succeeds versus having to run the check wen the user tries to add a WebAPK? Ultimately it would be good to be able to remove this. :)
Patchset #7 (id:240001) has been deleted
On 2017/02/07 00:06:42, dominickn wrote: > lgtm > > Can you add metrics in a follow up to measure how many times: > > - we fall back to shortcut A2HS because we fail the canInstallWebAPK check > - the deferred task succeeds versus having to run the check wen the user tries > to add a WebAPK? > > Ultimately it would be good to be able to remove this. :) Sure, file a bug crbug.com/689326 to track this.
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan: I need OWNERS review for: -chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java Thanks! https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2675803004/diff/160001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:431: // |InstallOrUpdateWebApkFromGooglePlay()| which is executed asynchronously. On 2017/02/06 23:12:12, pkotwicz wrote: > One small nit: "is either" -> "has either" > Otherwise looks good Done. https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:65: * 1) WebAPK is enabled; On 2017/02/06 23:12:12, pkotwicz wrote: > Nitzs: > - "WebAPK is enabled;" -> "WebAPKs are enabled" (alternatively "WebAPK feature > is enabled") > - Replace ';' with '.' Done. https://codereview.chromium.org/2675803004/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:113: */ On 2017/02/06 23:12:12, pkotwicz wrote: > This function is no longer used? It is used by ChromeApplicationInternal.
Patchset #8 (id:280001) has been deleted
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222,689191 ==========
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222,689191 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ==========
I added a check of whether the native is initialized to prevent crash when ChromeWebApkHost#isGooglePlayInstallEnabledByChromeFeature() is called but Chrome isn't running. Since it is only a one line change, and I believe it is very important for the launch, so I added it here. PTAL, thanks!
https://codereview.chromium.org/2675803004/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:116: return LibraryLoader.isInitialized() && nativeCanUseGooglePlayToInstallWebApk(); This change should be in a separate CL
Patchset #8 (id:300001) has been deleted
Revert the last change to keep this CL clean. PTAL, thanks!
PTAL, thanks!
Dan can you please take a look? LGTM with nit. https://codereview.chromium.org/2675803004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java (right): https://codereview.chromium.org/2675803004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:73: if (!isEnabled() || !nativeCanUseGooglePlayToInstallWebApk() Nit: You can call isGooglePlayInstallEnabledByChromeFeature() here and merge the first two conditions https://codereview.chromium.org/2675803004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/ChromeWebApkHost.java:112: * whether WebApk is enabled. Nit: Remove the last sentence. Add: "Does not check whether WebAPK install is enabled by Google Play."
Dan can you please take a look at the changes in ProcessInitializationHandler.java LGTM with nit.
Dan can you please take a look at the changes in ProcessInitializationHandler.java LGTM with nit.
Patchset #9 (id:340001) 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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.
lgtm for that file
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2675803004/#ps380001 (title: "Rebase.")
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": 380001, "attempt_start_ts": 1486498048878170,
"parent_rev": "c78653065fa6d8fb524e48a5d7232f3ab5eab4ab", "commit_rev":
"9c327a61afee72b188f3e80f6bfb576836b8ce1c"}
Message was sent while issue was closed.
Description was changed from ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 ========== to ========== Fall back to shortcut A2HS when Phonesky is out of date or unavailable. If Google Play Install gServices flags aren't turned on, the WebAPK install will fail when the Google Play Install flow is allowed via Finch flags. This CLs adds a check of whether the Google Play install API is available or not. If not, we will fallback to classic A2HS shortcut flow. The internal CL is: https://chrome-internal-review.googlesource.com/c/323685/ Test cases refer to: https://docs.google.com/document/d/1xS2JZ0I7hQenEAAe1sSc-w6iWY3wiOTF2e2WE8Qdv... BUG=687222 Review-Url: https://codereview.chromium.org/2675803004 Cr-Commit-Position: refs/heads/master@{#448715} Committed: https://chromium.googlesource.com/chromium/src/+/9c327a61afee72b188f3e80f6bfb... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:380001) as https://chromium.googlesource.com/chromium/src/+/9c327a61afee72b188f3e80f6bfb... |
