Various small cleanups in Sync's StartController

This reorders and renames some members, extracts logic into helpers,
removes an unnecessary ForTest method, etc. No functional changes.

While we're here, also switch from ThreadTaskRunnerHandle to
SequencedTaskRunnerHandle - nothing here requires thread affinity.

Bug: 846238, 854978
Change-Id: Iaeaf7664ec78a23acbc0dfaf84122dea9ca4ef38
Reviewed-on: https://chromium-review.googlesource.com/1109897Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569245}

components/sync/driver/startup_controller.cc

@@ -4,12 +4,14 @@
 
 #include "components/sync/driver/startup_controller.h"
 
+#include <utility>
+
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/single_thread_task_runner.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 
@@ -17,12 +19,33 @@ namespace syncer {
 
 namespace {
 
-// The amount of time we'll wait to initialize sync if no data type triggers
-// initialization via a StartSyncFlare.
-const int kDeferredInitFallbackSeconds = 10;
+// The amount of time we'll wait to initialize sync if no data type requests
+// immediately initialization.
+const int kDefaultDeferredInitDelaySeconds = 10;
+
+base::TimeDelta GetDeferredInitDelay() {
+  const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
+  if (cmdline->HasSwitch(switches::kSyncDeferredStartupTimeoutSeconds)) {
+    int timeout = 0;
+    if (base::StringToInt(cmdline->GetSwitchValueASCII(
+                              switches::kSyncDeferredStartupTimeoutSeconds),
+                          &timeout)) {
+      DCHECK_GE(timeout, 0);
+      DVLOG(2) << "Sync StartupController overriding startup timeout to "
+               << timeout << " seconds.";
+      return base::TimeDelta::FromSeconds(timeout);
+    }
+  }
+  return base::TimeDelta::FromSeconds(kDefaultDeferredInitDelaySeconds);
+}
+
+bool IsDeferredStartupEnabled() {
+  return !base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kSyncDisableDeferredStartup);
+}
 
-// Enum (for UMA, primarily) defining different events that cause us to
-// exit the "deferred" state of initialization and invoke start_engine.
+// Enum for UMA defining different events that cause us to exit the "deferred"
+// state of initialization and invoke start_engine.
 enum DeferredInitTrigger {
   // We have received a signal from a SyncableService requesting that sync
   // starts as soon as possible.
@@ -35,35 +58,19 @@ enum DeferredInitTrigger {
 }  // namespace
 
 StartupController::StartupController(const SyncPrefs* sync_prefs,
-                                     base::Callback<bool()> can_start,
-                                     base::Closure start_engine)
-    : bypass_setup_complete_(false),
+                                     base::RepeatingCallback<bool()> can_start,
+                                     base::RepeatingClosure start_engine)
+    : sync_prefs_(sync_prefs),
+      can_start_(std::move(can_start)),
+      start_engine_(std::move(start_engine)),
+      bypass_setup_complete_(false),
       received_start_request_(false),
       setup_in_progress_(false),
-      sync_prefs_(sync_prefs),
-      can_start_(can_start),
-      start_engine_(start_engine),
-      fallback_timeout_(
-          base::TimeDelta::FromSeconds(kDeferredInitFallbackSeconds)),
-      weak_factory_(this) {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDeferredStartupTimeoutSeconds)) {
-    int timeout = kDeferredInitFallbackSeconds;
-    if (base::StringToInt(
-            base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-                switches::kSyncDeferredStartupTimeoutSeconds),
-            &timeout)) {
-      DCHECK_GE(timeout, 0);
-      DVLOG(2) << "Sync StartupController overriding startup timeout to "
-               << timeout << " seconds.";
-      fallback_timeout_ = base::TimeDelta::FromSeconds(timeout);
-    }
-  }
-}
+      weak_factory_(this) {}
 
 StartupController::~StartupController() {}
 
-void StartupController::Reset(const ModelTypeSet registered_types) {
+void StartupController::Reset(const ModelTypeSet& registered_types) {
   received_start_request_ = false;
   bypass_setup_complete_ = false;
   start_up_time_ = base::Time();
@@ -86,16 +93,14 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
     start_up_time_ = base::Time::Now();
   }
 
-  if (deferred_option == STARTUP_DEFERRED &&
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup) &&
+  if (deferred_option == STARTUP_DEFERRED && IsDeferredStartupEnabled() &&
       sync_prefs_->GetPreferredDataTypes(registered_types_).Has(SESSIONS)) {
     if (first_start) {
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
           FROM_HERE,
           base::Bind(&StartupController::OnFallbackStartupTimerExpired,
                      weak_factory_.GetWeakPtr()),
-          fallback_timeout_);
+          GetDeferredInitDelay());
     }
     return;
   }
@@ -106,11 +111,6 @@ void StartupController::StartUp(StartUpDeferredOption deferred_option) {
   }
 }
 
