Allow app banners to be triggered by increases in site engagement.

Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org

Committed: https://crrev.com/ecc791758fba66a0906b9450780a23e898a44b63
Cr-Commit-Position: refs/heads/master@{#402726}

[dominickn, 2016-06-01 03:47:29]

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

[dominickn, 2016-06-01 03:47:30]

PTAL - this is an initial upload. Tests are still to be added, but your feedback
is welcome.

[benwells, 2016-06-02 04:44:36]

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

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:121: is_visible_ = false;
What's this for? Is this something you found in testing?

In particular I'm wondering whether this is something that should be checked at
the end of the pipeline for testing validity.

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:136: url == validated_url_ &&
is_visible_) {
I'm slightly worried there will be cases where we've navigated to a different
page on the same PWA when we get this call. This may not be a problem (e.g. it
might not be racy), and at the moment it will probably be fine as we'll get more
engagement calls later down the track with the right URL. But if we do any fancy
stuff to limit how often we get here (see next comment) it might become a
problem.

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:140: RequestAppBanner(url, false /*
is_debug_mode */);
IIUC this will be called a lot more now. Have you considered the impact of that?

[dominickn, 2016-06-02 04:54:38]

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

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:121: is_visible_ = false;
On 2016/06/02 04:44:36, benwells wrote:
> What's this for? Is this something you found in testing?
> 
> In particular I'm wondering whether this is something that should be checked
at
> the end of the pipeline for testing validity.

If a user has multiple tabs open (multiple WebContents), there's an
AppBannerManager on each one, and each one is listening to the
SiteEngagementService. Previously, since we triggered on main frame navigation,
only one app banner flow would be instigated at any time. With the change to the
callback interface, now it's possible that there are two tabs open to the same
URL. We only want the banner flow to start on the tab that's in the foreground.

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:136: url == validated_url_ &&
is_visible_) {
On 2016/06/02 04:44:36, benwells wrote:
> I'm slightly worried there will be cases where we've navigated to a different
> page on the same PWA when we get this call. This may not be a problem (e.g. it
> might not be racy), and at the moment it will probably be fine as we'll get
more
> engagement calls later down the track with the right URL. But if we do any
fancy
> stuff to limit how often we get here (see next comment) it might become a
> problem.

Yes, this is definitely a concern. We can certainly implement a check that |url|
is a subpath of the PWA's path + start_url, but that will require fetching and
parsing the manifest at the very least. A simpler check might be that |url| is a
subpath of |validated_url| that we got from DidFinishLoad. WDYT?

https://codereview.chromium.org/2024953005/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_manager.cc:140: RequestAppBanner(url, false /*
is_debug_mode */);
On 2016/06/02 04:44:36, benwells wrote:
> IIUC this will be called a lot more now. Have you considered the impact of
that?

I did think about this, and I believe this shouldn't be called that much more
often. User input is the noisiest source of engagement increases, but that's
gated to 10 seconds after navigation, and every 10 seconds after that. Media
playing is the main wildcard, but again that's on a periodic timer.

Once we remove the navigation based backend, I'm planning on moving the
have-we-shown-the-banner-before content settings check to here, so that the rest
of the flow doesn't start if we've already shown the banner upfront.

[dominickn, 2016-06-23 08:53:03]

PTAL, thanks! You should ignore the diff against the initial upload as a lot of
this code has changed.

Automatically testing the new triggering mechanism is very fiddly, since it will
require exposing the site engagement service through Java so the Android code
can control it properly. Do you mind if I leave that for a follow up CL?

