Query Tiles : Several fixes for UMA

This CL
1 - Adds TileUmaLogger to omnibox which was missing earlier.
2 - Fixes a bug in TileUmaLogger which wasn't recording top level tiles correctly.
3 - Added tests for TileUmaLogger.

Bug: 1091130
Change-Id: Ie556640c6b301cb26915462973d26c373667ab16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2230318Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775287}

chrome/android/BUILD.gn

@@ -990,6 +990,7 @@ android_library("chrome_test_java") {
     "//components/policy/android:policy_java",
     "//components/policy/android:policy_java_test_support",
     "//components/query_tiles:public_java",
+    "//components/query_tiles:test_support_java",
     "//components/safe_browsing/android:safe_browsing_java",
     "//components/schema_org/common:mojom_java",
     "//components/search_engines/android:java",

chrome/android/chrome_test_java_sources.gni

@@ -437,7 +437,6 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/query_tiles/OmniboxQueryTileSuggestionTest.java",
   "javatests/src/org/chromium/chrome/browser/query_tiles/QueryTileSectionTest.java",
   "javatests/src/org/chromium/chrome/browser/query_tiles/QueryTileSectionToOmniboxTest.java",
-  "javatests/src/org/chromium/chrome/browser/query_tiles/TestTileProvider.java",
   "javatests/src/org/chromium/chrome/browser/query_tiles/TileMatchers.java",
   "javatests/src/org/chromium/chrome/browser/query_tiles/ViewActions.java",
   "javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java",

chrome/android/java/src/org/chromium/chrome/browser/omnibox/suggestions/OmniboxQueryTileCoordinator.java

@@ -23,6 +23,7 @@ import org.chromium.components.browser_ui.widget.image_tiles.ImageTileCoordinato
 import org.chromium.components.browser_ui.widget.image_tiles.TileConfig;
 import org.chromium.components.query_tiles.QueryTile;
 import org.chromium.components.query_tiles.QueryTileConstants;
+import org.chromium.components.query_tiles.TileUmaLogger;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 import org.chromium.ui.UiUtils;
 
@@ -40,6 +41,7 @@ public class OmniboxQueryTileCoordinator {
 
     private final Context mContext;
     private final Callback<QueryTile> mSelectionCallback;
+    private final TileUmaLogger mTileUmaLogger;
     private ImageTileCoordinator mTileCoordinator;
     private ImageFetcher mImageFetcher;
     private Integer mTileWidth;
@@ -52,10 +54,12 @@ public class OmniboxQueryTileCoordinator {
     public OmniboxQueryTileCoordinator(Context context, Callback<QueryTile> selectionCallback) {
         mContext = context;
         mSelectionCallback = selectionCallback;
+        mTileUmaLogger = new TileUmaLogger(UMA_PREFIX);
     }
 
     /** Called to set the list of query tiles to be displayed in the suggestion. */
     public void setTiles(List<QueryTile> tiles) {
+        mTileUmaLogger.recordTilesLoaded(tiles);
         getTileCoordinator().setTiles(tiles == null ? new ArrayList<>() : new ArrayList<>(tiles));
     }
 
@@ -123,6 +127,7 @@ public class OmniboxQueryTileCoordinator {
 
     private void onTileClicked(ImageTile tile) {
         QueryTile queryTile = (QueryTile) tile;
+        mTileUmaLogger.recordTileClicked(queryTile);
         mSelectionCallback.onResult(queryTile);
     }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/query_tiles/OmniboxQueryTileSuggestionTest.java

@@ -49,6 +49,7 @@ import org.chromium.chrome.test.util.OmniboxTestUtils;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.embedder_support.util.UrlConstants;
 import org.chromium.components.query_tiles.QueryTile;
+import org.chromium.components.query_tiles.TestTileProvider;
 import org.chromium.components.query_tiles.TileProvider;
 import org.chromium.content_public.browser.UiThreadTaskTraits;
 import org.chromium.content_public.browser.test.util.Criteria;

chrome/android/javatests/src/org/chromium/chrome/browser/query_tiles/QueryTileSectionTest.java

@@ -43,6 +43,7 @@ import org.chromium.chrome.test.util.OmniboxTestUtils;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.embedder_support.util.UrlConstants;
 import org.chromium.components.query_tiles.QueryTile;
+import org.chromium.components.query_tiles.TestTileProvider;
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;

chrome/android/javatests/src/org/chromium/chrome/browser/query_tiles/QueryTileSectionToOmniboxTest.java

@@ -43,6 +43,7 @@ import org.chromium.chrome.test.util.OmniboxTestUtils;
 import org.chromium.chrome.test.util.browser.Features;
 import org.chromium.components.embedder_support.util.UrlConstants;
 import org.chromium.components.query_tiles.QueryTile;
+import org.chromium.components.query_tiles.TestTileProvider;
 
 /**
  * Provides a set of tests to validate that QueryTiles works properly in the NTP given the following

components/BUILD.gn

@@ -752,6 +752,7 @@ if (is_android) {
       "//components/gcm_driver/android:components_gcm_driver_junit_tests",
       "//components/permissions/android:components_permissions_junit_tests",
       "//components/policy/android:components_policy_junit_tests",
+      "//components/query_tiles:query_tiles_junit_tests",
       "//components/signin/core/browser/android:components_signin_junit_tests",
       "//components/variations/android:components_variations_junit_tests",
     ]

components/query_tiles/BUILD.gn

@@ -99,6 +99,29 @@ if (is_android) {
       "android/java/src/org/chromium/components/query_tiles/bridges/TileProviderBridge.java",
     ]
   }
+
+  android_library("test_support_java") {
+    sources = [ "android/java/src/org/chromium/components/query_tiles/TestTileProvider.java" ]
+
+    deps = [
+      ":public_java",
+      "//base:base_java",
+    ]
+  }
+
+  android_library("query_tiles_junit_tests") {
+    bypass_platform_checks = true
+    testonly = true
+    sources = [ "android/java/src/org/chromium/components/query_tiles/TileUmaLoggerTest.java" ]
+
+    deps = [
+      ":public_java",
+      ":test_support_java",
+      "//base:base_java",
+      "//base:base_junit_test_support",
+      "//third_party/junit",
+    ]
+  }
 }
 
 group("unit_tests") {

chrome/android/javatests/src/org/chromium/chrome/browser/query_tiles/TestTileProvider.java → components/query_tiles/android/java/src/org/chromium/components/query_tiles/TestTileProvider.java

@@ -2,11 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-package org.chromium.chrome.browser.query_tiles;
+package org.chromium.components.query_tiles;
 
 import org.chromium.base.Callback;
-import org.chromium.components.query_tiles.QueryTile;
-import org.chromium.components.query_tiles.TileProvider;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -14,7 +12,7 @@ import java.util.List;
 /**
  * Helper implementation of {@link TileProvider} for tests.
  */
-class TestTileProvider implements TileProvider {
+public class TestTileProvider implements TileProvider {
     private List<QueryTile> mTiles = new ArrayList<>();
 
     /**

components/query_tiles/android/java/src/org/chromium/components/query_tiles/TileUmaLogger.java

@@ -16,16 +16,27 @@ import java.util.List;
  */
 public class TileUmaLogger {
     private final String mHistogramPrefix;
-    private List<QueryTile> mTiles;
+    private List<QueryTile> mTopLevelTiles;
 
     public TileUmaLogger(String histogramPrefix) {
         mHistogramPrefix = histogramPrefix;
     }
 
+    /**
+     * Called when a new set of tiles are loaded.
+     * @param tiles The tiles loaded in the UI.
+     */
     public void recordTilesLoaded(List<QueryTile> tiles) {
-        mTiles = tiles;
+        if (tiles == null || tiles.isEmpty()) return;
+
+        // We only care about the first time tiles are loaded. The subsequent calls to this function
+        // are ignored as they might be called for sub-level tiles and we already have the tree
+        // information.
+        if (mTopLevelTiles != null) return;
+        mTopLevelTiles = tiles;
+
         RecordHistogram.recordCountHistogram(
-                "Search." + mHistogramPrefix + ".TileCount", mTiles.size());
+                "Search." + mHistogramPrefix + ".TileCount", mTopLevelTiles.size());
     }
 
     public void recordTileClicked(QueryTile tile) {
@@ -51,7 +62,7 @@ public class TileUmaLogger {
     }
 
     private boolean isTopLevelTile(String id) {
-        for (QueryTile tile : mTiles) {
+        for (QueryTile tile : mTopLevelTiles) {
             if (tile.id.equals(id)) return true;
         }
         return false;
@@ -60,14 +71,14 @@ public class TileUmaLogger {
     /**
      * Returns a UMA ID for the tile which will be used to identify the tile for metrics purposes.
      * The UMA ID is generated from the position of the tile in the query tile tree.
-     * Top level tiles will be numbered 1,2,3... while the next level tile start
-     * from 101 (101, 102, ... etc), 201 (201, 202, ... etc), and so on.
+     * Top level tiles will be numbered 0,1,2... while the next level tile start
+     * from 100 (100, 101, ... etc), 200 (200, 201, ... etc), and so on.
      */
     private int getTileUmaId(String tileId) {
         assert !TextUtils.isEmpty(tileId);
 
-        for (int i = 0; i < mTiles.size(); i++) {
-            int found = search(mTiles.get(i), tileId, i + 1);
+        for (int i = 0; i < mTopLevelTiles.size(); i++) {
+            int found = search(mTopLevelTiles.get(i), tileId, i);
             if (found != -1) return found;
         }
 
@@ -75,10 +86,11 @@ public class TileUmaLogger {
     }
 
     private int search(QueryTile tile, String id, int startPosition) {
+        if (id.equals(tile.id)) return startPosition;
+        startPosition = (startPosition + 1) * 100;
         for (int i = 0; i < tile.children.size(); i++) {
             QueryTile child = tile.children.get(i);
-            if (id.equals(child.id)) return startPosition + i;
-            int found = search(child, id, startPosition * 100);
+            int found = search(child, id, startPosition + i);
             if (found != -1) return found;
         }
         return -1;

components/query_tiles/android/java/src/org/chromium/components/query_tiles/TileUmaLoggerTest.java

+// Copyright 2020 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.
+
+package org.chromium.components.query_tiles;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.metrics.RecordHistogram;
+import org.chromium.base.metrics.test.ShadowRecordHistogram;
+import org.chromium.base.test.BaseRobolectricTestRunner;
+
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE, shadows = {ShadowRecordHistogram.class})
+public class TileUmaLoggerTest {
+    private TileUmaLogger mTileUmaLogger;
+    private TestTileProvider mTileProvider;
+
+    @Before
+    public void setUp() {
+        ShadowRecordHistogram.reset();
+        mTileProvider = new TestTileProvider(2, 12);
+        mTileUmaLogger = new TileUmaLogger("TestUiSurface");
+    }
+
+    @After
+    public void tearDown() {}
+
+    @Test
+    public void testTileClicked() {
+        mTileProvider.getQueryTiles(tiles -> {
+            mTileUmaLogger.recordTilesLoaded(tiles);
+
+            // Click top level tiles.
+            mTileUmaLogger.recordTileClicked(mTileProvider.getTileAt(0));
+            mTileUmaLogger.recordTileClicked(mTileProvider.getTileAt(11));
+            assertHistogramClicked(0, 1);
+            assertHistogramClicked(1, 0);
+            assertHistogramClicked(11, 1);
+            assertHistogramClicked(100, 0);
+            assertHistogramClicked(111, 0);
+            assertHistogramClicked(1200, 0);
+
+            // Click next level tiles.
+            mTileUmaLogger.recordTileClicked(mTileProvider.getTileAt(2, 3));
+            mTileUmaLogger.recordTileClicked(mTileProvider.getTileAt(2, 3));
+            assertHistogramClicked(0, 1);
+            assertHistogramClicked(303, 2);
+            assertHistogramClicked(11, 1);
+
+            // Click on the chip on fakebox.
+            mTileUmaLogger.recordSearchButtonClicked(mTileProvider.getTileAt(0, 2));
+            Assert.assertEquals(1,
+                    RecordHistogram.getHistogramValueCountForTesting(
+                            "Search.QueryTiles.NTP.Chip.SearchClicked", 102));
+        });
+    }
+
+    private void assertHistogramClicked(int tileHistogramIndex, int expected) {
+        int actual = RecordHistogram.getHistogramValueCountForTesting(
+                "Search.TestUiSurface.Tile.Clicked", tileHistogramIndex);
+        Assert.assertEquals(expected, actual);
+    }
+}