Fix the ability to bypass app banner engagement checks.

Fix the ability to bypass app banner engagement checks.

Following https://crrev.com/2778983005, the
chrome://flags/#bypass-app-banner-engagement-checks flag now does not
work unless there is an engagement increase to above the engagement
threshold. This is due to an oversight where the case of the flag being
turned on could trigger the banner pipeline, but was treated like the on
load installability experiment immediately prior to sending the
beforeinstallprompt event.

This CL fixes the bug by explicitly setting |has_sufficient_engagement_|
to true if the bypass flag is enabled (or if 0 engagement is required
for the banner to show).

BUG=714484
Review-Url: https://codereview.chromium.org/2833313002
Cr-Commit-Position: refs/heads/master@{#466590}
Committed: https://chromium.googlesource.com/chromium/src/+/64c866e4041adcf57e6e86438482b2c030151f0d

[dominickn, 2017-04-24 03:11:33]

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

[commit-bot: I haz the power, 2017-04-24 03:11:43]

Dry run: CQ is trying da patch.

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

[dominickn, 2017-04-24 03:11:56]

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

[dominickn, 2017-04-24 03:11:56]

PTAL - this will need a merge to M59.

[Matt Giuca, 2017-04-24 03:34:29]

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

[Matt Giuca, 2017-04-24 03:34:30]

drive-by lgtm with comment nits.

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

https://codereview.chromium.org/2833313002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:396: // banner, we can manually set
|has_sufficient_engagement_| to true.
Drop "we can".

Also "set |has_sufficient_engagement_| to true" is just describing the code.
Rewrite this for what it actually means to set this to true and why we do this
(e.g., "the rest of the install flow should operate as if the engagement
threshold has been met").

https://codereview.chromium.org/2833313002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:401: //  1. we have sufficient
engagement, or
Change the 1., 2., 3. since it no longer corresponds to the if statement:

1. we have sufficient engagement (or 0 engagement is required; see the above
check), or
2. the feature ... is enabled.

(Perhaps the numerals can be dropped also.)

[dominickn, 2017-04-24 03:42:30]

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

[dominickn, 2017-04-24 03:42:34]

Thanks!

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

https://codereview.chromium.org/2833313002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:396: // banner, we can manually set
|has_sufficient_engagement_| to true.
On 2017/04/24 03:34:30, Matt Giuca wrote:
> Drop "we can".
> 
> Also "set |has_sufficient_engagement_| to true" is just describing the code.
> Rewrite this for what it actually means to set this to true and why we do this
> (e.g., "the rest of the install flow should operate as if the engagement
> threshold has been met").

Done.

https://codereview.chromium.org/2833313002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:401: //  1. we have sufficient
engagement, or
On 2017/04/24 03:34:30, Matt Giuca wrote:
> Change the 1., 2., 3. since it no longer corresponds to the if statement:
> 
> 1. we have sufficient engagement (or 0 engagement is required; see the above
> check), or
> 2. the feature ... is enabled.
> 
> (Perhaps the numerals can be dropped also.)

Done.

[commit-bot: I haz the power, 2017-04-24 03:42:48]

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-04-24 04:19:50]

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

[commit-bot: I haz the power, 2017-04-24 04:19:51]

Dry run: This issue passed the CQ dry run.

[benwells, 2017-04-24 06:00:02]

lgtm

[dominickn, 2017-04-24 06:01:58]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-04-24 06:01:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org
Link to the patchset: https://codereview.chromium.org/2833313002/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-04-24 06:02:05]

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-04-24 06:05:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493013718370860,
"parent_rev": "17a207e062459d1d720824e03c96128901f7f531", "commit_rev":
"64c866e4041adcf57e6e86438482b2c030151f0d"}

[commit-bot: I haz the power, 2017-04-24 06:06:28]

Description was changed from

==========
Fix the ability to bypass app banner engagement checks.

Following https://crrev.com/2778983005, the
chrome://flags/#bypass-app-banner-engagement-checks flag now does not
work unless there is an engagement increase to above the engagement
threshold. This is due to an oversight where the case of the flag being
turned on could trigger the banner pipeline, but was treated like the on
load installability experiment immediately prior to sending the
beforeinstallprompt event.

This CL fixes the bug by explicitly setting |has_sufficient_engagement_|
to true if the bypass flag is enabled (or if 0 engagement is required
for the banner to show).

BUG=714484
==========

to

==========
Fix the ability to bypass app banner engagement checks.

Following https://crrev.com/2778983005, the
chrome://flags/#bypass-app-banner-engagement-checks flag now does not
work unless there is an engagement increase to above the engagement
threshold. This is due to an oversight where the case of the flag being
turned on could trigger the banner pipeline, but was treated like the on
load installability experiment immediately prior to sending the
beforeinstallprompt event.

This CL fixes the bug by explicitly setting |has_sufficient_engagement_|
to true if the bypass flag is enabled (or if 0 engagement is required
for the banner to show).

BUG=714484

Review-Url: https://codereview.chromium.org/2833313002
Cr-Commit-Position: refs/heads/master@{#466590}
Committed:
https://chromium.googlesource.com/chromium/src/+/64c866e4041adcf57e6e86438482...
==========

[commit-bot: I haz the power, 2017-04-24 06:06:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/64c866e4041adcf57e6e86438482...