Add new app banner metrics using InstallableStatusCode.

Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
Committed: https://crrev.com/df301a20d16cba20fa4cf459cf4130c1ecf4127c
Cr-Commit-Position: refs/heads/master@{#412143}

[dominickn, 2016-08-10 04:55:07]

Description was changed from

==========
Add new app banner metrics using InstallableErrorCode.

The InstallableChecker uses error codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

to

==========
Add new app banner metrics using InstallableErrorCode.

The InstallableManager reports error codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

[dominickn, 2016-08-10 05:01:53]

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

[dominickn, 2016-08-10 05:02:21]

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

[dominickn, 2016-08-10 05:02:21]

PTAL, thanks! I promise this CL is much smaller than the previous two in this
series. :)

[commit-bot: I haz the power, 2016-08-10 05:02:22]

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, 2016-08-10 06:11:26]

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

[commit-bot: I haz the power, 2016-08-10 06:11:27]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gone, 2016-08-10 21:02:51]

What'd you decide on the last CL about combining all of these entries into a
monolithic InstallableErrorCode?

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_manager.cc (right):

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_manager.cc:390: // there is no need to recheck
it here to avoid skew.
Seems like something that should be DCHECKED, as well.

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_manager_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_manager_browsertest.cc:136:
CheckInstallableErrorCodeHistogram(INSUFFICIENT_ENGAGEMENT, i+1, i+1);
Should be consistent about the i+1 vs i + 1 below.

[benwells, 2016-08-11 04:47:35]

This feels pretty manual / error prone - every exit condition needs to be found
and log something. I can't think of a simple and nice way to do it better, but
is there a way we can notice if one exit condition is missed, or if something
ends up logging two events for the one check?

For example we could have a boolean NeedToLogError, which is set whenever a new
pipeline is kicked off. Then, if a new pipeline is kicked off and NeedToLogError
is true, we know something is wrong and we can DCHECK. Or if we ever go to log
an error and NeedToLogError is false, we can also DCHECK.

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
File chrome/browser/banners/app_banner_manager_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/b...
chrome/browser/banners/app_banner_manager_browsertest.cc:34:
~AppBannerManagerTest() override { }
Nit: leave as {}

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/i...
File chrome/browser/installable/installable_logging.h (right):

