Update AppBannerManager & AppBannerManagerAndroid to request badge icon.

Update AppBannerManager & AppBannerManagerAndroid to request badge icon.

Update AppBannerManager & AppBannerManagerAndroid to request badge icon
when it requests the primary icon for Web apps and WebAPKs.

BUG=649771
Review-Url: https://codereview.chromium.org/2685363002
Cr-Commit-Position: refs/heads/master@{#452052}
Committed: https://chromium.googlesource.com/chromium/src/+/c59f97e2f5a21a13e672f0de7715e8bb2affc1bc

[F, 2017-02-10 16:06:03]

Description was changed from

==========
Update app banner manager


manager WIP

BUG=
==========

to

==========
Update AppBannerManager & AppBannerManagerAndroid to request badge icon.

Update AppBannerManager & AppBannerManagerAndroid to request badge icon
when it requests the primary icon for Web apps and WebAPKs.

BUG=649771
==========

[F, 2017-02-10 16:08:48]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 16:09:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-02-10 16:09:56]

zpeng@chromium.org changed reviewers:
+ dominickn@chromium.org

[F, 2017-02-10 16:09:56]

Hi Dominick, PTAL. Thanks!

[commit-bot: I haz the power, 2017-02-10 17:00:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 17:00:24]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-14 00:01:16]

Sorry for the delay on this one.

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_manager_android.cc:148: return
ShortcutHelper::GetIdealBadgeIconSizeInPx();
Since this is gated on WebAPKs, is there a way to have this efficiently check if
WebAPKs are enabled?

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:34: int g_current_request_id = -1;
The change to this is unrelated to this CL. Can you do it separately?

Additionally, I'm planning on removing the gCurrentRequestID in the future
anyway, so perhaps it's best to just leave this for now.

[F, 2017-02-14 19:20:10]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 19:20:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-02-14 19:22:42]

Thanks Dominick, PTAL!

I moved badge icon related stuff into AppBannerManagerAndroid since only WebAPK
uses badge icon.

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_manager_android.cc:148: return
ShortcutHelper::GetIdealBadgeIconSizeInPx();
On 2017/02/14 00:01:16, dominickn wrote:
> Since this is gated on WebAPKs, is there a way to have this efficiently check
if
> WebAPKs are enabled?

Done.

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/banners/app_...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2685363002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:34: int g_current_request_id = -1;
On 2017/02/14 00:01:16, dominickn wrote:
> The change to this is unrelated to this CL. Can you do it separately?
> 
> Additionally, I'm planning on removing the gCurrentRequestID in the future
> anyway, so perhaps it's best to just leave this for now.

Done.

[pkotwicz, 2017-02-14 20:02:07]

pkotwicz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[pkotwicz, 2017-02-14 20:02:08]

LGTM

https://codereview.chromium.org/2685363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:165:
InstallableParams AppBannerManagerAndroid::ParamsToPerformInstallableCheck() {
Nit: You can replace lines 166-170 with
InstallableParams params = AppBannerManager::ParamsToPerformInstallableCheck()

[F, 2017-02-14 20:10:36]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 20:11:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-02-14 20:11:20]

Thanks Dominick & Peter, PTAL!

https://codereview.chromium.org/2685363002/diff/20001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:165:
InstallableParams AppBannerManagerAndroid::ParamsToPerformInstallableCheck() {
On 2017/02/14 20:02:08, pkotwicz wrote:
> Nit: You can replace lines 166-170 with
> InstallableParams params = AppBannerManager::ParamsToPerformInstallableCheck()

Done. Thanks!

[commit-bot: I haz the power, 2017-02-14 21:06:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 21:06:44]

Dry run: This issue passed the CQ dry run.

[F, 2017-02-14 22:08:16]

Patchset #3 (id:40001) has been deleted

[F, 2017-02-14 22:08:31]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 22:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 23:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 23:24:23]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-15 06:05:47]

Looks good - just want to clarify one thing.

