[GRC]Add global clock ResourceCoordinatorClock.

Previously when we need to simulate clock ticks in tests, we need each
indivisual components like PageCoordinationUnitImpl to carry its own tick
clock, and we have to create helper functions to set the clock properly among
different components. This is fragile and redudant. This patch introduces a
ResourceCoordinatorClock which provides 4 helper methods to manipulate clock in
testing across components.

There's also a recent patch adding timing functions to
chrome/browser/resource_coordinator:
https://chromium-review.googlesource.com/c/chromium/src/+/723674

BUG=775691

Change-Id: I05e5d1fd688aaa35c8d8abcc4707439f00d9c958
Reviewed-on: https://chromium-review.googlesource.com/756211
Commit-Queue: lpy <lpy@chromium.org>
Reviewed-by: Zhen Wang <zhenw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515053}

services/resource_coordinator/BUILD.gn

@@ -45,6 +45,8 @@ source_set("lib") {
     "observers/metrics_collector.h",
     "observers/page_signal_generator_impl.cc",
     "observers/page_signal_generator_impl.h",
+    "resource_coordinator_clock.cc",
+    "resource_coordinator_clock.h",
     "resource_coordinator_service.cc",
     "resource_coordinator_service.h",
   ]

services/resource_coordinator/coordination_unit/page_coordination_unit_impl.cc

@@ -9,14 +9,14 @@
 #include "services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
 #include "services/resource_coordinator/observers/coordination_unit_graph_observer.h"
