Expose jank_monitor.h and CreateAccessibilityMetricsProvider.

These interfaces were hidden within //content but depended upon from
//chrome/browser. This clarifies the layering issues present but does
not completely fix them (see additions to DEPS). It does also resolve
`gn check` errors in //chrome/browser.

Specifically, profile_provider_chromeos.h includes jank_monitor.h
(holds a pointer and is an observer) and
chrome_metrics_service_client.cc includes
accessibility_metrics_provider.h (only to create one).
Let me know if there's a better way to layer this.

Split out from crrev.com/c/2383400

Bug: 898837
Change-Id: I4786b903e52164841d91b431d3bcd2cabc2df54e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2383515
Commit-Queue: Glen Robertson <glenrob@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803751}

chrome/browser/BUILD.gn

@@ -4095,6 +4095,10 @@ static_library("browser") {
   } else {  # Non - ChromeOS.
     sources += [
       "fullscreen.h",
+
+      # ChromeOS accessibility metrics provider is in chromeos_metrics_provider.
+      "metrics/accessibility_metrics_provider.cc",
+      "metrics/accessibility_metrics_provider.h",
       "policy/browser_signin_policy_handler.cc",
       "policy/browser_signin_policy_handler.h",
       "policy/cloud/user_cloud_policy_manager_builder.cc",

chrome/browser/metrics/DEPS

 include_rules = [
   "+chrome/services/util_win/util_win_impl.h",
-  "+content/browser/accessibility/accessibility_metrics_provider.h",
 ]

content/browser/accessibility/accessibility_metrics_provider.cc → chrome/browser/metrics/accessibility_metrics_provider.cc

@@ -2,9 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/accessibility/accessibility_metrics_provider.h"
+#include "chrome/browser/metrics/accessibility_metrics_provider.h"
 
-#include "content/browser/accessibility/browser_accessibility_state_impl.h"
+#include "content/public/browser/browser_accessibility_state.h"
 
 AccessibilityMetricsProvider::AccessibilityMetricsProvider() {}
 
@@ -12,6 +12,6 @@ AccessibilityMetricsProvider::~AccessibilityMetricsProvider() {}
 
 void AccessibilityMetricsProvider::ProvideCurrentSessionData(
     metrics::ChromeUserMetricsExtension* uma_proto) {
-  content::BrowserAccessibilityStateImpl::GetInstance()
+  content::BrowserAccessibilityState::GetInstance()
       ->UpdateUniqueUserHistograms();
 }

content/browser/accessibility/accessibility_metrics_provider.h → chrome/browser/metrics/accessibility_metrics_provider.h

@@ -2,11 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CONTENT_BROWSER_ACCESSIBILITY_ACCESSIBILITY_METRICS_PROVIDER_H_
-#define CONTENT_BROWSER_ACCESSIBILITY_ACCESSIBILITY_METRICS_PROVIDER_H_
+#ifndef CHROME_BROWSER_METRICS_ACCESSIBILITY_METRICS_PROVIDER_H_
+#define CHROME_BROWSER_METRICS_ACCESSIBILITY_METRICS_PROVIDER_H_
 
 #include "components/metrics/metrics_provider.h"
-#include "content/common/content_export.h"
 
 ////////////////////////////////////////////////////////////////////////////////
 //
@@ -16,8 +15,7 @@
 // histograms on Win, Mac and Android, enable accurate counting of unique users.
 //
 ////////////////////////////////////////////////////////////////////////////////
-class CONTENT_EXPORT AccessibilityMetricsProvider
-    : public metrics::MetricsProvider {
+class AccessibilityMetricsProvider : public metrics::MetricsProvider {
  public:
   AccessibilityMetricsProvider();
   ~AccessibilityMetricsProvider() override;
@@ -30,4 +28,4 @@ class CONTENT_EXPORT AccessibilityMetricsProvider
   DISALLOW_COPY_AND_ASSIGN(AccessibilityMetricsProvider);
 };
 
-#endif  // CONTENT_BROWSER_ACCESSIBILITY_ACCESSIBILITY_METRICS_PROVIDER_H_
+#endif  // CHROME_BROWSER_METRICS_ACCESSIBILITY_METRICS_PROVIDER_H_

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -101,7 +101,6 @@
 #include "components/ukm/field_trials_provider_helper.h"
 #include "components/ukm/ukm_service.h"
 #include "components/version_info/version_info.h"
-#include "content/browser/accessibility/accessibility_metrics_provider.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/histogram_fetcher.h"
@@ -170,6 +169,7 @@
 #endif
 
 #if !defined(OS_CHROMEOS)
+#include "chrome/browser/metrics/accessibility_metrics_provider.h"
 #include "chrome/browser/signin/chrome_signin_status_metrics_provider_delegate.h"
 #include "components/signin/core/browser/signin_status_metrics_provider.h"
 #endif  // !defined(OS_CHROMEOS)

chrome/browser/metrics/perf/profile_provider_chromeos.cc

@@ -96,8 +96,7 @@ void ProfileProvider::Init() {
 
   if (base::FeatureList::IsEnabled(kBrowserJankinessProfiling)) {
     // Set up the JankMonitor for watching browser jankiness.
-    jank_monitor_ =
-        base::MakeRefCounted<content::responsiveness::JankMonitor>();
+    jank_monitor_ = content::JankMonitor::Create();
     jank_monitor_->SetUp();
     jank_monitor_->AddObserver(this);
   }

chrome/browser/metrics/perf/profile_provider_chromeos.h

@@ -12,7 +12,7 @@
 #include "chrome/browser/sessions/session_restore.h"
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/login/login_state/login_state.h"
-#include "content/browser/scheduler/responsiveness/jank_monitor.h"
+#include "content/public/browser/jank_monitor.h"
 
 namespace metrics {
 
@@ -24,7 +24,7 @@ class SampledProfile;
 // mode, or user logging in, which it forwards to the registered collectors.
 class ProfileProvider : public chromeos::PowerManagerClient::Observer,
                         public chromeos::LoginState::Observer,
-                        public content::responsiveness::JankMonitor::Observer {
+                        public content::JankMonitor::Observer {
  public:
   ProfileProvider();
   ~ProfileProvider() override;
@@ -55,7 +55,7 @@ class ProfileProvider : public chromeos::PowerManagerClient::Observer,
   void OnJankStopped() override;
 
   // For testing.
-  scoped_refptr<content::responsiveness::JankMonitor> jank_monitor() const {
+  scoped_refptr<content::JankMonitor> jank_monitor() const {
     return jank_monitor_;
   }
   // For testing.
@@ -75,7 +75,7 @@ class ProfileProvider : public chromeos::PowerManagerClient::Observer,
   SessionRestore::CallbackSubscription
       on_session_restored_callback_subscription_;
 
-  scoped_refptr<content::responsiveness::JankMonitor> jank_monitor_;
+  scoped_refptr<content::JankMonitor> jank_monitor_;
 
   // Timestamp of the most recent jank observed.
   base::TimeTicks last_jank_start_time_;

content/browser/BUILD.gn

@@ -1667,8 +1667,8 @@ source_set("browser") {
     "scheduler/browser_ui_thread_scheduler.h",
     "scheduler/responsiveness/calculator.cc",
     "scheduler/responsiveness/calculator.h",
-    "scheduler/responsiveness/jank_monitor.cc",
-    "scheduler/responsiveness/jank_monitor.h",
+    "scheduler/responsiveness/jank_monitor_impl.cc",
+    "scheduler/responsiveness/jank_monitor_impl.h",
     "scheduler/responsiveness/message_loop_observer.cc",
     "scheduler/responsiveness/message_loop_observer.h",
     "scheduler/responsiveness/metric_source.cc",
@@ -2684,12 +2684,6 @@ source_set("browser") {
       "//chromeos/system",
       "//components/session_manager/core",
     ]
-  } else {
-    sources += [
-      # ChromeOS accessibility metrics provider is in chromeos_metrics_provider.
-      "accessibility/accessibility_metrics_provider.cc",
-      "accessibility/accessibility_metrics_provider.h",
-    ]
   }
 
   if (use_aura) {

content/browser/accessibility/browser_accessibility_state_impl.h

@@ -60,6 +60,7 @@ class CONTENT_EXPORT BrowserAccessibilityStateImpl
   bool IsAccessibleBrowser() override;
   void AddUIThreadHistogramCallback(base::OnceClosure callback) override;
   void AddOtherThreadHistogramCallback(base::OnceClosure callback) override;
+  void UpdateUniqueUserHistograms() override;
   void UpdateHistogramsForTesting() override;
   void SetCaretBrowsingState(bool enabled) override;
 
@@ -70,11 +71,6 @@ class CONTENT_EXPORT BrowserAccessibilityStateImpl
   // AXModeObserver
   void OnAXModeAdded(ui::AXMode mode) override;
 
-  // Fire frequent metrics signals to ensure users keeping browser open multiple
-  // days are counted each day, not only at launch. This is necessary, because
-  // UMA only aggregates uniques on a daily basis,
-  void UpdateUniqueUserHistograms();
-
   // Accessibility objects can have the "hot tracked" state set when
   // the mouse is hovering over them, but this makes tests flaky because
   // the test behaves differently when the mouse happens to be over an

content/browser/scheduler/responsiveness/jank_monitor.h → content/browser/scheduler/responsiveness/jank_monitor_impl.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_H_
-#define CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_H_
+#ifndef CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_IMPL_H_
+#define CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_IMPL_H_
 
 #include <atomic>
 
@@ -17,71 +17,24 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "content/browser/scheduler/responsiveness/metric_source.h"
+#include "content/public/browser/jank_monitor.h"
 
 namespace content {
 namespace responsiveness {
-// This class monitors the responsiveness of the browser to notify the presence
-// of janks to its observers. A jank is defined as a task or native event
-// running for longer than a threshold on the UI or IO thread. An observer of
-// this class is notified through the Observer interface on jank starts/stops so
-// the observer can take actions (e.g. gather system-wide profile to capture the
-// jank) *before* the janky task finishes execution. Notifications are sent on a
-// dedicated sequence internal to this class so the observer needs to be careful
-// with threading. For example, access to browser-related objects requires
-// posting a task to the UI thread.
-//
-// Internally, a timer (bound to the monitor sequence) is used to perform
-// periodic checks to decide the presence of janks. When a jank is detected, the
-// monitor notifies its observers that a jank has started (through the
-// Observer::OnJankStarted() method). The start of a jank is imprecise w.r.t.
-// the jank threshold. When a janky task has finished execution, the monitor
-// notifies the observers ASAP (through the Observer::OnJankStopped() method).
-//
-// Usage example:
-//
-// class Profiler : public Observer {
-//  public:
-//   void OnJankStarted() override; // Start the profiler.
-//   void OnJankStopped() override; // Stop the profiler.
-// }
-// Profiler* profiler = ...;
-//
-// scoped_refptr<JankMonitor> monitor = base::MakeRefCounted<JankMonitor>();
-// monitor->SetUp();
-// monitor->AddObserver(profiler);
-//
-// (Then start receiving notifications in Profiler::OnJankStarted() and
-// Profiler::OnJankStopped()).
-class CONTENT_EXPORT JankMonitor
-    : public base::RefCountedThreadSafe<JankMonitor>,
-      public content::responsiveness::MetricSource::Delegate {
- public:
-  // Interface for observing janky tasks from the monitor. Note that the
-  // callbacks are called *off* the UI thread. Post a task to the UI thread is
-  // necessary if you need to access browser-related objects.
-  class CONTENT_EXPORT Observer {
-   public:
-    virtual ~Observer();
-
-    virtual void OnJankStarted() = 0;
-    virtual void OnJankStopped() = 0;
-  };
-
-  JankMonitor();
 
-  void SetUp();
-  void Destroy();
+class CONTENT_EXPORT JankMonitorImpl : public content::JankMonitor,
+                                       public MetricSource::Delegate {
+ public:
+  JankMonitorImpl();
 
-  // AddObserver() and RemoveObserver() can be called on any sequence, but the
-  // notifications only take place on the monitor sequence. Note: do *not* call
-  // AddObserver() or RemoveObserver() synchronously in the observer callbacks,
-  // or undefined behavior will result.
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  // JankMonitor implementation:
+  void AddObserver(content::JankMonitor::Observer* observer) override;
+  void RemoveObserver(content::JankMonitor::Observer* observer) override;
+  void SetUp() override;
+  void Destroy() override;
 
  protected:
-  friend class base::RefCountedThreadSafe<JankMonitor>;
-  ~JankMonitor() override;
+  ~JankMonitorImpl() override;
 
   // MetricSource::Delegate implementation.
   void SetUpOnIOThread() override;
@@ -206,4 +159,4 @@ class CONTENT_EXPORT JankMonitor
 }  // namespace responsiveness.
 }  // namespace content.
 
-#endif  // CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_H_
+#endif  // CONTENT_BROWSER_SCHEDULER_RESPONSIVENESS_JANK_MONITOR_IMPL_H_

content/browser/scheduler/responsiveness/jank_monitor_unittest.cc → content/browser/scheduler/responsiveness/jank_monitor_impl_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/scheduler/responsiveness/jank_monitor.h"
+#include "content/browser/scheduler/responsiveness/jank_monitor_impl.h"
 
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -33,8 +33,7 @@ class TestObserver : public JankMonitor::Observer {
 
 class TestMetricSource : public MetricSource {
  public:
-  TestMetricSource(Delegate* delegate)
-      : MetricSource(delegate) {}
+  explicit TestMetricSource(Delegate* delegate) : MetricSource(delegate) {}
   ~TestMetricSource() override {}
 
   std::unique_ptr<NativeEventObserver> CreateNativeEventObserver() override {
@@ -42,7 +41,7 @@ class TestMetricSource : public MetricSource {
   }
 };
 
-class TestJankMonitor : public JankMonitor {
+class TestJankMonitor : public JankMonitorImpl {
  public:
   TestJankMonitor() {}
 
@@ -54,7 +53,7 @@ class TestJankMonitor : public JankMonitor {
     on_destroyed_ = std::move(on_destroyed);
   }
 
-  using JankMonitor::timer_running;
+  using JankMonitorImpl::timer_running;
 
  protected:
   ~TestJankMonitor() override {
@@ -68,7 +67,7 @@ class TestJankMonitor : public JankMonitor {
 
   void DestroyOnMonitorThread() override {
     destroy_on_monitor_thread_called_ = true;
-    JankMonitor::DestroyOnMonitorThread();
+    JankMonitorImpl::DestroyOnMonitorThread();
   }
 
  private:
@@ -230,7 +229,7 @@ TEST_F(JankMonitorTest, JankUIThreadReentrant) {
 TEST_F(JankMonitorTest, ReentrantResponsive) {
   auto enclosing_task = [&]() {
     // Run 5 responsive tasks in the inner runloop.
-    for (int i  = 0; i < 5; i++) {
+    for (int i = 0; i < 5; i++) {
       auto nested_responsive_task = [&]() {
         task_environment_.FastForwardBy(base::TimeDelta::FromMilliseconds(999));
 
@@ -357,14 +356,14 @@ TEST_F(JankMonitorTest, StartStopTimer) {
   EXPECT_FALSE(monitor_->timer_running());
 }
 
-class TestJankMonitorShutdownRace : public JankMonitor {
+class TestJankMonitorShutdownRace : public JankMonitorImpl {
  public:
   TestJankMonitorShutdownRace(base::WaitableEvent* shutdown_on_monitor_thread,
                               base::WaitableEvent* shutdown_on_ui_thread)
       : shutdown_on_monitor_thread_(shutdown_on_monitor_thread),
         shutdown_on_ui_thread_(shutdown_on_ui_thread) {}
 
-  using JankMonitor::timer_running;
+  using JankMonitorImpl::timer_running;
 
  protected:
   ~TestJankMonitorShutdownRace() override = default;
@@ -374,7 +373,7 @@ class TestJankMonitorShutdownRace : public JankMonitor {
   }
 
   void DestroyOnMonitorThread() override {
-    JankMonitor::DestroyOnMonitorThread();
+    JankMonitorImpl::DestroyOnMonitorThread();
 
     // Posts a task to the UI thread. Note that we run concurrently with the
     // destruction of MetricSource. Even if MetricSource is still active and
@@ -387,7 +386,7 @@ class TestJankMonitorShutdownRace : public JankMonitor {
   }
 
   void FinishDestroyMetricSource() override {
-    JankMonitor::FinishDestroyMetricSource();
+    JankMonitorImpl::FinishDestroyMetricSource();
 
     shutdown_on_ui_thread_->Signal();
   }
@@ -429,7 +428,7 @@ TEST(JankMonitorShutdownTest, ShutdownRace_TimerRestarted) {
   EXPECT_FALSE(jank_monitor->timer_running());
 }
 
-class TestJankMonitorShutdownRaceTimerFired : public JankMonitor {
+class TestJankMonitorShutdownRaceTimerFired : public JankMonitorImpl {
  public:
   TestJankMonitorShutdownRaceTimerFired(
       content::BrowserTaskEnvironment* task_environment)
@@ -449,11 +448,11 @@ class TestJankMonitorShutdownRaceTimerFired : public JankMonitor {
     // after MetricSource is destroyed.
     task_environment_->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
 
-    JankMonitor::FinishDestroyMetricSource();
+    JankMonitorImpl::FinishDestroyMetricSource();
   }
 
   void OnCheckJankiness() override {
-    JankMonitor::OnCheckJankiness();
+    JankMonitorImpl::OnCheckJankiness();
     monitor_timer_fired_ = true;
   }
 

content/public/browser/BUILD.gn

@@ -193,6 +193,7 @@ source_set("browser_sources") {
     "installed_payment_apps_finder.h",
     "invalidate_type.h",
     "is_uvpaa.h",
+    "jank_monitor.h",
     "javascript_dialog_manager.cc",
     "javascript_dialog_manager.h",
     "keyboard_event_processing_result.h",

content/public/browser/browser_accessibility_state.h

@@ -65,6 +65,11 @@ class CONTENT_EXPORT BrowserAccessibilityState {
   // thread, for example something that may block or run slowly.
   virtual void AddOtherThreadHistogramCallback(base::OnceClosure callback) = 0;
 
+  // Fire frequent metrics signals to ensure users keeping browser open multiple
+  // days are counted each day, not only at launch. This is necessary, because
+  // UMA only aggregates uniques on a daily basis,
+  virtual void UpdateUniqueUserHistograms() = 0;
+
   virtual void UpdateHistogramsForTesting() = 0;
 
   // Update BrowserAccessibilityState with the current status of caret browsing.

content/public/browser/jank_monitor.h

+// 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.
+
+#ifndef CONTENT_PUBLIC_BROWSER_JANK_MONITOR_H_
+#define CONTENT_PUBLIC_BROWSER_JANK_MONITOR_H_
+
+#include "base/memory/ref_counted.h"
+#include "content/common/content_export.h"
+
+namespace content {
+// This class monitors the responsiveness of the browser to notify the presence
+// of janks to its observers. A jank is defined as a task or native event
+// running for longer than a threshold on the UI or IO thread. An observer of
+// this class is notified through the Observer interface on jank starts/stops so
+// the observer can take actions (e.g. gather system-wide profile to capture the
+// jank) *before* the janky task finishes execution. Notifications are sent on a
+// dedicated sequence internal to this class so the observer needs to be careful
+// with threading. For example, access to browser-related objects requires
+// posting a task to the UI thread.
+//
+// Internally, a timer (bound to the monitor sequence) is used to perform
+// periodic checks to decide the presence of janks. When a jank is detected, the
+// monitor notifies its observers that a jank has started (through the
+// Observer::OnJankStarted() method). The start of a jank is imprecise w.r.t.
+// the jank threshold. When a janky task has finished execution, the monitor
+// notifies the observers ASAP (through the Observer::OnJankStopped() method).
+//
+// Usage example:
+//
+// class Profiler : public Observer {
+//  public:
+//   void OnJankStarted() override; // Start the profiler.
+//   void OnJankStopped() override; // Stop the profiler.
+// }
+// Profiler* profiler = ...;
+//
+// scoped_refptr<JankMonitor> monitor = JankMonitor::Create();
+// monitor->SetUp();
+// monitor->AddObserver(profiler);
+//
+// (Then start receiving notifications in Profiler::OnJankStarted() and
+// Profiler::OnJankStopped()).
+class CONTENT_EXPORT JankMonitor
+    : public base::RefCountedThreadSafe<JankMonitor> {
+ public:
+  // Interface for observing janky tasks from the monitor. Note that the
+  // callbacks are called *off* the UI thread. Post a task to the UI thread is
+  // necessary if you need to access browser-related objects.
+  class CONTENT_EXPORT Observer {
+   public:
+    virtual ~Observer();
+
+    virtual void OnJankStarted() = 0;
+    virtual void OnJankStopped() = 0;
+  };
+
+  static scoped_refptr<JankMonitor> Create();
+
+  // AddObserver() and RemoveObserver() can be called on any sequence, but the
+  // notifications only take place on the monitor sequence. Note: do *not* call
+  // AddObserver() or RemoveObserver() synchronously in the observer callbacks,
+  // or undefined behavior will result.
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
+  virtual void SetUp() = 0;
+  virtual void Destroy() = 0;
+
+ protected:
+  friend class base::RefCountedThreadSafe<JankMonitor>;
+
+  virtual ~JankMonitor();
+};
+
+}  // namespace content.
+
+#endif  // CONTENT_PUBLIC_BROWSER_JANK_MONITOR_H_

content/test/BUILD.gn

@@ -1913,7 +1913,7 @@ test("content_unittests") {
     "../browser/scheduler/browser_task_queues_unittest.cc",
     "../browser/scheduler/browser_ui_thread_scheduler_unittest.cc",
     "../browser/scheduler/responsiveness/calculator_unittest.cc",
-    "../browser/scheduler/responsiveness/jank_monitor_unittest.cc",
+    "../browser/scheduler/responsiveness/jank_monitor_impl_unittest.cc",
     "../browser/scheduler/responsiveness/metric_source_unittest.cc",
     "../browser/scheduler/responsiveness/watcher_unittest.cc",
     "../browser/screen_orientation/screen_orientation_provider_unittest.cc",