Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2290603005:
Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. (Closed)
DescriptionTrigger app banner when add to homescreen is
pressed and WebAPKs are enabled.
When user clicks A2HS from the app menu, a A2HS dialog shows to add
a shortcut. When WebAPK is enabled, an app banner info bar is expected
to show for WebAPK-compatible site.
This CL includes:
- AddToHomeScreenManager creates an app banner to install WebAPKs.
- Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo
instead of Manifest file.
- When installing WebAPK from the app menu, the app banner skips the state of
showing "Add to Homescrenn" on its button but jumps to "Adding" state and
starts the installation.
UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_vWpFjSA/edit#slide=id.g1503c2082e_0_28.
Committed: https://crrev.com/d256ebd2ba19a77c2c6d71f562f7db8b51254bc3
Cr-Commit-Position: refs/heads/master@{#417310}
Patch Set 1 #
Total comments: 14
Patch Set 2 : dfalcantara@'s comments. #Patch Set 3 : Remove InstallWebApk from infobar. #
Total comments: 20
Patch Set 4 : dominickn@ and dfalcantara@'s comments. #
Total comments: 9
Patch Set 5 : Nits. #Patch Set 6 : Remove the extra constructor in AppBannerInfoBarDelegateAndroid. #
Total comments: 2
Patch Set 7 : Nits. #
Total comments: 22
Patch Set 8 : pkasting@'s comments. #Patch Set 9 : Try to fix test failures #
Total comments: 78
Patch Set 10 : Clean ups #
Total comments: 1
Messages
Total messages: 66 (39 generated)
Description was changed from ========== Clicking A2HS from menu can install WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-caompatible site. BUG=627250 ========== to ========== Clicking A2HS from menu can install WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-caompatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu can install WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-caompatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ========== to ========== Clicking A2HS from menu can install WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu can install WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ========== to ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL(https://codereview.chromium.org/2259553002/). BUG=627250 ========== to ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL (https://codereview.chromium.org/2259553002/). BUG=627250 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. A refactoring is done in this CL to make AddToHomescreenManager creates a app banner infobar for WebAPK-compatible site. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL (https://codereview.chromium.org/2259553002/). BUG=627250 ========== to ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL (https://codereview.chromium.org/2259553002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. This CL depends on CL (https://codereview.chromium.org/2244223002/) and CL (https://codereview.chromium.org/2259553002/). BUG=627250 ========== to ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ========== to ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enalbed, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ========== to ========== Clicking A2HS from the app menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ==========
Description was changed from ========== Clicking A2HS from the app menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its buton but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ========== to ========== Clicking A2HS from the app menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ==========
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
hanxi@chromium.org changed reviewers: + dfalcantara@chromium.org, dominickn@chromium.org
Hi Dominick and Dan, could you please take a look? Thanks!
On 2016/08/30 21:33:29, Xi Han wrote: > Hi Dominick and Dan, could you please take a look? Thanks! (FYI: I'm OOO today, will look at this on Friday!)
https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:45: return info->url.is_empty(); return !info || info->url.is_empty(); https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:171: << "the associated WebContents is null."; Indentation is wrong. https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/android/webapps/add_to_homescreen_manager.cc:214: if (infobar) { if (!infobar) return; saves you a level of indentation. https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/android/webapps/add_to_homescreen_manager.h:79: void CreateInfobar(const ShortcutInfo& info, const SkBitmap& icon); CreateInfoBarForWebAPK() https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/android/webapps/add_to_homescreen_manager.h:98: bool is_compatible_; make the variable name explicitly is_webapk_compatible_. can't understand what it's compatible with if you're just looking at the code. https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... File chrome/browser/ui/android/infobars/app_banner_infobar_android.h (right): https://chromiumcodereview.appspot.com/2290603005/diff/100001/chrome/browser/... chrome/browser/ui/android/infobars/app_banner_infobar_android.h:42: void InstallWebApk(content::WebContents* web_contents); I don't think this function belongs here because it's a functional change. It even calls through directly to the delegate. Why don't you just call it on the delegate from AddToHomeScreenManager::CreateInfobar? You already have access to it there.
Patchset #2 (id:120001) has been deleted
Hi Dan, PTAL, thanks! https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:45: return info->url.is_empty(); On 2016/08/31 18:12:45, dfalcantara wrote: > return !info || info->url.is_empty(); Done. https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:171: << "the associated WebContents is null."; On 2016/08/31 18:12:45, dfalcantara wrote: > Indentation is wrong. Done. https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:214: if (infobar) { On 2016/08/31 18:12:45, dfalcantara wrote: > if (!infobar) > return; > > saves you a level of indentation. I will be more careful and use early exist as much as possible. https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.h (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.h:79: void CreateInfobar(const ShortcutInfo& info, const SkBitmap& icon); On 2016/08/31 18:12:46, dfalcantara wrote: > CreateInfoBarForWebAPK() Done. https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.h:98: bool is_compatible_; On 2016/08/31 18:12:45, dfalcantara wrote: > make the variable name explicitly is_webapk_compatible_. can't understand what > it's compatible with if you're just looking at the code. Done. https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/app_banner_infobar_android.h (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/app_banner_infobar_android.h:42: void InstallWebApk(content::WebContents* web_contents); On 2016/08/31 18:12:46, dfalcantara wrote: > I don't think this function belongs here because it's a functional change. It > even calls through directly to the delegate. Why don't you just call it on the > delegate from AddToHomeScreenManager::CreateInfobar? You already have access to > it there. Hmm, this call needs to be made after both AppBannerInfobarDelegateAndroid and AppBannerInfobarAndroid are initialized on Java and C++ sides. This is because the InstallWebAPK() not only starts the installation, but also updates the texts on the app banner infobar. However, in CreateInfoBarForWebAPK(), the delegate is a unique_ptr, and it becomes invalid as long as the infobar is created. So I can't call InstallWebAPK directly through the delegate, but via infobar and to the delegate. Beside, I don't want add more hack to the AppBannerInfobarAndroid.java to make the text is initialized with WebAPK logic (the "Adding" button). I would like to hear if you have better idea.
https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/app_banner_infobar_android.h (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/app_banner_infobar_android.h:42: void InstallWebApk(content::WebContents* web_contents); On 2016/08/31 20:11:48, Xi Han wrote: > On 2016/08/31 18:12:46, dfalcantara wrote: > > I don't think this function belongs here because it's a functional change. It > > even calls through directly to the delegate. Why don't you just call it on > the > > delegate from AddToHomeScreenManager::CreateInfobar? You already have access > to > > it there. > > Hmm, this call needs to be made after both AppBannerInfobarDelegateAndroid and > AppBannerInfobarAndroid are initialized on Java and C++ sides. This is because > the InstallWebAPK() not only starts the installation, but also updates the texts > on the app banner infobar. However, in CreateInfoBarForWebAPK(), the delegate is > a unique_ptr, and it becomes invalid as long as the infobar is created. So I > can't call InstallWebAPK directly through the delegate, but via infobar and to > the delegate. Beside, I don't want add more hack to the > AppBannerInfobarAndroid.java to make the text is initialized with WebAPK logic > (the "Adding" button). I would like to hear if you have better idea. Your change to AppBannerInfoBarAndroid::InstallWebApk() consists only of a call to delegate()->InstallWebApk(web_contents). That's called at the very end of CreateInfobar, where everything has already been built. Can't you just call InfoBarDelegate* delegate = static_cast<AppBannerInfoBarAndroid*>(infobar)->delegate(); static_cast<AppBannerInfoBarDelegate*>(delegate)->InstallWebApk(web_contents);?
Patchset #3 (id:160001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/app_banner_infobar_android.h (right): https://codereview.chromium.org/2290603005/diff/100001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/app_banner_infobar_android.h:42: void InstallWebApk(content::WebContents* web_contents); On 2016/08/31 20:24:22, dfalcantara wrote: > On 2016/08/31 20:11:48, Xi Han wrote: > > On 2016/08/31 18:12:46, dfalcantara wrote: > > > I don't think this function belongs here because it's a functional change. > It > > > even calls through directly to the delegate. Why don't you just call it on > > the > > > delegate from AddToHomeScreenManager::CreateInfobar? You already have > access > > to > > > it there. > > > > Hmm, this call needs to be made after both AppBannerInfobarDelegateAndroid and > > AppBannerInfobarAndroid are initialized on Java and C++ sides. This is because > > the InstallWebAPK() not only starts the installation, but also updates the > texts > > on the app banner infobar. However, in CreateInfoBarForWebAPK(), the delegate > is > > a unique_ptr, and it becomes invalid as long as the infobar is created. So I > > can't call InstallWebAPK directly through the delegate, but via infobar and to > > the delegate. Beside, I don't want add more hack to the > > AppBannerInfobarAndroid.java to make the text is initialized with WebAPK logic > > (the "Adding" button). I would like to hear if you have better idea. > > Your change to AppBannerInfoBarAndroid::InstallWebApk() consists only of a call > to delegate()->InstallWebApk(web_contents). That's called at the very end of > CreateInfobar, where everything has already been built. > > Can't you just call > > InfoBarDelegate* delegate = > static_cast<AppBannerInfoBarAndroid*>(infobar)->delegate(); > static_cast<AppBannerInfoBarDelegate*>(delegate)->InstallWebApk(web_contents);? You are right, updated. Thanks:)
lgtm https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:73: // disables the button to avoid usr interaction. It also starts the WebAPK's user interaction https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:218: static_cast<banners::AppBannerInfoBarDelegateAndroid*>(infobar->delegate()) Longer term, you're probably going to need to add InfoBarDelegate::AsAppBannerInfoBarDelegateAndroid() like all the other InfobarDelegate subclasses.
https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool isShortcutInfoEmpty(ShortcutInfo* info) { Nit: IsInfoEmpty(), and it should take a const ShortcutInfo*. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, Don't change the signature of this method. Instead, pull over the CreateShortcutInfo method into the anonymous namespace in this file, make it take a manifest and the appropriate URL arguments, and call it in this constructor. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:102: std::unique_ptr<ShortcutInfo> info_; Nit: shortcut_info_. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:229: std::unique_ptr<ShortcutInfo> AppBannerManagerAndroid::CreateShortcutInfo() { Put this method in the anonymous namespace in AppBannerInfoBarDelegateAndroid, and call it in the appropriate constructor. That means this file doesn't need to change.
Also: change the title to be "Trigger app banner when add to homescreen is pressed and WebAPKs are enabled" https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:193: std::unique_ptr<ShortcutInfo> info_ptr(new ShortcutInfo(info)); Inline these construction calls into line 195 using base::MakeUnique. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:216: base::WrapUnique(infobar)); WrapUnique is dispreferred as of a few weeks ago. Instead, you should use base::MakeUnique at line 201 when constructing the infobar to put it directly into a unique_ptr.
Description was changed from ========== Clicking A2HS from the app menu installs WebAPK for WebAPK-compatible site. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ========== to ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ==========
Patchset #5 (id:220001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:240001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool isShortcutInfoEmpty(ShortcutInfo* info) { On 2016/09/01 05:22:00, dominickn wrote: > Nit: IsInfoEmpty(), and it should take a const ShortcutInfo*. Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, On 2016/09/01 05:22:00, dominickn wrote: > Don't change the signature of this method. Instead, pull over the > CreateShortcutInfo method into the anonymous namespace in this file, make it > take a manifest and the appropriate URL arguments, and call it in this > constructor. I change the signature because AddToHonmeScreenManager doesn't receive a Manifest object when the onDataAvailable() is called from the data fetcher. A conversion from the Manifest to ShortcutInfo has been done in the data fetcher. Since AddToHonmeScreenManager will also create a banner, I change the signature to avoid doing the conversion again. How about keeping both constructors? The one with Manifest calls the one with ShortcutInfo. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:73: // disables the button to avoid usr interaction. It also starts the WebAPK's On 2016/08/31 21:07:39, dfalcantara wrote: > user interaction Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:102: std::unique_ptr<ShortcutInfo> info_; On 2016/09/01 05:22:00, dominickn wrote: > Nit: shortcut_info_. Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:229: std::unique_ptr<ShortcutInfo> AppBannerManagerAndroid::CreateShortcutInfo() { On 2016/09/01 05:22:00, dominickn wrote: > Put this method in the anonymous namespace in AppBannerInfoBarDelegateAndroid, > and call it in the appropriate constructor. That means this file doesn't need to > change. Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:193: std::unique_ptr<ShortcutInfo> info_ptr(new ShortcutInfo(info)); On 2016/09/01 05:29:02, dominickn wrote: > Inline these construction calls into line 195 using base::MakeUnique. Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:216: base::WrapUnique(infobar)); On 2016/09/01 05:29:02, dominickn wrote: > WrapUnique is dispreferred as of a few weeks ago. Instead, you should use > base::MakeUnique at line 201 when constructing the infobar to put it directly > into a unique_ptr. Done. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:218: static_cast<banners::AppBannerInfoBarDelegateAndroid*>(infobar->delegate()) On 2016/08/31 21:07:39, dfalcantara wrote: > Longer term, you're probably going to need to add > InfoBarDelegate::AsAppBannerInfoBarDelegateAndroid() like all the other > InfobarDelegate subclasses. Done.
lgtm % nits. Thanks Xi. https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, On 2016/09/01 18:44:13, Xi Han wrote: > On 2016/09/01 05:22:00, dominickn wrote: > > Don't change the signature of this method. Instead, pull over the > > CreateShortcutInfo method into the anonymous namespace in this file, make it > > take a manifest and the appropriate URL arguments, and call it in this > > constructor. > > I change the signature because AddToHonmeScreenManager doesn't receive a > Manifest object when the onDataAvailable() is called from the data fetcher. A > conversion from the Manifest to ShortcutInfo has been done in the data fetcher. > Since AddToHonmeScreenManager will also create a banner, I change the signature > to avoid doing the conversion again. > > How about keeping both constructors? The one with Manifest calls the one with > ShortcutInfo. Acknowledged - nice delegation of constructors. However, one of us should clean this up in a follow up CL by making AppBannerManagerAndroid pass a ShortcutInfo instead of a manifest (and then we can get rid of the third constructor and a bunch of the constructor arguments since they're already inside ShortcutInfo). https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool isInfoEmpty(const ShortcutInfo* info) { Nit: capital "I" in "Is" (C++ naming convention) https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:51: if (manifest.IsEmpty()) Nit (save a return statement): if (!manifest.IsEmpty()) { info_ptr->Update // etc. } return info_ptr; https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:198: -1 /* event_request_id */, true); Minor nit: add /* is_webapk */ after true inside the brackets. https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:215: banners::AppBannerInfoBarDelegateAndroid* delegate = Minor nit: move this line below 216 so you fetch the delegate pointer immediately before you use it.
Hi Donminick, I am a little confused about the suggestion of a follow up CL to make AppBannerManagerAndroid pass ShortcutInfo the the AppBannerInfoBarDelegateAndroid. If you take a look at "Patch Set 3", is that what you suggested? I can revert code back if you like:) https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, On 2016/09/02 01:00:58, dominickn wrote: > On 2016/09/01 18:44:13, Xi Han wrote: > > On 2016/09/01 05:22:00, dominickn wrote: > > > Don't change the signature of this method. Instead, pull over the > > > CreateShortcutInfo method into the anonymous namespace in this file, make it > > > take a manifest and the appropriate URL arguments, and call it in this > > > constructor. > > > > I change the signature because AddToHonmeScreenManager doesn't receive a > > Manifest object when the onDataAvailable() is called from the data fetcher. A > > conversion from the Manifest to ShortcutInfo has been done in the data > fetcher. > > Since AddToHonmeScreenManager will also create a banner, I change the > signature > > to avoid doing the conversion again. > > > > How about keeping both constructors? The one with Manifest calls the one with > > ShortcutInfo. > > Acknowledged - nice delegation of constructors. However, one of us should clean > this up in a follow up CL by making AppBannerManagerAndroid pass a ShortcutInfo > instead of a manifest (and then we can get rid of the third constructor and a > bunch of the constructor arguments since they're already inside ShortcutInfo). I am confused, since that is what I did before. I can revert it back if you like. https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool isInfoEmpty(const ShortcutInfo* info) { On 2016/09/02 01:00:58, dominickn wrote: > Nit: capital "I" in "Is" (C++ naming convention) Done. https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:51: if (manifest.IsEmpty()) On 2016/09/02 01:00:58, dominickn wrote: > Nit (save a return statement): > > if (!manifest.IsEmpty()) { > info_ptr->Update > // etc. > } > return info_ptr; Done. https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:198: -1 /* event_request_id */, true); On 2016/09/02 01:00:58, dominickn wrote: > Minor nit: add /* is_webapk */ after true inside the brackets. Done. https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:215: banners::AppBannerInfoBarDelegateAndroid* delegate = On 2016/09/02 01:00:58, dominickn wrote: > Minor nit: move this line below 216 so you fetch the delegate pointer > immediately before you use it. You can't get the delegate pointer once the std::move(infobar_ptr) is called, since the infobar_ptr will be invalid. I learned this from a scary and confusing print stack.
https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, On 2016/09/02 13:58:23, Xi Han wrote: > On 2016/09/02 01:00:58, dominickn wrote: > > On 2016/09/01 18:44:13, Xi Han wrote: > > > On 2016/09/01 05:22:00, dominickn wrote: > > > > Don't change the signature of this method. Instead, pull over the > > > > CreateShortcutInfo method into the anonymous namespace in this file, make > it > > > > take a manifest and the appropriate URL arguments, and call it in this > > > > constructor. > > > > > > I change the signature because AddToHonmeScreenManager doesn't receive a > > > Manifest object when the onDataAvailable() is called from the data fetcher. > A > > > conversion from the Manifest to ShortcutInfo has been done in the data > > fetcher. > > > Since AddToHonmeScreenManager will also create a banner, I change the > > signature > > > to avoid doing the conversion again. > > > > > > How about keeping both constructors? The one with Manifest calls the one > with > > > ShortcutInfo. > > > > Acknowledged - nice delegation of constructors. However, one of us should > clean > > this up in a follow up CL by making AppBannerManagerAndroid pass a > ShortcutInfo > > instead of a manifest (and then we can get rid of the third constructor and a > > bunch of the constructor arguments since they're already inside ShortcutInfo). > > I am confused, since that is what I did before. I can revert it back if you > like. Oh, wow, sorry for the water-in-the-brain reviewing, of course that's why you did it that way before. It now makes sense. Yes, put it back that way, and sorry for making you circle on this. :( https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/260001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:215: banners::AppBannerInfoBarDelegateAndroid* delegate = On 2016/09/02 13:58:23, Xi Han wrote: > On 2016/09/02 01:00:58, dominickn wrote: > > Minor nit: move this line below 216 so you fetch the delegate pointer > > immediately before you use it. > > You can't get the delegate pointer once the std::move(infobar_ptr) is called, > since the infobar_ptr will be invalid. I learned this from a scary and confusing > print stack. Acknowledged - I'm blind and didn't see the std::move.
hanxi@chromium.org changed reviewers: + cpu@chromium.org
Hi Dominick, PTAL, thanks! +cpu@chromium.org: I need owners review for: -components/infobars/core/infobar_delegate.h -components/infobars/core/infobar_delegate.cc Could you please take a look? Thanks! https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/180001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:54: std::unique_ptr<ShortcutInfo> info, On 2016/09/02 14:30:48, dominickn wrote: > On 2016/09/02 13:58:23, Xi Han wrote: > > On 2016/09/02 01:00:58, dominickn wrote: > > > On 2016/09/01 18:44:13, Xi Han wrote: > > > > On 2016/09/01 05:22:00, dominickn wrote: > > > > > Don't change the signature of this method. Instead, pull over the > > > > > CreateShortcutInfo method into the anonymous namespace in this file, > make > > it > > > > > take a manifest and the appropriate URL arguments, and call it in this > > > > > constructor. > > > > > > > > I change the signature because AddToHonmeScreenManager doesn't receive a > > > > Manifest object when the onDataAvailable() is called from the data > fetcher. > > A > > > > conversion from the Manifest to ShortcutInfo has been done in the data > > > fetcher. > > > > Since AddToHonmeScreenManager will also create a banner, I change the > > > signature > > > > to avoid doing the conversion again. > > > > > > > > How about keeping both constructors? The one with Manifest calls the one > > with > > > > ShortcutInfo. > > > > > > Acknowledged - nice delegation of constructors. However, one of us should > > clean > > > this up in a follow up CL by making AppBannerManagerAndroid pass a > > ShortcutInfo > > > instead of a manifest (and then we can get rid of the third constructor and > a > > > bunch of the constructor arguments since they're already inside > ShortcutInfo). > > > > I am confused, since that is what I did before. I can revert it back if you > > like. > > Oh, wow, sorry for the water-in-the-brain reviewing, of course that's why you > did it that way before. It now makes sense. Yes, put it back that way, and sorry > for making you circle on this. :( No worries, just reverted it:)
hanxi@chromium.org changed reviewers: + thestig@chromium.org
+ thestig@: I need owners review for: -components/infobars/core/infobar_delegate.h -components/infobars/core/infobar_delegate.cc Could you please take a look? Thanks!
Still lgtm % nit. https://codereview.chromium.org/2290603005/diff/300001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/300001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:101: GURL manifest_url_; Nit: manifest_url and icon_url aren't needed in the object anymore - they were only used to create ShortcutInfo. Delete them in the constructor and in the object.
thestig@ ping. https://codereview.chromium.org/2290603005/diff/300001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/300001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:101: GURL manifest_url_; On 2016/09/06 01:49:23, dominickn wrote: > Nit: manifest_url and icon_url aren't needed in the object anymore - they were > only used to create ShortcutInfo. Delete them in the constructor and in the > object. Done.
Description was changed from ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... This CL depends on CL (https://codereview.chromium.org/2244223002/). BUG=627250 ========== to ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... ==========
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...
On 2016/09/05 13:53:48, Xi Han wrote: > + thestig@: > I need owners review for: > -components/infobars/core/infobar_delegate.h > -components/infobars/core/infobar_delegate.cc > Could you please take a look? Thanks! I'm not in components/infobars/OWNERS though.
hanxi@chromium.org changed reviewers: + pkasting@chromium.org - cpu@chromium.org, thestig@chromium.org
hanxi@chromium.org changed reviewers: + cpu@chromium.org, thestig@chromium.org
+pkasting@: Could you please review the followings files: -components/infobars/core/infobar_delegate.h -components/infobars/core/infobar_delegate.cc Thanks! thestig@: I don't know why my code review extensions suggest you, sorry for the confusion. Send to pkasting@ instead.
A number of comments in here request cleanups of stuff you weren't newly adding in this CL. I don't believe I was asked to review this code previously, and it looks like it has some existing problems (I don't think the original authors/reviewers were as familiar with the infobar system as desirable). https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:161: void AppBannerInfoBarDelegateAndroid::InstallWebApk( Nit: This function just wraps AcceptWebApk(). Make that public instead. Rename it if necessary. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:163: if (!web_contents) { Will callers ever pass you null? If not, do nothing, or DCHECK. If they really will, say when this can be null/why that's not caller error, and handle it without logging. See comments elsewhere in CL regarding logging in general. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:242: } else if (is_webapk_) { Nit: No else after return https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:34: AppBannerInfoBarDelegateAndroid( Infobar delegate classes shouldn't have public constructors. The constructor should be private, and there should be a public Create() function that constructs the delegate and its infobar and adds the infobar to the relevant infobar service. A simple such example is at GeneratedPasswordSavedInfoBarDelegateAndroid::Create(). https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:37: std::unique_ptr<ShortcutInfo> info, Why are you passing and storing this by unique_ptr instead of passing by const ref and storing by value? The lone caller of this constructor always passes a non-null pointer, so this just obscures that. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:43: AppBannerInfoBarDelegateAndroid( Strongly consider splitting this infobar delegate into two different delegates. Right now there are two constructors that take very different parameter sets; only some of the API methods are applicable to each type; only some of the member variables are applicable to each type. Maybe what you want is two classes, possibly with a common base if they have shared functionality. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:116: }; // AppBannerInfoBarDelegateAndroid Nit: We don't use trailing comments like this when closing classes. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:47: std::unique_ptr<ShortcutInfo> info_ptr(new ShortcutInfo(GURL::EmptyGURL())); Don't use EmptyGURL() just because you want to pass an empty GURL. Use GURL() for that. EmptyGURL() is for functions that need to return a const ref to an empty GURL. Nit: Prefer a name like "shortcut_info" to a less-descriptive, code-redundant name like "info_ptr" (we already know it's some kind of pointer from the use of the -> operator). https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:214: RecordDidShowBanner("AppBanner.WebApp.Shown"); The fact that these blocks are basically duplicated, and that this one largely duplicates the desktop code, is more support for the idea that this sort of thing should all be happening in the infobar delegate Create() function, not in the caller here. Furthermore, these blocks are all incorrect: they all log things like "banner shown" without checking the return of AddInfoBar() to see if the infobars in question were actually shown. See comment elsewhere noting you have to check the return value of that. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:195: infobar_ptr.get()->delegate()->AsAppBannerInfoBarDelegateAndroid(); Don't add this function. You don't need it and we want to avoid expanding the surface in infobar_delegate.h unless necessary. We should only add such things when people need to do RTTI-like runtime type checking. Do something like this instead: auto infobar_delegate = base::MakeUnique<banners::AppBannerInfoBarDelegateAndroid>(...); auto* raw_delegate = infobar_delegate.get(); content::WebContents* web_contents = data_fetcher_->web_contents(); if (InfoBarService::FromWebContents(web_contents)->AddInfoBar( base::MakeUnique<AppBannerInfoBarAndroid>( std::move(infobar_delegate), data_fetcher_->shortcut_info().url, true))) raw_delegate->InstallWebApk(web_contents); Several notes on this. * I used auto more aggressively to reduce typename duplication and verbosity. * LOG(ERROR) is generally undesirable. It means "this can fail at runtime" but doesn't say when/why. It bloats the executable with the logging strings. And people generally don't have a plan on how to collect and analyze the LOG data anyway. Usually these should either be removed, replaced by DCHECKs, or replaced by simple conditional returns/error-handling. * In the specific cases here, the first LOG() can never fire because memory allocation would have had to fail (which would have resulted in a forced crash before returning). Whether the second can fire is unclear to me; if this can ever actually be null it should probably be a conditional return, but ensuring it can't be null and then doing nothing or using a DCHECK to assure the reader of that would be better. * You need to check the return value of AddInfoBar(). Don't assume it succeeded. * I avoided using "_ptr" to distinguish types because people have different intuitions on whether it means the "unique_ptr" (because the suffix matches) or the "raw pointer" (because that's an actual pointer and not an object). My instinct is the latter, for example. "Raw", OTOH, is clear.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #8 (id:340001) has been deleted
pkasting@: I addressed all your comments except the one to split the delegate into two classes. It isn't a trivial refactory so I would prefer to do in a follow up CL. PTAL, thanks! https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:161: void AppBannerInfoBarDelegateAndroid::InstallWebApk( On 2016/09/06 19:31:23, Peter Kasting wrote: > Nit: This function just wraps AcceptWebApk(). Make that public instead. Rename > it if necessary. Done. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:163: if (!web_contents) { On 2016/09/06 19:31:23, Peter Kasting wrote: > Will callers ever pass you null? If not, do nothing, or DCHECK. If they really > will, say when this can be null/why that's not caller error, and handle it > without logging. See comments elsewhere in CL regarding logging in general. Move the extra logic to the callers. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:242: } else if (is_webapk_) { On 2016/09/06 19:31:23, Peter Kasting wrote: > Nit: No else after return Done. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:34: AppBannerInfoBarDelegateAndroid( On 2016/09/06 19:31:23, Peter Kasting wrote: > Infobar delegate classes shouldn't have public constructors. The constructor > should be private, and there should be a public Create() function that > constructs the delegate and its infobar and adds the infobar to the relevant > infobar service. A simple such example is at > GeneratedPasswordSavedInfoBarDelegateAndroid::Create(). Done. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:37: std::unique_ptr<ShortcutInfo> info, On 2016/09/06 19:31:23, Peter Kasting wrote: > Why are you passing and storing this by unique_ptr instead of passing by const > ref and storing by value? The lone caller of this constructor always passes a > non-null pointer, so this just obscures that. Mostly because it is empty when the delegate is initialized for native app. In |AppBannerInfoBarDelegateAndroid|, we could declare "ShortcutInfo shortcut_info" or a unique_ptr. When initializing the delegate for native app, the |shorcut_info| it is always empty since it is not used nor initialized. So I could either introduce a default constructor for ShorcutInfo which doesn't have one, or use unique_ptr for the empty case of native app. Personally I prefer to use unique_ptr. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:43: AppBannerInfoBarDelegateAndroid( I agree, but I assume it is a big refactoring. It involves splitting logic on InfobarDelegate and Infobar on both C++ and Java side. Since the changes don't relate to this CL, I would prefer to do the refactoring in a follow up CL. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:116: }; // AppBannerInfoBarDelegateAndroid On 2016/09/06 19:31:23, Peter Kasting wrote: > Nit: We don't use trailing comments like this when closing classes. Done. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:47: std::unique_ptr<ShortcutInfo> info_ptr(new ShortcutInfo(GURL::EmptyGURL())); On 2016/09/06 19:31:23, Peter Kasting wrote: > Don't use EmptyGURL() just because you want to pass an empty GURL. Use GURL() > for that. EmptyGURL() is for functions that need to return a const ref to an > empty GURL. > > Nit: Prefer a name like "shortcut_info" to a less-descriptive, code-redundant > name like "info_ptr" (we already know it's some kind of pointer from the use of > the -> operator). Done. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:214: RecordDidShowBanner("AppBanner.WebApp.Shown"); On 2016/09/06 19:31:23, Peter Kasting wrote: > The fact that these blocks are basically duplicated, and that this one largely > duplicates the desktop code, is more support for the idea that this sort of > thing should all be happening in the infobar delegate Create() function, not in > the caller here. > > Furthermore, these blocks are all incorrect: they all log things like "banner > shown" without checking the return of AddInfoBar() to see if the infobars in > question were actually shown. See comment elsewhere noting you have to check > the return value of that. Updated. Make the Create return a bool, and only log things when AddInfoBar() succeeds. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:195: infobar_ptr.get()->delegate()->AsAppBannerInfoBarDelegateAndroid(); On 2016/09/06 19:31:23, Peter Kasting wrote: > Don't add this function. You don't need it and we want to avoid expanding the > surface in infobar_delegate.h unless necessary. We should only add such things > when people need to do RTTI-like runtime type checking. > > Do something like this instead: > > auto infobar_delegate = > base::MakeUnique<banners::AppBannerInfoBarDelegateAndroid>(...); > auto* raw_delegate = infobar_delegate.get(); > content::WebContents* web_contents = data_fetcher_->web_contents(); > if (InfoBarService::FromWebContents(web_contents)->AddInfoBar( > base::MakeUnique<AppBannerInfoBarAndroid>( > std::move(infobar_delegate), data_fetcher_->shortcut_info().url, > true))) > raw_delegate->InstallWebApk(web_contents); > > Several notes on this. > > * I used auto more aggressively to reduce typename duplication and verbosity. > * LOG(ERROR) is generally undesirable. It means "this can fail at runtime" but > doesn't say when/why. It bloats the executable with the logging strings. And > people generally don't have a plan on how to collect and analyze the LOG data > anyway. Usually these should either be removed, replaced by DCHECKs, or > replaced by simple conditional returns/error-handling. > * In the specific cases here, the first LOG() can never fire because memory > allocation would have had to fail (which would have resulted in a forced crash > before returning). Whether the second can fire is unclear to me; if this can > ever actually be null it should probably be a conditional return, but ensuring > it can't be null and then doing nothing or using a DCHECK to assure the reader > of that would be better. > * You need to check the return value of AddInfoBar(). Don't assume it > succeeded. > * I avoided using "_ptr" to distinguish types because people have different > intuitions on whether it means the "unique_ptr" (because the suffix matches) or > the "raw pointer" (because that's an actual pointer and not an object). My > instinct is the latter, for example. "Raw", OTOH, is clear. Thanks for all the suggestions. I will be more careful in the future.
Patchset #8 (id:360001) has been deleted
Patchset #9 (id:400001) has been deleted
LGTM https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:37: std::unique_ptr<ShortcutInfo> info, On 2016/09/07 15:53:17, Xi Han wrote: > On 2016/09/06 19:31:23, Peter Kasting wrote: > > Why are you passing and storing this by unique_ptr instead of passing by const > > ref and storing by value? The lone caller of this constructor always passes a > > non-null pointer, so this just obscures that. > > Mostly because it is empty when the delegate is initialized for native app. In > |AppBannerInfoBarDelegateAndroid|, we could declare "ShortcutInfo shortcut_info" > or a unique_ptr. When initializing the delegate for native app, the > |shorcut_info| it is always empty since it is not used nor initialized. So I > could either introduce a default constructor for ShorcutInfo which doesn't have > one, or use unique_ptr for the empty case of native app. Personally I prefer to > use unique_ptr. That makes sense for now. It seems like this can change if the delegate is split into pieces for the native versus web cases, so keep this change on the list of things to consider doing then. https://codereview.chromium.org/2290603005/diff/320001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:43: AppBannerInfoBarDelegateAndroid( On 2016/09/07 15:53:17, Xi Han wrote: > I agree, but I assume it is a big refactoring. It involves splitting logic on > InfobarDelegate and Infobar on both C++ and Java side. Since the changes don't > relate to this CL, I would prefer to do the refactoring in a follow up CL. I'm fine with that. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:38: using base::android::ScopedJavaLocalRef; Nit: Be cautious with using directives; prefer to use them only when it noticeably improves readability or shortens code. My rule of thumb is, if I can avoid the directive and the file is not longer, I avoid it. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool IsInfoEmpty(const ShortcutInfo* info) { Nit: Take a const unique_ptr& and you won't have to say .get() on all the callers. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:43: return !info || info->url.is_empty(); Note that if you split this class in the future, I think one type of delegate will always have a null pointer and the other always a non-null one, so this function can be simplified or even removed. Just something to keep in mind for later. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:46: } // anonymous namespace Nit: No need for this comment https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:60: const GURL& url = shortcut_info->url; Nit: Inline this into the caller below (even if you wanted a temp, you'd want to declare it down there where it's used anyway) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:66: Nit: Can't really figure out the logic on when this function does and doesn't insert newlines. I might remove this one and the next, and add one before the final "return true"? That way we have one block for trying to create the infobar, and then one set-off block for optionally calling AcceptWebApk(). https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:92: std::move(infobar_delegate), native_app_data); Nit: Might want to inline this below (or even both temps, but that's pretty darn ugly) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:93: Nit: Maybe remove this newline? https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:120: int newState = Java_AppBannerInfoBarDelegateAndroid_determineInstallState( Nit: |new_state| (this isn't Java) Or just inline this below. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:130: if (!infobar()) How can this ever be null? A delegate should never have no infobar. (2 places) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:135: if (!web_contents) Can this really be called when the infobar is unowned? You shouldn't be checking this unless you need to (e.g. if this callback can occur while the infobar is animating closed). If you do need to, you should probably note why. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:180: // the "Open" button is pressed, then open the installed WebAPK. Nit: then -> so https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:195: DVLOG(1) << "Trigger the installation of the WebAPK."; Nit: Avoid checking in logging if possible. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:197: *shortcut_info_.get(), Nit: .get() not necessary when dereferencing a smart pointer with '*' (3 places) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:201: // Returns false to prevent the infobar from disappearing. Nit: We can see it returns false; maybe instead say "Prevent the infobar from disappearing, because..." (or "so...") with an explanation of why this matters. Put a newline above comments like this since they describe the subsequenct block of code. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:205: // Open the WebAPK. Nit: Consider making this shorter arm the one in the indented case above, just to minimize the amount of code that gets indented. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:254: JNIEnv* env = base::android::AttachCurrentThread(); Nit: Just inline this below https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:284: if (!web_contents) How can this ever be null? We should never be getting InfoBarDismissed() while not owned. Similar question applies to Accept() below. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:296: } else if (!IsInfoEmpty(shortcut_info_.get())) { Can both these conditions fail, or is one always true? https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:323: else if (is_webapk_) Nit: No else after return (2 places) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:335: if (tab == nullptr) { Can this be null? When? Nit: "if (!tab)" is probably a more common Chromium idiom. (2 places) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:347: if (was_opened) { Nit: No {} https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:383: infobar()->RemoveSelf(); Nit: Might be safer to do this after the next function call since this call can delete |this|? I don't know if that next call can reach back to anything in this object. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:385: DVLOG(1) << "The WebAPK installation failed."; Nit: Similarly, avoid this logging unless it's actively helping you debug an issue. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:411: if (tab == nullptr) { Again, can this be null? When? https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:33: // Creates and shows the infobar a web app. Nit: Grammar. Also, rather than "shows" I would be more specific, e.g. "Creates an infobar and delegate for promoting the installation of a web app, and adds the infobar to the InfoBarManager for |web_contents|." https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:76: // installed. Rather than describing when this is called (which can get out of date as people add or modify callsites, not updating this comment), just say what it actually does, and what its return value means. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:30: using base::android::ScopedJavaLocalRef; Nit: See earlier nit on using directives. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:39: const char kPlayInlineReferrer[] = "playinline=chrome_inline"; Nit: Each of these temps is used in only one function. Define them in the functions in which they're used. Or just inline them where used, if the variable names don't add much. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:45: std::unique_ptr<ShortcutInfo> shortcut_info(new ShortcutInfo(GURL())); Nit: Prefer "= base::MakeUnique<ShortcutInfo>(GURL())" to raw new. Feel free to use auto as needed to avoid re-spelling the type name. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:201: bool is_webapk = ChromeWebApkHost::AreWebApkEnabled(); Nit: Just inline below https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:206: false /* start_install_webapk*/)) { Nit: Need space before */ (or just remove the comment, people should have param names in their IDE cross-references or codesearch) https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:280: return true; Nit: Shorter: const bool correct_platform = (platform == "play"); if (correct_platform && !id.empty()) return true; ReportStatus(correct_platform ? NO_ID_SPECIFIED : PLATFORM_NOT_SUPPORTED_ON_ANDROID); return false; https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:294: return value_str; Nit: Shorter, more readable, and more efficient: for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { if (it.GetKey() == name) return it.GetValue(); } https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:296: return ""; Nit: "" -> std::string() https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:314: return ScopedJavaLocalRef<jobject>(manager->GetJavaBannerManager()); Nit: Seems like this could be: return manager ? manager->GetJavaBannerManager() : ScopedJavaLocalRef<jobject>(); https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:176: true /* start_install_webapk*/); Nit: Space before */, or remove these
Patchset #10 (id:440001) has been deleted
Patchset #10 (id:460001) has been deleted
https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:38: using base::android::ScopedJavaLocalRef; On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Be cautious with using directives; prefer to use them only when it > noticeably improves readability or shortens code. > > My rule of thumb is, if I can avoid the directive and the file is not longer, I > avoid it. I will keep this in mind. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool IsInfoEmpty(const ShortcutInfo* info) { On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Take a const unique_ptr& and you won't have to say .get() on all the > callers. It can't be a unique_ptr since when this function is called, we need to pass std::move(shortcut_info_). https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:43: return !info || info->url.is_empty(); On 2016/09/08 04:56:57, Peter Kasting wrote: > Note that if you split this class in the future, I think one type of delegate > will always have a null pointer and the other always a non-null one, so this > function can be simplified or even removed. Just something to keep in mind for > later. Acknowledged. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:46: } // anonymous namespace On 2016/09/08 04:56:56, Peter Kasting wrote: > Nit: No need for this comment Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:60: const GURL& url = shortcut_info->url; On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Inline this into the caller below (even if you wanted a temp, you'd want to > declare it down there where it's used anyway) It has to be done before std::move(shortcut_info) is called, otherwise the pointer shortcut_info will be invalid. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:66: On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Can't really figure out the logic on when this function does and doesn't > insert newlines. I might remove this one and the next, and add one before the > final "return true"? That way we have one block for trying to create the > infobar, and then one set-off block for optionally calling AcceptWebApk(). Sure, updated. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:92: std::move(infobar_delegate), native_app_data); On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Might want to inline this below (or even both temps, but that's pretty darn > ugly) Inline the infobar one only, since the delegate one is too long. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:93: On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Maybe remove this newline? Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:120: int newState = Java_AppBannerInfoBarDelegateAndroid_determineInstallState( On 2016/09/08 04:56:56, Peter Kasting wrote: > Nit: |new_state| (this isn't Java) > > Or just inline this below. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:130: if (!infobar()) On 2016/09/08 04:56:57, Peter Kasting wrote: > How can this ever be null? A delegate should never have no infobar. (2 places) Add DCHECK instead. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:135: if (!web_contents) On 2016/09/08 04:56:57, Peter Kasting wrote: > Can this really be called when the infobar is unowned? You shouldn't be > checking this unless you need to (e.g. if this callback can occur while the > infobar is animating closed). If you do need to, you should probably note why. Removed. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:180: // the "Open" button is pressed, then open the installed WebAPK. On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: then -> so Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:195: DVLOG(1) << "Trigger the installation of the WebAPK."; On 2016/09/08 04:56:56, Peter Kasting wrote: > Nit: Avoid checking in logging if possible. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:197: *shortcut_info_.get(), On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: .get() not necessary when dereferencing a smart pointer with '*' (3 places) Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:201: // Returns false to prevent the infobar from disappearing. On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: We can see it returns false; maybe instead say "Prevent the infobar from > disappearing, because..." (or "so...") with an explanation of why this matters. > > Put a newline above comments like this since they describe the subsequenct block > of code. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:205: // Open the WebAPK. On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Consider making this shorter arm the one in the indented case above, just > to minimize the amount of code that gets indented. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:254: JNIEnv* env = base::android::AttachCurrentThread(); On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Just inline this below Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:284: if (!web_contents) On 2016/09/08 04:56:56, Peter Kasting wrote: > How can this ever be null? We should never be getting InfoBarDismissed() while > not owned. > > Similar question applies to Accept() below. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:296: } else if (!IsInfoEmpty(shortcut_info_.get())) { On 2016/09/08 04:56:57, Peter Kasting wrote: > Can both these conditions fail, or is one always true? I believe one is always true, see the DCHECH in the Create() function. Yes, it's better to use else her. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:323: else if (is_webapk_) On 2016/09/08 04:56:56, Peter Kasting wrote: > Nit: No else after return (2 places) Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:335: if (tab == nullptr) { On 2016/09/08 04:56:57, Peter Kasting wrote: > Can this be null? When? > > Nit: "if (!tab)" is probably a more common Chromium idiom. (2 places) It looks like the tab shouldn't be null, but I am not 100 percent sure. Add a DCHECK here. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:347: if (was_opened) { On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:383: infobar()->RemoveSelf(); On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Might be safer to do this after the next function call since this call can > delete |this|? I don't know if that next call can reach back to anything in > this object. In the UI, we would like to see a toast shown after the infobar disappears, that is main reason the RemoveSelf is called earlier. The next one is a static function in Java, so I assume it would be fine. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:385: DVLOG(1) << "The WebAPK installation failed."; On 2016/09/08 04:56:56, Peter Kasting wrote: > Nit: Similarly, avoid this logging unless it's actively helping you debug an > issue. Personally I still prefer to have this log which helps identify whether the callback is called even if the installation fails. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:411: if (tab == nullptr) { On 2016/09/08 04:56:57, Peter Kasting wrote: > Again, can this be null? When? Add a DCHECK instead. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.h (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:33: // Creates and shows the infobar a web app. On 2016/09/08 04:56:57, Peter Kasting wrote: > Nit: Grammar. > > Also, rather than "shows" I would be more specific, e.g. "Creates an infobar and > delegate for promoting the installation of a web app, and adds the infobar to > the InfoBarManager for |web_contents|." Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.h:76: // installed. On 2016/09/08 04:56:57, Peter Kasting wrote: > Rather than describing when this is called (which can get out of date as people > add or modify callsites, not updating this comment), just say what it actually > does, and what its return value means. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:30: using base::android::ScopedJavaLocalRef; On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: See earlier nit on using directives. Acknowledged. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:39: const char kPlayInlineReferrer[] = "playinline=chrome_inline"; On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Each of these temps is used in only one function. Define them in the > functions in which they're used. Or just inline them where used, if the > variable names don't add much. Personally I would prefer to declare them at the beginning of the class, in case they will be used again in the future. Especially the fourth one is used twice. Since the first one is inline now, just remove it, but leave the other three. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:45: std::unique_ptr<ShortcutInfo> shortcut_info(new ShortcutInfo(GURL())); On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Prefer "= base::MakeUnique<ShortcutInfo>(GURL())" to raw new. > > Feel free to use auto as needed to avoid re-spelling the type name. Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:201: bool is_webapk = ChromeWebApkHost::AreWebApkEnabled(); On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Just inline below Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:206: false /* start_install_webapk*/)) { On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Need space before */ (or just remove the comment, people should have param > names in their IDE cross-references or codesearch) Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:280: return true; On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Shorter: > > const bool correct_platform = (platform == "play"); > if (correct_platform && !id.empty()) > return true; > ReportStatus(correct_platform ? NO_ID_SPECIFIED > : PLATFORM_NOT_SUPPORTED_ON_ANDROID); > return false; Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:294: return value_str; On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Shorter, more readable, and more efficient: > > for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { > if (it.GetKey() == name) > return it.GetValue(); > } Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:296: return ""; On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: "" -> std::string() Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:314: return ScopedJavaLocalRef<jobject>(manager->GetJavaBannerManager()); On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Seems like this could be: > > return manager ? manager->GetJavaBannerManager() > : ScopedJavaLocalRef<jobject>(); Done. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/webapps/add_to_homescreen_manager.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/webapps/add_to_homescreen_manager.cc:176: true /* start_install_webapk*/); On 2016/09/08 04:56:58, Peter Kasting wrote: > Nit: Space before */, or remove these 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, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2290603005/#ps480001 (title: "Clean ups")
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 ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... ========== to ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... ========== to ========== Trigger app banner when add to homescreen is pressed and WebAPKs are enabled. When user clicks A2HS from the app menu, a A2HS dialog shows to add a shortcut. When WebAPK is enabled, an app banner info bar is expected to show for WebAPK-compatible site. This CL includes: - AddToHomeScreenManager creates an app banner to install WebAPKs. - Refactory in AppBannerInfobarDelegateAndroid that passes in a ShortcutInfo instead of Manifest file. - When installing WebAPK from the app menu, the app banner skips the state of showing "Add to Homescrenn" on its button but jumps to "Adding" state and starts the installation. UX review see https://docs.google.com/presentation/d/1QbHDigLFR1oucsnlXQ9f6AxQYYI6Ka0Gl0A_v.... Committed: https://crrev.com/d256ebd2ba19a77c2c6d71f562f7db8b51254bc3 Cr-Commit-Position: refs/heads/master@{#417310} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d256ebd2ba19a77c2c6d71f562f7db8b51254bc3 Cr-Commit-Position: refs/heads/master@{#417310}
Message was sent while issue was closed.
https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:42: bool IsInfoEmpty(const ShortcutInfo* info) { On 2016/09/08 15:49:10, Xi Han wrote: > On 2016/09/08 04:56:57, Peter Kasting wrote: > > Nit: Take a const unique_ptr& and you won't have to say .get() on all the > > callers. > > It can't be a unique_ptr since when this function is called, we need to pass > std::move(shortcut_info_). I said "const unique_ptr&", not "unique_ptr". https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:60: const GURL& url = shortcut_info->url; On 2016/09/08 15:49:09, Xi Han wrote: > On 2016/09/08 04:56:57, Peter Kasting wrote: > > Nit: Inline this into the caller below (even if you wanted a temp, you'd want > to > > declare it down there where it's used anyway) > > It has to be done before std::move(shortcut_info) is called, otherwise the > pointer shortcut_info will be invalid. Oh bah. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:383: infobar()->RemoveSelf(); On 2016/09/08 15:49:09, Xi Han wrote: > On 2016/09/08 04:56:57, Peter Kasting wrote: > > Nit: Might be safer to do this after the next function call since this call > can > > delete |this|? I don't know if that next call can reach back to anything in > > this object. > > In the UI, we would like to see a toast shown after the infobar disappears, that > is main reason the RemoveSelf is called earlier. Are these orders perceptibly different to the user? RemoveSelf() should start the bar animating closed and return immediately. It doesn't block. https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... File chrome/browser/android/banners/app_banner_manager_android.cc (right): https://codereview.chromium.org/2290603005/diff/420001/chrome/browser/android... chrome/browser/android/banners/app_banner_manager_android.cc:39: const char kPlayInlineReferrer[] = "playinline=chrome_inline"; On 2016/09/08 15:49:10, Xi Han wrote: > On 2016/09/08 04:56:58, Peter Kasting wrote: > > Nit: Each of these temps is used in only one function. Define them in the > > functions in which they're used. Or just inline them where used, if the > > variable names don't add much. > > Personally I would prefer to declare them at the beginning of the class, in case > they will be used again in the future. Especially the fourth one is used twice. Twice yes, but in adjacent statements in the same function. http://google.github.io/styleguide/cppguide.html#Local_Variables says "...declare [variables] in as local a scope as possible, and as close to the first use as possible." While arguably constants are not the same as variables, declaring them somewhere widely separate from their use means that code referencing them now requires the reader to scan to a different place to see what the constant value actually is. It also makes it easier to have these left around after their uses are deleted. And if these are used in multiple places in the future they can always be factored out then. More directly in this case, I think kPlayInlineReferrer deserves a named constant anyway, but the kXXXName ones don't -- those string constants should just be inlined directly into the code that uses them, since the constant names just add verbosity to the actual strings without adding any clarity or explanation. So I'd inline those two and then make a decision about how to handle the last one based on how compelling you think my above arguments are :) https://codereview.chromium.org/2290603005/diff/480001/chrome/browser/android... File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc (right): https://codereview.chromium.org/2290603005/diff/480001/chrome/browser/android... chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:279: if (!native_app_data_.is_null()) { Nit: Reverse this condition and the arms, so the "else" isn't mentally a double-negative |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