https://codereview.chromium.org/2685363002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:169: if
(ChromeWebApkHost::CanInstallWebApk()) {
This is now called twice in this class. I actually think that its value might be
constant over the life of the banner manager:

 * Google Play check is done once and the value is cached in
ChromeWebApkHost.java
 * Install from unknown sources check is a command line flag check, and you need
to restart if you change flags
 * Enabled check checks prefs, which are updated once

pkotwicz: do you agree with this assessment? If so, we should add a boolean
instance variable can_install_webapk_ and set it in the AppBannerManagerAndroid
constructor.

[pkotwicz, 2017-02-15 19:19:06]

Computing ChromeWebApkHost::CanInstallWebApk() once in the constructor sounds
good to me

[F, 2017-02-15 20:05:29]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[F, 2017-02-15 20:05:43]

Thanks Dominick & Peter, PTAL!

https://codereview.chromium.org/2685363002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:169: if
(ChromeWebApkHost::CanInstallWebApk()) {
On 2017/02/15 06:05:47, dominickn wrote:
> This is now called twice in this class. I actually think that its value might
be
> constant over the life of the banner manager:
> 
>  * Google Play check is done once and the value is cached in
> ChromeWebApkHost.java
>  * Install from unknown sources check is a command line flag check, and you
need
> to restart if you change flags
>  * Enabled check checks prefs, which are updated once
> 
> pkotwicz: do you agree with this assessment? If so, we should add a boolean
> instance variable can_install_webapk_ and set it in the
AppBannerManagerAndroid
> constructor.

Done.

[commit-bot: I haz the power, 2017-02-15 20:06:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 21:55:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 21:55:01]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-02-15 22:00:16]

Still LGTM

[dominickn, 2017-02-15 23:38:05]

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:126:
can_install_webapk_ = ChromeWebApkHost::CanInstallWebApk();
Set this in the constructor, not in RequestAppBanner. I think pkotwicz and I
agree that the value won't change over the lifetime of this class.

[pkotwicz, 2017-02-16 15:43:03]

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:126:
can_install_webapk_ = ChromeWebApkHost::CanInstallWebApk();
Dominick, I differ to your judgement

I am ok with either option. Setting this in
AppBannerManagerAndroid::RequestAppBanner() has the nice property of the JNI
call not being done if the banner is never shown because engagement is not high
enough. It is possible to make it clearer that
ChromeWebApkHost::CanInstallWebApk() is called here for this reason.

[dominickn, 2017-02-17 00:54:44]

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:126:
can_install_webapk_ = ChromeWebApkHost::CanInstallWebApk();
On 2017/02/16 15:43:03, pkotwicz wrote:
> Dominick, I differ to your judgement
> 
> I am ok with either option. Setting this in
> AppBannerManagerAndroid::RequestAppBanner() has the nice property of the JNI
> call not being done if the banner is never shown because engagement is not
high
> enough. It is possible to make it clearer that
> ChromeWebApkHost::CanInstallWebApk() is called here for this reason.

This class already makes a JNI call when it's initialised, and the engagement
threshold is low enough that it's likely RequestAppBanner will be called
multiple times over the lifetime of the AppBannerManagerAndroid class. So
calling this method once in the constructor should result in the least number of
calls to CanInstallWebApk() overall. Otherwise we're redundantly calling
CanInstallWebApk() once per banner flow, which could be as often as once per
navigation if you're going through a bunch of sites you visit a lot.

[F, 2017-02-21 15:51:43]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 15:52:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-02-21 15:53:36]

Thanks Dominick & Peter, PTAL!

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_manager_android.cc (right):

https://codereview.chromium.org/2685363002/diff/80001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_manager_android.cc:126:
can_install_webapk_ = ChromeWebApkHost::CanInstallWebApk();
On 2017/02/17 00:54:44, dominickn wrote:
> On 2017/02/16 15:43:03, pkotwicz wrote:
> > Dominick, I differ to your judgement
> > 
> > I am ok with either option. Setting this in
> > AppBannerManagerAndroid::RequestAppBanner() has the nice property of the JNI
> > call not being done if the banner is never shown because engagement is not
> high
> > enough. It is possible to make it clearer that
> > ChromeWebApkHost::CanInstallWebApk() is called here for this reason.
> 
> This class already makes a JNI call when it's initialised, and the engagement
> threshold is low enough that it's likely RequestAppBanner will be called
> multiple times over the lifetime of the AppBannerManagerAndroid class. So
> calling this method once in the constructor should result in the least number
of
> calls to CanInstallWebApk() overall. Otherwise we're redundantly calling
> CanInstallWebApk() once per banner flow, which could be as often as once per
> navigation if you're going through a bunch of sites you visit a lot.

Done. Thanks for the explanation :)

[commit-bot: I haz the power, 2017-02-21 16:49:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 16:49:35]

Dry run: This issue passed the CQ dry run.

[dominickn, 2017-02-21 23:31:52]

lgtm, thanks

[F, 2017-02-22 15:04:10]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-02-22 15:04:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2685363002/#ps100001
(title: "Addressing comments (move can_install init to constructor)")

[commit-bot: I haz the power, 2017-02-22 15:04:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 15:08:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487775850627300,
"parent_rev": "5ed489e5390dea9fae7f395832d9c0c9dc459609", "commit_rev":
"c59f97e2f5a21a13e672f0de7715e8bb2affc1bc"}

[commit-bot: I haz the power, 2017-02-22 15:08:51]

Description was changed from

==========
Update AppBannerManager & AppBannerManagerAndroid to request badge icon.

Update AppBannerManager & AppBannerManagerAndroid to request badge icon
when it requests the primary icon for Web apps and WebAPKs.

BUG=649771
==========

to

==========
Update AppBannerManager & AppBannerManagerAndroid to request badge icon.

Update AppBannerManager & AppBannerManagerAndroid to request badge icon
when it requests the primary icon for Web apps and WebAPKs.

BUG=649771

Review-Url: https://codereview.chromium.org/2685363002
Cr-Commit-Position: refs/heads/master@{#452052}
Committed:
https://chromium.googlesource.com/chromium/src/+/c59f97e2f5a21a13e672f0de7715...
==========

[commit-bot: I haz the power, 2017-02-22 15:08:52]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c59f97e2f5a21a13e672f0de7715...