Use MetricsReporter to log time in Tab Search

This CL aims to test MetricsReporter as a non-extension replacement for metricsPrivate. Adding metrics - Tabs.TabSearch.Mojo.TabUpdated, Tabs.TabSearch.Mojo.TabUpdated.IsOverlap A measurement that starts in C++ and ends in WebUI. For overlapping measurements, the first measurement will be recorded. IsOverlap metric counts overlapping measurements. - Tabs.TabSearch.Mojo.SwitchToTab, Tabs.TabSearch.Mojo.SwitchToTab.IsOverlap A measurement that starts in WebUI and ends in C++. - Tabs.TabSearch.WebUI.TabListDataReceived2 Tabs.TabSearch.WebUI.TabListDataReceived2.IsOverlap A measurement that both starts and ends in C++. Also add gating feature flag TabSearchUseMetricsReporter. Bug: 1269417 Change-Id: I0e38b8c103f2bc3d92be982de76bed316fcd73d7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3235137 Reviewed-by: Alex Gough <ajgo@chromium.org> Reviewed-by: Roman Arora <romanarora@chromium.org> Reviewed-by: Yuheng Huang <yuhengh@chromium.org> Reviewed-by: Joe Mason <joenotcharles@google.com> Commit-Queue: Keren Zhu <kerenzhu@chromium.org> Cr-Commit-Position: refs/heads/main@{#1002859}
chromium · May 12, 2022 · 8287a26 · 8287a26
1 parent e9b2da4
commit 8287a26
Show file tree

Hide file tree

Showing 16 changed files with 225 additions and 10 deletions.
diff --git a/chrome/browser/chrome_browser_interface_binders.cc b/chrome/browser/chrome_browser_interface_binders.cc
@@ -179,6 +179,7 @@
 #include "ui/webui/resources/cr_components/history_clusters/history_clusters.mojom.h"
 #include "ui/webui/resources/cr_components/most_visited/most_visited.mojom.h"
 #include "ui/webui/resources/js/browser_command/browser_command.mojom.h"
+#include "ui/webui/resources/js/metrics_reporter/metrics_reporter.mojom.h"
 #endif  // BUILDFLAG(IS_ANDROID)
 
 #if BUILDFLAG(IS_WIN) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_LINUX) || \
@@ -918,6 +919,10 @@ void PopulateChromeWebUIFrameBinders(
 
   RegisterWebUIControllerInterfaceBinder<tab_search::mojom::PageHandlerFactory,
                                          TabSearchUI>(map);
+  if (base::FeatureList::IsEnabled(features::kTabSearchUseMetricsReporter)) {
+    RegisterWebUIControllerInterfaceBinder<
+        metrics_reporter::mojom::PageMetricsHost, TabSearchUI>(map);
+  }
 
   RegisterWebUIControllerInterfaceBinder<
       download_shelf::mojom::PageHandlerFactory, DownloadShelfUI>(map);

diff --git a/chrome/browser/resources/tab_search/BUILD.gn b/chrome/browser/resources/tab_search/BUILD.gn
@@ -29,6 +29,7 @@ if (optimize_webui) {
 
     excludes = [
       "chrome://resources/js/cr.m.js",
+      "chrome://resources/js/metrics_reporter/metrics_reporter.js",
       "chrome://resources/mojo/mojo/public/js/bindings.js",
       "chrome://resources/mojo/mojo/public/mojom/base/time.mojom-webui.js",
       "chrome://resources/mojo/mojo/public/mojom/base/token.mojom-webui.js",
@@ -169,6 +170,7 @@ ts_library("build_ts") {
   deps = [
     "//third_party/polymer/v3_0:library",
     "//ui/webui/resources:library",
+    "//ui/webui/resources/js/metrics_reporter:build_ts",
     "//ui/webui/resources/mojo:library",
   ]
   extra_deps = [

diff --git a/chrome/browser/resources/tab_search/app.ts b/chrome/browser/resources/tab_search/app.ts
@@ -18,6 +18,7 @@ import './strings.m.js';
 
 import {assert} from 'chrome://resources/js/assert_ts.js';
 import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
+import {MetricsReporter, MetricsReporterImpl} from 'chrome://resources/js/metrics_reporter/metrics_reporter.js';
 import {listenOnce} from 'chrome://resources/js/util.m.js';
 import {Token} from 'chrome://resources/mojo/mojo/public/mojom/base/token.mojom-webui.js';
 import {IronA11yAnnouncer} from 'chrome://resources/polymer/v3_0/iron-a11y-announcer/iron-a11y-announcer.js';
@@ -125,6 +126,8 @@ export class TabSearchAppElement extends PolymerElement {
   private searchResultText_: string;
 
   private apiProxy_: TabSearchApiProxy = TabSearchApiProxyImpl.getInstance();
+  private metricsReporter_: MetricsReporter|null;
+  private useMetricsReporter_: boolean;
   private listenerIds_: Array<number> = [];
   private tabGroupsMap_: Map<string, TabGroup> = new Map();
   private recentlyClosedTabGroups_: Array<TabGroupData> = [];
@@ -162,6 +165,13 @@ export class TabSearchAppElement extends PolymerElement {
         true /*expanded*/);
   }
 
+  get metricsReporter(): MetricsReporter {
+    if (!this.metricsReporter_) {
+      this.metricsReporter_ = MetricsReporterImpl.getInstance();
+    }
+    return this.metricsReporter_;
+  }
+
   override ready() {
     super.ready();
 
@@ -186,6 +196,8 @@ export class TabSearchAppElement extends PolymerElement {
         },
       ],
     });
+
+    this.useMetricsReporter_ = loadTimeData.getBoolean('useMetricsReporter');
   }
 
   override connectedCallback() {
@@ -247,11 +259,32 @@ export class TabSearchAppElement extends PolymerElement {
 
   private updateTabs_() {
     const getTabsStartTimestamp = Date.now();
+
+    if (this.useMetricsReporter_) {
+      const isMarkOverlap =
+          this.metricsReporter.hasLocalMark('TabListDataReceived');
+      chrome.metricsPrivate.recordBoolean(
+          'Tabs.TabSearch.WebUI.TabListDataReceived2.IsOverlap', isMarkOverlap);
+      if (!isMarkOverlap) {
+        this.metricsReporter.mark('TabListDataReceived');
+      }
+    }
+
     this.apiProxy_.getProfileData().then(({profileData}) => {
       chrome.metricsPrivate.recordTime(
           'Tabs.TabSearch.WebUI.TabListDataReceived',
           Math.round(Date.now() - getTabsStartTimestamp));
 
+      if (this.useMetricsReporter_) {
+        // TODO(crbug.com/1269417): this is a side-by-side comparison of
+        // metrics reporter histogram vs. old histogram. Cleanup when the
+        // experiment ends.
+        this.metricsReporter.measure('TabListDataReceived')
+            .then(
+                e => this.metricsReporter.umaReportTime(
+                    'Tabs.TabSearch.WebUI.TabListDataReceived2', e))
+            .then(() => this.metricsReporter.clearMark('TabListDataReceived'));
+      }
       // The infinite-list produces viewport-filled events whenever a data or
       // scroll position change triggers the the viewport fill logic.
       listenOnce(this.$.tabsList, 'viewport-filled', () => {
@@ -270,18 +303,29 @@ export class TabSearchAppElement extends PolymerElement {
     const tabData = this.tabData_(
         tab, inActiveWindow, TabItemType.OPEN_TAB, this.tabGroupsMap_);
     // Replace the tab with the same tabId and trigger rerender.
-    for (let i = 0; i < this.openTabs_.length; ++i) {
+    let foundTab = false;
+    for (let i = 0; i < this.openTabs_.length && !foundTab; ++i) {
       if (this.openTabs_[i]!.tab.tabId === tab.tabId) {
         this.openTabs_[i] = tabData;
         this.updateFilteredTabs_();
-        return;
+        foundTab = true;
       }
     }
 
     // If the updated tab's id is not found in the existing open tabs, add it
     // to the list.
-    this.openTabs_.push(tabData);
-    this.updateFilteredTabs_();
+    if (!foundTab) {
+      this.openTabs_.push(tabData);
+      this.updateFilteredTabs_();
+    }
+
+    if (this.useMetricsReporter_) {
+      this.metricsReporter.measure('TabUpdated')
+          .then(
+              e => this.metricsReporter.umaReportTime(
+                  'Tabs.TabSearch.Mojo.TabUpdated', e))
+          .then(() => this.metricsReporter.clearMark('TabUpdated'));
+    }
   }
 
   private onTabsRemoved_(tabsRemovedInfo: TabsRemovedInfo) {
@@ -398,6 +442,16 @@ export class TabSearchAppElement extends PolymerElement {
     let action;
     switch (itemData.type) {
       case TabItemType.OPEN_TAB:
+        if (this.useMetricsReporter_) {
+          const isMarkOverlap =
+              this.metricsReporter.hasLocalMark('SwitchToTab');
+          chrome.metricsPrivate.recordBoolean(
+              'Tabs.TabSearch.Mojo.SwitchToTab.IsOverlap', isMarkOverlap);
+          if (!isMarkOverlap) {
+            this.metricsReporter.mark('SwitchToTab');
+          }
+        }
+
         const isMediaTab = tabHasMediaAlerts((itemData as TabData).tab as Tab);
         const tabIndexRelativeToSection =
             isMediaTab ? tabIndex : tabIndex - this.filteredMediaTabsCount_;

diff --git a/chrome/browser/ui/ui_features.cc b/chrome/browser/ui/ui_features.cc
@@ -219,6 +219,9 @@ const base::FeatureParam<int> kTabSearchRecentlyClosedDefaultItemDisplayCount{
 const base::FeatureParam<int> kTabSearchRecentlyClosedTabCountThreshold{
     &kTabSearchRecentlyClosed, "TabSearchRecentlyClosedTabCountThreshold", 100};
 
+const base::Feature kTabSearchUseMetricsReporter{
+    "TabSearchUseMetricsReporter", base::FEATURE_DISABLED_BY_DEFAULT};
+
 const base::Feature kToolbarUseHardwareBitmapDraw{
     "ToolbarUseHardwareBitmapDraw", base::FEATURE_DISABLED_BY_DEFAULT};
 

diff --git a/chrome/browser/ui/ui_features.h b/chrome/browser/ui/ui_features.h
@@ -165,6 +165,8 @@ extern const base::FeatureParam<int>
 // count have been met.
 extern const base::FeatureParam<int> kTabSearchRecentlyClosedTabCountThreshold;
 
+extern const base::Feature kTabSearchUseMetricsReporter;
+
 // Determines how screenshots of the toolbar uses Software or Hardware drawing.
 // Works on Android 10+.
 extern const base::Feature kToolbarUseHardwareBitmapDraw;

diff --git a/chrome/browser/ui/webui/tab_search/tab_search_page_handler.cc b/chrome/browser/ui/webui/tab_search/tab_search_page_handler.cc
@@ -34,6 +34,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "chrome/browser/ui/tabs/tab_utils.h"
 #include "chrome/browser/ui/ui_features.h"
+#include "chrome/browser/ui/webui/metrics_reporter/metrics_reporter.h"
 #include "chrome/browser/ui/webui/tab_search/tab_search_prefs.h"
 #include "chrome/browser/ui/webui/util/image_util.h"
 #include "chrome/common/webui_url_constants.h"
@@ -84,11 +85,13 @@ TabSearchPageHandler::TabSearchPageHandler(
     mojo::PendingReceiver<tab_search::mojom::PageHandler> receiver,
     mojo::PendingRemote<tab_search::mojom::Page> page,
     content::WebUI* web_ui,
-    ui::MojoBubbleWebUIController* webui_controller)
+    ui::MojoBubbleWebUIController* webui_controller,
+    MetricsReporter* metrics_reporter)
     : receiver_(this, std::move(receiver)),
       page_(std::move(page)),
       web_ui_(web_ui),
       webui_controller_(webui_controller),
+      metrics_reporter_(metrics_reporter),
       debounce_timer_(std::make_unique<base::RetainingOneShotTimer>(
           FROM_HERE,
           kTabsChangeDelay,
@@ -190,6 +193,17 @@ void TabSearchPageHandler::SwitchToTab(
   const TabDetails& details = optional_details.value();
   details.tab_strip_model->ActivateTabAt(details.index);
   details.browser->window()->Activate();
+  if (base::FeatureList::IsEnabled(features::kTabSearchUseMetricsReporter)) {
+    metrics_reporter_->Measure(
+        "SwitchToTab",
+        base::BindOnce(
+            [](MetricsReporter* metrics_reporter, base::TimeDelta duration) {
+              base::UmaHistogramTimes("Tabs.TabSearch.Mojo.SwitchToTab",
+                                      duration);
+              metrics_reporter->ClearMark("SwitchToTab");
+            },
+            metrics_reporter_));
+  }
 }
 
 void TabSearchPageHandler::OpenRecentlyClosedEntry(int32_t session_id) {
@@ -556,6 +570,14 @@ void TabSearchPageHandler::TabChangedAt(content::WebContents* contents,
   Browser* active_browser = chrome::FindLastActive();
   TRACE_EVENT0("browser", "TabSearchPageHandler:TabChangedAt");
 
+  if (base::FeatureList::IsEnabled(features::kTabSearchUseMetricsReporter)) {
+    bool is_mark_overlap = metrics_reporter_->HasLocalMark("TabUpdated");
+    base::UmaHistogramBoolean("Tabs.TabSearch.Mojo.TabUpdated.IsOverlap",
+                              is_mark_overlap);
+    if (!is_mark_overlap)
+      metrics_reporter_->Mark("TabUpdated");
+  }
+
   auto tab_update_info = tab_search::mojom::TabUpdateInfo::New();
   tab_update_info->in_active_window = (browser == active_browser);
   tab_update_info->tab = GetTab(browser->tab_strip_model(), contents, index);

diff --git a/chrome/browser/ui/webui/tab_search/tab_search_page_handler.h b/chrome/browser/ui/webui/tab_search/tab_search_page_handler.h
@@ -23,6 +23,7 @@
 #include "url/gurl.h"
 
 class Browser;
+class MetricsReporter;
 
 // These values are persisted to logs. Entries should not be renumbered and
 // numeric values should never be reused.
@@ -48,7 +49,8 @@ class TabSearchPageHandler : public tab_search::mojom::PageHandler,
       mojo::PendingReceiver<tab_search::mojom::PageHandler> receiver,
       mojo::PendingRemote<tab_search::mojom::Page> page,
       content::WebUI* web_ui,
-      ui::MojoBubbleWebUIController* webui_controller);
+      ui::MojoBubbleWebUIController* webui_controller,
+      MetricsReporter* metrics_reporter);
   TabSearchPageHandler(const TabSearchPageHandler&) = delete;
   TabSearchPageHandler& operator=(const TabSearchPageHandler&) = delete;
   ~TabSearchPageHandler() override;
@@ -142,6 +144,7 @@ class TabSearchPageHandler : public tab_search::mojom::PageHandler,
   mojo::Remote<tab_search::mojom::Page> page_;
   const raw_ptr<content::WebUI> web_ui_;
   const raw_ptr<ui::MojoBubbleWebUIController> webui_controller_;
+  const raw_ptr<MetricsReporter> metrics_reporter_;
   BrowserTabStripTracker browser_tab_strip_tracker_{this, this};
   std::unique_ptr<base::RetainingOneShotTimer> debounce_timer_;
 

diff --git a/chrome/browser/ui/webui/tab_search/tab_search_page_handler_unittest.cc b/chrome/browser/ui/webui/tab_search/tab_search_page_handler_unittest.cc
@@ -17,6 +17,8 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_utils.h"
 #include "chrome/browser/ui/ui_features.h"
+#include "chrome/browser/ui/webui/metrics_reporter/metrics_reporter.h"
+#include "chrome/browser/ui/webui/metrics_reporter/mock_metrics_reporter.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/test_browser_window.h"
 #include "chrome/test/base/testing_profile_manager.h"
@@ -108,7 +110,8 @@ class TestTabSearchPageHandler : public TabSearchPageHandler {
             mojo::PendingReceiver<tab_search::mojom::PageHandler>(),
             std::move(page),
             web_ui,
-            webui_controller) {
+            webui_controller,
+            &metrics_reporter_) {
     mock_debounce_timer_ = new base::MockRetainingOneShotTimer();
     SetTimerForTesting(base::WrapUnique(mock_debounce_timer_.get()));
   }
@@ -118,6 +121,7 @@ class TestTabSearchPageHandler : public TabSearchPageHandler {
 
  private:
   raw_ptr<base::MockRetainingOneShotTimer> mock_debounce_timer_;
+  testing::NiceMock<MockMetricsReporter> metrics_reporter_;
 };
 
 class TabSearchPageHandlerTest : public BrowserWithTestWindowTest {

diff --git a/chrome/browser/ui/webui/tab_search/tab_search_ui.cc b/chrome/browser/ui/webui/tab_search/tab_search_ui.cc
@@ -65,6 +65,11 @@ TabSearchUI::TabSearchUI(content::WebUI* web_ui)
   // Add the configuration parameters for fuzzy search.
   source->AddBoolean("useFuzzySearch", base::FeatureList::IsEnabled(
                                            features::kTabSearchFuzzySearch));
+
+  source->AddBoolean(
+      "useMetricsReporter",
+      base::FeatureList::IsEnabled(features::kTabSearchUseMetricsReporter));
+
   source->AddBoolean(
       "alsoShowMediaTabsinOpenTabsSection",
       GetFieldTrialParamByFeatureAsBool(
@@ -124,6 +129,11 @@ void TabSearchUI::BindInterface(
   page_factory_receiver_.Bind(std::move(receiver));
 }
 
+void TabSearchUI::BindInterface(
+    mojo::PendingReceiver<metrics_reporter::mojom::PageMetricsHost> receiver) {
+  metrics_reporter_.BindInterface(std::move(receiver));
+}
+
 void TabSearchUI::CreatePageHandler(
     mojo::PendingRemote<tab_search::mojom::Page> page,
     mojo::PendingReceiver<tab_search::mojom::PageHandler> receiver) {
@@ -144,5 +154,5 @@ void TabSearchUI::CreatePageHandler(
   // TODO(tluk): Investigate whether we can avoid recreating this multiple times
   // per instance of the TabSearchUI.
   page_handler_ = std::make_unique<TabSearchPageHandler>(
-      std::move(receiver), std::move(page), web_ui(), this);
+      std::move(receiver), std::move(page), web_ui(), this, &metrics_reporter_);
 }
diff --git a/chrome/browser/ui/webui/tab_search/tab_search_ui.h b/chrome/browser/ui/webui/tab_search/tab_search_ui.h
@@ -8,13 +8,15 @@
 #include <memory>
 
 #include "base/timer/elapsed_timer.h"
+#include "chrome/browser/ui/webui/metrics_reporter/metrics_reporter.h"
 #include "chrome/browser/ui/webui/tab_search/tab_search.mojom.h"
 #include "chrome/browser/ui/webui/tab_search/tab_search_page_handler.h"
 #include "chrome/browser/ui/webui/webui_load_timer.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "ui/webui/mojo_bubble_web_ui_controller.h"
+#include "ui/webui/resources/js/metrics_reporter/metrics_reporter.mojom.h"
 
 class TabSearchUI : public ui::MojoBubbleWebUIController,
                     public tab_search::mojom::PageHandlerFactory {
@@ -28,6 +30,8 @@ class TabSearchUI : public ui::MojoBubbleWebUIController,
   // interface passing the pending receiver that will be internally bound.
   void BindInterface(
       mojo::PendingReceiver<tab_search::mojom::PageHandlerFactory> receiver);
+  void BindInterface(
+      mojo::PendingReceiver<metrics_reporter::mojom::PageMetricsHost> receiver);
 
   TabSearchPageHandler* page_handler_for_testing() {
     return page_handler_.get();
@@ -40,6 +44,7 @@ class TabSearchUI : public ui::MojoBubbleWebUIController,
       mojo::PendingReceiver<tab_search::mojom::PageHandler> receiver) override;
 
   std::unique_ptr<TabSearchPageHandler> page_handler_;
+  MetricsReporter metrics_reporter_;
 
   mojo::Receiver<tab_search::mojom::PageHandlerFactory> page_factory_receiver_{
       this};

diff --git a/chrome/test/data/webui/tab_search/BUILD.gn b/chrome/test/data/webui/tab_search/BUILD.gn
@@ -25,6 +25,10 @@ ts_library("build_ts") {
     "chrome://webui-test/*|" +
         rebase_path("$root_gen_dir/chrome/test/data/webui/tsc/*",
                     target_gen_dir),
+    "chrome://webui-test/metrics_reporter/*|" +
+        rebase_path(
+            "$root_gen_dir/chrome/test/data/webui/metrics_reporter/tsc/*",
+            target_gen_dir),
   ]
   in_files = [
     "bimap_test.ts",
@@ -37,6 +41,9 @@ ts_library("build_ts") {
     "tab_search_test_helper.ts",
     "test_tab_search_api_proxy.ts",
   ]
-  deps = [ "//chrome/browser/resources/tab_search:build_ts" ]
+  deps = [
+    "../metrics_reporter:build_ts",
+    "//chrome/browser/resources/tab_search:build_ts",
+  ]
   extra_deps = [ "..:generate_definitions" ]
 }
diff --git a/chrome/test/data/webui/tab_search/tab_search_app_test.ts b/chrome/test/data/webui/tab_search/tab_search_app_test.ts
@@ -4,9 +4,11 @@
 
 import 'chrome://webui-test/mojo_webui_test_support.js';
 
+import {MetricsReporterImpl} from 'chrome://resources/js/metrics_reporter/metrics_reporter.js';
 import {keyDownOn} from 'chrome://resources/polymer/v3_0/iron-test-helpers/mock-interactions.js';
 import {ProfileData, RecentlyClosedTab, Tab, TabAlertState, TabGroupColor, TabSearchApiProxyImpl, TabSearchAppElement, TabSearchItem} from 'chrome://tab-search.top-chrome/tab_search.js';
 import {assertEquals, assertFalse, assertNotEquals, assertTrue} from 'chrome://webui-test/chai_assert.js';
+import {MockedMetricsReporter} from 'chrome://webui-test/metrics_reporter/mocked_metrics_reporter.js';
 import {flushTasks, waitAfterNextRender} from 'chrome://webui-test/test_util.js';
 
 import {createProfileData, createTab, generateSampleDataFromSiteNames, generateSampleRecentlyClosedTabs, generateSampleRecentlyClosedTabsFromSiteNames, generateSampleTabsFromSiteNames, SAMPLE_RECENTLY_CLOSED_DATA, SAMPLE_WINDOW_DATA, SAMPLE_WINDOW_DATA_WITH_MEDIA_TAB, SAMPLE_WINDOW_HEIGHT, sampleToken} from './tab_search_test_data.js';
@@ -42,6 +44,8 @@ suite('TabSearchAppTest', () => {
       loadTimeOverriddenData?: {[key: string]: number|string|boolean}) {
     initLoadTimeDataWithDefaults(loadTimeOverriddenData);
 
+    MetricsReporterImpl.setInstanceForTest(new MockedMetricsReporter());
+
     testProxy = new TestTabSearchApiProxy();
     testProxy.setProfileData(sampleData);
     TabSearchApiProxyImpl.setInstance(testProxy);