Use the shared instance of base::Default{,Tick}Clock in reading_list/

This CL changes the ownership of base::Clock and base::TickClock from
injectee-owned to injecter-owned. Before this CL, these instances are
owned by the owner of the injectee or one of the injectees themselves.
That makes the ownership handling complex.

After this CL, the injectee of clocks never own the clock. Instead,
injecters owns a clock for testing, and a shared clock is used on the
production code.

Bug: 789079
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I2c31982370181504cb80f2084ddf7cd15fb043a8
Reviewed-on: https://chromium-review.googlesource.com/975503Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546077}

components/ntp_snippets/reading_list/reading_list_suggestions_provider_unittest.cc

@@ -30,11 +30,8 @@ using ::testing::Property;
 class ReadingListSuggestionsProviderTest : public ::testing::Test {
  public:
   ReadingListSuggestionsProviderTest() {
-    std::unique_ptr<base::SimpleTestClock> clock =
-        std::make_unique<base::SimpleTestClock>();
-    clock_ = clock.get();
     model_ = std::make_unique<ReadingListModelImpl>(
-        /*storage_layer=*/nullptr, /*pref_service=*/nullptr, std::move(clock));
+        /*storage_layer=*/nullptr, /*pref_service=*/nullptr, &clock_);
   }
 
   void CreateProvider() {
@@ -56,23 +53,23 @@ class ReadingListSuggestionsProviderTest : public ::testing::Test {
   void AddEntries() {
     model_->AddEntry(url_unread1_, kTitleUnread1,
                      reading_list::ADDED_VIA_CURRENT_APP);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(url_unread2_, kTitleUnread2,
                      reading_list::ADDED_VIA_CURRENT_APP);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(url_read1_, kTitleRead1,
                      reading_list::ADDED_VIA_CURRENT_APP);
     model_->SetReadStatus(url_read1_, true);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(url_unread3_, kTitleUnread3,
                      reading_list::ADDED_VIA_CURRENT_APP);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(url_unread4_, kTitleUnread4,
                      reading_list::ADDED_VIA_CURRENT_APP);
   }
 
  protected:
-  base::SimpleTestClock* clock_;
+  base::SimpleTestClock clock_;
   std::unique_ptr<ReadingListModelImpl> model_;
   testing::StrictMock<MockContentSuggestionsProviderObserver> observer_;
   std::unique_ptr<ReadingListSuggestionsProvider> provider_;
@@ -117,7 +114,7 @@ TEST_F(ReadingListSuggestionsProviderTest, ReturnsOnlyUnreadSuggestion) {
   std::string title_read1 = "title_read1";
   model_->AddEntry(url_unread1, title_unread1,
                    reading_list::ADDED_VIA_CURRENT_APP);
-  clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+  clock_.Advance(base::TimeDelta::FromMilliseconds(10));
   model_->AddEntry(url_read1, title_read1, reading_list::ADDED_VIA_CURRENT_APP);
   model_->SetReadStatus(url_read1, true);
 

components/reading_list/core/reading_list_model_impl.cc

@@ -16,12 +16,12 @@
 ReadingListModelImpl::ReadingListModelImpl(
     std::unique_ptr<ReadingListModelStorage> storage,
     PrefService* pref_service,
-    std::unique_ptr<base::Clock> clock)
+    base::Clock* clock)
     : entries_(std::make_unique<ReadingListEntries>()),
       unread_entry_count_(0),
       read_entry_count_(0),
       unseen_entry_count_(0),
-      clock_(std::move(clock)),
+      clock_(clock),
       pref_service_(pref_service),
       has_unseen_(false),
       loaded_(false),
@@ -30,7 +30,7 @@ ReadingListModelImpl::ReadingListModelImpl(
   DCHECK(clock_);
   if (storage) {
     storage_layer_ = std::move(storage);
-    storage_layer_->SetReadingListModel(this, this, clock_.get());
+    storage_layer_->SetReadingListModel(this, this, clock_);
   } else {
     loaded_ = true;
   }

components/reading_list/core/reading_list_model_impl.h

@@ -34,7 +34,7 @@ class ReadingListModelImpl : public ReadingListModel,
   // |clock| will be used to timestamp all the operations.
   ReadingListModelImpl(std::unique_ptr<ReadingListModelStorage> storage_layer,
                        PrefService* pref_service,
-                       std::unique_ptr<base::Clock> clock_);
+                       base::Clock* clock_);
 
   ReadingListModelImpl();
 
@@ -141,8 +141,7 @@ class ReadingListModelImpl : public ReadingListModel,
   // Set the unseen flag to true.
   void SetUnseenFlag();
 
-  // |storage_layer_| depends on |clock_| so keep the order.
-  std::unique_ptr<base::Clock> clock_;
+  base::Clock* clock_;
   std::unique_ptr<ReadingListModelStorage> storage_layer_;
   PrefService* pref_service_;
   bool has_unseen_;

components/reading_list/core/reading_list_model_unittest.cc

@@ -159,20 +159,15 @@ class ReadingListModelTest : public ReadingListModelObserver,
                              public testing::Test {
  public:
   ReadingListModelTest() : callback_called_(false) {
-    auto clock = std::make_unique<base::SimpleTestClock>();
-    clock_ = clock.get();
-    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr,
-                                                    std::move(clock));
+    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr, &clock_);
     ClearCounts();
     model_->AddObserver(this);
   }
   ~ReadingListModelTest() override {}
 
-  void SetStorage(std::unique_ptr<TestReadingListStorage> storage,
-                  std::unique_ptr<base::SimpleTestClock> clock) {
-    clock_ = clock.get();
+  void SetStorage(std::unique_ptr<TestReadingListStorage> storage) {
     model_ = std::make_unique<ReadingListModelImpl>(std::move(storage), nullptr,
-                                                    std::move(clock));
+                                                    &clock_);
     ClearCounts();
     model_->AddObserver(this);
   }
@@ -303,8 +298,7 @@ class ReadingListModelTest : public ReadingListModelObserver,
   bool callback_called_;
 
   std::unique_ptr<ReadingListModelImpl> model_;
-  // Owned by |model_|;
-  base::SimpleTestClock* clock_;
+  base::SimpleTestClock clock_;
 };
 
 // Tests creating an empty model.
