NTP: Fix metrics recording crash by plumbing the necessary data to Java.

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

Index: chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
index 5325fcefa20308327e82408ba5754bbf75f378d6..2503f96c85e0df2d6c66ababa0e4a86e4a82cf92 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
@@ -104,6 +104,13 @@ public class NewTabPage
     private static final int CTA_IMAGE_CLICKED = 1;
     private static final int ANIMATED_LOGO_CLICKED = 2;
 
+    // Identifiers for the various tile sources.
+    private static final String HISTOGRAM_CLIENT_NAME = "client";
+    private static final String HISTOGRAM_SERVER_NAME = "server";
+    private static final String HISTOGRAM_SERVER_PREFIX = "server";
+    private static final String HISTOGRAM_POPULAR_NAME = "popular";
+    private static final String HISTOGRAM_WHITELIST_NAME = "whitelist";
+
     private static MostVisitedSites sMostVisitedSitesForTests;
 
     private final Tab mTab;
@@ -527,11 +534,7 @@ public class NewTabPage
             // If not visible when loading completes, wait until onShown is received.
             if (!mTab.isHidden()) recordNTPShown();
 
-            int tileTypes[] = new int[items.length];
-            for (int i = 0; i < items.length; i++) {
-                tileTypes[i] = items[i].getTileType();
-            }
-            mMostVisitedSites.recordTileTypeMetrics(tileTypes);
+            recordTileTypeMetrics(items);
 
             if (isNtpOfflinePagesEnabled()) {
                 final int maxNumTiles = 12;
@@ -547,6 +550,48 @@ public class NewTabPage
     };
 
     /**
+     * Records metrics about which types of tiles are displayed.
+     * @param items An array of MostVisitedItem objects to record.
+     */
+    public static void recordTileTypeMetrics(MostVisitedItem[] items) {

[Marc Treib, 2016/06/29 08:19:31]

Hmmmm. I know I said differently on the bug, but I think I'd prefer to pass the
info back to C++ and record the metrics there after all. We've been trying to
get all the NTP metrics for all platforms in one place, because the current
practice of things being scattered everywhere and duplicated for various
platforms has led to a bunch of inconsistencies (if you're curious:
https://docs.google.com/spreadsheets/d/1W9eXR0G1GiJbd8SCX_mGSVqnxnTy-r7NsJKvG...).

If you pass the sources and providerIndices back along with the tileTypes, then
that should also fix the bug, right?

Bernhard, Chris, WDYT?

[Bernhard Bauer, 2016/06/29 10:17:43]

I somewhat prefer doing the tallying up here in Java and then only passing final
counts to C++. Could we define the interface on the C++ side so it will accept
the counts? That should still avoid inconsistencies.

[Marc Treib, 2016/06/29 10:23:11]

Hm, we would have one count per "source+providerIndex". So we'd need to pass
some sort of key-value store. Not sure that's worth the complexity...
Mid-term, I'd like to get rid of the provider indices altogether, since they
don't give any useful information anyway, but that's not going to happen before
branch point.

[dewittj, 2016/06/30 17:48:53]

I passed the types, sources, and indices back to C++; wdyt about that strategy?

[Marc Treib, 2016/07/01 10:01:03]

SGTM

+        // Filled with zeros by default.
+        int[] countsPerType = new int[MostVisitedTileType.NUM_TILE_TYPES];
+
+        for (int i = 0; i < items.length; ++i) {
+            MostVisitedItem item = items[i];
+            int tileType = item.getTileType();
+            ++countsPerType[tileType];

[Bernhard Bauer, 2016/06/29 10:17:43]

Small nit: I'm not sure to what extent the microoptimization of preincrement
instead of postincrement applies to Java. Just in terms of readability, I
somewhat prefer postincrement if the expression value is not needed, because it
reads a bit more imperative.

[dewittj, 2016/06/30 17:48:53]

Done.

+            String histogram = "NewTabPage.TileType."
+                    + getSourceHistogramName(item.getSource(), item.getProviderIndex());
+            RecordHistogram.recordLinearCountHistogram(histogram, tileType, 0,
+                    MostVisitedTileType.NUM_TILE_TYPES, MostVisitedTileType.NUM_TILE_TYPES + 1);
+        }
+
+        RecordHistogram.recordSparseSlowlyHistogram(
+                "NewTabPage.IconsReal", countsPerType[MostVisitedTileType.ICON_REAL]);
+        RecordHistogram.recordSparseSlowlyHistogram(
+                "NewTabPage.IconsColor", countsPerType[MostVisitedTileType.ICON_COLOR]);
+        RecordHistogram.recordSparseSlowlyHistogram(
+                "NewTabPage.IconsGray", countsPerType[MostVisitedTileType.ICON_DEFAULT]);
+    }
+
+    private static String getSourceHistogramName(int source, int providerIndex) {
+        switch (source) {
+            case MostVisitedSource.TOP_SITES:
+                return HISTOGRAM_CLIENT_NAME;
+            case MostVisitedSource.POPULAR:
+                return HISTOGRAM_POPULAR_NAME;
+            case MostVisitedSource.WHITELIST:
+                return HISTOGRAM_WHITELIST_NAME;
+            case MostVisitedSource.SUGGESTIONS_SERVICE:
+                return providerIndex >= 0 ? HISTOGRAM_SERVER_PREFIX + providerIndex
+                                          : HISTOGRAM_SERVER_NAME;
+        }
+        assert false; // An unknown source was used.

[Bernhard Bauer, 2016/06/29 10:17:43]

Nit: additional space before comments. Also, you could add the value to the
assertion message.

[dewittj, 2016/06/30 17:48:53]

Done.

+        return "";
+    }
+
+    /**
      * Constructs a NewTabPage.
      * @param activity The activity used for context to create the new tab page's View.
      * @param tab The Tab that is showing this new tab page.