[Icon cacher] Add metrics for downloading favicons for NTP Tiles

[Icon cacher] Add metrics for downloading favicons for NTP Tiles

This CL adds metrics that report success for downloading favicons on
demand (both for existing code for PopularSites and for the new code for
MostLikely).

BUG=536988
Review-Url: https://codereview.chromium.org/2888393002
Cr-Commit-Position: refs/heads/master@{#473941}
Committed: https://chromium.googlesource.com/chromium/src/+/00addb8394658f4a3012746d2c9389fefdfb3020

[jkrcal, 2017-05-18 13:33:00]

jkrcal@chromium.org changed reviewers:
+ holte@chromium.org, sfiera@chromium.org

[jkrcal, 2017-05-18 13:33:01]

Chris, could you PTAL?

Steven, could you PTAL at tools/metrics/*?

[jkrcal, 2017-05-18 13:33:14]

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

[commit-bot: I haz the power, 2017-05-18 13:33:35]

Dry run: CQ is trying da patch.

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

[sfiera, 2017-05-18 13:40:32]

https://codereview.chromium.org/2888393002/diff/1/components/ntp_tiles/icon_c...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2888393002/diff/1/components/ntp_tiles/icon_c...
components/ntp_tiles/icon_cacher_impl_unittest.cc:454: 
Could you also please test that cached and baked-in icons do not result in any
stats?

If it simplifies things for you, I would be OK putting the histogram checking in
the main tests. Metrics shouldn't be "separate and extra", they should be part
of the functionality :) (but separate is also fine by me)

[jkrcal, 2017-05-18 14:09:55]

Thanks! PTAL, again.

https://codereview.chromium.org/2888393002/diff/1/components/ntp_tiles/icon_c...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2888393002/diff/1/components/ntp_tiles/icon_c...
components/ntp_tiles/icon_cacher_impl_unittest.cc:454: 
On 2017/05/18 13:40:32, sfiera wrote:
> Could you also please test that cached and baked-in icons do not result in any
> stats?
> 
> If it simplifies things for you, I would be OK putting the histogram checking
in
> the main tests. Metrics shouldn't be "separate and extra", they should be part
> of the functionality :) (but separate is also fine by me)

Done.

I've merged histograms expects back into the original tests. I've put them only
into a subset of tests, where it makes sense to me.

[sfiera, 2017-05-18 14:25:02]

LGTM

Subset of tests is great.

[jkrcal, 2017-05-19 05:57:31]

jkrcal@chromium.org changed reviewers:
+ rkaplow@chromium.org, rpanchdara@google.com
- holte@chromium.org, sfiera@chromium.org

[jkrcal, 2017-05-19 05:57:31]

It seems Steven went OOO.
Robert, could you PTAL at metrics/*?

[jkrcal, 2017-05-19 07:18:05]

Description was changed from

==========
[Icon cacher] Add metrics for downloading favicons for NTP Tiles

This CL adds metrics that report success for downloading favicons on
demand (both for existing code for PopularSites and for the new code for
MostLikely).

BUG=536988
==========

to

==========
[Icon cacher] Add metrics for downloading favicons for NTP Tiles

This CL adds metrics that report success for downloading favicons on
demand (both for existing code for PopularSites and for the new code for
MostLikely).

BUG=536988
==========

[jkrcal, 2017-05-19 07:18:05]

jkrcal@chromium.org changed reviewers:
+ sfiera@chromium.org
- rpanchdara@google.com

[rkaplow, 2017-05-19 20:19:11]

https://codereview.chromium.org/2888393002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2888393002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:42:
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileFaviconFetched.popular", result,
instead of using an enum, can you use a boolean histogram?

You can also use a boolean as the enum type in the xml

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

https://codereview.chromium.org/2888393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44062: +<histogram
name="NewTabPage.TileFaviconFetched.popular"
name nit, usually we do caps in between segments, so Popular

[jkrcal, 2017-05-22 07:59:42]

Thanks, Robert! Could you PTAL, again?

https://codereview.chromium.org/2888393002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2888393002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:42:
UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileFaviconFetched.popular", result,
On 2017/05/19 20:19:11, rkaplow wrote:
> instead of using an enum, can you use a boolean histogram?
> 
> You can also use a boolean as the enum type in the xml

Done.

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

https://codereview.chromium.org/2888393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44062: +<histogram
name="NewTabPage.TileFaviconFetched.popular"
On 2017/05/19 20:19:11, rkaplow wrote:
> name nit, usually we do caps in between segments, so Popular

Done.

[jkrcal, 2017-05-22 07:59:57]

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

[commit-bot: I haz the power, 2017-05-22 08:00:12]

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, 2017-05-22 10:02:01]

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

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

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-05-23 15:21:49]

Robert, friendly ping!

[rkaplow, 2017-05-23 15:23:32]

lgtm

[jkrcal, 2017-05-23 15:26:20]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-05-23 15:26:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2888393002/#ps40001
(title: "Robert's comments")

[jkrcal, 2017-05-23 15:26:31]

Thanks!

[commit-bot: I haz the power, 2017-05-23 15:27: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-05-23 16:54:08]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495553180312940,
"parent_rev": "5634194e49b9dee7385e757ecdd4bd9fd2206065", "commit_rev":
"00addb8394658f4a3012746d2c9389fefdfb3020"}

[commit-bot: I haz the power, 2017-05-23 16:54:21]

Description was changed from

==========
[Icon cacher] Add metrics for downloading favicons for NTP Tiles

This CL adds metrics that report success for downloading favicons on
demand (both for existing code for PopularSites and for the new code for
MostLikely).

BUG=536988
==========

to

==========
[Icon cacher] Add metrics for downloading favicons for NTP Tiles

This CL adds metrics that report success for downloading favicons on
demand (both for existing code for PopularSites and for the new code for
MostLikely).

BUG=536988

Review-Url: https://codereview.chromium.org/2888393002
Cr-Commit-Position: refs/heads/master@{#473941}
Committed:
https://chromium.googlesource.com/chromium/src/+/00addb8394658f4a3012746d2c93...
==========

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

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