ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411
Committed: https://crrev.com/9677e78a4a25747b9b4ea86b6536461125f0b235
Cr-Commit-Position: refs/heads/master@{#437838}

[mastiz, 2016-12-08 08:39:02]

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

[commit-bot: I haz the power, 2016-12-08 08:39:23]

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-12-08 08:46:33]

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

[commit-bot: I haz the power, 2016-12-08 08:46:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2016-12-08 09:54:10]

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

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

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-12-08 09:58:20]

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

[commit-bot: I haz the power, 2016-12-08 09:58:21]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[mastiz, 2016-12-08 10:05:33]

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

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

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

[mastiz, 2016-12-08 10:12:03]

Description was changed from

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=
==========

to

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411
==========

[commit-bot: I haz the power, 2016-12-08 10:13:30]

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

[commit-bot: I haz the power, 2016-12-08 10:13:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mastiz, 2016-12-08 10:36:53]

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

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

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-12-08 11:04:27]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mastiz, 2016-12-08 11:33:28]

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

[commit-bot: I haz the power, 2016-12-08 11:33:41]

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

[mastiz, 2016-12-08 12:32:08]

mastiz@chromium.org changed reviewers:
+ treib@chromium.org

[commit-bot: I haz the power, 2016-12-08 12:54:41]

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

[commit-bot: I haz the power, 2016-12-08 12:54:42]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-12-08 12:58:22]

Nice!

