Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2259553002:
Make AppBannerInfoBar install WebAPK. (Closed)
DescriptionMake AppBannerInfoBar install WebAPK.
On UI, the text for installing a WebAPK in the App Banner is:
1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful
installation
2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast
for an unsucessful installation.
WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid
to show the above text. Besides, checking whether a site is WebAPK
Compattive is based on 1) installable check + 2) url is over
"https" + 3) WebAPK is enabled on command line.
Currently, WebAPK is installed as an untrusted source with Android
installation dialog pops up. User could choose cancel the installation
even after the WebAPK has been downloaded locally. To detect the
cancel behavior, AppBannerInfobarDelegateAndroid checks whether the
WebAPK has been installed or not after Chrome is resumed. This isn't an
ideal way, but it seems we can't find a better way to detect the cancel
event. This logic will be removed when WebAPK is installed by Play soon.
This CL disables creating WebAPKs by selecting add-to-homescreen from
the app menu. Instead, in a follow up CL, selecting from the app menu
will make the app banner info bar shown to install WebAPKs.
BUG=627250
Committed: https://crrev.com/9f38f74bb0feebf033b9ddd9e28ffe78d067a356
Cr-Commit-Position: refs/heads/master@{#415365}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase. #
Total comments: 7
Patch Set 3 : Move logic to WebApkInstaller. #
Total comments: 49
Patch Set 4 : Remove webpak-metrics and nits. #
Total comments: 28
Patch Set 5 : Another round of comments. #Patch Set 6 : Add a TODO. #
Total comments: 6
Patch Set 7 : dfalcantara@'s comments. #
Total comments: 2
Patch Set 8 : Nits. #
Total comments: 31
Patch Set 9 : Nits #Patch Set 10 : Rebase. #Messages
Total messages: 53 (32 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Make AppBannerInfoBar install WebAPK. BUG= ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. BUG=627250 ==========
hanxi@chromium.org changed reviewers: + pkotwicz@chromium.org, yfriedman@chromium.org
Hi Peter, please take a look, thanks! https://codereview.chromium.org/2259553002/diff/100001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2259553002/diff/100001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:44: const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess(); I will change the command line check in another CL.
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: Exceeded global retry quota
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL(https://codereview.chromium.org/2259093003/). BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL(https://codereview.chromium.org/2259093003/). BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL(https://codereview.chromium.org/2259093003). BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL(https://codereview.chromium.org/2259093003). BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL (https://codereview.chromium.org/2259093003/). BUG=627250 ==========
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org
hanxi@chromium.org changed reviewers: + dominickn@chromium.org
Xi thank you for taking this task on! This is one of the last remaining pieces to get the install flow working https://codereview.chromium.org/2259553002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:30: public static final int INSTALL_STATE_INSTALLED_WEBAPK = 4; Can we pass in whether the infobar is for a WebAPK into the constructor and not have WebAPK-specific states? https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:45: return ChromeWebApkHost::AreWebApkEnabled() && start_url.SchemeIs("https"); Is checking https:// necessary? Doesn't service worker require https:// ? https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (left): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:76: if (ChromeWebApkHost::AreWebApkEnabled()) { Doesn't this make adding-to-homescreen from the app menu no longer install WebAPKs? https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:58: const FinishCallback& finish_callback); Can we change the callback to be called when the installation has failed or succeeded (as opposed to when we send the install intent to Google Play)? Can we move the WebAPK specific logic in AppBannerInfoBarDelegateAndroid.java to WebApkInstaller.java (ApplicationStateListener and InstallerDelegate.Observer). In my opinion it would be awesome if AppBannerInfoBarDelegateAndroid.java had no WebAPK related code at all. I think that my suggestion makes things simpler right now and gives us less work once WebApkInstaller.java talks to Google Play in the future Yaron, thoughts?
I have some quick comments. Peter's suggestion to try and minimize the WebAPK specific code in the info bar is good and +1 to it. Moreover, I wonder whether we need all the WebAPK specific codes injected into the banner and installable metrics. All web app banners banners will install only WebAPKs once WebAPKs are shipped right? So the metric will only reflect who has the flag enabled and goes through the WebAPK flow. In the future, the "web app banner" buckets will be exclusively WebAPKs, meaning that the WebAPK buckets will be redundant. If you need metrics, it may be better to add a new separate metric for WebAPKs so we don't pollute the existing banner ones. https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:44: bool isWebApk(const GURL& start_url) { Call this AreWebAPKsEnabled. Check only the command line flag, and move the HTTPS check outside the method. https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/install... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:83: case SHOWING_WEBAPK_BANNER: Make sure the order matches the declaration. https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/install... File chrome/browser/installable/installable_logging.h (right): https://codereview.chromium.org/2259553002/diff/120001/chrome/browser/install... chrome/browser/installable/installable_logging.h:46: SHOWING_WEBAPK_BANNER, If you must add this, add it before MAX_ERROR_CODE. This histogram backs an UMA histogram and must be append-only.
Patchset #3 (id:140001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:144: if (mButton == null || mAppData == null && !mIsWebApk) return; Put braces around this to make sure the grouping is correct. I guess you want if (mButton == null || (mAppData == null && !mIsWebApk)) return; https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:150: if (mIsWebApk) return; Can you add a comment indicating why you don't need to change the button state in this case? https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:155: text = mIsWebApk ? getContext().getString(R.string.app_banner_installing_webapk) Random thought: do we need to having the separate strings here? It looks like one is "Installing..." and the other is "Adding...". Why not make it the same for both, something like "Working..." ? You don't need to address this here, just something for us to remember. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:114: if (launchIntent != null) { Nit: make this a single line if statement. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:158: // Open the WebApk. Nit: remove this redundant comment https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:177: String packageName = data != null ? data.packageName() : mWebApkPackage; Nit: brackets around the conditional https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:328: base::Unretained(this)); I'm a bit nervous about this unretained call, because it's relying on the info bar being kept alive without ref counting or weak pointers. I can't help but wonder whether we could see some use after free issues because of this. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:356: base::Unretained(this))); See the other comment about base::Unretained. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:373: static_cast<AppBannerInfoBarAndroid*>(infobar()) Does this need the static cast at all? It's a method on the base class. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:45: return ChromeWebApkHost::AreWebApkEnabled(); Remove this method and just call ChromeWebApkHost::AreWebApkEnabled() in the call site. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (left): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:18: #include "chrome/browser/android/webapk/webapk_installer.h" Don't you still need this header since you're instantiated a WebApkInstaller? https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/banners... File chrome/browser/banners/app_banner_settings_helper.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/banners... chrome/browser/banners/app_banner_settings_helper.cc:200: std::string getMetric( This should return const std::string or a base::StringPiece, and be called GetRapporMetric. Though I think you're removing this method anyway. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/install... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:83: case SHOWING_WEBAPK_BANNER: Remove as discussed.
Mostly nits Thank you for moving the install monitoring logic into WebApkInstaller.java! I know that it was a big refactor https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:67: mIsWebApk = isWebApk; We don't seem to use |mIsWebApk| anymore. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:112: private boolean openApp(Context context, String packageName) { This function always returns true. Can it be void instead? https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:47: mListener = createApplicationStateListener(); Why is this created in the constructor instead of in WebApkInstaller#installAsyncFromNative() https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:100: if (mInstallTask != task) return; Can we move lines 101 - 106 into a dedicated function? This enables the ApplicationStateListener to share some code with createInstallerDelegateObserver() https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:141: void AppBannerInfoBarDelegateAndroid::OnInstallFinished( It looks like this function is no longer called in the WebAPK path https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:324: UpdateInstallState(env, nullptr); I am not a fan of these calls. We know exactly which state we want to transition to. Can we call UpdateInstallState(INSTALL_STATE_INSTALLING) directly? I know that the UpdateInstallState() call in OnWebApkInstallFinished() is more complicated because we want to double check that the WebAPK was in fact installed. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (left): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:76: if (ChromeWebApkHost::AreWebApkEnabled()) { It is worth mentioning in the CL description that this CL disables creating WebAPKs by selecting add-to-homescreen from the app menu https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:155: WebApkInstaller::~WebApkInstaller() { Noob question: Is calling WebApkInstaller#destroy() necessary? (I know that C++ objects owned by Java objects need to be destroyed explicitly but I am wondering whether Java objects owned by C++ objects are destroyed automatically via the ScopedJavaGlobalRef destructor) https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:336: const std::string& package_name) { Can you set |webapk_package_| here? This avoids passing |webapk_package_| from one function to the next https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:399: java_package_name); For updates you should call onSuccess() only once the WebAPK has been updated. You can detect whether a WebAPK has been updated by checking the Android version code of the installed WebAPK. For now you can keep the stub in WebApkInstaller#updateAsyncFromNative() For the sake of clarity I am suggesting: if (task_type_ == INSTALL) { success = StartInstallingDownloadedWebApk(env, java_file_path, java_package_name); } else if (task_type_ == UPDATE) { success = StartUpdateUsingDownloadedWebApk(env, java_file_path, java_package_name); } if (!success) OnFailure();
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL depends on CL (https://codereview.chromium.org/2259093003/). BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL CL disables creating WebAPKs by selecting add-to-homescreen from the app menu, and in a follow up CL we will make selecting from the app menu create an app banner info bar to install WebAPK. BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL CL disables creating WebAPKs by selecting add-to-homescreen from the app menu, and in a follow up CL we will make selecting from the app menu create an app banner info bar to install WebAPK. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu, and in a follow up CL we will make selecting from the app menu create an app banner info bar to install WebAPK. BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu, and in a follow up CL we will make selecting from the app menu create an app banner info bar to install WebAPK. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, selecting from the app menu will make the app banner info bar shown to install the WebAPK, which is in a follow up CL. BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, selecting from the app menu will make the app banner info bar shown to install the WebAPK, which is in a follow up CL. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install the WebAPK. BUG=627250 ==========
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install the WebAPK. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install WebAPKs. BUG=627250 ==========
Patchset #4 (id:220001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:144: if (mButton == null || mAppData == null && !mIsWebApk) return; On 2016/08/25 04:25:56, dominickn wrote: > Put braces around this to make sure the grouping is correct. I guess you want > > if (mButton == null || (mAppData == null && !mIsWebApk)) return; Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:150: if (mIsWebApk) return; On 2016/08/25 04:25:56, dominickn wrote: > Can you add a comment indicating why you don't need to change the button state > in this case? Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:155: text = mIsWebApk ? getContext().getString(R.string.app_banner_installing_webapk) On 2016/08/25 04:25:56, dominickn wrote: > Random thought: do we need to having the separate strings here? It looks like > one is "Installing..." and the other is "Adding...". Why not make it the same > for both, something like "Working..." ? > > You don't need to address this here, just something for us to remember. Not sure why the text isn't designed the same as native app at the first place, I just follows the UI review (https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v...). https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:67: mIsWebApk = isWebApk; On 2016/08/25 22:23:15, pkotwicz wrote: > We don't seem to use |mIsWebApk| anymore. Good catch, removed. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:112: private boolean openApp(Context context, String packageName) { On 2016/08/25 22:23:15, pkotwicz wrote: > This function always returns true. Can it be void instead? Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:114: if (launchIntent != null) { On 2016/08/25 04:25:56, dominickn wrote: > Nit: make this a single line if statement. Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:158: // Open the WebApk. On 2016/08/25 04:25:56, dominickn wrote: > Nit: remove this redundant comment Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:177: String packageName = data != null ? data.packageName() : mWebApkPackage; On 2016/08/25 04:25:56, dominickn wrote: > Nit: brackets around the conditional Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:47: mListener = createApplicationStateListener(); On 2016/08/25 22:23:15, pkotwicz wrote: > Why is this created in the constructor instead of in > WebApkInstaller#installAsyncFromNative() Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:100: if (mInstallTask != task) return; On 2016/08/25 22:23:15, pkotwicz wrote: > Can we move lines 101 - 106 into a dedicated function? This enables the > ApplicationStateListener to share some code with > createInstallerDelegateObserver() Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:141: void AppBannerInfoBarDelegateAndroid::OnInstallFinished( On 2016/08/25 22:23:15, pkotwicz wrote: > It looks like this function is no longer called in the WebAPK path Removed. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:324: UpdateInstallState(env, nullptr); On 2016/08/25 22:23:15, pkotwicz wrote: > I am not a fan of these calls. We know exactly which state we want to transition > to. > > Can we call UpdateInstallState(INSTALL_STATE_INSTALLING) directly? > > I know that the UpdateInstallState() call in OnWebApkInstallFinished() is more > complicated because we want to double check that the WebAPK was in fact > installed. The state INSTALL_STATE_INSTALLING is defined in AppBannerInfoBarAndroid on Java side, and the delegate always ask the infobar which is the new state, and let the infobar update itself. The advantage is the states are only declared in one place. Do we really want to change this design? https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:328: base::Unretained(this)); On 2016/08/25 04:25:56, dominickn wrote: > I'm a bit nervous about this unretained call, because it's relying on the info > bar being kept alive without ref counting or weak pointers. I can't help but > wonder whether we could see some use after free issues because of this. Use WeakPtrFactory instead. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:356: base::Unretained(this))); On 2016/08/25 04:25:56, dominickn wrote: > See the other comment about base::Unretained. Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:373: static_cast<AppBannerInfoBarAndroid*>(infobar()) On 2016/08/25 04:25:56, dominickn wrote: > Does this need the static cast at all? It's a method on the base class. Removed. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:45: return ChromeWebApkHost::AreWebApkEnabled(); On 2016/08/25 04:25:56, dominickn wrote: > Remove this method and just call ChromeWebApkHost::AreWebApkEnabled() in the > call site. Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/shortcut_helper.cc (left): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:18: #include "chrome/browser/android/webapk/webapk_installer.h" On 2016/08/25 04:25:56, dominickn wrote: > Don't you still need this header since you're instantiated a WebApkInstaller? You are right, add it back. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/shortcut_helper.cc:76: if (ChromeWebApkHost::AreWebApkEnabled()) { On 2016/08/25 22:23:15, pkotwicz wrote: > It is worth mentioning in the CL description that this CL disables creating > WebAPKs by selecting add-to-homescreen from the app menu Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:155: WebApkInstaller::~WebApkInstaller() { On 2016/08/25 22:23:15, pkotwicz wrote: > Noob question: Is calling WebApkInstaller#destroy() necessary? > > (I know that C++ objects owned by Java objects need to be destroyed explicitly > but I am wondering whether Java objects owned by C++ objects are destroyed > automatically via the ScopedJavaGlobalRef destructor) That is a good point. The ScopedJavaGlobalRef destructor will reset the Java pointer in c++, but it doesn't set the native pointer in Jave to 0. I am not sure whether that matters. AppBannerInfoBarDelegateAndroid is an example that Java object owned by c++, and it does have a destroy function in Java. I found ResourceManager(https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/resources/ResourceManager.java?rcl=0&l=180) is created and destroy in the same way as I did. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:336: const std::string& package_name) { On 2016/08/25 22:23:15, pkotwicz wrote: > Can you set |webapk_package_| here? This avoids passing |webapk_package_| from > one function to the next Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:399: java_package_name); On 2016/08/25 22:23:16, pkotwicz wrote: > For updates you should call onSuccess() only once the WebAPK has been updated. > You can detect whether a WebAPK has been updated by checking the Android version > code of the installed WebAPK. For now you can keep the stub in > WebApkInstaller#updateAsyncFromNative() > > For the sake of clarity I am suggesting: > > if (task_type_ == INSTALL) { > success = StartInstallingDownloadedWebApk(env, java_file_path, > java_package_name); > } else if (task_type_ == UPDATE) { > success = StartUpdateUsingDownloadedWebApk(env, java_file_path, > java_package_name); > } > > if (!success) > OnFailure(); Yes, the update will also get a callback for the real success. Updated. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/banners... File chrome/browser/banners/app_banner_settings_helper.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/banners... chrome/browser/banners/app_banner_settings_helper.cc:200: std::string getMetric( On 2016/08/25 04:25:56, dominickn wrote: > This should return const std::string or a base::StringPiece, and be called > GetRapporMetric. Though I think you're removing this method anyway. Done. https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/install... File chrome/browser/installable/installable_logging.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/install... chrome/browser/installable/installable_logging.cc:83: case SHOWING_WEBAPK_BANNER: On 2016/08/25 04:25:56, dominickn wrote: > Remove as discussed. Done.
https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:86: AppBannerInfoBarDelegateAndroid::~AppBannerInfoBarDelegateAndroid() { Add weak_ptr_factory_.InvalidateWeakPtrs(); here to ensure that all outstanding weak pointers are dead at destruction. https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:342: weak_ptr_factory_.GetWeakPtr())); This method is invoked through a base::Bind call on a weak pointer (bound in AcceptWebApk). As I understand it, to ensure weak pointer thread safety, this method should be run on the UI thread (because it is then posting a weak pointer to the UI thread) (see comments in weak_ptr.h - weak pointers must be dereferenced and invalidated on the same thread). Can you verify that this method is being invoked on the UI thread? If it is, you can avoid the PostTask and just call RemoveInfoBarOnUIThread (and add DCHECK_CURRENTLY_ONs to verify). And if it isn't, it may not be thread-safe to use weak pointers here.
Don't know if I was explicitly asked to review this, but here goes. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:49: private boolean mIsWebApkInstalling = false; booleans default to false, so you don't need to initialize it here. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:111: if (launchIntent != null) context.startActivity(launchIntent); To be safe, use a try/catch when starting the Activity in case the app can't be run. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:56: mNativePointer = 0; Is it worth unregistering the application state listener here? I'm worried that onInstallFinishedInternal fails to run somehow and leaves a useless listener that runs for every activity status change. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:106: private void onInstallFinishedInternal(boolean success) { You need to check if the native pointer is still valid at this point because installing is asynchronous. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:1531: Adding… Why is this distinct from the regular app banner? If anything, the regular banner should have an "Adding..." message and the apk installing banner should say "Installing..." https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:1596: Unable to add Has this string gone through UX review? "Unable to add" is too generic a message to indicate what failed. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer.h (right): https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/browser/... chrome/browser/android/webapk/webapk_installer.h:83: static bool Register(JNIEnv* env); I'd put this at the bottom of the public section.
A few nits https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:324: UpdateInstallState(env, nullptr); I didn't realize that INSTALL_STATE_INSTALLING was defined in Java. I am uncomfortable about we have hacked AppBannerInfoBarDelegateAndroid#determineInstallState() to work for WebAPKs but I don't have a suggestion on how to improve things https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:155: WebApkInstaller::~WebApkInstaller() { Thank you for looking into this. I learned something today! https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:146: @CalledByNative Can this function return void too? https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:13: import android.provider.Settings; Can you please rebase this CL onto https://codereview.chromium.org/2264753002/ https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:108: mInstallTask = null; Do you need to call mInstallTask#cancel() in order to make the onInstallFinished() callback get called sooner rather than later?
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Dominick, Peter, and Dan: PTAL, thanks! @Dan: I was suggested to add reviewers one by one, so thanks for reviewing it early:) https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/200001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:324: UpdateInstallState(env, nullptr); On 2016/08/26 22:31:24, pkotwicz wrote: > I didn't realize that INSTALL_STATE_INSTALLING was defined in Java. I am > uncomfortable about we have hacked > AppBannerInfoBarDelegateAndroid#determineInstallState() to work for WebAPKs but > I don't have a suggestion on how to improve things I am not a fan of the way of how the state is updated, but if we want to avoid duplicate the states on the C++ side, this is the best way that I can think of:( https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:49: private boolean mIsWebApkInstalling = false; On 2016/08/26 21:12:26, dfalcantara wrote: > booleans default to false, so you don't need to initialize it here. Done. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:111: if (launchIntent != null) context.startActivity(launchIntent); On 2016/08/26 21:12:26, dfalcantara wrote: > To be safe, use a try/catch when starting the Activity in case the app can't be > run. Done. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:146: @CalledByNative On 2016/08/26 22:31:25, pkotwicz wrote: > Can this function return void too? Yes, this function doesn't need to return a boolean anymore. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:13: import android.provider.Settings; On 2016/08/26 22:31:25, pkotwicz wrote: > Can you please rebase this CL onto https://codereview.chromium.org/2264753002/ Well, the CL you mentioned depends on this CL. After changes here, that CL becomes easier. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:56: mNativePointer = 0; On 2016/08/26 21:12:27, dfalcantara wrote: > Is it worth unregistering the application state listener here? I'm worried that > onInstallFinishedInternal fails to run somehow and leaves a useless listener > that runs for every activity status change. Added. It doesn't hurt to check in case the listener wasn't unregistered due to some corner cases. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:106: private void onInstallFinishedInternal(boolean success) { On 2016/08/26 21:12:27, dfalcantara wrote: > You need to check if the native pointer is still valid at this point because > installing is asynchronous. Done. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:108: mInstallTask = null; On 2016/08/26 22:31:25, pkotwicz wrote: > Do you need to call mInstallTask#cancel() in order to make the > onInstallFinished() callback get called sooner rather than later? We cannot control when the onInstallFinished() is called, since the monitoring of the successful case is by mInstallTask. If user choose "cancel" the installation (the unsuccessful case), the onInstallFinished() doesn't be called at all. https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1531: Adding… On 2016/08/26 21:12:27, dfalcantara wrote: > Why is this distinct from the regular app banner? If anything, the regular > banner should have an "Adding..." message and the apk installing banner should > say "Installing..." This is a good question. I don't know why the design is like this at the first place, but the text is from the WebAPK installation flow UI review (https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v...). https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:1596: Unable to add On 2016/08/26 21:12:27, dfalcantara wrote: > Has this string gone through UX review? "Unable to add" is too generic a > message to indicate what failed. Yes, it is from the UX review too. https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:86: AppBannerInfoBarDelegateAndroid::~AppBannerInfoBarDelegateAndroid() { On 2016/08/26 19:19:34, dominickn wrote: > Add weak_ptr_factory_.InvalidateWeakPtrs(); here to ensure that all outstanding > weak pointers are dead at destruction. Done. https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:342: weak_ptr_factory_.GetWeakPtr())); On 2016/08/26 19:19:34, dominickn wrote: > This method is invoked through a base::Bind call on a weak pointer (bound in > AcceptWebApk). As I understand it, to ensure weak pointer thread safety, this > method should be run on the UI thread (because it is then posting a weak pointer > to the UI thread) (see comments in weak_ptr.h - weak pointers must be > dereferenced and invalidated on the same thread). > > Can you verify that this method is being invoked on the UI thread? If it is, you > can avoid the PostTask and just call RemoveInfoBarOnUIThread (and add > DCHECK_CURRENTLY_ONs to verify). And if it isn't, it may not be thread-safe to > use weak pointers here. Good catch. The code doesn't need to be posted to UI thread, due to Peter's refactory in WebApkInstaller. Now we don't need to use the weak pointer. https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:83: static bool Register(JNIEnv* env); On 2016/08/26 21:12:27, dfalcantara wrote: > I'd put this at the bottom of the public section. Done.
I initially reviewed it because someone added me as a reviewer, making it show up in my review queue and trigger emails about looking at it :P https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:1531: Adding… On 2016/08/29 14:44:52, Xi Han wrote: > On 2016/08/26 21:12:27, dfalcantara wrote: > > Why is this distinct from the regular app banner? If anything, the regular > > banner should have an "Adding..." message and the apk installing banner should > > say "Installing..." > > This is a good question. I don't know why the design is like this at the first > place, but the text is from the WebAPK installation flow UI review > (https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v...). Acknowledged. https://chromiumcodereview.appspot.com/2259553002/diff/240001/chrome/android/... chrome/android/java/strings/android_chrome_strings.grd:1596: Unable to add On 2016/08/29 14:44:52, Xi Han wrote: > On 2016/08/26 21:12:27, dfalcantara wrote: > > Has this string gone through UX review? "Unable to add" is too generic a > > message to indicate what failed. > > Yes, it is from the UX review too. Acknowledged. https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:115: } catch (ActivityNotFoundException e) { Output a Log.e here for bug reports to prevent this from being a completely silent failure. https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer.h (right): https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/browser/... chrome/browser/android/webapk/webapk_installer.h:37: // server, download it, and install it. Indicate that the native side WebApkInstaller owns the Java side. https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/browser/... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://chromiumcodereview.appspot.com/2259553002/diff/320001/chrome/browser/... chrome/browser/android/webapk/webapk_installer_unittest.cc:69: OnSuccess(); This isn't exactly like the code flow it's testing because this calls OnSuccess() immediately, while the other one is asynchronous. Can you post a task to run after it returns from this function?
Hi Dan: PTAL, thanks! https://codereview.chromium.org/2259553002/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:115: } catch (ActivityNotFoundException e) { On 2016/08/29 17:24:09, dfalcantara wrote: > Output a Log.e here for bug reports to prevent this from being a completely > silent failure. Done. https://codereview.chromium.org/2259553002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.h (right): https://codereview.chromium.org/2259553002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.h:37: // server, download it, and install it. On 2016/08/29 17:24:09, dfalcantara wrote: > Indicate that the native side WebApkInstaller owns the Java side. Done. https://codereview.chromium.org/2259553002/diff/320001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer_unittest.cc (right): https://codereview.chromium.org/2259553002/diff/320001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer_unittest.cc:69: OnSuccess(); On 2016/08/29 17:24:09, dfalcantara wrote: > This isn't exactly like the code flow it's testing because this calls > OnSuccess() immediately, while the other one is asynchronous. Can you post a > task to run after it returns from this function? Done.
lgtm on my end, but you need to wait for Dominick since he already started reviewing this. https://codereview.chromium.org/2259553002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:119: Log.e(TAG, "Fail to open app : %s!", packageName, e); nit: Failed
Thanks Dan! Yes, I am waiting for their comments back. Dominick & Peter, PTAL, thanks! https://codereview.chromium.org/2259553002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:119: Log.e(TAG, "Fail to open app : %s!", packageName, e); On 2016/08/29 17:58:17, dfalcantara wrote: > nit: Failed Done.
lgtm % nits https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:26: #include "content/public/browser/browser_thread.h" Nit: is this include needed any more? https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:97: weak_ptr_factory_.InvalidateWeakPtrs(); Nit: put this line first in the destructor for safety. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:294: if (webapk_package_name_.empty()) { Nit: this conditional confused me a lot when I first saw it. Please add a comment either here or in the .h file above webapk_package_name_ saying that "webapk_package_name_ is set when the WebAPK has finished installing. If it is empty, request WebAPK installation. This is required because the user may decide to cancel the installation after accepting the banner." https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:320: return false; Nit: add an additional comment here along the lines of "We don't open the WebAPK after it is installed because <reason>". (perhaps because it may take too long?) https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:342: DVLOG(1) << "The installation of WebAPK failed."; Nit: "The WebAPK installation failed".
LGTM with nits. Thank you for bearing with me! https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:108: mInstallTask = null; I think that calling mInstallTask#cancel() is the right thing to do. Not doing the call does not affect the correctness of your code since you have the null check on line 100. My understanding is that InstallerDelegate checks whether the WebAPK is installed every second. If the WebAPK is not installed after 3 minutes it stops polling and calls onInstallFinished(). Calling InstallerDelegate#cancel() prior to the 3 minutes being up prevents unnecessary polling. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:152: // a failure toast will be shown. Nit: "just disappear" -> "disappear" Reason: less words is better. Some people have compiled lists of words to avoid. (e.g. http://writetodone.com/three-words-you-should-eliminate-from-your-writing-2/). "just" is not on the list but it probably should be. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:159: text = mIsWebApk ? getContext().getString(R.string.app_banner_installing_webapk) Super nit: Cache the result of getContext() https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:51: * the package name of the WebAPK is available. Nits: - WebPK -> WebAPK - "This flag is set before the package name of the WebAPK is available." -> "This flag is set while the WebAPK is being installed." https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:53: private boolean mIsWebApkInstalling; Nit: |mIsWebApkInstalling| -> |mIsInstallingWebApk| https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:162: if (InstallerDelegate.isInstalled(packageManager, packageName)) { Nit: Move the initialization of the |context| variable to where it is used https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:187: /** Set the flag of whether a installation process is started for the WebAPK. */ Nits: - "a installation process" -> "the installation process" - "is started" -> "has been started" (I looked it up and "is started" is legal but is used infrequently http://english.stackexchange.com/questions/268308/which-is-correct-is-started...) https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:39: /** Monitors an installation in progress. */ How about: "Monitors installation progress." https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:153: * Currently WebAPKs aren't installed by Play. If user could cancel the How about: "... A user can cancel the installation when the Android native installation dialog shows. The only way to detect the user cancelling the installation is to check whether ..." In your sentence, I am confused about: "checking whether the WebAPK is installed" is the only way to know WHAT? https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:294: if (webapk_package_name_.empty()) { This if() statement also confused me at first I beleive that |webapk_package_name_| exists to differentiate between the "Add to Homescreen" button being pressed and the "Open" button being pressed. The "Add to Homescreen" button morphs into an "Open" button once the WebAPK is installed https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:403: jboolean success) { Nit: Reorder this function to match order in .h file https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:427: bool WebApkInstaller::Register(JNIEnv* env) { Nit: Reorder this function to match order in .h file
LGTM with nits. Thank you for bearing with me! https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:108: mInstallTask = null; I think that calling mInstallTask#cancel() is the right thing to do. Not doing the call does not affect the correctness of your code since you have the null check on line 100. My understanding is that InstallerDelegate checks whether the WebAPK is installed every second. If the WebAPK is not installed after 3 minutes it stops polling and calls onInstallFinished(). Calling InstallerDelegate#cancel() prior to the 3 minutes being up prevents unnecessary polling. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:152: // a failure toast will be shown. Nit: "just disappear" -> "disappear" Reason: less words is better. Some people have compiled lists of words to avoid. (e.g. http://writetodone.com/three-words-you-should-eliminate-from-your-writing-2/). "just" is not on the list but it probably should be. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:159: text = mIsWebApk ? getContext().getString(R.string.app_banner_installing_webapk) Super nit: Cache the result of getContext() https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:51: * the package name of the WebAPK is available. Nits: - WebPK -> WebAPK - "This flag is set before the package name of the WebAPK is available." -> "This flag is set while the WebAPK is being installed." https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:53: private boolean mIsWebApkInstalling; Nit: |mIsWebApkInstalling| -> |mIsInstallingWebApk| https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:162: if (InstallerDelegate.isInstalled(packageManager, packageName)) { Nit: Move the initialization of the |context| variable to where it is used https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:187: /** Set the flag of whether a installation process is started for the WebAPK. */ Nits: - "a installation process" -> "the installation process" - "is started" -> "has been started" (I looked it up and "is started" is legal but is used infrequently http://english.stackexchange.com/questions/268308/which-is-correct-is-started...) https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:39: /** Monitors an installation in progress. */ How about: "Monitors installation progress." https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:153: * Currently WebAPKs aren't installed by Play. If user could cancel the How about: "... A user can cancel the installation when the Android native installation dialog shows. The only way to detect the user cancelling the installation is to check whether ..." In your sentence, I am confused about: "checking whether the WebAPK is installed" is the only way to know WHAT? https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:294: if (webapk_package_name_.empty()) { This if() statement also confused me at first I beleive that |webapk_package_name_| exists to differentiate between the "Add to Homescreen" button being pressed and the "Open" button being pressed. The "Add to Homescreen" button morphs into an "Open" button once the WebAPK is installed https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:403: jboolean success) { Nit: Reorder this function to match order in .h file https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:427: bool WebApkInstaller::Register(JNIEnv* env) { Nit: Reorder this function to match order in .h file
Patchset #9 (id:380001) has been deleted
https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:108: mInstallTask = null; On 2016/08/30 04:06:23, pkotwicz wrote: > I think that calling mInstallTask#cancel() is the right thing to do. Not doing > the call does not affect the correctness of your code since you have the null > check on line 100. > > My understanding is that InstallerDelegate checks whether the WebAPK is > installed every second. If the WebAPK is not installed after 3 minutes it stops > polling and calls onInstallFinished(). Calling InstallerDelegate#cancel() prior > to the 3 minutes being up prevents unnecessary polling. > > Just one thing that I don't understand: the mInstallTask is the InstallerDelegate, why we call mInstallTask.cancle() right before set mInstallTask to null? For the "cancel" case, mInstallTask#onInstallFinished() doesn't get called at all, the mListener#onApplicationStateChange() calls nativeOnInstallFinished() instead. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:152: // a failure toast will be shown. On 2016/08/30 04:06:23, pkotwicz wrote: > Nit: "just disappear" -> "disappear" > > Reason: less words is better. Some people have compiled lists of words to avoid. > (e.g. > http://writetodone.com/three-words-you-should-eliminate-from-your-writing-2/). > "just" is not on the list but it probably should be. Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java:159: text = mIsWebApk ? getContext().getString(R.string.app_banner_installing_webapk) On 2016/08/30 04:06:23, pkotwicz wrote: > Super nit: Cache the result of getContext() Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:51: * the package name of the WebAPK is available. On 2016/08/30 04:06:23, pkotwicz wrote: > Nits: > - WebPK -> WebAPK > - "This flag is set before the package name of the WebAPK is available." -> > "This flag is set while the WebAPK is being installed." Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:53: private boolean mIsWebApkInstalling; On 2016/08/30 04:06:23, pkotwicz wrote: > Nit: |mIsWebApkInstalling| -> |mIsInstallingWebApk| Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:162: if (InstallerDelegate.isInstalled(packageManager, packageName)) { On 2016/08/30 04:06:23, pkotwicz wrote: > Nit: Move the initialization of the |context| variable to where it is used It is first used in line 160 to get the packageManager. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:187: /** Set the flag of whether a installation process is started for the WebAPK. */ On 2016/08/30 04:06:23, pkotwicz wrote: > Nits: > - "a installation process" -> "the installation process" > - "is started" -> "has been started" (I looked it up and "is started" is legal > but is used infrequently > http://english.stackexchange.com/questions/268308/which-is-correct-is-started...) Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:39: /** Monitors an installation in progress. */ On 2016/08/30 04:06:24, pkotwicz wrote: > How about: "Monitors installation progress." Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:153: * Currently WebAPKs aren't installed by Play. If user could cancel the On 2016/08/30 04:06:24, pkotwicz wrote: > How about: "... A user can cancel the installation when the Android native > installation dialog shows. The only way to detect the user cancelling the > installation is to check whether ..." > > In your sentence, I am confused about: > "checking whether the WebAPK is installed" is the only way to know WHAT? It is better, thanks. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:26: #include "content/public/browser/browser_thread.h" On 2016/08/29 23:31:29, dominickn wrote: > Nit: is this include needed any more? Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:97: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/08/29 23:31:29, dominickn wrote: > Nit: put this line first in the destructor for safety. Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:294: if (webapk_package_name_.empty()) { On 2016/08/30 04:06:24, pkotwicz wrote: > This if() statement also confused me at first > > I beleive that |webapk_package_name_| exists to differentiate between the "Add > to Homescreen" button being pressed and the "Open" button being pressed. The > "Add to Homescreen" button morphs into an "Open" button once the WebAPK is > installed Peter's explanation is correct. Add a comment here. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:320: return false; On 2016/08/29 23:31:29, dominickn wrote: > Nit: add an additional comment here along the lines of "We don't open the WebAPK > after it is installed because <reason>". (perhaps because it may take too long?) Returns false is to prevent the infobar from disappearing. Sorry for the confusion. Add a comment here too. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:342: DVLOG(1) << "The installation of WebAPK failed."; On 2016/08/29 23:31:29, dominickn wrote: > Nit: "The WebAPK installation failed". Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... File chrome/browser/android/webapk/webapk_installer.cc (right): https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:403: jboolean success) { On 2016/08/30 04:06:24, pkotwicz wrote: > Nit: Reorder this function to match order in .h file Done. https://codereview.chromium.org/2259553002/diff/360001/chrome/browser/android... chrome/browser/android/webapk/webapk_installer.cc:427: bool WebApkInstaller::Register(JNIEnv* env) { On 2016/08/30 04:06:24, pkotwicz wrote: > Nit: Reorder this function to match order in .h file Done.
The CQ bit was checked by hanxi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, dominickn@chromium.org, pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2259553002/#ps420001 (title: "Rebase.")
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 ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install WebAPKs. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install WebAPKs. BUG=627250 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install WebAPKs. BUG=627250 ========== to ========== Make AppBannerInfoBar install WebAPK. On UI, the text for installing a WebAPK in the App Banner is: 1) "Add to Homescreen" -> "Adding" -> "Open" for a sucessful installation 2) "Add to Homescreen" -> "Adding" -> "Unable to save" toast for an unsucessful installation. WebAPK-specific logic is added to AppBannerInfobarDelegateAndroid to show the above text. Besides, checking whether a site is WebAPK Compattive is based on 1) installable check + 2) url is over "https" + 3) WebAPK is enabled on command line. Currently, WebAPK is installed as an untrusted source with Android installation dialog pops up. User could choose cancel the installation even after the WebAPK has been downloaded locally. To detect the cancel behavior, AppBannerInfobarDelegateAndroid checks whether the WebAPK has been installed or not after Chrome is resumed. This isn't an ideal way, but it seems we can't find a better way to detect the cancel event. This logic will be removed when WebAPK is installed by Play soon. This CL disables creating WebAPKs by selecting add-to-homescreen from the app menu. Instead, in a follow up CL, selecting from the app menu will make the app banner info bar shown to install WebAPKs. BUG=627250 Committed: https://crrev.com/9f38f74bb0feebf033b9ddd9e28ffe78d067a356 Cr-Commit-Position: refs/heads/master@{#415365} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9f38f74bb0feebf033b9ddd9e28ffe78d067a356 Cr-Commit-Position: refs/heads/master@{#415365} |