-void StartupController::OverrideFallbackTimeoutForTest(
-    const base::TimeDelta& timeout) {
-  fallback_timeout_ = timeout;
-}
-
 void StartupController::TryStart() {
   if (!can_start_.Run()) {
     return;
@@ -145,8 +145,7 @@ void StartupController::RecordTimeDeferred() {
 }
 
 void StartupController::OnFallbackStartupTimerExpired() {
-  DCHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kSyncDisableDeferredStartup));
+  DCHECK(IsDeferredStartupEnabled());
 
   if (!start_engine_time_.is_null())
     return;
@@ -162,15 +161,13 @@ void StartupController::OnFallbackStartupTimerExpired() {
 std::string StartupController::GetEngineInitializationStateString() const {
   if (!start_engine_time_.is_null())
     return "Started";
-  else if (!start_up_time_.is_null())
+  if (!start_up_time_.is_null())
     return "Deferred";
-  else
-    return "Not started";
+  return "Not started";
 }
 
 void StartupController::OnDataTypeRequestsSyncStartup(ModelType type) {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kSyncDisableDeferredStartup)) {
+  if (!IsDeferredStartupEnabled()) {
     DVLOG(2) << "Ignoring data type request for sync startup: "
              << ModelTypeToString(type);
     return;

components/sync/driver/startup_controller.h

@@ -21,8 +21,8 @@ class SyncPrefs;
 class StartupController {
  public:
   StartupController(const SyncPrefs* sync_prefs,
-                    base::Callback<bool()> can_start,
-                    base::Closure start_engine);
+                    base::RepeatingCallback<bool()> can_start,
+                    base::RepeatingClosure start_engine);
   ~StartupController();
 
   // Starts up sync if it is requested by the user and preconditions are met.
@@ -45,7 +45,7 @@ class StartupController {
   // touch values that are explicitly set and reset by higher layers to
   // tell this class whether a setup UI dialog is being shown to the user.
   // See setup_in_progress_.
-  void Reset(const ModelTypeSet registered_types);
+  void Reset(const ModelTypeSet& registered_types);
 
   // Sets the setup in progress flag and tries to start sync if it's true.
   void SetSetupInProgress(bool setup_in_progress);
@@ -54,8 +54,6 @@ class StartupController {
   base::Time start_engine_time() const { return start_engine_time_; }
   std::string GetEngineInitializationStateString() const;
 
-  void OverrideFallbackTimeoutForTest(const base::TimeDelta& timeout);
-
  private:
   enum StartUpDeferredOption { STARTUP_DEFERRED, STARTUP_IMMEDIATE };
 
@@ -65,17 +63,32 @@ class StartupController {
   // Records time spent in deferred state with UMA histograms.
   void RecordTimeDeferred();
 
+  const SyncPrefs* sync_prefs_;
+
+  // A function that can be invoked repeatedly to determine whether sync can be
+  // started. |start_engine_| should not be invoked unless this returns true.
+  const base::RepeatingCallback<bool()> can_start_;
+
+  // The callback we invoke when it's time to call expensive
+  // startup routines for the sync engine.
+  const base::RepeatingClosure start_engine_;
+
+  // Used to compute preferred_types from SyncPrefs as-needed.
+  ModelTypeSet registered_types_;
+
   // If true, will bypass the FirstSetupComplete check when triggering sync
-  // startup.
+  // startup. Set in TryStartImmediately.
   bool bypass_setup_complete_;
 
-  // True if we should start sync ASAP because either a SyncableService has
-  // requested it, or we're done waiting for a sign and decided to go ahead.
+  // True if we should start sync ASAP because either a data type has
+  // requested it, or TryStartImmediately was called, or our deferred startup
+  // timer has expired.
   bool received_start_request_;
 
   // The time that StartUp() is called. This is used to calculate time spent
   // in the deferred state; that is, after StartUp and before invoking the
-  // start_engine_ callback.
+  // start_engine_ callback. If this is non-null, then a (possibly deferred)
+  // startup has been triggered.
   base::Time start_up_time_;
 
   // If |true|, there is setup UI visible so we should not start downloading
@@ -85,24 +98,10 @@ class StartupController {
   // due to explicit requests to do so via SetSetupInProgress.
   bool setup_in_progress_;
 
-  const SyncPrefs* sync_prefs_;
-
-  // A function that can be invoked repeatedly to determine whether sync can be
-  // started. |start_engine_| should not be invoked unless this returns true.
-  base::Callback<bool()> can_start_;
-
-  // The callback we invoke when it's time to call expensive
-  // startup routines for the sync engine.
-  base::Closure start_engine_;
-
-  // The time at which we invoked the start_engine_ callback.
+  // The time at which we invoked the |start_engine_| callback. If this is
+  // non-null, then |start_engine_| shouldn't be called again.
   base::Time start_engine_time_;
 
-  base::TimeDelta fallback_timeout_;
-
-  // Used to compute preferred_types from SyncPrefs as-needed.
-  ModelTypeSet registered_types_;
-
   base::WeakPtrFactory<StartupController> weak_factory_;
 };
 

components/sync/driver/startup_controller_unittest.cc

@@ -29,6 +29,9 @@ class StartupControllerTest : public testing::Test {
   StartupControllerTest() : can_start_(false), started_(false) {}
 
   void SetUp() override {
+    base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+        switches::kSyncDeferredStartupTimeoutSeconds, "0");
+
     SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
     sync_prefs_ = std::make_unique<SyncPrefs>(&pref_service_);
     controller_ = std::make_unique<StartupController>(
@@ -37,19 +40,10 @@ class StartupControllerTest : public testing::Test {
         base::Bind(&StartupControllerTest::FakeStartBackend,
                    base::Unretained(this)));
     controller_->Reset(UserTypes());
-    controller_->OverrideFallbackTimeoutForTest(
-        base::TimeDelta::FromSeconds(0));
   }
 
-  bool CanStart() { return can_start_; }
-
   void SetCanStart(bool can_start) { can_start_ = can_start; }
 
-  void FakeStartBackend() {
-    started_ = true;
-    sync_prefs()->SetFirstSetupComplete();
-  }
-
   void ExpectStarted() {
     EXPECT_TRUE(started());
     EXPECT_EQ(kStateStringStarted,
@@ -77,6 +71,13 @@ class StartupControllerTest : public testing::Test {
   SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
 
  private:
+  bool CanStart() { return can_start_; }
+
+  void FakeStartBackend() {
+    started_ = true;
+    sync_prefs()->SetFirstSetupComplete();
+  }
+
   bool can_start_;
   bool started_;
   base::MessageLoop message_loop_;