Add UMA stats for which icon types are shown and clicked on the NTP.

Add UMA stats for which icon types are shown and clicked on the NTP.

This adds two new histograms (with suffixes):

  NewTabPage.TileType.<source> - Records the type of each most visited
      tile that's shown on the NTP. Valid values are IconReal, IconColor,
      IconDefault, ThumbnailLocal, ThumbnailServer, ThumbnailDefault.

  NewTabPage.TileTypeClicked.<source> - Records the type of each most
      visited tile that the user clicks on. Same values as above.

In both cases <source> can be one of "client", "server8", "popular", or
another value indicating the source of the most visited suggestions.

This CL also refactors a fair amount to support this change:
 - There's a new C++/Java shared enum called MostVisitedTileType, with
   the same values as the new histograms.
 - Each MostVisitedItem in Java stores its tile type.
 - NewTabPage.IconsGray and related histograms are now recorded in
   most_visited_sites.cc alongside NewTabPage.NumberOfGrayTiles, since
   these histograms are equivalent (one is for the icon NTP; one for the
   thumbnail NTP)

BUG=536294
Committed: https://crrev.com/2a47ce7d25f4ccbe62d02e1dd90961cc526bc036
Cr-Commit-Position: refs/heads/master@{#351869}

[newt (away), 2015-09-28 22:34:20]

newt@chromium.org changed reviewers:
+ mathp@chromium.org, tedchoc@chromium.org

[newt (away), 2015-09-28 22:34:21]

mathp: PTAL at most_visited_sites.cc/h
tedchoc: PTAL at everything else

Thanks!

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:67:
private static final String ICON_NTP_FIELD_TRIAL_NAME = "IconNTP";
I moved finch logic from NewTabPageView to NewTabPage because 1) NewTabPage
needs to know about the icon-NTP finch group now, and 2) this logic really
belongs in the manager, not the view, anyway.

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
(left):

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:1091:
RecordHistogram.recordCount100Histogram("NewTabPage.IconsGray", mNumGrayIcons);
these metrics are now recorded in most_visited_sites.cc

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:142:
public void recordTileTypeMetrics(int[] tileTypes, boolean isIconMode) {
This was only every used for metrics recording, so I've made the name more
specific, especially since it now takes metrics-related parameters.

https://codereview.chromium.org/1373983003/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:780: InitiateTopSitesQuery();
There's no need to requery the server suggestions if we're only showing local
suggestions. So, we can just call InitiateTopSitesQuery().

https://codereview.chromium.org/1373983003/diff/20001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.h (left):

https://codereview.chromium.org/1373983003/diff/20001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.h:201: int num_local_thumbs_;
These counts are no longer needed, since RecordTileTypeMetrics() gets a list of
tile types from Java.

[Ted C, 2015-09-28 23:08:45]

lgtm

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
(right):

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java:120:
*         MostVisitedTileType.
for this and below, I would {@link them}

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:55:
*            server-provided.
indenting is off

https://codereview.chromium.org/1373983003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:138:
*            tile that's currently showing.
same indenting thing...should either be 8 for aligned with "An array" above.

[newt (away), 2015-09-29 16:05:20]

treib: FYI

[Mathieu, 2015-09-29 21:31:44]

most_visited_sites lgtm. Thanks Newton!

[newt (away), 2015-09-29 23:18:37]

newt@chromium.org changed reviewers:
+ asvitkine@chromium.org

[newt (away), 2015-09-29 23:18:37]

asvitkine: PTAL at histograms.xml

[newt (away), 2015-09-29 23:19:27]

newt@chromium.org changed reviewers:
+ thestig@chromium.org

[newt (away), 2015-09-29 23:19:28]

TBR thestig for chrome.gyp

[Lei Zhang, 2015-09-29 23:23:55]

lgtm

[Alexei Svitkine (slow), 2015-09-30 14:42:16]

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:401: for (size_t i = 0; i <
tile_types.size(); i++) {
Nit: ++i

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:403: counts_per_type[tile_type]++;
Ditto

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:405:
kImpressionTypeHistogramFormat,
Nit: Given you're only using kImpressionTypeHistogramFormat once, I think it's
easier to have it be inlined, so reader can clearly tell the number of args is
correct, etc.

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:418:
UMA_HISTOGRAM_COUNTS_100("NewTabPage.NumberOfThumbnailTiles",
Seems like previously this was being logged as UMA_HISTOGRAM_SPARSE_SLOWLY(()
and now you're changing the macro.

If that's the intention, please rename the histogram and mark the old one as
<obsolete> in the XML file. Otherwise, the two will have different bucketing
schemes which will make aggregation between version make no sense on the
dashboard.

[newt (away), 2015-09-30 17:53:46]

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:401: for (size_t i = 0; i <
tile_types.size(); i++) {
On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> Nit: ++i

Done.

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:403: counts_per_type[tile_type]++;
On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> Ditto

Done.

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:405:
kImpressionTypeHistogramFormat,
On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> Nit: Given you're only using kImpressionTypeHistogramFormat once, I think it's
> easier to have it be inlined, so reader can clearly tell the number of args is
> correct, etc.

Done. And moved the other histogram names inline for consistency.

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:418:
UMA_HISTOGRAM_COUNTS_100("NewTabPage.NumberOfThumbnailTiles",
On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> Seems like previously this was being logged as UMA_HISTOGRAM_SPARSE_SLOWLY(()
> and now you're changing the macro.
> 
> If that's the intention, please rename the histogram and mark the old one as
> <obsolete> in the XML file. Otherwise, the two will have different bucketing
> schemes which will make aggregation between version make no sense on the
> dashboard.

Yes, they previously used UMA_HISTOGRAM_SPARSE_SLOWLY. Since the values were
always between 0 and 8, I thought it would make more sense to use
UMA_HISTOGRAM_COUNTS_100. But if that messes up the dashboard, I'll just switch
them back to UMA_HISTOGRAM_SPARSE_SLOWLY.

[newt (away), 2015-09-30 18:01:10]

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

[commit-bot: I haz the power, 2015-09-30 18:02:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373983003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373983003/80001

[Alexei Svitkine (slow), 2015-09-30 18:11:58]

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:418:
UMA_HISTOGRAM_COUNTS_100("NewTabPage.NumberOfThumbnailTiles",
On 2015/09/30 17:53:46, newt wrote:
> On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> > Seems like previously this was being logged as
UMA_HISTOGRAM_SPARSE_SLOWLY(()
> > and now you're changing the macro.
> > 
> > If that's the intention, please rename the histogram and mark the old one as
> > <obsolete> in the XML file. Otherwise, the two will have different bucketing
> > schemes which will make aggregation between version make no sense on the
> > dashboard.
> 
> Yes, they previously used UMA_HISTOGRAM_SPARSE_SLOWLY. Since the values were
> always between 0 and 8, I thought it would make more sense to use
> UMA_HISTOGRAM_COUNTS_100. But if that messes up the dashboard, I'll just
switch
> them back to UMA_HISTOGRAM_SPARSE_SLOWLY.

UMA_HISTOGRAM_COUNTS_100 does exponential scaling. I think using either
SPARSE_SLOWLY or ENUMERATION macros would be ok.

[commit-bot: I haz the power, 2015-09-30 18:15:32]

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

[commit-bot: I haz the power, 2015-09-30 18:15:33]

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

[newt (away), 2015-09-30 19:07:50]

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/60001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:418:
UMA_HISTOGRAM_COUNTS_100("NewTabPage.NumberOfThumbnailTiles",
On 2015/09/30 18:11:58, Alexei Svitkine wrote:
> On 2015/09/30 17:53:46, newt wrote:
> > On 2015/09/30 14:42:16, Alexei Svitkine wrote:
> > > Seems like previously this was being logged as
> UMA_HISTOGRAM_SPARSE_SLOWLY(()
> > > and now you're changing the macro.
> > > 
> > > If that's the intention, please rename the histogram and mark the old one
as
> > > <obsolete> in the XML file. Otherwise, the two will have different
bucketing
> > > schemes which will make aggregation between version make no sense on the
> > > dashboard.
> > 
> > Yes, they previously used UMA_HISTOGRAM_SPARSE_SLOWLY. Since the values were
> > always between 0 and 8, I thought it would make more sense to use
> > UMA_HISTOGRAM_COUNTS_100. But if that messes up the dashboard, I'll just
> switch
> > them back to UMA_HISTOGRAM_SPARSE_SLOWLY.
> 
> UMA_HISTOGRAM_COUNTS_100 does exponential scaling. I think using either
> SPARSE_SLOWLY or ENUMERATION macros would be ok.

Thanks for explaining. I've reverted these three histograms to use
UMA_HISTOGRAM_SPARSE_SLOWLY. The three histograms above (IconsReal, IconsColor,
etc) were previously using UMA_HISTOGRAM_COUNTS_100 (or really, the Java
equivalent), so I've left them unchanged.

[Alexei Svitkine (slow), 2015-09-30 19:52:05]

lgtm % comments

https://codereview.chromium.org/1373983003/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1373983003/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:459:
return fieldTrialGroup.equals(ICON_NTP_ENABLED_GROUP);
Current best practices for variations is to check .startsWith() instead of
.equals(), which gives you more flexibility.

https://codereview.chromium.org/1373983003/diff/80001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1373983003/diff/80001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:402: counts_per_type[ICON_REAL]);
UMA_HISTOGRAM_COUNTS_100 allocates 50 buckets. I think it's a bit wasteful,
suggest switching to UMA_HISTOGRAM_SPARSE_SLOWLY() like the ones below it.

Looking at your data, all your values are below <12 in the data, which are exact
even in UMA_HISTOGRAM_COUNTS_100(). Suggest making consistent with the else.

[newt (away), 2015-09-30 20:40:54]

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:459:
return fieldTrialGroup.equals(ICON_NTP_ENABLED_GROUP);
On 2015/09/30 19:52:05, Alexei Svitkine wrote:
> Current best practices for variations is to check .startsWith() instead of
> .equals(), which gives you more flexibility.

Done.

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
File chrome/browser/android/most_visited_sites.cc (right):

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
chrome/browser/android/most_visited_sites.cc:402: counts_per_type[ICON_REAL]);
On 2015/09/30 19:52:05, Alexei Svitkine wrote:
> UMA_HISTOGRAM_COUNTS_100 allocates 50 buckets. I think it's a bit wasteful,
> suggest switching to UMA_HISTOGRAM_SPARSE_SLOWLY() like the ones below it.
> 
> Looking at your data, all your values are below <12 in the data, which are
exact
> even in UMA_HISTOGRAM_COUNTS_100(). Suggest making consistent with the else.

Done.

I had assumed that UMA_HISTOGRAM_COUNTS_100 was faster/simpler/more efficient
than UMA_HISTOGRAM_SPARSE_SLOWLY, based on their names, but it sounds like this
isn't the case, at least when only a few values will be reported. What does the
"SLOWLY" mean in the name anyway? 

It would be nice if the UMA_HISTOGRAM_SPARSE_SLOWLY macro had a comment
explaining itself.

[Alexei Svitkine (slow), 2015-09-30 20:59:52]

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
File chrome/browser/android/most_visited_sites.cc (right):

https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
chrome/browser/android/most_visited_sites.cc:402: counts_per_type[ICON_REAL]);
On 2015/09/30 20:40:54, newt wrote:
> On 2015/09/30 19:52:05, Alexei Svitkine wrote:
> > UMA_HISTOGRAM_COUNTS_100 allocates 50 buckets. I think it's a bit wasteful,
> > suggest switching to UMA_HISTOGRAM_SPARSE_SLOWLY() like the ones below it.
> > 
> > Looking at your data, all your values are below <12 in the data, which are
> exact
> > even in UMA_HISTOGRAM_COUNTS_100(). Suggest making consistent with the else.
> 
> Done.
> 
> I had assumed that UMA_HISTOGRAM_COUNTS_100 was faster/simpler/more efficient
> than UMA_HISTOGRAM_SPARSE_SLOWLY, based on their names, but it sounds like
this
> isn't the case, at least when only a few values will be reported. What does
the
> "SLOWLY" mean in the name anyway? 
> 
> It would be nice if the UMA_HISTOGRAM_SPARSE_SLOWLY macro had a comment
> explaining itself.

It is slower because it grabs a lock on that histogram object and uses a map.
It's a tiny bit of overhead, so shouldn't be used in tight loops, but otherwise
fast. But it's more memory efficient.

Agree that comments on the macro would be good.

[newt (away), 2015-09-30 21:03:52]

On 2015/09/30 20:59:52, Alexei Svitkine wrote:
>
https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
> File chrome/browser/android/most_visited_sites.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1373983003/diff/80001/chrome/browser/a...
> chrome/browser/android/most_visited_sites.cc:402: counts_per_type[ICON_REAL]);
> On 2015/09/30 20:40:54, newt wrote:
> > On 2015/09/30 19:52:05, Alexei Svitkine wrote:
> > > UMA_HISTOGRAM_COUNTS_100 allocates 50 buckets. I think it's a bit
wasteful,
> > > suggest switching to UMA_HISTOGRAM_SPARSE_SLOWLY() like the ones below it.
> > > 
> > > Looking at your data, all your values are below <12 in the data, which are
> > exact
> > > even in UMA_HISTOGRAM_COUNTS_100(). Suggest making consistent with the
else.
> > 
> > Done.
> > 
> > I had assumed that UMA_HISTOGRAM_COUNTS_100 was faster/simpler/more
efficient
> > than UMA_HISTOGRAM_SPARSE_SLOWLY, based on their names, but it sounds like
> this
> > isn't the case, at least when only a few values will be reported. What does
> the
> > "SLOWLY" mean in the name anyway? 
> > 
> > It would be nice if the UMA_HISTOGRAM_SPARSE_SLOWLY macro had a comment
> > explaining itself.
> 
> It is slower because it grabs a lock on that histogram object and uses a map.
> It's a tiny bit of overhead, so shouldn't be used in tight loops, but
otherwise
> fast. But it's more memory efficient.

Gotcha. Thanks for explaining.

[newt (away), 2015-09-30 21:32:36]

The CQ bit was checked by newt@chromium.org

[newt (away), 2015-09-30 21:32:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
tedchoc@chromium.org, thestig@chromium.org, asvitkine@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1373983003/#ps120001 (title: "rebased")

[commit-bot: I haz the power, 2015-09-30 21:33:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373983003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373983003/120001

[newt (away), 2015-09-30 21:33:41]

The CQ bit was unchecked by newt@chromium.org

[newt (away), 2015-09-30 21:35:02]

The CQ bit was checked by newt@chromium.org

[newt (away), 2015-09-30 21:35:02]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
tedchoc@chromium.org, asvitkine@chromium.org, thestig@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1373983003/#ps140001 (title: "fixed
test")

[commit-bot: I haz the power, 2015-09-30 21:36:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373983003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373983003/140001

[commit-bot: I haz the power, 2015-10-01 00:36:38]

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

[commit-bot: I haz the power, 2015-10-01 00:36:39]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[newt (away), 2015-10-01 16:53:49]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2015-10-01 16:54:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373983003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373983003/140001

[commit-bot: I haz the power, 2015-10-01 20:10:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-01 20:10:44]

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