[benwells, 2016-06-28 00:57:50]

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (left):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:66: void
AppBannerManager::DidNavigateMainFrame(
Why did you change this from DidNavigateMainFrame to DidFinishNavigation?

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:99: load_finished_ = false;
Seems strange that this is set false only if we're using the site engagement
service, but set true regardless later on.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:126: banner_requested_) {
This logic reads weirdly. If banner_requested is true, set banner_requested to
false and request the banner? Maybe this variable should be called something
else?

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:170: // Stop observing so we don't
double-trigger the banner.
What will happen if you have two visible web contents showing the same origin,
and you are flipping between them? Could you trigger on both, or would the
content setting prevent that?

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:98: bool is_hidden_;
I recall we chatted about this, and you were going to test if triggering a
banner while a web contents is hidden has any problems. Did you do the test?

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:103: bool banner_requested_;
Instead of these two bools, did you consider using an enum to represent the
valid states? E.g. might be 'LOADING, LOAD_FINISHED, ELIGIBLE_FOR_BANNER,
BANNER_REQUESTED'.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:75: const char
kBannerSiteEngagementParamsKey[] = "use_site_engagement";
So the plan is to drop all the previous experiments, and have a new experiment?

Sorry if I've already asked about that :)

[dominickn, 2016-06-28 03:52:29]

Thanks! Looks like the build is broken on the ChromeOS bots.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (left):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:66: void
AppBannerManager::DidNavigateMainFrame(
On 2016/06/28 00:57:50, benwells wrote:
> Why did you change this from DidNavigateMainFrame to DidFinishNavigation?

DidNavigateMainFrame is deprecated and is being replaced by DidFinishNavigation
in the PlzNavigate project. The content folks have advised migrating these
methods.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:99: load_finished_ = false;
On 2016/06/28 00:57:50, benwells wrote:
> Seems strange that this is set false only if we're using the site engagement
> service, but set true regardless later on.

The non site engagement triggered banners don't use this variable for
consistency with existing behaviour. I'll move the setting of the variable
inside the equivalent check.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:126: banner_requested_) {
On 2016/06/28 00:57:50, benwells wrote:
> This logic reads weirdly. If banner_requested is true, set banner_requested to
> false and request the banner? Maybe this variable should be called something
> else?

Changed to banner eligible.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:170: // Stop observing so we don't
double-trigger the banner.
On 2016/06/28 00:57:50, benwells wrote:
> What will happen if you have two visible web contents showing the same origin,
> and you are flipping between them? Could you trigger on both, or would the
> content setting prevent that?

The content setting should prevent that, since a banner being shown immediately
sets the APP_BANNER_EVENT_DID_SHOW. There will be a potential race if the two
banners are triggered before one of them can set the DID_SHOW event, but in
practice the banner flow happens so quickly that I'm not even sure it would be
possible to reliably reproduce this.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:98: bool is_hidden_;
On 2016/06/28 00:57:50, benwells wrote:
> I recall we chatted about this, and you were going to test if triggering a
> banner while a web contents is hidden has any problems. Did you do the test?

Thanks for reminding me about this. The right thing happens and the banner
triggers when you switch back, so I'm removing this variable.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:103: bool banner_requested_;
On 2016/06/28 00:57:50, benwells wrote:
> Instead of these two bools, did you consider using an enum to represent the
> valid states? E.g. might be 'LOADING, LOAD_FINISHED, ELIGIBLE_FOR_BANNER,
> BANNER_REQUESTED'.

We need to know BANNER_REQUESTED independently of LOAD_FINISHED; we'd end up
having to have two enums stored instead of two bools. If you feel that it's more
readable, I don't mind changing it.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:75: const char
kBannerSiteEngagementParamsKey[] = "use_site_engagement";
On 2016/06/28 00:57:50, benwells wrote:
> So the plan is to drop all the previous experiments, and have a new
experiment?
> 
> Sorry if I've already asked about that :)

Yes - the plan is to basically replace the existing AppBannerTriggering
experiment with some new groups which set use_site_enagement to true.

[benwells, 2016-06-29 03:38:24]

lgtm with an optional nit

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:126: banner_requested_) {
On 2016/06/28 03:52:29, dominickn wrote:
> On 2016/06/28 00:57:50, benwells wrote:
> > This logic reads weirdly. If banner_requested is true, set banner_requested
to
> > false and request the banner? Maybe this variable should be called something
> > else?
> 
> Changed to banner eligible.

Hmmm thanks.

Nit: maybe banner_request_queued_ or something would be better?

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:170: // Stop observing so we don't
double-trigger the banner.
On 2016/06/28 03:52:29, dominickn wrote:
> On 2016/06/28 00:57:50, benwells wrote:
> > What will happen if you have two visible web contents showing the same
origin,
> > and you are flipping between them? Could you trigger on both, or would the
> > content setting prevent that?
> 
> The content setting should prevent that, since a banner being shown
immediately
> sets the APP_BANNER_EVENT_DID_SHOW. There will be a potential race if the two
> banners are triggered before one of them can set the DID_SHOW event, but in
> practice the banner flow happens so quickly that I'm not even sure it would be
> possible to reliably reproduce this.

Acknowledged.

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.h (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.h:103: bool banner_requested_;
On 2016/06/28 03:52:29, dominickn wrote:
> On 2016/06/28 00:57:50, benwells wrote:
> > Instead of these two bools, did you consider using an enum to represent the
> > valid states? E.g. might be 'LOADING, LOAD_FINISHED, ELIGIBLE_FOR_BANNER,
> > BANNER_REQUESTED'.
> 
> We need to know BANNER_REQUESTED independently of LOAD_FINISHED; we'd end up
> having to have two enums stored instead of two bools. If you feel that it's
more
> readable, I don't mind changing it.

I was finding the logic hard to follow and thought that might make it easier,
but two enums sounds worse :D

[dominickn, 2016-06-29 04:17:41]

Description was changed from

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
==========

to

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org
==========

[dominickn, 2016-06-29 04:18:58]

Thanks! TBR'ing sky@ for the call site change in browser.cc

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager.cc (right):

https://codereview.chromium.org/2024953005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:126: banner_requested_) {
On 2016/06/29 03:38:24, benwells wrote:
> On 2016/06/28 03:52:29, dominickn wrote:
> > On 2016/06/28 00:57:50, benwells wrote:
> > > This logic reads weirdly. If banner_requested is true, set
banner_requested
> to
> > > false and request the banner? Maybe this variable should be called
something
> > > else?
> > 
> > Changed to banner eligible.
> 
> Hmmm thanks.
> 
> Nit: maybe banner_request_queued_ or something would be better?

Done.

[dominickn, 2016-06-29 04:19:05]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-06-29 04:19:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/2024953005/#ps80001
(title: "Address reviewer comments. Rebase")

[commit-bot: I haz the power, 2016-06-29 04:19: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, 2016-06-29 05:04:51]

Description was changed from

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org
==========

to

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-06-29 05:05:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-29 05:06:35]

Description was changed from

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org
==========

to

==========
Allow app banners to be triggered by increases in site engagement.

This CL makes the app banner manager a site engagement observer. This
allows banners to be triggered when the engagement for a site reaches
a sufficient level, rather than being restricted to triggering on
navigations.

The experiment switch for enabling site engagement banners is moved back
into the AppBannerTriggering variation from the site engagement
variation, as site engagement has now launched to the stable channel.

A future CL will remove the existing navigation-based heuristic
entirely.

BUG=616322
TBR=sky@chromium.org

Committed: https://crrev.com/ecc791758fba66a0906b9450780a23e898a44b63
Cr-Commit-Position: refs/heads/master@{#402726}
==========

[commit-bot: I haz the power, 2016-06-29 05:06:36]

Patchset 5 (id:??) landed as
https://crrev.com/ecc791758fba66a0906b9450780a23e898a44b63
Cr-Commit-Position: refs/heads/master@{#402726}