Consolidate Prefetching tasks time sources to using base::Clock

This change adds a base::Clock pointer to the base TaskQueue tasks and
updates all time related calls in tasks and tests in:

components/offline_pages/core/prefetch/tasks/

Bug: 906903
Change-Id: Ia701af7d49dcd5376c903598e61f490721149df2
Reviewed-on: https://chromium-review.googlesource.com/c/1344829Reviewed-by: Dmitry Titov <dimich@chromium.org>
Reviewed-by: Dan H <harringtond@google.com>
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611010}

components/offline_pages/core/BUILD.gn

@@ -60,6 +60,8 @@ static_library("core") {
     "model/store_thumbnail_task.h",
     "model/update_file_path_task.cc",
     "model/update_file_path_task.h",
+    "offline_clock.cc",
+    "offline_clock.h",
     "offline_event_logger.cc",
     "offline_event_logger.h",
     "offline_page_archiver.cc",

components/offline_pages/core/offline_clock.cc

+// Copyright (c) 2018 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 "components/offline_pages/core/offline_clock.h"
+
+#include "base/time/default_clock.h"
+
+namespace offline_pages {
+
+namespace {
+base::Clock* custom_clock_ = nullptr;
+}
+
+base::Clock* OfflineClock() {
+  if (custom_clock_)
+    return custom_clock_;
+  return base::DefaultClock::GetInstance();
+}
+
+void SetOfflineClockForTesting(base::Clock* clock) {
+  custom_clock_ = clock;
+}
+
+}  // namespace offline_pages

components/offline_pages/core/offline_clock.h

+// Copyright (c) 2018 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 COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_CLOCK_H_
+#define COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_CLOCK_H_
+
+namespace base {
+class Clock;
+}
+
+namespace offline_pages {
+
+// Returns the clock to be used for obtaining the current time. This function
+// can be called from any threads.
+base::Clock* OfflineClock();
+
+// Allows tests to override the clock returned by |OfflineClock()|.
+void SetOfflineClockForTesting(base::Clock* clock);
+
+}  // namespace offline_pages
+
+#endif  // COMPONENTS_OFFLINE_PAGES_CORE_OFFLINE_CLOCK_H_

components/offline_pages/core/prefetch/tasks/add_unique_urls_task.cc

@@ -13,7 +13,9 @@
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/time/clock.h"
 #include "base/time/time.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -90,7 +92,7 @@ Result AddUrlsAndCleanupZombiesSync(
       FindExistingPrefetchItemsInNamespaceSync(db, name_space);
 
   int added_row_count = 0;
-  base::Time now = base::Time::Now();
+  base::Time now = OfflineClock()->Now();
   // Insert rows in reverse order to ensure that the beginning of the list has
   // the newest timestamp.  This will cause it to be prefetched first.
   for (auto candidate_iter = candidate_prefetch_urls.rbegin();

components/offline_pages/core/prefetch/tasks/download_archives_task.cc

@@ -7,8 +7,8 @@
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/time/default_clock.h"
-#include "base/time/time.h"
+#include "base/time/clock.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_downloader.h"
@@ -76,7 +76,7 @@ bool MarkItemAsDownloading(sql::Database* db,
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(0, static_cast<int>(PrefetchItemState::DOWNLOADING));
   statement.BindString(1, guid);
-  statement.BindInt64(2, store_utils::ToDatabaseTime(base::Time::Now()));
+  statement.BindInt64(2, store_utils::ToDatabaseTime(OfflineClock()->Now()));
   statement.BindInt64(3, offline_id);
   return statement.Run();
 }
@@ -100,8 +100,7 @@ std::unique_ptr<ItemsToDownload> SelectAndMarkItemsForDownloadSync(
     return nullptr;
   }
 
-  PrefetchDownloaderQuota downloader_quota(db,
-                                           base::DefaultClock::GetInstance());
+  PrefetchDownloaderQuota downloader_quota(db, OfflineClock());
   int64_t available_quota = downloader_quota.GetAvailableQuotaBytes();
   if (available_quota <= 0 && !IsLimitlessPrefetchingEnabled())
     return nullptr;
@@ -185,7 +184,7 @@ void DownloadArchivesTask::Run() {
   }
 
   prefetch_store_->Execute(
-      base::BindOnce(SelectAndMarkItemsForDownloadSync),
+      base::BindOnce(&SelectAndMarkItemsForDownloadSync),
       base::BindOnce(&DownloadArchivesTask::SendItemsToPrefetchDownloader,
                      weak_ptr_factory_.GetWeakPtr()),
       std::unique_ptr<ItemsToDownload>());

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task.cc

@@ -9,8 +9,9 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
-#include "base/time/default_clock.h"
+#include "base/time/clock.h"
 #include "components/offline_pages/core/client_id.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_gcm_handler.h"
 #include "components/offline_pages/core/prefetch/prefetch_network_request_factory.h"
@@ -53,9 +54,7 @@ struct FetchedUrl {
 // This is maximum URLs that Offline Page Service can take in one request.
 const int kMaxUrlsToSend = 100;
 
-bool UpdateStateSync(sql::Database* db,
-                     const int64_t offline_id,
-                     base::Clock* clock) {
+bool UpdateStateSync(sql::Database* db, const int64_t offline_id) {
   static const char kSql[] =
       "UPDATE prefetch_items"
       " SET state = ?,"
@@ -66,7 +65,7 @@ bool UpdateStateSync(sql::Database* db,
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(
       0, static_cast<int>(PrefetchItemState::SENT_GENERATE_PAGE_BUNDLE));
-  statement.BindInt64(1, store_utils::ToDatabaseTime(clock->Now()));
+  statement.BindInt64(1, store_utils::ToDatabaseTime(OfflineClock()->Now()));
   statement.BindInt64(2, offline_id);
   return statement.Run();
 }
@@ -109,8 +108,7 @@ bool MarkUrlFinishedWithError(sql::Database* db, const FetchedUrl& url) {
   return statement.Run();
 }
 
-std::unique_ptr<UrlAndIds> SelectUrlsToPrefetchSync(base::Clock* clock,
-                                                    sql::Database* db) {
+std::unique_ptr<UrlAndIds> SelectUrlsToPrefetchSync(sql::Database* db) {
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return nullptr;
@@ -131,7 +129,7 @@ std::unique_ptr<UrlAndIds> SelectUrlsToPrefetchSync(base::Clock* clock,
 
   auto url_and_ids = std::make_unique<UrlAndIds>();
   for (const auto& url : *urls) {
-    if (!UpdateStateSync(db, url.offline_id, clock))
+    if (!UpdateStateSync(db, url.offline_id))
       return nullptr;
     url_and_ids->urls.push_back(std::move(url.requested_url));
     url_and_ids->ids.push_back({url.offline_id, std::move(url.client_id)});
@@ -150,8 +148,7 @@ GeneratePageBundleTask::GeneratePageBundleTask(
     PrefetchGCMHandler* gcm_handler,
     PrefetchNetworkRequestFactory* request_factory,
     PrefetchRequestFinishedCallback callback)
-    : clock_(base::DefaultClock::GetInstance()),
-      prefetch_dispatcher_(prefetch_dispatcher),
+    : prefetch_dispatcher_(prefetch_dispatcher),
       prefetch_store_(prefetch_store),
       gcm_handler_(gcm_handler),
       request_factory_(request_factory),
@@ -162,16 +159,12 @@ GeneratePageBundleTask::~GeneratePageBundleTask() {}
 
 void GeneratePageBundleTask::Run() {
   prefetch_store_->Execute(
-      base::BindOnce(&SelectUrlsToPrefetchSync, clock_),
+      base::BindOnce(&SelectUrlsToPrefetchSync),
       base::BindOnce(&GeneratePageBundleTask::StartGeneratePageBundle,
                      weak_factory_.GetWeakPtr()),
       std::unique_ptr<UrlAndIds>());
 }
 
-void GeneratePageBundleTask::SetClockForTesting(base::Clock* clock) {
-  clock_ = clock;
-}
-
 void GeneratePageBundleTask::StartGeneratePageBundle(
     std::unique_ptr<UrlAndIds> url_and_ids) {
   if (!url_and_ids) {

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task.h

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
-#include "base/time/clock.h"
 #include "components/gcm_driver/instance_id/instance_id.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -37,16 +36,12 @@ class GeneratePageBundleTask : public Task {
   // Task implementation.
   void Run() override;
 
-  void SetClockForTesting(base::Clock* clock);
-
  private:
   void StartGeneratePageBundle(std::unique_ptr<UrlAndIds> url_and_ids);
   void GotRegistrationId(std::unique_ptr<UrlAndIds> url_and_ids,
                          const std::string& id,
                          instance_id::InstanceID::Result result);
 
-  base::Clock* clock_;
-
   PrefetchDispatcher* prefetch_dispatcher_;
   PrefetchStore* prefetch_store_;
   PrefetchGCMHandler* gcm_handler_;

components/offline_pages/core/prefetch/tasks/generate_page_bundle_task_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/test/mock_callback.h"
 #include "base/test/simple_test_clock.h"
 #include "base/time/time.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
@@ -106,7 +107,7 @@ TEST_F(GeneratePageBundleTaskTest, TaskMakesNetworkRequest) {
   GeneratePageBundleTask task(dispatcher(), store(), gcm_handler(),
                               prefetch_request_factory(),
                               request_callback.Get());
-  task.SetClockForTesting(&clock);
+  SetOfflineClockForTesting(&clock);
   RunTask(&task);
 
   // Note: even though the requested URLs checked further below are in undefined

components/offline_pages/core/prefetch/tasks/mark_operation_done_task.cc

@@ -9,6 +9,8 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/time/clock.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/prefetch_network_request_factory.h"
@@ -29,7 +31,7 @@ bool UpdatePrefetchItemsSync(sql::Database* db,
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt(0, static_cast<int>(PrefetchItemState::RECEIVED_GCM));
-  statement.BindInt64(1, store_utils::ToDatabaseTime(base::Time::Now()));
+  statement.BindInt64(1, store_utils::ToDatabaseTime(OfflineClock()->Now()));
   statement.BindInt(2, static_cast<int>(PrefetchItemState::AWAITING_GCM));
   statement.BindString(3, operation_name);
 

components/offline_pages/core/prefetch/tasks/metrics_finalization_task.cc

@@ -13,6 +13,9 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/time/clock.h"
+#include "base/time/time.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
 #include "components/offline_pages/core/prefetch/store/prefetch_store.h"
@@ -160,7 +163,7 @@ bool ReportMetricsAndFinalizeSync(sql::Database* db) {
 
   const std::vector<PrefetchItemStats> urls = FetchUrlsSync(db);
 
-  base::Time now = base::Time::Now();
+  base::Time now = OfflineClock()->Now();
   for (const auto& url : urls) {
     MarkUrlAsZombie(db, now, url.offline_id);
   }

components/offline_pages/core/prefetch/tasks/stale_entry_finalizer_task.cc

@@ -8,6 +8,9 @@
 
 #include "base/bind.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/time/clock.h"
+#include "base/time/time.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/offline_store_utils.h"
 #include "components/offline_pages/core/prefetch/prefetch_dispatcher.h"
 #include "components/offline_pages/core/prefetch/prefetch_downloader.h"
@@ -172,8 +175,7 @@ void ReportAndFinalizeStuckItems(base::Time now, sql::Database* db) {
   }
 }
 
-Result FinalizeStaleEntriesSync(StaleEntryFinalizerTask::NowGetter now_getter,
-                                sql::Database* db) {
+Result FinalizeStaleEntriesSync(sql::Database* db) {
   sql::Transaction transaction(db);
   if (!transaction.Begin())
     return Result::NO_MORE_WORK;
@@ -189,7 +191,7 @@ Result FinalizeStaleEntriesSync(StaleEntryFinalizerTask::NowGetter now_getter,
       // Bucket 3.
       PrefetchItemState::DOWNLOADING, PrefetchItemState::IMPORTING,
   }};
-  base::Time now = now_getter.Run();
+  base::Time now = OfflineClock()->Now();
   for (PrefetchItemState state : expirable_states) {
     if (!FinalizeStaleItems(state, now, db))
       return Result::NO_MORE_WORK;
@@ -217,7 +219,6 @@ StaleEntryFinalizerTask::StaleEntryFinalizerTask(
     PrefetchStore* prefetch_store)
     : prefetch_dispatcher_(prefetch_dispatcher),
       prefetch_store_(prefetch_store),
-      now_getter_(base::BindRepeating(&base::Time::Now)),
       weak_ptr_factory_(this) {
   DCHECK(prefetch_dispatcher_);
   DCHECK(prefetch_store_);
@@ -226,15 +227,10 @@ StaleEntryFinalizerTask::StaleEntryFinalizerTask(
 StaleEntryFinalizerTask::~StaleEntryFinalizerTask() {}
 
 void StaleEntryFinalizerTask::Run() {
-  prefetch_store_->Execute(
-      base::BindOnce(&FinalizeStaleEntriesSync, now_getter_),
-      base::BindOnce(&StaleEntryFinalizerTask::OnFinished,
-                     weak_ptr_factory_.GetWeakPtr()),
-      Result::NO_MORE_WORK);
-}
-
-void StaleEntryFinalizerTask::SetNowGetterForTesting(NowGetter now_getter) {
-  now_getter_ = now_getter;
+  prefetch_store_->Execute(base::BindOnce(&FinalizeStaleEntriesSync),
+                           base::BindOnce(&StaleEntryFinalizerTask::OnFinished,
+                                          weak_ptr_factory_.GetWeakPtr()),
+                           Result::NO_MORE_WORK);
 }
 
 void StaleEntryFinalizerTask::OnFinished(Result result) {

components/offline_pages/core/prefetch/tasks/stale_entry_finalizer_task.h

@@ -27,7 +27,6 @@ class PrefetchStore;
 class StaleEntryFinalizerTask : public Task {
  public:
   enum class Result { NO_MORE_WORK, MORE_WORK_NEEDED };
-  using NowGetter = base::RepeatingCallback<base::Time()>;
 
   StaleEntryFinalizerTask(PrefetchDispatcher* prefetch_dispatcher,
                           PrefetchStore* prefetch_store);
@@ -35,10 +34,6 @@ class StaleEntryFinalizerTask : public Task {
 
   void Run() override;
 
-  // Allows tests to control the source of current time values used internally
-  // for freshness checks.
-  void SetNowGetterForTesting(NowGetter now_getter);
-
   // Will be set to true upon after an error-free run.
   Result final_status() const { return final_status_; }
 
@@ -51,9 +46,6 @@ class StaleEntryFinalizerTask : public Task {
   // Prefetch store to execute against. Not owned.
   PrefetchStore* prefetch_store_;
 
-  // Defaults to base::Time::Now upon construction.
-  NowGetter now_getter_;
-
   Result final_status_ = Result::NO_MORE_WORK;
 
   base::WeakPtrFactory<StaleEntryFinalizerTask> weak_ptr_factory_;

components/offline_pages/core/prefetch/tasks/stale_entry_finalizer_task_unittest.cc

@@ -10,8 +10,10 @@
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/simple_test_clock.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/core/offline_clock.h"
 #include "components/offline_pages/core/prefetch/mock_prefetch_item_generator.h"
 #include "components/offline_pages/core/prefetch/prefetch_item.h"
 #include "components/offline_pages/core/prefetch/prefetch_types.h"
@@ -50,16 +52,15 @@ class StaleEntryFinalizerTaskTest : public PrefetchTaskTestBase {
  protected:
   TestPrefetchDispatcher dispatcher_;
   std::unique_ptr<StaleEntryFinalizerTask> stale_finalizer_task_;
-  base::Time fake_now_;
+  base::SimpleTestClock simple_test_clock_;
 };
 
 void StaleEntryFinalizerTaskTest::SetUp() {
   PrefetchTaskTestBase::SetUp();
   stale_finalizer_task_ =
       std::make_unique<StaleEntryFinalizerTask>(dispatcher(), store());
-  fake_now_ = base::Time() + base::TimeDelta::FromDays(100);
-  stale_finalizer_task_->SetNowGetterForTesting(base::BindRepeating(
-      [](base::Time t) -> base::Time { return t; }, fake_now_));
+  simple_test_clock_.SetNow(base::Time() + base::TimeDelta::FromDays(100));
+  SetOfflineClockForTesting(&simple_test_clock_);
 }
 
 void StaleEntryFinalizerTaskTest::TearDown() {
@@ -71,8 +72,8 @@ PrefetchItem StaleEntryFinalizerTaskTest::CreateAndInsertItem(
     PrefetchItemState state,
     int time_delta_in_hours) {
   PrefetchItem item(item_generator()->CreateItem(state));
-  item.freshness_time =
-      fake_now_ + base::TimeDelta::FromHours(time_delta_in_hours);
+  item.freshness_time = simple_test_clock_.Now() +
+                        base::TimeDelta::FromHours(time_delta_in_hours);
   item.creation_time = item.freshness_time;
   EXPECT_TRUE(store_util()->InsertPrefetchItem(item))
       << "Failed inserting item with state " << static_cast<int>(state);