[WebUI] MetricsReporter test support enhancement

* Add HasLocalMark() used for testing the availability of mark locally.
* Add mocked C++ and JS MetricsReporter.
* Allow disabling JS MetricsReporter for test at runtime. This is a
  workaround to the restrictions on TS compilation and runtime resource
  loading.
* Fix an issue that JS getMark() is returning null because of misuse of
  mojo API.

Bug: 1269417
Change-Id: Ia60be3631c09079b4083eaa3d6f3aa646bea886f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3571913
Reviewed-by: Roman Arora <romanarora@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Keren Zhu <kerenzhu@chromium.org>
Reviewed-by: Yuheng Huang <yuhengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002056}

chrome/browser/ui/webui/metrics_reporter/metrics_reporter.cc

@@ -71,6 +71,10 @@ void MetricsReporter::HasMark(const std::string& name,
           std::move(callback)));
 }
 
+bool MetricsReporter::HasLocalMark(const std::string& name) {
+  return marks_.count(name) > 0;
+}
+
 void MetricsReporter::ClearMark(const std::string& name) {
   marks_.erase(name);
   page_->OnClearMark(name);

chrome/browser/ui/webui/metrics_reporter/metrics_reporter.h

@@ -28,13 +28,14 @@ class MetricsReporter : public metrics_reporter::mojom::PageMetricsHost {
   using MeasureCallback = base::OnceCallback<void(base::TimeDelta delta)>;
   using HasMarkCallback = base::OnceCallback<void(bool)>;
 
-  void Mark(const std::string& name);
-  void Measure(const std::string& start_mark, MeasureCallback callback);
-  void Measure(const std::string& start_mark,
-               const std::string& end_mark,
-               MeasureCallback callback);
-  void HasMark(const std::string& name, HasMarkCallback callback);
-  void ClearMark(const std::string& name);
+  virtual void Mark(const std::string& name);
+  virtual void Measure(const std::string& start_mark, MeasureCallback callback);
+  virtual void Measure(const std::string& start_mark,
+                       const std::string& end_mark,
+                       MeasureCallback callback);
+  virtual void HasMark(const std::string& name, HasMarkCallback callback);
+  virtual bool HasLocalMark(const std::string& name);
+  virtual void ClearMark(const std::string& name);
 
   void BindInterface(
       mojo::PendingReceiver<metrics_reporter::mojom::PageMetricsHost> receiver);

chrome/browser/ui/webui/metrics_reporter/metrics_reporter_unittest.cc

@@ -130,6 +130,20 @@ TEST_F(WebUIMetricsReporterTest, HasMark) {
   metrics_reporter_.HasMark("remote_mark", TestHasMarkCallback(true));
 }
 
+// HasLocalMark() should check only local marks.
+TEST_F(WebUIMetricsReporterTest, HasLocalMark) {
+  EXPECT_FALSE(metrics_reporter_.HasLocalMark("local_mark"));
+  metrics_reporter_.Mark("local_mark");
+  EXPECT_TRUE(metrics_reporter_.HasLocalMark("local_mark"));
+
+  ON_CALL(page_metrics_, OnGetMark("remote_mark", _))
+      .WillByDefault([](const std::string& mark,
+                        MetricsReporter::OnGetMarkCallback callback) {
+        std::move(callback).Run(base::TimeTicks::Now().since_origin());
+      });
+  EXPECT_FALSE(metrics_reporter_.HasLocalMark("remote_mark"));
+}
+
 // ClearMark() should clear both local and remote marks.
 TEST_F(WebUIMetricsReporterTest, ClearMark) {
   EXPECT_CALL(page_metrics_, OnClearMark("remote_mark")).Times(1);

chrome/browser/ui/webui/metrics_reporter/mock_metrics_reporter.cc

@@ -0,0 +1,9 @@
+// Copyright 2022 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 "chrome/browser/ui/webui/metrics_reporter/mock_metrics_reporter.h"
+
+MockMetricsReporter::MockMetricsReporter() = default;
+
+MockMetricsReporter::~MockMetricsReporter() = default;

chrome/browser/ui/webui/metrics_reporter/mock_metrics_reporter.h

@@ -0,0 +1,30 @@
+// Copyright 2022 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.
+
+#ifndef CHROME_BROWSER_UI_WEBUI_METRICS_REPORTER_MOCK_METRICS_REPORTER_H_
+#define CHROME_BROWSER_UI_WEBUI_METRICS_REPORTER_MOCK_METRICS_REPORTER_H_
+
+#include "chrome/browser/ui/webui/metrics_reporter/metrics_reporter.h"
+
+#include "testing/gmock/include/gmock/gmock.h"
+
+class MockMetricsReporter : public MetricsReporter {
+ public:
+  MockMetricsReporter();
+  ~MockMetricsReporter() override;
+
+  MOCK_METHOD1(Mark, void(const std::string&));
+  MOCK_METHOD2(Measure,
+               void(const std::string&, MetricsReporter::MeasureCallback));
+  MOCK_METHOD3(Measure,
+               void(const std::string&,
+                    const std::string&,
+                    MetricsReporter::MeasureCallback));
+  MOCK_METHOD2(HasMark,
+               void(const std::string&, MetricsReporter::HasMarkCallback));
+  MOCK_METHOD1(HasLocalMark, bool(const std::string&));
+  MOCK_METHOD1(ClearMark, void(const std::string&));
+};
+
+#endif  // CHROME_BROWSER_UI_WEBUI_METRICS_REPORTER_MOCK_METRICS_REPORTER_H_

chrome/test/BUILD.gn

@@ -6605,6 +6605,8 @@ test("unit_tests") {
       "../browser/ui/webui/managed_ui_handler_unittest.cc",
       "../browser/ui/webui/management/management_ui_handler_unittest.cc",
       "../browser/ui/webui/metrics_reporter/metrics_reporter_unittest.cc",
+      "../browser/ui/webui/metrics_reporter/mock_metrics_reporter.cc",
+      "../browser/ui/webui/metrics_reporter/mock_metrics_reporter.h",
       "../browser/ui/webui/new_tab_page/new_tab_page_handler_unittest.cc",
       "../browser/ui/webui/privacy_sandbox/privacy_sandbox_dialog_handler_unittest.cc",
       "../browser/ui/webui/sanitized_image_source_unittest.cc",

chrome/test/data/webui/metrics_reporter/BUILD.gn

@@ -17,11 +17,15 @@ generate_grd("build_grdp") {
 ts_library("build_ts") {
   root_dir = "."
   out_dir = "$target_gen_dir/tsc"
+  composite = true
   tsconfig_base = "tsconfig_base.json"
   path_mappings = [ "chrome://webui-test/*|" +
                     rebase_path("$root_gen_dir/chrome/test/data/webui/tsc/*",
                                 target_gen_dir) ]
-  in_files = [ "metrics_reporter_test.ts" ]
+  in_files = [
+    "metrics_reporter_test.ts",
+    "mocked_metrics_reporter.ts",
+  ]
   definitions = [ "//tools/typescript/definitions/chrome_timeticks.d.ts" ]
   deps = [ "//ui/webui/resources/js/metrics_reporter:build_ts" ]
   extra_deps = [ "..:generate_definitions" ]

chrome/test/data/webui/metrics_reporter/metrics_reporter_test.ts

@@ -5,7 +5,7 @@
 import 'chrome://webui-test/mojo_webui_test_support.js';
 
 import {BrowserProxyImpl} from 'chrome://resources/js/metrics_reporter/browser_proxy.js';
-import {MetricsReporter} from 'chrome://resources/js/metrics_reporter/metrics_reporter.js';
+import {MetricsReporter, MetricsReporterImpl} from 'chrome://resources/js/metrics_reporter/metrics_reporter.js';
 import {PageMetricsCallbackRouter} from 'chrome://resources/js/metrics_reporter/metrics_reporter.mojom-webui.js';
 import {assertEquals, assertFalse, assertTrue} from 'chrome://webui-test/chai_assert.js';
 import {TestBrowserProxy} from 'chrome://webui-test/test_browser_proxy.js';
@@ -37,7 +37,7 @@ suite('MetricsReporterTest', function() {
     apiProxy.setResultFor('getCallbackRouter', callbackRouter);
     apiProxy.setResultFor('now', now);
     BrowserProxyImpl.setInstance(apiProxy);
-    metricsReporter = new MetricsReporter();
+    metricsReporter = new MetricsReporterImpl();
   });
 
   test('markAndMeasureLoally', async function() {
@@ -67,6 +67,12 @@ suite('MetricsReporterTest', function() {
     assertTrue(await metricsReporter.hasMark('mark'));
   });
 
+  test('hasLocalMark', function() {
+    assertFalse(metricsReporter.hasLocalMark('mark'));
+    metricsReporter.mark('mark');
+    assertTrue(metricsReporter.hasLocalMark('mark'));
+  });
+
   test('hasMarkRemote', async function() {
     apiProxy.setResultFor('getMark', makeMarkPromiseResult(now));
     assertTrue(await metricsReporter.hasMark('mark'));

chrome/test/data/webui/metrics_reporter/mocked_metrics_reporter.ts

@@ -0,0 +1,25 @@
+// Copyright 2022 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.
+
+import {MetricsReporter} from 'chrome://resources/js/metrics_reporter/metrics_reporter.js';
+
+export class MockedMetricsReporter implements MetricsReporter {
+  mark(_name: string): void {}
+
+  measure(_startMark: string, _endMark?: string): Promise<bigint> {
+    return Promise.resolve(0n);
+  }
+
+  hasMark(_name: string): Promise<boolean> {
+    return Promise.resolve(false);
+  }
+
+  hasLocalMark(_name: string): boolean {
+    return false;
+  }
+
+  clearMark(_name: string): void {}
+
+  umaReportTime(_histogram: string, _time: bigint): void {}
+}

ui/webui/BUILD.gn

@@ -25,7 +25,10 @@ static_library("webui") {
     "//services/service_manager/public/cpp",
   ]
 
-  public_deps = [ "//ui/webui/resources/js/browser_command:mojo_bindings" ]
+  public_deps = [
+    "//ui/webui/resources/js/browser_command:mojo_bindings",
+    "//ui/webui/resources/js/metrics_reporter:mojo_bindings",
+  ]
 
   if (!is_android && !is_ios) {
     public_deps += [

ui/webui/resources/js/metrics_reporter/metrics_reporter.ts

@@ -45,23 +45,51 @@ function timeToMojo(mark: bigint): TimeDelta {
  *       metricsReporter.umaReportTime(
  *         metricsReporter.measure('StartMark'));
  *     }
+ *
+ * Caveats:
+ *   1. measure() will assert if the mark is not available. You can use
+ *      the following code to prevent execution from being interrupted.
+ *
+ *     metricsReporter.measure('StartMark').then(duration =>
+ *       metricsReporter.umaReportTime('Your.Histogram', duration))
+ *
+ *   2. measure() will record inaccurate time if a mark is reused for
+ *      overlapping measurements. To prevent this from happening, you
+ *      can use hasLocalMark() to test before calling mark() and use
+ *      clearMark() to erase the mark after measure().
  */
-export class MetricsReporter {
+
+export interface MetricsReporter {
+  mark(name: string): void;
+  measure(startMark: string, endMark?: string): Promise<bigint>;
+  hasMark(name: string): Promise<boolean>;
+  hasLocalMark(name: string): boolean;
+  clearMark(name: string): void;
+  umaReportTime(histogram: string, time: bigint): void;
+}
+
+export class MetricsReporterImpl implements MetricsReporter {
   private marks_: Map<string, bigint> = new Map();
   private browserProxy_: BrowserProxy = BrowserProxyImpl.getInstance();
 
   constructor() {
     const callbackRouter = this.browserProxy_.getCallbackRouter();
-    callbackRouter.onGetMark.addListener((name: string) => {
-      return this.marks_.has(name) ? timeToMojo(this.marks_.get(name)!) : null;
-    });
+    callbackRouter.onGetMark.addListener(
+        (name: string) => ({
+          markedTime:
+              this.marks_.has(name) ? timeToMojo(this.marks_.get(name)!) : null
+        }));
 
     callbackRouter.onClearMark.addListener(
         (name: string) => this.marks_.delete(name));
   }
 
   static getInstance(): MetricsReporter {
-    return instance || (instance = new MetricsReporter());
+    return instance || (instance = new MetricsReporterImpl());
+  }
+
+  static setInstanceForTest(newInstance: MetricsReporter) {
+    instance = newInstance;
   }
 
   mark(name: string) {
@@ -100,8 +128,12 @@ export class MetricsReporter {
     return remoteMark !== null && remoteMark.markedTime !== null;
   }
 
+  hasLocalMark(name: string): boolean {
+    return this.marks_.has(name);
+  }
+
   clearMark(name: string) {
-    assert(this.marks_.delete(name));
+    this.marks_.delete(name);
     this.browserProxy_.clearMark(name);
   }