[Sync] Simplify sync startup behavior.

components/sync_driver/startup_controller.cc

 // Copyright 2014 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/sync_driver/startup_controller.h"
 
 #include <string>
 
 #include "base/command_line.h"
 #include "base/location.h"
@@ skipping 21 matching lines @@
   // starts as soon as possible.
   TRIGGER_DATA_TYPE_REQUEST,
   // No data type requested sync to start and our fallback timer expired.
   TRIGGER_FALLBACK_TIMER,
   MAX_TRIGGER_VALUE
 };
 
 }  // 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),
       start_backend_(start_backend),
       fallback_timeout_(
           base::TimeDelta::FromSeconds(kDeferredInitFallbackSeconds)),
       first_start_(true),
       weak_factory_(this) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSyncDeferredStartupTimeoutSeconds)) {
@@ skipping 76 matching lines @@
     return false;
   }
 
   // TODO(tim): Seems wrong to always record this histogram here...
   // If we got here then tokens are loaded and user logged in and sync is
   // enabled. If OAuth refresh token is not available then something is wrong.
   // When PSS requests access token, OAuth2TokenService will return error and
   // 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() {
   DCHECK(!start_up_time_.is_null());
   base::TimeDelta time_deferred = base::Time::Now() - start_up_time_;
   UMA_HISTOGRAM_CUSTOM_TIMES("Sync.Startup.TimeDeferred2",
       time_deferred,
       base::TimeDelta::FromSeconds(0),
       base::TimeDelta::FromMinutes(2),
       60);
@@ skipping 47 matching lines @@
                               syncer::MODEL_TYPE_COUNT);
     UMA_HISTOGRAM_ENUMERATION("Sync.Startup.DeferredInitTrigger",
                               TRIGGER_DATA_TYPE_REQUEST,
                               MAX_TRIGGER_VALUE);
   }
   received_start_request_ = true;
   TryStart();
 }
 
 }  // namespace browser_sync