Add metrics for app banner preventDefault() and prompt().

Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
Committed: https://crrev.com/4f6382c45e105322038b6fa1885cd1f479ef5ff8
Cr-Commit-Position: refs/heads/master@{#404039}

[dominickn, 2016-07-05 02:49:19]

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

[dominickn, 2016-07-05 02:49:20]

PTAL, thanks!

[benwells, 2016-07-05 02:56:20]

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

https://codereview.chromium.org/2122783002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_data_fetcher.cc:221: page_requested_prompt_ =
true;
Should we also track something here?

https://codereview.chromium.org/2122783002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2122783002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65508: <int value="10" label="Web app
banner was requested by the site"/>
What does this mean? I find the labels for this metric really confusing...

[dominickn, 2016-07-05 03:15:33]

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

https://codereview.chromium.org/2122783002/diff/1/chrome/browser/banners/app_...
chrome/browser/banners/app_banner_data_fetcher.cc:221: page_requested_prompt_ =
true;
On 2016/07/05 02:56:20, benwells wrote:
> Should we also track something here?

Good catch. The BeforeInstallPromptEvent should ensure that this method is only
called when valid, so it's fine to track the metric outside the conditional.

https://codereview.chromium.org/2122783002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2122783002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65508: <int value="10" label="Web app
banner was requested by the site"/>
On 2016/07/05 02:56:20, benwells wrote:
> What does this mean? I find the labels for this metric really confusing...

The "requested" state occurs after the manifest was fetched and validated, but
before the service worker is checked or the icon fetched. Why? I'm not sure.
Perhaps at the time having a manifest with the requisite banner-eligible bits
was the site's way of saying "please show a banner for me". :)

[dominickn, 2016-07-05 04:57:57]

Revised as per in-person discussion. PTAL, thanks!

[benwells, 2016-07-05 05:34:49]

lgtm.

Just to be clear - the number of times the banner is shown and the page doesn't
prevent default and doesn't call prompt, would be _COMPLETE - _CALLED_PROMPT -
is that right?

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

https://codereview.chromium.org/2122783002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:119:
TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED);
Nit: This name is a bit misleading, I think it means
PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT, is that right?

[benwells, 2016-07-05 05:36:18]

Hang on, there is a NO_ACTION value which doesn't appear to be used - is that a
mistake?

[dominickn, 2016-07-05 06:00:09]

Dang, today has been a poor day. :(

Latest patchset actually properly uses the NO_ACTION value, with a little
rewiring to accurately handle it. Also made the naming more explicit/consistent.

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

https://codereview.chromium.org/2122783002/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_data_fetcher.cc:119:
TrackBeforeInstallEvent(BEFORE_INSTALL_EVENT_PROMPT_NOT_CALLED);
On 2016/07/05 05:34:49, benwells wrote:
> Nit: This name is a bit misleading, I think it means
> PROMPT_NOT_CALLED_AFTER_PREVENT_DEFAULT, is that right?

Done.

[dominickn, 2016-07-05 06:04:27]

Patchset #4 (id:60001) has been deleted

[benwells, 2016-07-05 06:47:43]

lgtm :)

[Ilya Sherman, 2016-07-06 23:15:54]

LGTM % nits

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_metrics.h (right):

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_metrics.h:60: enum BeforeInstallEvent {
nit: Please document that this enum is used to back an UMA histogram, and should
therefore be treated as append-only.

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_metrics.h:61: BEFORE_INSTALL_EVENT_MIN = 0,
Hmm, why are you reserving this bucket?  It doesn't appear to be used.

https://codereview.chromium.org/2122783002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2122783002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65564: +             after
preventDefault()"/>
Optional nit: These labels are pretty long, which will likely make them hard to
read in the dashboard UI.  I'd recommend shortening them by omitting the shared
"BeforeInstallPromptEvent" prefix, and also omitting some of the explanatory
text, e.g. just "prompt() not called" (or maybe a bit of additional text to
clarify that this is only recorded when preventDefault() is called, if that's
true).  You can still include the explanatory text as on-hover text if you'd
like:

<int value="6" label="prompt() not
called">BeforeInstallPromptEvent.preventDefault() was called, and
BeforeInstallPromptEvent.prompt() was not subsequently called, so no banner was
shown.</int>

[dominickn, 2016-07-07 00:15:41]

Description was changed from

==========
Add metrics for app banner preventDefault() and prompt()

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
==========

to

==========
Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
==========

[dominickn, 2016-07-07 00:16:02]

Thanks!

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_metrics.h (right):

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_metrics.h:60: enum BeforeInstallEvent {
On 2016/07/06 23:15:53, Ilya Sherman wrote:
> nit: Please document that this enum is used to back an UMA histogram, and
should
> therefore be treated as append-only.

Done.

https://codereview.chromium.org/2122783002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_metrics.h:61: BEFORE_INSTALL_EVENT_MIN = 0,
On 2016/07/06 23:15:53, Ilya Sherman wrote:
> Hmm, why are you reserving this bucket?  It doesn't appear to be used.

It's mainly for consistency with the remainder of the histogram enums in this
file. :)

https://codereview.chromium.org/2122783002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2122783002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65564: +             after
preventDefault()"/>
On 2016/07/06 23:15:53, Ilya Sherman wrote:
> Optional nit: These labels are pretty long, which will likely make them hard
to
> read in the dashboard UI.  I'd recommend shortening them by omitting the
shared
> "BeforeInstallPromptEvent" prefix, and also omitting some of the explanatory
> text, e.g. just "prompt() not called" (or maybe a bit of additional text to
> clarify that this is only recorded when preventDefault() is called, if that's
> true).  You can still include the explanatory text as on-hover text if you'd
> like:
> 
> <int value="6" label="prompt() not
> called">BeforeInstallPromptEvent.preventDefault() was called, and
> BeforeInstallPromptEvent.prompt() was not subsequently called, so no banner
was
> shown.</int>

Done.

[dominickn, 2016-07-07 00:16:09]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-07-07 00:16:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2122783002/#ps100001
(title: "Address nits")

[commit-bot: I haz the power, 2016-07-07 00:16:53]

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-07-07 02:41:54]

Description was changed from

==========
Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
==========

to

==========
Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
==========

[commit-bot: I haz the power, 2016-07-07 02:41:55]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-07-07 02:42:16]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 02:43:56]

Description was changed from

==========
Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716
==========

to

==========
Add metrics for app banner preventDefault() and prompt().

The beforeinstallprompt event signals an imminent app install banner for
a page. This CL adds UMA stats for preventDefault() being called on the
event (stopping the banner from appearing), and for prompt() being
called on the event (making the banner appear subsequent to
a preventDefault()).

BUG=625716

Committed: https://crrev.com/4f6382c45e105322038b6fa1885cd1f479ef5ff8
Cr-Commit-Position: refs/heads/master@{#404039}
==========

[commit-bot: I haz the power, 2016-07-07 02:43:57]

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