Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2363183002:
Skip installation process if WebAPK is already installed. (Closed)
DescriptionSkip installation process if WebAPK is already installed.
BUG=638614
Committed: https://crrev.com/4bb5896ca96a1cda9666cdb33578ff90b53b4c65
Cr-Commit-Position: refs/heads/master@{#422677}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Sync code up to date #Patch Set 3 : Addressing comments #
Total comments: 4
Patch Set 4 : Addressing comments #
Messages
Total messages: 31 (11 generated)
Description was changed from ========== Skip installation process if WebAPK is already installed. BUG= ========== to ========== Skip installation process if WebAPK is already installed. BUG=638614 ==========
zpeng@chromium.org changed reviewers: + dfalcantara@chromium.org, hanxi@chromium.org, pkotwicz@chromium.org
Hi Dan, Peter, and Xi, Can you please take a look? The proposed approach is to bypass installation process if WebAPK is already installed, and to leave a TODO for further investigation. Right now the banner class has many active issues with it, such as itemized installation metrics & Issue 648980, and I'd like to spend more time on it. Thanks!
zpeng@chromium.org changed reviewers: - hanxi@chromium.org
I think the bypass approach of calling success callback is best for now, because it seems to have the least impact on the existing code: 1. For the app banner bar to show "Open", its state needs to be updated. For the moment, the only state that shows "Open" is "Installed", which is the state that is updated in the success callback. We may want to introduce more states, but I think that calls for further investigation; 2. We can check the installed status in AddToHomeScreenManager; however, the results need to be passed into the banner, whose ownership AddToHomeScreenManager does not have. As a result, we need to pass the information when we create the banner. At this moment, the Create function uses flags. I'm reluctant to add more flags, as that would propagate the changes to non-webapk code. I think we may want to introduce an Option class to encapsulate all the flags and possibly initial states of the banner in the future. 3. I think the current control flow and states of the banner needs a bit tweaking or some overhaul, as the menu button "AddToHomeScreen" invokes the banner and 1) clicks "Install" if the webapk is not installed, but 2) does nothing if the webapk is installed. Also the banner has other issues (itemized installation metrics, Issue 648980, etc.) In general, I propose the bypass as a feasible temporary fix. I'd like to explore long-term fixes in follow-up CLs. Also, removing Xi as a reviewer because she's going on vacation :)
agrieve@chromium.org changed reviewers: + agrieve@chromium.org
just a fly-by :) https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:474: private static boolean isWebApkInstalled(String url) { nit: probably better to delete this method now and have native side all queryWebApkPackage() instead. (to minimize the JNI surface)
dfalcantara@chromium.org changed reviewers: + dominickn@chromium.org
Replacing myself with Dominick since he owns this code.
https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:463: if (!ChromeWebApkHost.isEnabled()) { Style nit: one-line this if statement and remove the braces. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:41: // TODO(zpeng): Reorganize control flow to allow itemized metrics. Nit: don't have a TODO without a crbug link. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:223: } else { Nit: remove the else clause (no else after return) - just have the contents outside of the else. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:74: // TODO(zpeng): Move function from public to private. Remove this comment, since this method was moved to private in https://codereview.chromium.org/2375503003
Your CL looks very good. A few minor comments. >> I think the bypass approach of calling success callback >> is best for now, because >> it seems to have the least impact on the existing code: >> 1. For the app banner bar to show "Open", its state >> needs to be updated. For the >> moment, the only state that shows "Open" is "Installed", >> which is the state that >> is updated in the success callback. We may want to >> introduce more states, but I >> think that calls for further investigation; - As you suggest in #2, I agree that we should pass in whether the WebAPK has been installed to AppBannerInfobarDelegateAndroid::Create(). I am not sure if you are suggesting something different in #1 than in #2 >> 2. We can check the installed status in AddToHomeScreenManager; however, the >> results need to be passed into the banner, whose ownership >> AddToHomeScreenManager does not have. As a result, we need to pass the >> information when we create the banner. At this moment, the Create function uses >> flags. I'm reluctant to add more flags, as that would propagate the changes to >> non-webapk code. I think we may want to introduce an Option class to encapsulate >> all the flags and possibly initial states of the banner in the future. - An Option class sounds good to me - We should split AppBannerInfobarDelegateAndroid into two. One class which deals with native app banners and one class which deals with web app and WebAPK banners. (NativeAppBannerInfobarDelegateAndroid & WebappBannerInfobarDelegateAndroid perhaps?) Currently, AppBannerInfobarDelegateAndroid has very little common code between the code that it runs for native apps and the code that it runs for web apps / WebAPKs >> 3. I think the current control flow and states of the banner needs a bit >> tweaking or some overhaul, as the menu button "AddToHomeScreen" invokes the >> banner and 1) clicks "Install" if the webapk is not installed, but 2) does >> nothing if the webapk is installed. Also the banner has other issues (itemized >> installation metrics, Issue 648980, etc.) - Dumb question: what do you mean by "itemized installation metrics"? https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:225: TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED); Shouldn't you log: webapk::TrackUserAction(webapk::USER_ACTION_OPEN) instead of TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED) It is not clear that we shouldn't log the USER_RESPONSE_WEB_APP_ACCEPTED histogram. It feels like the histogram should only be logged when the "user has accepted a WebAPK install" https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:227: true, installed_webapk_package_name); Calling AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished() is not completely right because OnWebApkInstallFinished() logs the INSTALL_EVENT_WEB_APP_INSTALLED metric (which in my opinion we do not want because no installation took place) My proposal is to move lines 406 - 414 to their own function: SetCanOpenWebApk(const std::string& webapk_package_name) https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:428: } According to the TODO in https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_met... we should be recording USER_ACTION_OPEN_DISMISS when the WebAPK is already installed. For now, I don't mind if you add a new InstallState, ALREADY_INSTALLED, in order to determine which metric to track. I agree that passing whether the WebAPK is already installed into AppBannerInfoBarDelegateAndroid::Create() is the correct long term solution https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.h:99: // Returns the package name of the WebAPK if WebAPKS are enabled and there is Nit: WebAPKS -> WebAPKs https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_manager.cc:71: void AddToHomescreenManager::Start(content::WebContents* web_contents) { Thank you for making the changes in this file. They are good changes. However, the changes are not related to the remainder of this change list. Can you move them to a separate change list? https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_manager.h:73: Everyone has a slightly different commenting style. It is unclear to me that your version of the comments is better than the existing comments. Your version does not seem to be worse than the existing comments either. I would advise not to modify the existing comments because: - it is unclear that your version is better. - you do not have changes in this file. If you had added a member variable in this CL you could probably get away with changing the comments for an unrelated member variable.
Hi Andrew, Dominick, and Peter, Thanks for the comments! PTAL
Hi Andrew, Dominick, and Peter, Thanks for the comments! PTAL P.S. Sorry for sending a reply with no comments... https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:463: if (!ChromeWebApkHost.isEnabled()) { On 2016/09/28 02:10:25, dominickn wrote: > Style nit: one-line this if statement and remove the braces. Done. https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:474: private static boolean isWebApkInstalled(String url) { On 2016/09/27 13:45:42, agrieve wrote: > nit: probably better to delete this method now and have native side all > queryWebApkPackage() instead. (to minimize the JNI surface) Thanks for the suggestion! I'll do it in a follow up CL. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:41: // TODO(zpeng): Reorganize control flow to allow itemized metrics. On 2016/09/28 02:10:25, dominickn wrote: > Nit: don't have a TODO without a crbug link. Done. Thanks for the information! https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:223: } else { On 2016/09/28 02:10:25, dominickn wrote: > Nit: remove the else clause (no else after return) - just have the contents > outside of the else. Done. Thanks for the information! https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:225: TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED); On 2016/09/29 14:18:54, pkotwicz wrote: > Shouldn't you log: > webapk::TrackUserAction(webapk::USER_ACTION_OPEN) instead of > TrackUserResponse(USER_RESPONSE_WEB_APP_ACCEPTED) > > It is not clear that we shouldn't log the USER_RESPONSE_WEB_APP_ACCEPTED > histogram. It feels like the histogram should only be logged when the "user has > accepted a WebAPK install" Resolved in offline discussion. Filed new issue https://bugs.chromium.org/p/chromium/issues/detail?id=651615. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:227: true, installed_webapk_package_name); On 2016/09/29 14:18:54, pkotwicz wrote: > Calling AppBannerInfoBarDelegateAndroid::OnWebApkInstallFinished() is not > completely right because OnWebApkInstallFinished() logs the > INSTALL_EVENT_WEB_APP_INSTALLED metric (which in my opinion we do not want > because no installation took place) > > My proposal is to move lines 406 - 414 to their own function: > SetCanOpenWebApk(const std::string& webapk_package_name) Resolved in offline discussion. See above. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:428: } On 2016/09/29 14:18:54, pkotwicz wrote: > According to the TODO in > https://cs.chromium.org/chromium/src/chrome/browser/android/webapk/webapk_met... > we should be recording USER_ACTION_OPEN_DISMISS when the WebAPK is already > installed. > > For now, I don't mind if you add a new InstallState, ALREADY_INSTALLED, in order > to determine which metric to track. I agree that passing whether the WebAPK is > already installed into AppBannerInfoBarDelegateAndroid::Create() is the correct > long term solution Resolved in offline discussion. See above. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/bann... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:74: // TODO(zpeng): Move function from public to private. On 2016/09/28 02:10:26, dominickn wrote: > Remove this comment, since this method was moved to private in > https://codereview.chromium.org/2375503003 Done. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/shor... File chrome/browser/android/shortcut_helper.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/shor... chrome/browser/android/shortcut_helper.h:99: // Returns the package name of the WebAPK if WebAPKS are enabled and there is On 2016/09/29 14:18:54, pkotwicz wrote: > Nit: WebAPKS -> WebAPKs Done. https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_manager.cc:71: void AddToHomescreenManager::Start(content::WebContents* web_contents) { On 2016/09/29 14:18:54, pkotwicz wrote: > Thank you for making the changes in this file. They are good changes. However, > the changes are not related to the remainder of this change list. Can you move > them to a separate change list? Done. Will do :) https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://codereview.chromium.org/2363183002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapps/add_to_homescreen_manager.h:73: On 2016/09/29 14:18:54, pkotwicz wrote: > Everyone has a slightly different commenting style. It is unclear to me that > your version of the comments is better than the existing comments. Your version > does not seem to be worse than the existing comments either. > > I would advise not to modify the existing comments because: > - it is unclear that your version is better. > - you do not have changes in this file. If you had added a member variable in > this CL you could probably get away with changing the comments for an unrelated > member variable. Done.
LGTM - I have filed crbug.com/651894 about splitting app_banner_infobar_delegate_android.cc into version for native apps and version for WebAPKs - I asked what Felix meant by "itemized installation metrics" in comment #3. He is referring to needing to log to UMA webapk::USER_ACTION_OPEN instead of USER_ACTION_INSTALLED_OPEN when a user taps the "Open" button in the banner when the banner is shown and the WebAPK is already installed (crbug.com/651615)
https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:221: // Bypass the installation process. If you keep this comment, make it more explicit, like: "Bypass installation since the WebAPK is already installed" https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:223: weak_ptr_factory_.GetWeakPtr()->OnWebApkInstallFinished( Directly call OnWebApkInstallFinished(true, installed_webapk_package_name). Going via the weak pointer is only necessary if you need to bind a callback to be run later.
Hi Dominick, PTAL Thanks! https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:221: // Bypass the installation process. On 2016/09/30 23:32:24, dominickn wrote: > If you keep this comment, make it more explicit, like: "Bypass installation > since the WebAPK is already installed" Done. https://codereview.chromium.org/2363183002/diff/30001/chrome/browser/android/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:223: weak_ptr_factory_.GetWeakPtr()->OnWebApkInstallFinished( On 2016/09/30 23:32:24, dominickn wrote: > Directly call OnWebApkInstallFinished(true, installed_webapk_package_name). > Going via the weak pointer is only necessary if you need to bind a callback to > be run later. Done. Thanks!
https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:463: if (!ChromeWebApkHost.isEnabled()) { Dominick, is there a place in the Google Java style guide which suggests inlining one line if() statements? I am asking because I have not been doing this
https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java (right): https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:463: if (!ChromeWebApkHost.isEnabled()) { On 2016/10/03 17:17:10, pkotwicz wrote: > Dominick, is there a place in the Google Java style guide which suggests > inlining one line if() statements? > > I am asking because I have not been doing this I found this on Google Android Java style guide: https://source.android.com/source/code-style.html#use-standard-brace-style. It would seem either is acceptable.
On 2016/10/03 17:31:58, F wrote: > https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java > (right): > > https://codereview.chromium.org/2363183002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:463: if > (!ChromeWebApkHost.isEnabled()) { > On 2016/10/03 17:17:10, pkotwicz wrote: > > Dominick, is there a place in the Google Java style guide which suggests > > inlining one line if() statements? > > > > I am asking because I have not been doing this > > I found this on Google Android Java style guide: > https://source.android.com/source/code-style.html#use-standard-brace-style. It > would seem either is acceptable. The general consensus for Chrome Android code is to make if statements one-line without braces if the contents of the if body are short enough (e.g. several examples in this file, and many, many others if you run something like git grep " if.*[^{]$" chrome/android/). lgtm.
The CQ bit was checked by zpeng@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/2363183002/#ps50001 (title: "Addressing comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by dfalcantara@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Skip installation process if WebAPK is already installed. BUG=638614 ========== to ========== Skip installation process if WebAPK is already installed. BUG=638614 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001)
Message was sent while issue was closed.
Description was changed from ========== Skip installation process if WebAPK is already installed. BUG=638614 ========== to ========== Skip installation process if WebAPK is already installed. BUG=638614 Committed: https://crrev.com/4bb5896ca96a1cda9666cdb33578ff90b53b4c65 Cr-Commit-Position: refs/heads/master@{#422677} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4bb5896ca96a1cda9666cdb33578ff90b53b4c65 Cr-Commit-Position: refs/heads/master@{#422677} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
