Move banner counting logic much later in the pipeline

Move banner counting logic much later in the pipeline

The banner can fail to be shown for any number of reasons,
from a user navigation to a failed icon fetch.  Record that
the banner could have been shown only right before the
infobar is created.

This fixes situations where a user already had a native app
installed and the website requested a banner for it, which
permanently fills a slot in app dictionary for the site --
even if the Play Store tells the AppBannerManager that the
app is installed and no banner should be shown.

BUG=453170, 452825
Committed: https://crrev.com/e7020ca28ac9bc1960f824beeeabff85db333131
Cr-Commit-Position: refs/heads/master@{#315193}

[gone, 2015-02-06 22:28:25]

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

[gone, 2015-02-06 22:28:26]

Turns out I was being bitten by a combination of two things: the maximum amount
of app banners a site can show is capped to three, but native app banners were
being recorded as being 'shown' even if the Play Store library indicated that
the app was installed (and would stop trying to show the banner).

[benwells, 2015-02-07 09:27:40]

Ah makes sense, lgtm.

I had put it earlier to avoid unnecessarily downloading icons if the banner
wasn't going to be shown. In the long term it would be good to get a hook in
from the java stuff to do check if the banner should be shown after the install
state has been checked but before the icon is downloaded.

A slight modification to this would be to leave manifest apps as they were to
avoid the icon download. That would make the two code paths inconsistent, so
I'll leave it up to you.

[benwells, 2015-02-07 09:28:21]

On 2015/02/07 09:27:40, benwells wrote:
> Ah makes sense, lgtm.
> 
> I had put it earlier to avoid unnecessarily downloading icons if the banner
> wasn't going to be shown. In the long term it would be good to get a hook in
> from the java stuff to do check if the banner should be shown after the
install
> state has been checked but before the icon is downloaded.
> 
> A slight modification to this would be to leave manifest apps as they were to
> avoid the icon download. That would make the two code paths inconsistent, so
> I'll leave it up to you.

I'm guessing you want to land this asap, so i'll check the box for you.

[benwells, 2015-02-07 09:28:25]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2015-02-07 09:29:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/905913002/1

[commit-bot: I haz the power, 2015-02-07 12:30:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-07 12:31:24]

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

[gone, 2015-02-09 19:07:01]

On 2015/02/07 09:27:40, benwells wrote:
> Ah makes sense, lgtm.
> 
> I had put it earlier to avoid unnecessarily downloading icons if the banner
> wasn't going to be shown. In the long term it would be good to get a hook in
> from the java stuff to do check if the banner should be shown after the
install
> state has been checked but before the icon is downloaded.
> 
> A slight modification to this would be to leave manifest apps as they were to
> avoid the icon download. That would make the two code paths inconsistent, so
> I'll leave it up to you.

I'm for keeping the logic consistent, but I think the problem is that we've
actually got two states wrapped into the "Could show" event:
(1) Site wants to show a banner, but no assets have been downloaded
(2) Chrome has downloaded all the assets and is ready to show the banner

We end up tying showing the banner to the site wanting to show it (via recording
that the site could show the banner immediately before checking if the site has
been visited enough), but record that the banner was shown and effectively
penalize the site if we fail to follow through.  We could account for it by
separating out the APP_BANNER_EVENT_DID_SHOW event from CheckIfShouldShow...

We'd also need to either change the Play Store service to respond if an app is
not installable, or induce a timeout where we give up on waiting for the Play
Store (which is more feasible).  What do you think?

[benwells, 2015-02-10 03:43:41]

On 2015/02/09 19:07:01, dfalcantara wrote:
> On 2015/02/07 09:27:40, benwells wrote:
> > Ah makes sense, lgtm.
> > 
> > I had put it earlier to avoid unnecessarily downloading icons if the banner
> > wasn't going to be shown. In the long term it would be good to get a hook in
> > from the java stuff to do check if the banner should be shown after the
> install
> > state has been checked but before the icon is downloaded.
> > 
> > A slight modification to this would be to leave manifest apps as they were
to
> > avoid the icon download. That would make the two code paths inconsistent, so
> > I'll leave it up to you.
> 
> I'm for keeping the logic consistent, but I think the problem is that we've
> actually got two states wrapped into the "Could show" event:
> (1) Site wants to show a banner, but no assets have been downloaded
> (2) Chrome has downloaded all the assets and is ready to show the banner
> 
> We end up tying showing the banner to the site wanting to show it (via
recording
> that the site could show the banner immediately before checking if the site
has
> been visited enough), but record that the banner was shown and effectively
> penalize the site if we fail to follow through.  We could account for it by
> separating out the APP_BANNER_EVENT_DID_SHOW event from CheckIfShouldShow...
> 
> We'd also need to either change the Play Store service to respond if an app is
> not installable, or induce a timeout where we give up on waiting for the Play
> Store (which is more feasible).  What do you think?

Seems reasonable if I understand you. You're not suggesting adding a new event,
just changing where they are recorded. Is that right?

I am not sure what it means practically for native apps. For web apps I think it
means recording COULD_SHOW where it was before, and recording DID_SHOW where it
is now or maybe later. As Ted suggested on Alex's doc we could record it once
the banner has been visible for a few seconds.

For native apps I guess it means adding a new hook into the java code at the
appropriate place (finished checking eligibility, about to start preparing to
show the banner).