sync: centralize sync startup decisions in TryStart.

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 9b8e922e9d28ddcc8dfe68a01b833e05e18ef0b0..a19f7e593bb3af33019c1aec831418d766ee1f9d 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -271,7 +271,11 @@ void ProfileSyncService::TryStart() {
   // loaded, and we don't want to generate spurious auth errors.
   if (IsSyncTokenAvailable() ||
       (!auto_start_enabled_ && token_service->TokensLoadedFromDB())) {

[kochi, 2012/11/30 07:42:02]

How about reversing these conditions just for return? i.e.

if (!IsSyncTokenAvailable() &&
    !(!auto_start_enabled_ && token_service->TokensLoadedFromDB())
  return;
...
if (!HasSyncSetupCompleted() && !setup_in_progress_ &&
    !auto_start_enabled_)
  return;

  // All green!
  StartUp();

Logically all these conditions can be combined in one
if (), but for the clarity, it may be better to discompose
the conditions and comment for each.  Compilers should be
able to optimize.

[tim (not reviewing), 2012/11/30 18:51:13]

I had played around with this but all the negatives made it feel equally
confusing to me :)  But, I changed it... willing to give it a whirl and let it
sink in!

-    if (HasSyncSetupCompleted() || auto_start_enabled_) {
+    // Make sure it's safe to start -- the user has either declared intent (

[Andrew T Wilson (Slow), 2012/11/30 14:52:15]

A dangling parenthesis? YOU MONSTER

[tim (not reviewing), 2012/11/30 18:51:13]

Hah. Shame on me. Fixed.

+    // setup has either completed, or the user is in that process now and we
+    // should start the backend to download account control state / encryption
+    // information), or this is a platform where the intent to sync is implicit.
+    if (HasSyncSetupCompleted() || setup_in_progress_ || auto_start_enabled_) {
       // If sync setup has completed we always start the backend.
       // 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
@@ -1716,11 +1720,9 @@ void ProfileSyncService::OnSyncManagedPrefChange(bool is_sync_managed) {
   NotifyObservers();
   if (is_sync_managed) {
     DisableForUser();
-  } else if (HasSyncSetupCompleted() &&
-             IsSyncEnabledAndLoggedIn() &&
-             IsSyncTokenAvailable()) {
+  } else {
     // Previously-configured sync has been re-enabled, so start sync now.

[Andrew T Wilson (Slow), 2012/11/30 14:52:15]

Comment is now obsolete since we don't know that sync has been configured (we no
longer check HasSyncSetupCompleted() in this routine). Maybe this should say
"Sync is no longer disabled by policy - try starting it up if appropriate." 

[tim (not reviewing), 2012/11/30 18:51:13]

Done.

-    StartUp();
+    TryStart();
   }
 }
 
@@ -1771,34 +1773,19 @@ void ProfileSyncService::Observe(int type,
       const TokenService::TokenAvailableDetails& token_details =
           *(content::Details<const TokenService::TokenAvailableDetails>(
               details).ptr());
-      if (IsTokenServiceRelevant(token_details.service()) &&
-          IsSyncEnabledAndLoggedIn() &&
-          IsSyncTokenAvailable()) {
-        if (backend_initialized_)
-          backend_->UpdateCredentials(GetCredentials());
-        else
-          StartUp();
-      }
-      break;
-    }
+      if (!IsTokenServiceRelevant(token_details.service()))
+        break;
+    } // Fall through.

[Andrew T Wilson (Slow), 2012/11/30 14:52:15]

Falling through is fine, although the code is simple enough now that you could
just duplicate those 4 lines.

[tim (not reviewing), 2012/11/30 18:51:13]

Done.

     case chrome::NOTIFICATION_TOKEN_LOADING_FINISHED: {
       // This notification gets fired when TokenService loads the tokens
       // from storage.
-      if (IsSyncEnabledAndLoggedIn()) {
-        // Don't start up sync and generate an auth error on auto_start
-        // platforms as they have their own way to resolve TokenService errors.
-        // (crbug.com/128592).
-        if (auto_start_enabled_ && !IsSyncTokenAvailable())
-          break;
-
-        // Initialize the backend if sync is enabled. If the sync token was
-        // not loaded, GetCredentials() will generate invalid credentials to
-        // cause the backend to generate an auth error (crbug.com/121755).
-        if (backend_initialized_)
-          backend_->UpdateCredentials(GetCredentials());
-        else
-          StartUp();
-      }
+      // Initialize the backend if sync is enabled. If the sync token was
+      // not loaded, GetCredentials() will generate invalid credentials to
+      // cause the backend to generate an auth error (crbug.com/121755).
+      if (backend_initialized_)
+        backend_->UpdateCredentials(GetCredentials());
+      else
+        TryStart();
       break;
     }
     default: {