@@ -324,10 +318,9 @@ TEST_F(ReadingListModelTest, EmptyLoaded) {
 // Tests load model.
 TEST_F(ReadingListModelTest, ModelLoaded) {
   ClearCounts();
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
   storage->AddSampleEntries();
-  SetStorage(std::move(storage), std::move(clock));
+  SetStorage(std::move(storage));
 
   AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0);
   std::map<GURL, std::string> loaded_entries;
@@ -349,9 +342,8 @@ TEST_F(ReadingListModelTest, ModelLoaded) {
 
 // Tests adding entry.
 TEST_F(ReadingListModelTest, AddEntry) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   ClearCounts();
 
   const ReadingListEntry& entry =
@@ -376,9 +368,8 @@ TEST_F(ReadingListModelTest, AddEntry) {
 
 // Tests adding an entry that already exists.
 TEST_F(ReadingListModelTest, AddExistingEntry) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   GURL url = GURL("http://example.com");
   std::string title = "\n  \tsample Test ";
   model_->AddEntry(url, title, reading_list::ADDED_VIA_CURRENT_APP);
@@ -405,12 +396,11 @@ TEST_F(ReadingListModelTest, AddExistingEntry) {
 
 // Tests addin entry from sync.
 TEST_F(ReadingListModelTest, SyncAddEntry) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   auto entry = std::make_unique<ReadingListEntry>(
-      GURL("http://example.com"), "sample", AdvanceAndGetTime(clock_));
-  entry->SetRead(true, AdvanceAndGetTime(clock_));
+      GURL("http://example.com"), "sample", AdvanceAndGetTime(&clock_));
+  entry->SetRead(true, AdvanceAndGetTime(&clock_));
   ClearCounts();
 
   model_->SyncAddEntry(std::move(entry));
@@ -423,9 +413,8 @@ TEST_F(ReadingListModelTest, SyncAddEntry) {
 
 // Tests updating entry from sync.
 TEST_F(ReadingListModelTest, SyncMergeEntry) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   model_->AddEntry(GURL("http://example.com"), "sample",
                    reading_list::ADDED_VIA_CURRENT_APP);
   const base::FilePath distilled_path(FILE_PATH_LITERAL("distilled/page.html"));
@@ -440,8 +429,8 @@ TEST_F(ReadingListModelTest, SyncMergeEntry) {
   int64_t local_update_time = local_entry->UpdateTime();
 
   auto sync_entry = std::make_unique<ReadingListEntry>(
-      GURL("http://example.com"), "sample", AdvanceAndGetTime(clock_));
-  sync_entry->SetRead(true, AdvanceAndGetTime(clock_));
+      GURL("http://example.com"), "sample", AdvanceAndGetTime(&clock_));
+  sync_entry->SetRead(true, AdvanceAndGetTime(&clock_));
   ASSERT_GT(sync_entry->UpdateTime(), local_update_time);
   int64_t sync_update_time = sync_entry->UpdateTime();
   EXPECT_TRUE(sync_entry->DistilledPath().empty());
@@ -463,9 +452,8 @@ TEST_F(ReadingListModelTest, SyncMergeEntry) {
 
 // Tests deleting entry.
 TEST_F(ReadingListModelTest, RemoveEntryByUrl) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   model_->AddEntry(GURL("http://example.com"), "sample",
                    reading_list::ADDED_VIA_CURRENT_APP);
   ClearCounts();
@@ -496,9 +484,8 @@ TEST_F(ReadingListModelTest, RemoveEntryByUrl) {
 
 // Tests deleting entry from sync.
 TEST_F(ReadingListModelTest, RemoveSyncEntryByUrl) {
-  auto clock = std::make_unique<base::SimpleTestClock>();
-  auto storage = std::make_unique<TestReadingListStorage>(this, clock.get());
-  SetStorage(std::move(storage), std::move(clock));
+  auto storage = std::make_unique<TestReadingListStorage>(this, &clock_);
+  SetStorage(std::move(storage));
   model_->AddEntry(GURL("http://example.com"), "sample",
                    reading_list::ADDED_VIA_CURRENT_APP);
   ClearCounts();

components/reading_list/core/reading_list_store_unittest.cc

@@ -95,11 +95,8 @@ class ReadingListStoreTest : public testing::Test,
         base::BindOnce(&syncer::ModelTypeStoreTestUtil::MoveStoreToCallback,
                        std::move(store_)),
         CreateModelTypeChangeProcessor());
-    auto clock = std::make_unique<base::SimpleTestClock>();
-    clock_ = clock.get();
-    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr,
-                                                    std::move(clock));
-    reading_list_store_->SetReadingListModel(model_.get(), this, clock_);
+    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr, &clock_);
+    reading_list_store_->SetReadingListModel(model_.get(), this, &clock_);
 
     base::RunLoop().RunUntilIdle();
   }
@@ -175,7 +172,7 @@ class ReadingListStoreTest : public testing::Test,
 
   std::unique_ptr<syncer::ModelTypeStore> store_;
   std::unique_ptr<ReadingListModelImpl> model_;
-  base::SimpleTestClock* clock_;
+  base::SimpleTestClock clock_;
   std::unique_ptr<ReadingListStore> reading_list_store_;
   int put_called_;
   int delete_called_;
@@ -195,9 +192,9 @@ TEST_F(ReadingListStoreTest, CheckEmpties) {
 
 TEST_F(ReadingListStoreTest, SaveOneRead) {
   ReadingListEntry entry(GURL("http://read.example.com/"), "read title",
-                         AdvanceAndGetTime(clock_));
-  entry.SetRead(true, AdvanceAndGetTime(clock_));
-  AdvanceAndGetTime(clock_);
+                         AdvanceAndGetTime(&clock_));
+  entry.SetRead(true, AdvanceAndGetTime(&clock_));
+  AdvanceAndGetTime(&clock_);
   reading_list_store_->SaveEntry(entry);
   AssertCounts(1, 0, 0, 0, 0);
   syncer::EntityData* data = put_multimap_["http://read.example.com/"].get();
@@ -210,7 +207,7 @@ TEST_F(ReadingListStoreTest, SaveOneRead) {
 
 TEST_F(ReadingListStoreTest, SaveOneUnread) {
   ReadingListEntry entry(GURL("http://unread.example.com/"), "unread title",
-                         AdvanceAndGetTime(clock_));
+                         AdvanceAndGetTime(&clock_));
   reading_list_store_->SaveEntry(entry);
   AssertCounts(1, 0, 0, 0, 0);
   syncer::EntityData* data = put_multimap_["http://unread.example.com/"].get();
@@ -224,8 +221,8 @@ TEST_F(ReadingListStoreTest, SaveOneUnread) {
 TEST_F(ReadingListStoreTest, SyncMergeOneEntry) {
   syncer::EntityChangeList remote_input;
   ReadingListEntry entry(GURL("http://read.example.com/"), "read title",
-                         AdvanceAndGetTime(clock_));
-  entry.SetRead(true, AdvanceAndGetTime(clock_));
+                         AdvanceAndGetTime(&clock_));
+  entry.SetRead(true, AdvanceAndGetTime(&clock_));
   std::unique_ptr<sync_pb::ReadingListSpecifics> specifics =
       entry.AsReadingListSpecifics();
 
@@ -248,8 +245,8 @@ TEST_F(ReadingListStoreTest, SyncMergeOneEntry) {
 
 TEST_F(ReadingListStoreTest, ApplySyncChangesOneAdd) {
   ReadingListEntry entry(GURL("http://read.example.com/"), "read title",
-                         AdvanceAndGetTime(clock_));
-  entry.SetRead(true, AdvanceAndGetTime(clock_));
+                         AdvanceAndGetTime(&clock_));
+  entry.SetRead(true, AdvanceAndGetTime(&clock_));
   std::unique_ptr<sync_pb::ReadingListSpecifics> specifics =
       entry.AsReadingListSpecifics();
   syncer::EntityData data;
@@ -269,13 +266,13 @@ TEST_F(ReadingListStoreTest, ApplySyncChangesOneAdd) {
 }
 
 TEST_F(ReadingListStoreTest, ApplySyncChangesOneMerge) {
-  AdvanceAndGetTime(clock_);
+  AdvanceAndGetTime(&clock_);
   model_->AddEntry(GURL("http://unread.example.com/"), "unread title",
                    reading_list::ADDED_VIA_CURRENT_APP);
 
   ReadingListEntry new_entry(GURL("http://unread.example.com/"), "unread title",
-                             AdvanceAndGetTime(clock_));
-  new_entry.SetRead(true, AdvanceAndGetTime(clock_));
+                             AdvanceAndGetTime(&clock_));
+  new_entry.SetRead(true, AdvanceAndGetTime(&clock_));
   std::unique_ptr<sync_pb::ReadingListSpecifics> specifics =
       new_entry.AsReadingListSpecifics();
   syncer::EntityData data;
@@ -296,10 +293,10 @@ TEST_F(ReadingListStoreTest, ApplySyncChangesOneMerge) {
 TEST_F(ReadingListStoreTest, ApplySyncChangesOneIgnored) {
   // Read entry but with unread URL as it must update the other one.
   ReadingListEntry old_entry(GURL("http://unread.example.com/"),
-                             "old unread title", AdvanceAndGetTime(clock_));
-  old_entry.SetRead(true, AdvanceAndGetTime(clock_));
+                             "old unread title", AdvanceAndGetTime(&clock_));
+  old_entry.SetRead(true, AdvanceAndGetTime(&clock_));
 
-  AdvanceAndGetTime(clock_);
+  AdvanceAndGetTime(&clock_);
   model_->AddEntry(GURL("http://unread.example.com/"), "new unread title",
                    reading_list::ADDED_VIA_CURRENT_APP);
   AssertCounts(0, 0, 0, 0, 0);

ios/chrome/browser/reading_list/offline_url_utils_unittest.cc

@@ -127,7 +127,7 @@ TEST_F(OfflineURLUtilsTest, IsOfflineURL) {
 // Checks that the offline URLs are correctly detected by |IsOfflineURL|.
 TEST_F(OfflineURLUtilsTest, IsOfflineURLValid) {
   auto reading_list_model = std::make_unique<ReadingListModelImpl>(
-      nullptr, nullptr, std::make_unique<base::DefaultClock>());
+      nullptr, nullptr, base::DefaultClock::GetInstance());
   GURL entry_url("http://entry_url.com");
   base::FilePath distilled_path("distilled/page.html");
   GURL distilled_url("http://distilled_url.com");

ios/chrome/browser/reading_list/reading_list_model_factory.cc

@@ -72,9 +72,9 @@ std::unique_ptr<KeyedService> ReadingListModelFactory::BuildServiceInstanceFor(
   std::unique_ptr<ReadingListStore> store = std::make_unique<ReadingListStore>(
       std::move(store_factory), std::move(change_processor));
   std::unique_ptr<KeyedService> reading_list_model =
-      std::make_unique<ReadingListModelImpl>(
-          std::move(store), chrome_browser_state->GetPrefs(),
-          std::make_unique<base::DefaultClock>());
+      std::make_unique<ReadingListModelImpl>(std::move(store),
+                                             chrome_browser_state->GetPrefs(),
+                                             base::DefaultClock::GetInstance());
   return reading_list_model;
 }
 

ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm

@@ -73,7 +73,7 @@ class ReadingListWebStateObserverTest : public web::WebTest {
     test_navigation_manager->SetLastCommittedItem(last_committed_item_.get());
     test_web_state_.SetNavigationManager(std::move(test_navigation_manager));
     reading_list_model_ = std::make_unique<ReadingListModelImpl>(
-        nullptr, nullptr, std::make_unique<base::DefaultClock>());
+        nullptr, nullptr, base::DefaultClock::GetInstance());
     reading_list_model_->AddEntry(GURL(kTestURL), kTestTitle,
                                   reading_list::ADDED_VIA_CURRENT_APP);
     ReadingListWebStateObserver::CreateForWebState(&test_web_state_,

ios/chrome/browser/ui/reading_list/reading_list_collection_view_controller_unittest.mm

@@ -58,7 +58,7 @@ class ReadingListCollectionViewControllerTest : public PlatformTest {
         .WillRepeatedly(PostReply<5>(favicon_base::FaviconRawBitmapResult()));
 
     reading_list_model_.reset(new ReadingListModelImpl(
-        nullptr, nullptr, std::make_unique<base::DefaultClock>()));
+        nullptr, nullptr, base::DefaultClock::GetInstance()));
     large_icon_service_.reset(new favicon::LargeIconService(
         &mock_favicon_service_, /*image_fetcher=*/nullptr));
     mediator_ =

ios/chrome/browser/ui/reading_list/reading_list_coordinator_unittest.mm

@@ -115,7 +115,7 @@ class ReadingListCoordinatorTest : public web::WebTestWithWebState {
     browser_state_ = builder.Build();
 
     reading_list_model_.reset(new ReadingListModelImpl(
-        nullptr, nullptr, std::make_unique<base::DefaultClock>()));
+        nullptr, nullptr, base::DefaultClock::GetInstance()));
     large_icon_service_.reset(new favicon::LargeIconService(
         &mock_favicon_service_, /*image_fetcher=*/nullptr));
     mediator_ =

ios/chrome/browser/ui/reading_list/reading_list_mediator_unittest.mm

@@ -30,11 +30,7 @@ using testing::_;
 class ReadingListMediatorTest : public PlatformTest {
  public:
   ReadingListMediatorTest() {
-    std::unique_ptr<base::SimpleTestClock> clock =
-        std::make_unique<base::SimpleTestClock>();
-    clock_ = clock.get();
-    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr,
-                                                    std::move(clock));
+    model_ = std::make_unique<ReadingListModelImpl>(nullptr, nullptr, &clock_);
     EXPECT_CALL(mock_favicon_service_,
                 GetLargestRawFaviconForPageURL(_, _, _, _, _))
         .WillRepeatedly(
@@ -49,10 +45,10 @@ class ReadingListMediatorTest : public PlatformTest {
     model_->SetReadStatus(GURL("http://chromium.org/read1"), true);
     model_->AddEntry(GURL("http://chromium.org/unread2"), "unread2",
                      reading_list::ADDED_VIA_CURRENT_APP);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(no_title_entry_url_, "",
                      reading_list::ADDED_VIA_CURRENT_APP);
-    clock_->Advance(base::TimeDelta::FromMilliseconds(10));
+    clock_.Advance(base::TimeDelta::FromMilliseconds(10));
     model_->AddEntry(GURL("http://chromium.org/read2"), "read2",
                      reading_list::ADDED_VIA_CURRENT_APP);
     model_->SetReadStatus(GURL("http://chromium.org/read2"), true);
@@ -69,7 +65,7 @@ class ReadingListMediatorTest : public PlatformTest {
   testing::StrictMock<favicon::MockFaviconService> mock_favicon_service_;
   std::unique_ptr<ReadingListModelImpl> model_;
   ReadingListMediator* mediator_;
-  base::SimpleTestClock* clock_;
+  base::SimpleTestClock clock_;
   GURL no_title_entry_url_;
   std::unique_ptr<favicon::LargeIconService> large_icon_service_;