https://chromiumcodereview.appspot.com/2178833002/diff/60001/chrome/browser/i...
chrome/browser/installable/installable_logging.h:19: enum InstallableErrorCode {
Nit: Maybe this type should be called something else, as not all of these are
errors.

[dominickn, 2016-08-11 05:11:33]

Description was changed from

==========
Add new app banner metrics using InstallableErrorCode.

The InstallableManager reports error codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

to

==========
Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

[dominickn, 2016-08-11 06:44:59]

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

[commit-bot: I haz the power, 2016-08-11 06:45:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-11 07:00:19]

Changed InstallableErrorCode to InstallableStatusCode, and added a boolean
need_to_log_error_ with DCHECKs. I had to change
AppBannerSettingsHelper::ShouldShowBanner to support this. Thoughts?

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

https://codereview.chromium.org/2178833002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:390: // there is no need to recheck
it here to avoid skew.
On 2016/08/10 21:02:51, dfalcantara wrote:
> Seems like something that should be DCHECKED, as well.

Done.

https://codereview.chromium.org/2178833002/diff/60001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_manager_browsertest.cc (right):

https://codereview.chromium.org/2178833002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:34:
~AppBannerManagerTest() override { }
On 2016/08/11 04:47:34, benwells wrote:
> Nit: leave as {}

Done.

https://codereview.chromium.org/2178833002/diff/60001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager_browsertest.cc:136:
CheckInstallableErrorCodeHistogram(INSUFFICIENT_ENGAGEMENT, i+1, i+1);
On 2016/08/10 21:02:51, dfalcantara wrote:
> Should be consistent about the i+1 vs i + 1 below.

Done.

[benwells, 2016-08-11 07:14:23]

overall i like it, just some questions.

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

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:310: need_to_log_status_ = false;
I don't understand how this works - won't the DCHECK in ReportStatus fire when
ShowBannder later calls into it?

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:319: if (!IsDebugMode() &&
!CheckIfShouldShowBanner()) {
Why is this changing?

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:394: if
(AppBannerSettingsHelper::HasSufficientEngagement(score)) {
Why is this changing?

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:429: DCHECK(!IsDebugMode());
Could you have a comment explaining why this DCHECK is here? It isn't
immediately obvious.

[commit-bot: I haz the power, 2016-08-11 07:18:31]

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

[commit-bot: I haz the power, 2016-08-11 07:18:32]

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

[dominickn, 2016-08-11 08:12:54]

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

[commit-bot: I haz the power, 2016-08-11 08:13:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-11 08:13:38]

Thanks!

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

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:310: need_to_log_status_ = false;
On 2016/08/11 07:14:23, benwells wrote:
> I don't understand how this works - won't the DCHECK in ReportStatus fire when
> ShowBannder later calls into it?

need_to_log_status_ is set to true in RequestAppBanner, which is the start of
the app banner pipeline. Otherwise, it should not be set to true until that
method is called.

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:319: if (!IsDebugMode() &&
!CheckIfShouldShowBanner()) {
On 2016/08/11 07:14:23, benwells wrote:
> Why is this changing?

The only way is_active_ will be false here is if Stop() is called, since this
method is called synchronously from a location which already checks is_active_
is true.

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:394: if
(AppBannerSettingsHelper::HasSufficientEngagement(score)) {
On 2016/08/11 07:14:23, benwells wrote:
> Why is this changing?

Ah, I had a ReportStatus branch here that was removed in the last patchset since
this is before a banner is actually requested. Putting it back the way it was.

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:429: DCHECK(!IsDebugMode());
On 2016/08/11 07:14:23, benwells wrote:
> Could you have a comment explaining why this DCHECK is here? It isn't
> immediately obvious.

Done.

[commit-bot: I haz the power, 2016-08-11 09:08:29]

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

[commit-bot: I haz the power, 2016-08-11 09:08:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[benwells, 2016-08-12 01:34:23]

lgtm with the race addressed

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

https://codereview.chromium.org/2178833002/diff/80001/chrome/browser/banners/...
chrome/browser/banners/app_banner_manager.cc:310: need_to_log_status_ = false;
On 2016/08/11 08:13:38, dominickn wrote:
> On 2016/08/11 07:14:23, benwells wrote:
> > I don't understand how this works - won't the DCHECK in ReportStatus fire
when
> > ShowBannder later calls into it?
> 
> need_to_log_status_ is set to true in RequestAppBanner, which is the start of
> the app banner pipeline. Otherwise, it should not be set to true until that
> method is called.

OK, as discussed offline there might be some subtle race still if the callbacks
are fired after Stop is called.

[dominickn, 2016-08-12 04:35:59]

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

[commit-bot: I haz the power, 2016-08-12 04:36:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2016-08-12 04:37:37]

Thanks! I've clarified the comments, and added a call to invalidate weak
pointers in Stop() to catch the race condition.

+isherman: PTAL at histograms

dfalcantara: when you have a moment, PTAL at the Android banner code (all very
minor changes)

[commit-bot: I haz the power, 2016-08-12 05:19:10]

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

[commit-bot: I haz the power, 2016-08-12 05:19:11]

Dry run: This issue passed the CQ dry run.

[dominickn, 2016-08-12 05:44:21]

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

[dominickn, 2016-08-12 05:44:21]

Actually +isherman this time. PTAL at histograms, thanks!

[gone, 2016-08-15 20:39:14]

lgtm

[Ilya Sherman, 2016-08-15 22:02:20]

Metrics LGTM % a nit:

https://codereview.chromium.org/2178833002/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_metrics.cc (right):

https://codereview.chromium.org/2178833002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_metrics.cc:21:
"AppBanners.InstallableStatusCode";
nit: This doesn't match the name in histograms.xml -- please update one or the
other.

[dominickn, 2016-08-16 00:30:30]

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

[dominickn, 2016-08-16 00:30:43]

Thanks!

https://codereview.chromium.org/2178833002/diff/120001/chrome/browser/banners...
File chrome/browser/banners/app_banner_metrics.cc (right):

https://codereview.chromium.org/2178833002/diff/120001/chrome/browser/banners...
chrome/browser/banners/app_banner_metrics.cc:21:
"AppBanners.InstallableStatusCode";
On 2016/08/15 22:02:20, Ilya Sherman wrote:
> nit: This doesn't match the name in histograms.xml -- please update one or the
> other.

Whoops, done.

[commit-bot: I haz the power, 2016-08-16 00:31:03]

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, 2016-08-16 01:30:30]

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

[commit-bot: I haz the power, 2016-08-16 01:30:32]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dominickn, 2016-08-16 01:31:20]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-08-16 01:31:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, isherman@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2178833002/#ps140001
(title: "Fix histogram name")

[commit-bot: I haz the power, 2016-08-16 01:32:12]

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-08-16 02:32:44]

Description was changed from

==========
Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

to

==========
Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

[commit-bot: I haz the power, 2016-08-16 02:32:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-16 02:34:45]

Description was changed from

==========
Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921
==========

to

==========
Add new app banner metrics using InstallableStatusCode.

The InstallableManager reports status codes to signal if the checking
pipeline failed in some way. This CL introduces a new UMA metric to
report the value of this code, and introduces new codes to ensure that
every run through the banner pipeline results in precisely one code.
This new metric is intended to be the final source of truth for banners.

Additional testing is added to ensure the correct histogram values are
recorded. The existing metrics will be removed in a future CL.

BUG=628921

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

[commit-bot: I haz the power, 2016-08-16 02:34:46]

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