Start banner pipeline immediately after navigating, if enough engagement

Start banner pipeline immediately after navigating, if enough engagement

Previously if a site had already earned enough engagement and was
navigated to, the banner pipeline would not be triggered until further
engagement was earned. With this change it will trigger immediately.

This could result in slight changes of behavior if sites are holding on
to the beforeinstallprompt event and using it to enable certain parts
of the UI (for example a button).

This change also tweaks some conditions in tests for changes in the
underlying state machine.

BUG=729919
Review-Url: https://codereview.chromium.org/2957523003
Cr-Commit-Position: refs/heads/master@{#482115}
Committed: https://chromium.googlesource.com/chromium/src/+/8d9f82995bc634440aed9ec773861dc376b08e70

[benwells, 2017-06-23 04:39:27]

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

[commit-bot: I haz the power, 2017-06-23 04:39:38]

Dry run: CQ is trying da patch.

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

[benwells, 2017-06-23 04:46:13]

Description was changed from

==========
Start banner pipeline immediately after navigating, if enough engagement

Previously if a site had already earned enough engagement and was
navigated to, the banner pipeline would not be triggered until further
engagement was earned. With this change it will trigger immediately.

This could result in slight changes of behavior if sites are holding on
to the beforeinstallprompt event and using it to enable certain parts
of the UI (for example a button).

BUG=729919
==========

to

==========
Start banner pipeline immediately after navigating, if enough engagement

Previously if a site had already earned enough engagement and was
navigated to, the banner pipeline would not be triggered until further
engagement was earned. With this change it will trigger immediately.

This could result in slight changes of behavior if sites are holding on
to the beforeinstallprompt event and using it to enable certain parts
of the UI (for example a button).

This change also tweaks some conditions in tests for changes in the
underlying state machine.

BUG=729919
==========

[benwells, 2017-06-23 04:46:56]

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

[commit-bot: I haz the power, 2017-06-23 04:54:04]

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

[commit-bot: I haz the power, 2017-06-23 04:54:05]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dominickn, 2017-06-23 04:55:36]

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

https://codereview.chromium.org/2957523003/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:437: SiteEngagementService::Get(
This is called quite a lot. Instead, just have a site_engagement_service_ member
(we already fetch it in the constructor for this object to set up the observer
anyway).

[dominickn, 2017-06-23 04:57:45]

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

https://codereview.chromium.org/2957523003/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:437: SiteEngagementService::Get(
On 2017/06/23 04:55:35, dominickn wrote:
> This is called quite a lot. Instead, just have a site_engagement_service_
member
> (we already fetch it in the constructor for this object to set up the observer
> anyway).

That might also let you combine this if statement with the one above.

[benwells, 2017-06-23 05:12:04]

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

https://codereview.chromium.org/2957523003/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:437: SiteEngagementService::Get(
On 2017/06/23 04:57:44, dominickn wrote:
> On 2017/06/23 04:55:35, dominickn wrote:
> > This is called quite a lot. Instead, just have a site_engagement_service_
> member
> > (we already fetch it in the constructor for this object to set up the
observer
> > anyway).
> 
> That might also let you combine this if statement with the one above.

Done.

https://codereview.chromium.org/2957523003/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:437: SiteEngagementService::Get(
On 2017/06/23 04:55:35, dominickn wrote:
> This is called quite a lot. Instead, just have a site_engagement_service_
member
> (we already fetch it in the constructor for this object to set up the observer
> anyway).

Done.

[dominickn, 2017-06-23 05:16:01]

lgtm, thanks!

[benwells, 2017-06-23 05:17:10]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2017-06-23 05:17:19]

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-06-23 05:30:35]

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

[commit-bot: I haz the power, 2017-06-23 05:30:36]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[benwells, 2017-06-23 05:54:13]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2017-06-23 05:54: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-06-23 06:09:00]

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

[commit-bot: I haz the power, 2017-06-23 06:09:01]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[benwells, 2017-06-23 22:39:41]

The CQ bit was checked by benwells@chromium.org

[benwells, 2017-06-23 22:39:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2957523003/#ps40001
(title: "Simpler per Dom's suggestion")

[commit-bot: I haz the power, 2017-06-23 22:40:07]

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-06-23 23:57:16]

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

[commit-bot: I haz the power, 2017-06-23 23:57:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benwells, 2017-06-24 01:22:52]

The CQ bit was checked by benwells@chromium.org

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

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-06-24 01:59:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498267372481960,
"parent_rev": "c0107a9fa095726283fe62a50133ec27a9acf7fd", "commit_rev":
"8d9f82995bc634440aed9ec773861dc376b08e70"}

[commit-bot: I haz the power, 2017-06-24 01:59:50]

Description was changed from

==========
Start banner pipeline immediately after navigating, if enough engagement

Previously if a site had already earned enough engagement and was
navigated to, the banner pipeline would not be triggered until further
engagement was earned. With this change it will trigger immediately.

This could result in slight changes of behavior if sites are holding on
to the beforeinstallprompt event and using it to enable certain parts
of the UI (for example a button).

This change also tweaks some conditions in tests for changes in the
underlying state machine.

BUG=729919
==========

to

==========
Start banner pipeline immediately after navigating, if enough engagement

Previously if a site had already earned enough engagement and was
navigated to, the banner pipeline would not be triggered until further
engagement was earned. With this change it will trigger immediately.

This could result in slight changes of behavior if sites are holding on
to the beforeinstallprompt event and using it to enable certain parts
of the UI (for example a button).

This change also tweaks some conditions in tests for changes in the
underlying state machine.

BUG=729919

Review-Url: https://codereview.chromium.org/2957523003
Cr-Commit-Position: refs/heads/master@{#482115}
Committed:
https://chromium.googlesource.com/chromium/src/+/8d9f82995bc634440aed9ec77386...
==========

[commit-bot: I haz the power, 2017-06-24 01:59:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8d9f82995bc634440aed9ec77386...