+#include "services/resource_coordinator/resource_coordinator_clock.h"
 
 namespace resource_coordinator {
 
 PageCoordinationUnitImpl::PageCoordinationUnitImpl(
     const CoordinationUnitID& id,
     std::unique_ptr<service_manager::ServiceContextRef> service_ref)
-    : CoordinationUnitInterface(id, std::move(service_ref)),
-      clock_(new base::DefaultTickClock()) {}
+    : CoordinationUnitInterface(id, std::move(service_ref)) {}
 
 PageCoordinationUnitImpl::~PageCoordinationUnitImpl() {
   for (auto* child_frame : frame_coordination_units_)
@@ -117,22 +117,17 @@ bool PageCoordinationUnitImpl::GetExpectedTaskQueueingDuration(
 base::TimeDelta PageCoordinationUnitImpl::TimeSinceLastNavigation() const {
   if (navigation_committed_time_.is_null())
     return base::TimeDelta();
-  return clock_->NowTicks() - navigation_committed_time_;
+  return ResourceCoordinatorClock::NowTicks() - navigation_committed_time_;
 }
 
 base::TimeDelta PageCoordinationUnitImpl::TimeSinceLastVisibilityChange()
     const {
-  return clock_->NowTicks() - visibility_change_time_;
-}
-
-void PageCoordinationUnitImpl::SetClockForTest(
-    std::unique_ptr<base::TickClock> test_clock) {
-  clock_ = std::move(test_clock);
+  return ResourceCoordinatorClock::NowTicks() - visibility_change_time_;
 }
 
 void PageCoordinationUnitImpl::OnEventReceived(mojom::Event event) {
   if (event == mojom::Event::kNavigationCommitted)
-    navigation_committed_time_ = clock_->NowTicks();
+    navigation_committed_time_ = ResourceCoordinatorClock::NowTicks();
   for (auto& observer : observers())
     observer.OnPageEventReceived(this, event);
 }
@@ -141,7 +136,7 @@ void PageCoordinationUnitImpl::OnPropertyChanged(
     const mojom::PropertyType property_type,
     int64_t value) {
   if (property_type == mojom::PropertyType::kVisible)
-    visibility_change_time_ = clock_->NowTicks();
+    visibility_change_time_ = ResourceCoordinatorClock::NowTicks();
   for (auto& observer : observers())
     observer.OnPagePropertyChanged(this, property_type, value);
 }

services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h

@@ -6,7 +6,6 @@
 #define SERVICES_RESOURCE_COORDINATOR_COORDINATION_UNIT_PAGE_COORDINATION_UNIT_IMPL_H_
 
 #include "base/macros.h"
-#include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "services/resource_coordinator/coordination_unit/coordination_unit_base.h"
 
@@ -56,8 +55,6 @@ class PageCoordinationUnitImpl
   // PageCoordinationUnit.
   base::TimeDelta TimeSinceLastVisibilityChange() const;
 
-  void SetClockForTest(std::unique_ptr<base::TickClock> test_clock);
-
   const std::set<FrameCoordinationUnitImpl*>&
   frame_coordination_units_for_testing() const {
     return frame_coordination_units_;
@@ -79,7 +76,6 @@ class PageCoordinationUnitImpl
 
   std::set<FrameCoordinationUnitImpl*> frame_coordination_units_;
 
-  std::unique_ptr<base::TickClock> clock_;
   base::TimeTicks visibility_change_time_;
   // Main frame navigation committed time.
   base::TimeTicks navigation_committed_time_;

services/resource_coordinator/coordination_unit/page_coordination_unit_impl_unittest.cc

@@ -12,13 +12,36 @@
 #include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
 #include "services/resource_coordinator/public/cpp/coordination_unit_types.h"
 #include "services/resource_coordinator/public/interfaces/coordination_unit.mojom.h"
+#include "services/resource_coordinator/resource_coordinator_clock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace resource_coordinator {
 
 namespace {
 
-class PageCoordinationUnitImplTest : public CoordinationUnitTestHarness {};
+class PageCoordinationUnitImplTest : public CoordinationUnitTestHarness {
+ public:
+  void SetUp() override {
+    ResourceCoordinatorClock::SetClockForTesting(
+        std::make_unique<base::SimpleTestTickClock>());
+    clock_ = static_cast<base::SimpleTestTickClock*>(
+        ResourceCoordinatorClock::GetClockForTesting());
+
+    // Sets a valid starting time.
+    clock_->SetNowTicks(base::TimeTicks::Now());
+  }
+
+  void TearDown() override {
+    clock_ = nullptr;
+    ResourceCoordinatorClock::ResetClockForTesting();
+  }
+
+ protected:
+  void AdvanceClock(base::TimeDelta delta) { clock_->Advance(delta); }
+
+ private:
+  base::SimpleTestTickClock* clock_ = nullptr;
+};
 
 }  // namespace
 
@@ -129,48 +152,36 @@ TEST_F(PageCoordinationUnitImplTest,
 
 TEST_F(PageCoordinationUnitImplTest, TimeSinceLastVisibilityChange) {
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
-  base::SimpleTestTickClock* clock = new base::SimpleTestTickClock();
-  cu_graph.page->SetClockForTest(
-      std::unique_ptr<base::SimpleTestTickClock>(clock));
 
   cu_graph.page->SetVisibility(true);
   EXPECT_TRUE(cu_graph.page->IsVisible());
-  clock->Advance(base::TimeDelta::FromSeconds(42));
+  AdvanceClock(base::TimeDelta::FromSeconds(42));
   EXPECT_EQ(base::TimeDelta::FromSeconds(42),
             cu_graph.page->TimeSinceLastVisibilityChange());
 
   cu_graph.page->SetVisibility(false);
-  clock->Advance(base::TimeDelta::FromSeconds(23));
+  AdvanceClock(base::TimeDelta::FromSeconds(23));
   EXPECT_EQ(base::TimeDelta::FromSeconds(23),
             cu_graph.page->TimeSinceLastVisibilityChange());
   EXPECT_FALSE(cu_graph.page->IsVisible());
-  // The clock is owned and destructed by the page cu.
-  clock = nullptr;
 }
 
 TEST_F(PageCoordinationUnitImplTest, TimeSinceLastNavigation) {
   MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
-  base::SimpleTestTickClock* clock = new base::SimpleTestTickClock();
-  // Sets a valid starting time.
-  clock->SetNowTicks(base::TimeTicks::Now());
-  cu_graph.page->SetClockForTest(
-      std::unique_ptr<base::SimpleTestTickClock>(clock));
   // Before any commit events, timedelta should be 0.
   EXPECT_TRUE(cu_graph.page->TimeSinceLastNavigation().is_zero());
 
   // 1st navigation.
   cu_graph.page->OnMainFrameNavigationCommitted();
-  clock->Advance(base::TimeDelta::FromSeconds(11));
+  AdvanceClock(base::TimeDelta::FromSeconds(11));
   EXPECT_EQ(base::TimeDelta::FromSeconds(11),
             cu_graph.page->TimeSinceLastNavigation());
 
   // 2nd navigation.
   cu_graph.page->OnMainFrameNavigationCommitted();
-  clock->Advance(base::TimeDelta::FromSeconds(17));
+  AdvanceClock(base::TimeDelta::FromSeconds(17));
   EXPECT_EQ(base::TimeDelta::FromSeconds(17),
             cu_graph.page->TimeSinceLastNavigation());
-  // The clock is owned and destructed by the page cu.
-  clock = nullptr;
 }
 
 }  // namespace resource_coordinator

services/resource_coordinator/observers/metrics_collector.cc

@@ -11,6 +11,7 @@
 #include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
 #include "services/resource_coordinator/public/cpp/coordination_unit_id.h"
 #include "services/resource_coordinator/public/cpp/resource_coordinator_features.h"
+#include "services/resource_coordinator/resource_coordinator_clock.h"
 
 namespace resource_coordinator {
 
@@ -51,8 +52,7 @@ size_t GetNumCoresidentTabs(const PageCoordinationUnitImpl* page_cu) {
 }
 
 MetricsCollector::MetricsCollector()
-    : clock_(&default_tick_clock_),
-      max_ukm_cpu_usage_measurements_(kDefaultMaxCPUUsageMeasurements) {
+    : max_ukm_cpu_usage_measurements_(kDefaultMaxCPUUsageMeasurements) {
   UpdateWithFieldTrialParams();
 }
 
@@ -91,7 +91,7 @@ void MetricsCollector::OnFramePropertyChanged(
   if (property_type == mojom::PropertyType::kAudible) {
     bool audible = static_cast<bool>(value);
     if (!audible) {
-      frame_data.last_audible_time = clock_->NowTicks();
+      frame_data.last_audible_time = ResourceCoordinatorClock::NowTicks();
       return;
     }
     auto* page_cu = frame_cu->GetPageCoordinationUnit();
@@ -101,7 +101,7 @@ void MetricsCollector::OnFramePropertyChanged(
     }
     // Audio is considered to have started playing if the page has never
     // previously played audio, or has been silent for at least one minute.
-    auto now = clock_->NowTicks();
+    auto now = ResourceCoordinatorClock::NowTicks();
     if (frame_data.last_audible_time + kMaxAudioSlientTimeout < now) {
       MetricsReportRecord& record =
           metrics_report_record_map_.find(page_cu->id())->second;
@@ -199,10 +199,6 @@ void MetricsCollector::OnPageEventReceived(
   }
 }
 
-void MetricsCollector::SetClockForTest(base::TickClock* test_clock) {
-  const_cast<base::TickClock*&>(clock_) = test_clock;
-}
-
 bool MetricsCollector::ShouldReportMetrics(
     const PageCoordinationUnitImpl* page_cu) {
   return page_cu->TimeSinceLastNavigation() > kMetricsReportDelayTimeout;

services/resource_coordinator/observers/metrics_collector.h

@@ -7,7 +7,6 @@
 
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
@@ -56,8 +55,6 @@ class MetricsCollector : public CoordinationUnitGraphObserver {
   void OnPageEventReceived(const PageCoordinationUnitImpl* page_cu,
                            const mojom::Event event) override;
 
-  void SetClockForTest(base::TickClock* test_clock);
-
  private:
   struct MetricsReportRecord {
     MetricsReportRecord();
@@ -112,10 +109,6 @@ class MetricsCollector : public CoordinationUnitGraphObserver {
   void UpdateWithFieldTrialParams();
   void ResetMetricsReportRecord(CoordinationUnitID cu_id);
 
-  // Note: |clock_| is always |&default_tick_clock_|, except during unit
-  // testing.
-  base::DefaultTickClock default_tick_clock_;
-  base::TickClock* const clock_;
   std::map<CoordinationUnitID, FrameData> frame_data_map_;
   // The metrics_report_record_map_ is used to record whether a metric was
   // already reported to avoid reporting multiple metrics.

services/resource_coordinator/observers/metrics_collector_unittest.cc

@@ -10,6 +10,7 @@
 #include "services/resource_coordinator/coordination_unit/coordination_unit_test_harness.h"
 #include "services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h"
 #include "services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h"
+#include "services/resource_coordinator/resource_coordinator_clock.h"
 
 namespace resource_coordinator {
 
@@ -29,28 +30,26 @@ class MAYBE_MetricsCollectorTest : public CoordinationUnitTestHarness {
   MAYBE_MetricsCollectorTest() : CoordinationUnitTestHarness() {}
 
   void SetUp() override {
-    clock_ = new base::SimpleTestTickClock();
+    MetricsCollector* metrics_collector = new MetricsCollector();
+    ResourceCoordinatorClock::SetClockForTesting(
+        std::make_unique<base::SimpleTestTickClock>());
+    clock_ = static_cast<base::SimpleTestTickClock*>(
+        ResourceCoordinatorClock::GetClockForTesting());
+
     // Sets a valid starting time.
     clock_->SetNowTicks(base::TimeTicks::Now());
-    MetricsCollector* metrics_collector = new MetricsCollector();
-    metrics_collector->SetClockForTest(clock_);
     coordination_unit_manager().RegisterObserver(
         base::WrapUnique(metrics_collector));
   }
 
-  void TearDown() override { clock_ = nullptr; }
+  void TearDown() override {
+    clock_ = nullptr;
+    ResourceCoordinatorClock::ResetClockForTesting();
+  }
 
  protected:
   void AdvanceClock(base::TimeDelta delta) { clock_->Advance(delta); }
 
-  TestCoordinationUnitWrapper<PageCoordinationUnitImpl>
-  CreatePageCoordinationUnitWithClock() {
-    auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
-    page_cu->SetClockForTest(
-        std::unique_ptr<base::SimpleTestTickClock>(clock_));
-    return page_cu;
-  }
-
   base::HistogramTester histogram_tester_;
   base::SimpleTestTickClock* clock_ = nullptr;
 
@@ -59,7 +58,7 @@ class MAYBE_MetricsCollectorTest : public CoordinationUnitTestHarness {
 };
 
 TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstAudioStartsUMA) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -111,7 +110,7 @@ TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstAudioStartsUMA) {
 
 TEST_F(MAYBE_MetricsCollectorTest,
        FromBackgroundedToFirstAudioStartsUMA5MinutesTimeout) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -133,7 +132,7 @@ TEST_F(MAYBE_MetricsCollectorTest,
 }
 
 TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstTitleUpdatedUMA) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
 
   page_cu->OnMainFrameNavigationCommitted();
@@ -166,7 +165,7 @@ TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstTitleUpdatedUMA) {
 
 TEST_F(MAYBE_MetricsCollectorTest,
        FromBackgroundedToFirstTitleUpdatedUMA5MinutesTimeout) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
 
   page_cu->OnMainFrameNavigationCommitted();
@@ -183,7 +182,7 @@ TEST_F(MAYBE_MetricsCollectorTest,
 }
 
 TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstAlertFiredUMA) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -219,7 +218,7 @@ TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstAlertFiredUMA) {
 
 TEST_F(MAYBE_MetricsCollectorTest,
        FromBackgroundedToFirstAlertFiredUMA5MinutesTimeout) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -240,7 +239,7 @@ TEST_F(MAYBE_MetricsCollectorTest,
 
 TEST_F(MAYBE_MetricsCollectorTest,
        FromBackgroundedToFirstNonPersistentNotificationCreatedUMA) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -277,7 +276,7 @@ TEST_F(MAYBE_MetricsCollectorTest,
 TEST_F(
     MAYBE_MetricsCollectorTest,
     FromBackgroundedToFirstNonPersistentNotificationCreatedUMA5MinutesTimeout) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   auto frame_cu = CreateCoordinationUnit<FrameCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
   coordination_unit_manager().OnCoordinationUnitCreated(frame_cu.get());
@@ -297,7 +296,7 @@ TEST_F(
 }
 
 TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstFaviconUpdatedUMA) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
 
   page_cu->OnMainFrameNavigationCommitted();
@@ -330,7 +329,7 @@ TEST_F(MAYBE_MetricsCollectorTest, FromBackgroundedToFirstFaviconUpdatedUMA) {
 
 TEST_F(MAYBE_MetricsCollectorTest,
        FromBackgroundedToFirstFaviconUpdatedUMA5MinutesTimeout) {
-  auto page_cu = CreatePageCoordinationUnitWithClock();
+  auto page_cu = CreateCoordinationUnit<PageCoordinationUnitImpl>();
   coordination_unit_manager().OnCoordinationUnitCreated(page_cu.get());
 
   page_cu->OnMainFrameNavigationCommitted();

services/resource_coordinator/resource_coordinator_clock.cc

+// Copyright 2017 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 "services/resource_coordinator/resource_coordinator_clock.h"
+
+#include "base/time/tick_clock.h"
+
+namespace resource_coordinator {
+
+namespace {
+
+std::unique_ptr<base::TickClock>& g_tick_clock_for_testing() {
+  static std::unique_ptr<base::TickClock> tick_clock_for_testing = nullptr;
+  return tick_clock_for_testing;
+}
+
+}  // namespace
+
+base::TimeTicks ResourceCoordinatorClock::NowTicks() {
+  return g_tick_clock_for_testing() ? g_tick_clock_for_testing()->NowTicks()
+                                    : base::TimeTicks::Now();
+}
+
+base::TickClock* ResourceCoordinatorClock::GetClockForTesting() {
+  return g_tick_clock_for_testing().get();
+}
+
+void ResourceCoordinatorClock::ResetClockForTesting() {
+  g_tick_clock_for_testing().reset();
+}
+
+void ResourceCoordinatorClock::SetClockForTesting(
+    std::unique_ptr<base::TickClock> tick_clock) {
+  DCHECK(!g_tick_clock_for_testing());
+  g_tick_clock_for_testing() = std::move(tick_clock);
+}
+
+}  // namespace resource_coordinator

services/resource_coordinator/resource_coordinator_clock.h

+// Copyright 2017 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 SERVICES_RESOURCE_COORDINATOR_RESOURCE_COORDINATOR_CLOCK_H_
+#define SERVICES_RESOURCE_COORDINATOR_RESOURCE_COORDINATOR_CLOCK_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/time/time.h"
+
+namespace base {
+class TickClock;
+}
+
+namespace resource_coordinator {
+
+// ResourceCoordinatorClock provides timing methods that resource_coordinator/
+// needs at a central place, and facilitates testing across components when
+// clock manipulation is needed.
+class ResourceCoordinatorClock {
+ public:
+  // Returns time from the testing TickClock if set; otherwise returns time from
+  // TimeTicks::Now().
+  static base::TimeTicks NowTicks();
+
+  static base::TickClock* GetClockForTesting();
+
+  // Sets a TickClock for testing, the ownership of the |tick_clock| will be
+  // transferred to the global clock.
+  static void SetClockForTesting(std::unique_ptr<base::TickClock> tick_clock);
+
+  static void ResetClockForTesting();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ResourceCoordinatorClock);
+};
+
+}  // namespace resource_coordinator
+
+#endif  // SERVICES_RESOURCE_COORDINATOR_RESOURCE_COORDINATOR_CLOCK_H_