[Sync] Simplify sync startup behavior.

components/sync_driver/startup_controller.cc

Index: components/sync_driver/startup_controller.cc
diff --git a/components/sync_driver/startup_controller.cc b/components/sync_driver/startup_controller.cc
index 4db2bb2371814495aac3a98fe234a4af1bbc2b07..e98e97b9d85e0ed3125a32b28844a7c957bf3d02 100644
--- a/components/sync_driver/startup_controller.cc
+++ b/components/sync_driver/startup_controller.cc
@@ -39,14 +39,12 @@ enum DeferredInitTrigger {
 }  // namespace
 
 StartupController::StartupController(
-    ProfileSyncServiceStartBehavior start_behavior,
     const ProfileOAuth2TokenService* token_service,
     const sync_driver::SyncPrefs* sync_prefs,
     const SigninManagerWrapper* signin,
     base::Closure start_backend)
     : received_start_request_(false),
       setup_in_progress_(false),
-      auto_start_enabled_(start_behavior == AUTO_START),
       sync_prefs_(sync_prefs),
       token_service_(token_service),
       signin_(signin),
@@ -143,34 +141,18 @@ bool StartupController::TryStart() {
   // PSS will show error to user asking to reauthenticate.
   UMA_HISTOGRAM_BOOLEAN("Sync.RefreshTokenAvailable", true);
 
-  // If sync setup has completed we always start the backend. If the user is in
-  // the process of setting up now, we should start the backend to download
-  // account control state / encryption information). If autostart is enabled,
-  // but we haven't completed sync setup, we try to start sync anyway, since
-  // it's possible we crashed/shutdown after logging in but before the backend
-  // finished initializing the last time.
+  // For performance reasons, defer the heavy lifting for sync init if:
   //
-  // However, the only time we actually need to start sync _immediately_ is if
-  // we haven't completed sync setup and the user is in the process of setting
-  // up - either they just signed in (for the first time) on an auto-start
-  // platform or they explicitly kicked off sync setup, and e.g we need to
-  // fetch account details like encryption state to populate UI. Otherwise,
-  // for performance reasons and maximizing parallelism at chrome startup, we
-  // defer the heavy lifting for sync init until things have calmed down.
-  if (sync_prefs_->IsFirstSetupComplete()) {
-    // For first time, defer start if data type hasn't requested sync to avoid
-    // stressing browser start.
-    if (!received_start_request_ && first_start_)
-      return StartUp(STARTUP_BACKEND_DEFERRED);
-    else
-      return StartUp(STARTUP_IMMEDIATE);
-  } else if (setup_in_progress_ || auto_start_enabled_) {
-    // We haven't completed sync setup. Start immediately if the user explicitly
-    // kicked this off or we're supposed to automatically start syncing.
+  // - This is the first start of sync, to prevent competing for resources
+  //   during browser startup.
+  // - No datatype has requested an immediate start of sync.
+  // - Sync does not need to start up the backend immediately to provide control
+  //   state and encryption information to the UI.
+  if (first_start_ && !received_start_request_ && !setup_in_progress_) {

[Nicolas Zea, 2016/02/29 21:21:55]

This doesn't seem right. Previously if this was CrOS/Android, and it's the first
startup we would start immediately. On the other hand, if it's a restart, we
would defer. The first_start condition here doesn't seem to make sense.

It would  also be good to explain in the comment whether all of the items stated
need to be true.

[maxbogue, 2016/03/09 02:00:59]

I've clarified the comment; first_start means the first start in this browser
lifetime, not the first start of sync since sync was enabled. Now that you've
mentioned that, isn't this fixing a bug? Don't we want sync startup to still
defer on ChromeOS and Android so it doesn't slow down the browser startup?
Gah I think my ", and" clauses got eaten by a vim buffer. Fixed.

[Nicolas Zea, 2016/03/09 19:49:45]

Sync should always start up immediately the first time it gets set up. It's only
on restarts that we want to defer. 

Now that I look more closely, this first_start_ boolean was actually introduced
for backup/rollback, and doesn't relate to first time sync vs restart. I think
we can probably get rid of it altogether.

Seems like the condition here should be:
if (setup_in_progress || received_start_request || !FirstSetupComplete)
.. immediate
else
.. deferred

That said, I don't think that catches the first time startup case on
Android/ChromeOS. In those platforms, maybe you need to manually trigger
received_start_request in the same place you set FirstSetupComplete?

[maxbogue, 2016/03/10 18:48:15]

I was wondering if first_start_ was actually needed any more. Glad to see it's
not. I've made the change you suggested and updated the comment. I believe it
will still work on ChromeOS and Android because auto start doesn't flip the
FirstSetupComplete flag until after the backend has started up anyways.

+    return StartUp(STARTUP_BACKEND_DEFERRED);
+  } else {
     return StartUp(STARTUP_IMMEDIATE);
   }
-
-  return false;
 }
 
 void StartupController::RecordTimeDeferred() {