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

components/ntp_tiles/metrics.cc

Index: components/ntp_tiles/metrics.cc
diff --git a/components/ntp_tiles/metrics.cc b/components/ntp_tiles/metrics.cc
index 1b9a64fced2270fe4c4c98d9d102f7852232409a..8704e270644714d5fd699a1cac87d4e96a96161e 100644
--- a/components/ntp_tiles/metrics.cc
+++ b/components/ntp_tiles/metrics.cc
@@ -10,6 +10,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/stringprintf.h"
+#include "components/rappor/public/rappor_utils.h"
 
 namespace ntp_tiles {
 namespace metrics {
@@ -55,15 +56,16 @@ std::string GetSourceHistogramName(NTPTileSource source) {
 
 }  // namespace
 
-void RecordPageImpression(
-    const std::vector<std::pair<NTPTileSource, MostVisitedTileType>>& tiles) {
+void RecordPageImpression(const std::vector<TileImpression>& tiles,
+                          rappor::RapporService* rappor_service) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles", tiles.size());
 
   int counts_per_type[NUM_RECORDED_TILE_TYPES] = {0};
   bool have_tile_types = false;
   for (int index = 0; index < static_cast<int>(tiles.size()); index++) {
-    NTPTileSource source = tiles[index].first;
-    MostVisitedTileType tile_type = tiles[index].second;
+    NTPTileSource source = tiles[index].source;
+    MostVisitedTileType tile_type = tiles[index].type;
+    const GURL& url = tiles[index].url;
 
     UMA_HISTOGRAM_ENUMERATION("NewTabPage.SuggestionsImpression", index,
                               kMaxNumTiles);
@@ -86,6 +88,23 @@ void RecordPageImpression(
     std::string tile_type_histogram =
         base::StringPrintf("NewTabPage.TileType.%s", source_name.c_str());
     LogHistogramEvent(tile_type_histogram, tile_type, NUM_RECORDED_TILE_TYPES);
+
+    switch (tile_type) {
+      case ICON_COLOR:
+        rappor::SampleDomainAndRegistryFromGURL(
+            rappor_service, "NTP.SuggestionsImpressions.IconsColor", url);

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

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...

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

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

+        break;
+      case ICON_DEFAULT:
+        rappor::SampleDomainAndRegistryFromGURL(
+            rappor_service, "NTP.SuggestionsImpressions.IconsGray", url);
+        break;
+      case ICON_REAL:
+        rappor::SampleDomainAndRegistryFromGURL(
+            rappor_service, "NTP.SuggestionsImpressions.IconsReal", url);
+        break;
+      default:

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

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?)

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

Done.

+        break;
+    }
   }
 
   if (have_tile_types) {