https://codereview.chromium.org/2557513007/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/2557513007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:113:
* @param tileUrls An array of strings indicating the URL of each tile
nitty nit: end comment with "."

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.cc (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:95: rappor_service,
"NTP.SuggestionsImpressions.IconsColor", url);
It's a bit unfortunate that these use a different naming convention vs the
histograms ("NTP" vs "NewTabPage"). But I guess staying consistent with the
existing Rappor metrics is more important...

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:105: default:
Explicitly list the uninteresting cases? Then we'll get a compile error if a new
one is added that's not handled here.
Or alternatively, add a comment about why it's okay to ignore all other cases.
(And maybe add a NOTREACHED()? Due to the "if (tile_type >=
NUM_RECORDED_TILE_TYPES)" above we shouldn't even get here, right?)

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.h:50: GURL url;
Add a constructor? As it is, it's easy to accidentally leave |source|
uninitialized. (In most places where these are constructed, you can then use
emplace_back, so it won't be any less convenient)

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics_unittest.cc (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics_unittest.cc:63: ElementsAre(base::Bucket(/*min=*/1,
/*count=*/4),
Can we use the enum entries for "min" here, instead of ints? IMO that'd make
this easier to read.

[mastiz, 2016-12-08 13:47:07]

PTAL.

https://codereview.chromium.org/2557513007/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
(right):

https://codereview.chromium.org/2557513007/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java:113:
* @param tileUrls An array of strings indicating the URL of each tile
On 2016/12/08 12:58:22, Marc Treib wrote:
> nitty nit: end comment with "."

Done.

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.cc (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:95: rappor_service,
"NTP.SuggestionsImpressions.IconsColor", url);
On 2016/12/08 12:58:22, Marc Treib wrote:
> It's a bit unfortunate that these use a different naming convention vs the
> histograms ("NTP" vs "NewTabPage"). But I guess staying consistent with the
> existing Rappor metrics is more important...

Acknowledged, that was my rationale, although I don't have a strong preference.

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:105: default:
On 2016/12/08 12:58:22, Marc Treib wrote:
> Explicitly list the uninteresting cases? Then we'll get a compile error if a
new
> one is added that's not handled here.
> Or alternatively, add a comment about why it's okay to ignore all other cases.
> (And maybe add a NOTREACHED()? Due to the "if (tile_type >=
> NUM_RECORDED_TILE_TYPES)" above we shouldn't even get here, right?)

Done.

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.h (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics.h:50: GURL url;
On 2016/12/08 12:58:22, Marc Treib wrote:
> Add a constructor? As it is, it's easy to accidentally leave |source|
> uninitialized. (In most places where these are constructed, you can then use
> emplace_back, so it won't be any less convenient)

Done.

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
File components/ntp_tiles/metrics_unittest.cc (right):

https://codereview.chromium.org/2557513007/diff/100001/components/ntp_tiles/m...
components/ntp_tiles/metrics_unittest.cc:63: ElementsAre(base::Bucket(/*min=*/1,
/*count=*/4),
On 2016/12/08 12:58:22, Marc Treib wrote:
> Can we use the enum entries for "min" here, instead of ints? IMO that'd make
> this easier to read.

Done.

[Marc Treib, 2016-12-08 13:52:59]

LGTM with some tiny nits. Thanks!

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:12: #include
"chrome/browser/browser_process.h"
Not needed I think?

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:172:
ntp_tiles::metrics::THUMBNAIL, GURL::EmptyGURL());
From the comment on EmptyGURL(), I think in this case just "GURL()" would be
preferred.

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.cc (right):

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:94: break;
Can this case happen?

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:111: case UNKNOWN_TILE_TYPE:
IMO this is one of the rare instances where fallthrough is okay, i.e. only a
single NOTREACHED() after the 3 cases.

[mastiz, 2016-12-08 14:02:51]

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc (right):

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:12: #include
"chrome/browser/browser_process.h"
On 2016/12/08 13:52:59, Marc Treib wrote:
> Not needed I think?

Removed leftover, thanks.

https://codereview.chromium.org/2557513007/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc:172:
ntp_tiles::metrics::THUMBNAIL, GURL::EmptyGURL());
On 2016/12/08 13:52:59, Marc Treib wrote:
> From the comment on EmptyGURL(), I think in this case just "GURL()" would be
> preferred.

Done.

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
File components/ntp_tiles/metrics.cc (right):

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:94: break;
On 2016/12/08 13:52:59, Marc Treib wrote:
> Can this case happen?

Yes, NONE is 0 and thus < NUM_RECORDED_TILE_TYPES.

In practice, UMA metrics like MostVisitedTileType also reflect a small but
non-zero value for bucket 0.

https://codereview.chromium.org/2557513007/diff/120001/components/ntp_tiles/m...
components/ntp_tiles/metrics.cc:111: case UNKNOWN_TILE_TYPE:
On 2016/12/08 13:52:59, Marc Treib wrote:
> IMO this is one of the rare instances where fallthrough is okay, i.e. only a
> single NOTREACHED() after the 3 cases.

Done.

[mastiz, 2016-12-08 14:07:48]

mastiz@chromium.org changed reviewers:
+ asvitkine@chromium.org, bauerb@chromium.org

[mastiz, 2016-12-08 14:07:49]

bauerb@chromium.org: Please review changes in Java.

asvitkine@chromium.org: Please review rappor.xml and the overall approach: do
you think Rappor can be a good fit here? We don't have good estimates but I'd
say the top sites will certainly be >1% of the total, but 5% seems too high.

[Bernhard Bauer, 2016-12-08 15:12:04]

lgtm

[Alexei Svitkine (slow), 2016-12-09 18:24:27]

lgtm

[mastiz, 2016-12-12 08:07:27]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-12-12 08:07:28]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2557513007/#ps130001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-12-12 08:07:38]

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-12-12 08:09:10]

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

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

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2016-12-12 08:14:16]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2016-12-12 08:14:16]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2557513007/#ps150001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-12-12 08:14:28]

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-12-12 09:55:54]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1481530456544240,
"parent_rev": "331e862a1d51d88829e8c8e4e87b871fb9b9f3ad", "commit_rev":
"2d6145d14fd6d5c5c8d8d3e7d94bc96e1fa1e053"}

[commit-bot: I haz the power, 2016-12-12 09:56:29]

Description was changed from

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411
==========

to

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411

Review-Url: https://codereview.chromium.org/2557513007
==========

[commit-bot: I haz the power, 2016-12-12 09:56:30]

Committed patchset #9 (id:150001)

[commit-bot: I haz the power, 2016-12-12 15:11:13]

Description was changed from

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411

Review-Url: https://codereview.chromium.org/2557513007
==========

to

==========
ntp_tiles::metrics: Add rappor metrics for impression URLs per icon type.

The goal is to list the top domains that show non-real icons and then
debug them individually. If we find evidence that such domains do
actually provide large icons, it would suggest there's client-side bugs
that prevent from displaying this icon (either it wasn't fetched, the
wrong version was kept around, it was garbage-collected, etc.).

BUG=672411

Committed: https://crrev.com/9677e78a4a25747b9b4ea86b6536461125f0b235
Cr-Commit-Position: refs/heads/master@{#437838}
==========

[commit-bot: I haz the power, 2016-12-12 15:11:15]

Patchset 9 (id:??) landed as
https://crrev.com/9677e78a4a25747b9b4ea86b6536461125f0b235
Cr-Commit-Position: refs/heads/master@{#437838}