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
Cr-Commit-Position: refs/heads/master@{#437838}

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

@@ -637,13 +637,15 @@ public class NewTabPage
 
             int tileTypes[] = new int[items.length];
             int sources[] = new int[items.length];
+            String tileUrls[] = new String[items.length];
 
             for (int i = 0; i < items.length; i++) {
                 tileTypes[i] = items[i].getTileType();
                 sources[i] = items[i].getSource();
+                tileUrls[i] = items[i].getUrl();
             }
 
-            mMostVisitedSites.recordPageImpression(tileTypes, sources);
+            mMostVisitedSites.recordPageImpression(tileTypes, sources, tileUrls);
 
             if (isNtpOfflinePagesEnabled()) {
                 final int maxNumTiles = 12;

chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java

@@ -110,9 +110,10 @@ public class MostVisitedSites {
      *                  tile that's currently showing.
      * @param sources An array of values from NTPTileSource indicating the source of each tile
      *                that's currently showing.
+     * @param tileUrls An array of strings indicating the URL of each tile.
      */
-    public void recordPageImpression(int[] tileTypes, int[] sources) {
-        nativeRecordPageImpression(mNativeMostVisitedSitesBridge, tileTypes, sources);
+    public void recordPageImpression(int[] tileTypes, int[] sources, String[] tileUrls) {
+        nativeRecordPageImpression(mNativeMostVisitedSitesBridge, tileTypes, sources, tileUrls);
     }
 
     /**
@@ -131,8 +132,8 @@ public class MostVisitedSites {
     private native void nativeAddOrRemoveBlacklistedUrl(
             long nativeMostVisitedSitesBridge, String url,
             boolean addUrl);
-    private native void nativeRecordPageImpression(long nativeMostVisitedSitesBridge,
-            int[] tileTypes, int[] sources);
+    private native void nativeRecordPageImpression(
+            long nativeMostVisitedSitesBridge, int[] tileTypes, int[] sources, String[] tileUrls);
     private native void nativeRecordOpenedMostVisitedItem(
             long nativeMostVisitedSitesBridge, int index, int tileType, int source);
 }

chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java

@@ -68,7 +68,7 @@ public class FakeMostVisitedSites extends MostVisitedSites {
     }
 
     @Override
-    public void recordPageImpression(int[] sources, int[] tileTypes) {
+    public void recordPageImpression(int[] sources, int[] tileTypes, String[] tileUrls) {
         // Metrics are stubbed out.
     }
 

chrome/browser/android/ntp/most_visited_sites_bridge.cc

@@ -31,6 +31,7 @@
 #include "components/ntp_tiles/icon_cacher.h"
 #include "components/ntp_tiles/metrics.h"
 #include "components/ntp_tiles/popular_sites.h"
+#include "components/rappor/rappor_service_impl.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/url_data_source.h"
@@ -48,6 +49,7 @@ using base::android::ToJavaArrayOfStrings;
 using base::android::ToJavaIntArray;
 using content::BrowserThread;
 using ntp_tiles::metrics::MostVisitedTileType;
+using ntp_tiles::metrics::TileImpression;
 using ntp_tiles::MostVisitedSites;
 using ntp_tiles::MostVisitedSitesSupervisor;
 using ntp_tiles::NTPTileSource;
@@ -203,22 +205,29 @@ void MostVisitedSitesBridge::RecordPageImpression(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jintArray>& jtile_types,
-    const JavaParamRef<jintArray>& jsources) {
+    const JavaParamRef<jintArray>& jsources,
+    const JavaParamRef<jobjectArray>& jtile_urls) {
   std::vector<int> int_sources;
   base::android::JavaIntArrayToIntVector(env, jsources, &int_sources);
   std::vector<int> int_tile_types;
   base::android::JavaIntArrayToIntVector(env, jtile_types, &int_tile_types);
+  std::vector<std::string> string_tile_urls;
+  base::android::AppendJavaStringArrayToStringVector(env, jtile_urls,
+                                                     &string_tile_urls);
 
   DCHECK_EQ(int_sources.size(), int_tile_types.size());
+  DCHECK_EQ(int_sources.size(), string_tile_urls.size());
 
-  std::vector<std::pair<NTPTileSource, MostVisitedTileType>> tiles;
+  std::vector<TileImpression> tiles;
   for (size_t i = 0; i < int_sources.size(); i++) {
     NTPTileSource source = static_cast<NTPTileSource>(int_sources[i]);
     MostVisitedTileType tile_type =
         static_cast<MostVisitedTileType>(int_tile_types[i]);
-    tiles.emplace_back(source, tile_type);
+
+    tiles.emplace_back(source, tile_type, GURL(string_tile_urls[i]));
   }
-  ntp_tiles::metrics::RecordPageImpression(tiles);
+  ntp_tiles::metrics::RecordPageImpression(tiles,
+                                           g_browser_process->rappor_service());
 }
 
 void MostVisitedSitesBridge::RecordOpenedMostVisitedItem(

chrome/browser/android/ntp/most_visited_sites_bridge.h

@@ -44,7 +44,8 @@ class MostVisitedSitesBridge {
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jintArray>& jtile_types,
-      const base::android::JavaParamRef<jintArray>& jsources);
+      const base::android::JavaParamRef<jintArray>& jsources,
+      const base::android::JavaParamRef<jobjectArray>& jtile_urls);
   void RecordOpenedMostVisitedItem(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,

chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc

@@ -155,9 +155,7 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   DVLOG(1) << "Emitting NTP load time: " << load_time << ", "
            << "number of tiles: " << impression_was_logged_.count();
 
-  std::vector<std::pair<ntp_tiles::NTPTileSource,
-                        ntp_tiles::metrics::MostVisitedTileType>>
-      tiles;
+  std::vector<ntp_tiles::metrics::TileImpression> tiles;
   bool has_server_side_suggestions = false;
   for (int i = 0; i < kNumMostVisited; i++) {
     if (!impression_was_logged_[i]) {
@@ -167,10 +165,14 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
         ntp_tiles::NTPTileSource::SUGGESTIONS_SERVICE) {
       has_server_side_suggestions = true;
     }
+    // No URL passed since we're not interested in favicon-related Rappor
+    // metrics.
     tiles.emplace_back(impression_tile_source_[i],
-                       ntp_tiles::metrics::THUMBNAIL);
+                       ntp_tiles::metrics::THUMBNAIL, GURL());
   }
-  ntp_tiles::metrics::RecordPageImpression(tiles);
+
+  // Not interested in Rappor metrics.
+  ntp_tiles::metrics::RecordPageImpression(tiles, /*rappor_service=*/nullptr);
 
   LogLoadTimeHistogram("NewTabPage.LoadTime", load_time);
 

components/ntp_tiles/BUILD.gn

@@ -46,6 +46,7 @@ static_library("ntp_tiles") {
     "//components/image_fetcher",
     "//components/pref_registry",
     "//components/prefs",
+    "//components/rappor/public",
     "//components/search_engines",
     "//components/url_formatter",
     "//components/variations",
@@ -77,6 +78,7 @@ source_set("unit_tests") {
   testonly = true
   sources = [
     "icon_cacher_unittest.cc",
+    "metrics_unittest.cc",
     "most_visited_sites_unittest.cc",
     "popular_sites_unittest.cc",
   ]
@@ -89,6 +91,7 @@ source_set("unit_tests") {
     "//components/favicon_base",
     "//components/image_fetcher",
     "//components/pref_registry:pref_registry",
+    "//components/rappor:test_support",
     "//components/sync_preferences:test_support",
     "//net:test_support",
     "//testing/gmock",

components/ntp_tiles/DEPS

@@ -7,6 +7,7 @@ include_rules = [
   "+components/history/core/browser",
   "+components/pref_registry",
   "+components/prefs",
+  "+components/rappor",
   "+components/search_engines",
   "+components/suggestions",
   "+components/sync_preferences",

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,27 @@ 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 NONE:
+        break;
+      case ICON_COLOR:
+        rappor::SampleDomainAndRegistryFromGURL(
+            rappor_service, "NTP.SuggestionsImpressions.IconsColor", url);
+        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;
+      case NUM_RECORDED_TILE_TYPES:  // Fall through.
+      case THUMBNAIL:  // Fall through.
+      case UNKNOWN_TILE_TYPE:
+        NOTREACHED();
+    }
   }
 
   if (have_tile_types) {

components/ntp_tiles/metrics.h

@@ -9,6 +9,11 @@
 #include <vector>
 
 #include "components/ntp_tiles/ntp_tile.h"
+#include "url/gurl.h"
+
+namespace rappor {
+class RapporService;
+}  // namespace rappor
 
 namespace ntp_tiles {
 namespace metrics {
@@ -39,10 +44,22 @@ enum MostVisitedTileType {
   UNKNOWN_TILE_TYPE,
 };
 
+struct TileImpression {
+  TileImpression(NTPTileSource source,
+                 MostVisitedTileType type,
+                 const GURL& url)
+      : source(source), type(type), url(url) {}
+
+  NTPTileSource source;
+  MostVisitedTileType type;
+  GURL url;
+};
+
 // Records an NTP impression, after all tiles have loaded.
-// Includes the visual types (see above) of all visible tiles.
-void RecordPageImpression(
-    const std::vector<std::pair<NTPTileSource, MostVisitedTileType>>& tiles);
+// Includes the visual types (see above) of all visible tiles. If
+// |rappor_service| is null, no rappor metrics will be reported.
+void RecordPageImpression(const std::vector<TileImpression>& tiles,
+                          rappor::RapporService* rappor_service);
 
 // Records a click on a tile.
 void RecordTileClick(int index,

components/ntp_tiles/metrics_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/ntp_tiles/metrics.h"
+
+#include <stddef.h>
+
+#include <string>
+#include <vector>
+
+#include "base/macros.h"
+#include "base/test/histogram_tester.h"
+#include "components/rappor/test_rappor_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace ntp_tiles {
+namespace metrics {
+namespace {
+
+using testing::ElementsAre;
+using testing::IsEmpty;
+
+TEST(RecordPageImpressionTest, ShouldRecordUmaForIcons) {
+  base::HistogramTester histogram_tester;
+  RecordPageImpression(
+      {{NTPTileSource::TOP_SITES, ICON_REAL, GURL()},
+       {NTPTileSource::TOP_SITES, ICON_REAL, GURL()},
+       {NTPTileSource::TOP_SITES, ICON_REAL, GURL()},
+       {NTPTileSource::TOP_SITES, ICON_COLOR, GURL()},
+       {NTPTileSource::TOP_SITES, ICON_COLOR, GURL()},
+       {NTPTileSource::SUGGESTIONS_SERVICE, ICON_REAL, GURL()},
+       {NTPTileSource::SUGGESTIONS_SERVICE, ICON_DEFAULT, GURL()},
+       {NTPTileSource::POPULAR, ICON_COLOR, GURL()}},
+      /*rappor_service=*/nullptr);
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
+              ElementsAre(base::Bucket(/*min=*/8, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
+                  base::Bucket(/*min=*/1, /*count=*/1),
+                  base::Bucket(/*min=*/2, /*count=*/1),
+                  base::Bucket(/*min=*/3, /*count=*/1),
+                  base::Bucket(/*min=*/4, /*count=*/1),
+                  base::Bucket(/*min=*/5, /*count=*/1),
+                  base::Bucket(/*min=*/6, /*count=*/1),
+                  base::Bucket(/*min=*/7, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(base::Bucket(/*min=*/5, /*count=*/1),
+                  base::Bucket(/*min=*/6, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
+                  base::Bucket(/*min=*/1, /*count=*/1),
+                  base::Bucket(/*min=*/2, /*count=*/1),
+                  base::Bucket(/*min=*/3, /*count=*/1),
+                  base::Bucket(/*min=*/4, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples(
+                  "NewTabPage.SuggestionsImpression.popular"),
+              ElementsAre(base::Bucket(/*min=*/7, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType"),
+              ElementsAre(base::Bucket(/*min=*/ICON_REAL, /*count=*/4),
+                          base::Bucket(/*min=*/ICON_COLOR, /*count=*/3),
+                          base::Bucket(/*min=*/ICON_DEFAULT, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.server"),
+              ElementsAre(base::Bucket(/*min=*/ICON_REAL, /*count=*/1),
+                          base::Bucket(/*min=*/ICON_DEFAULT, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.client"),
+              ElementsAre(base::Bucket(/*min=*/ICON_REAL, /*count=*/3),
+                          base::Bucket(/*min=*/ICON_COLOR, /*count=*/2)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.popular"),
+              ElementsAre(base::Bucket(/*min=*/ICON_COLOR, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsReal"),
+              ElementsAre(base::Bucket(/*min=*/4, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsColor"),
+              ElementsAre(base::Bucket(/*min=*/3, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsGray"),
+              ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
+}
+
+TEST(RecordPageImpressionTest, ShouldRecordUmaForThumbnails) {
+  base::HistogramTester histogram_tester;
+  RecordPageImpression({{NTPTileSource::TOP_SITES, THUMBNAIL, GURL()},
+                        {NTPTileSource::SUGGESTIONS_SERVICE, THUMBNAIL, GURL()},
+                        {NTPTileSource::POPULAR, THUMBNAIL, GURL()}},
+                       /*rappor_service=*/nullptr);
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
+              ElementsAre(base::Bucket(/*min=*/3, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(base::Bucket(/*min=*/0, /*count=*/1),
+                  base::Bucket(/*min=*/1, /*count=*/1),
+                  base::Bucket(/*min=*/2, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(base::Bucket(/*min=*/1, /*count=*/1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(base::Bucket(/*min=*/0, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples(
+                  "NewTabPage.SuggestionsImpression.popular"),
+              ElementsAre(base::Bucket(/*min=*/2, /*count=*/1)));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType"), IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.server"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.client"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.TileType.popular"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsReal"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsColor"),
+              IsEmpty());
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.IconsGray"),
+              IsEmpty());
+}
+
+TEST(RecordPageImpressionTest, ShouldRecordRappor) {
+  rappor::TestRapporServiceImpl rappor_service;
+
+  RecordPageImpression(
+      {{NTPTileSource::TOP_SITES, ICON_REAL, GURL("http://www.site1.com/")},
+       {NTPTileSource::TOP_SITES, ICON_COLOR, GURL("http://www.site2.com/")},
+       {NTPTileSource::TOP_SITES, ICON_DEFAULT, GURL("http://www.site3.com/")},
+       {NTPTileSource::TOP_SITES, THUMBNAIL, GURL("http://www.site4.com/")}},
+      &rappor_service);
+
+  // Thumbnail shouldn't get reported.
+  EXPECT_EQ(3, rappor_service.GetReportsCount());
+
+  {
+    std::string sample;
+    rappor::RapporType type;
+    EXPECT_TRUE(rappor_service.GetRecordedSampleForMetric(
+        "NTP.SuggestionsImpressions.IconsReal", &sample, &type));
+    EXPECT_EQ("site1.com", sample);
+    EXPECT_EQ(rappor::ETLD_PLUS_ONE_RAPPOR_TYPE, type);
+  }
+
+  {
+    std::string sample;
+    rappor::RapporType type;
+    EXPECT_TRUE(rappor_service.GetRecordedSampleForMetric(
+        "NTP.SuggestionsImpressions.IconsColor", &sample, &type));
+    EXPECT_EQ("site2.com", sample);
+    EXPECT_EQ(rappor::ETLD_PLUS_ONE_RAPPOR_TYPE, type);
+  }
+
+  {
+    std::string sample;
+    rappor::RapporType type;
+    EXPECT_TRUE(rappor_service.GetRecordedSampleForMetric(
+        "NTP.SuggestionsImpressions.IconsGray", &sample, &type));
+    EXPECT_EQ("site3.com", sample);
+    EXPECT_EQ(rappor::ETLD_PLUS_ONE_RAPPOR_TYPE, type);
+  }
+}
+
+}  // namespace
+}  // namespace metrics
+}  // namespace ntp_tiles

tools/metrics/rappor/rappor.xml

@@ -1229,6 +1229,36 @@ now we are only interested in H264, VP8 and VP9.
   </summary>
 </rappor-metric>
 
+<rappor-metric name="NTP.SuggestionsImpressions.IconsColor"
+    type="UMA_RAPPOR_TYPE">
+  <owner>mastiz@chromium.org</owner>
+  <summary>
+    The eTLD+1 of a most visited tile displayed in the NTP, for cases where the
+    fallback color was used (as opposed to having an icon, or simply being
+    gray).
+  </summary>
+</rappor-metric>
+
+<rappor-metric name="NTP.SuggestionsImpressions.IconsGray"
+    type="UMA_RAPPOR_TYPE">
+  <owner>mastiz@chromium.org</owner>
+  <summary>
+    The eTLD+1 of a most visited tile displayed in the NTP, for cases where the
+    gray fallback icon was used (as opposed to having an icon, or a fallback
+    color).
+  </summary>
+</rappor-metric>
+
+<rappor-metric name="NTP.SuggestionsImpressions.IconsReal"
+    type="UMA_RAPPOR_TYPE">
+  <owner>mastiz@chromium.org</owner>
+  <summary>
+    The eTLD+1 of a most visited tile displayed in the NTP, for cases where the
+    real icon was used (as opposed to having a fallback color or simply being
+    gray).
+  </summary>
+</rappor-metric>
+
 <rappor-metric name="Navigation.Scheme.Data" type="ETLD_PLUS_ONE">
   <owner>meacer@chromium.org</owner>
   <summary>