sync: centralize sync startup decisions in TryStart.

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2012 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 "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 252 matching lines @@
   TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
   if (!token_service)
     return;
   // Don't start the backend if the token service hasn't finished loading tokens
   // yet (if the backend is started before the sync token has been loaded,
   // GetCredentials() will return bogus credentials). On auto_start platforms
   // (like ChromeOS) we don't start sync until tokens are loaded, because the
   // user can be "signed in" on those platforms long before the tokens get
   // 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
       // logging in but before the backend finished initializing the last time.
       // Note that if we haven't finished setting up sync, backend bring up will
       // be done by the wizard.
       StartUp();
     }
   }
 }
@@ skipping 1424 matching lines @@
   DCHECK(encrypted_types_.Has(syncer::PASSWORDS));
   // We may be called during the setup process before we're
   // initialized.  In this case, we default to the sensitive types.
   return encrypted_types_;
 }
 
 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();
   }
 }
 
 void ProfileSyncService::Observe(int type,
                                  const content::NotificationSource& source,
                                  const content::NotificationDetails& details) {
   switch (type) {
     case chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL: {
       const GoogleServiceSigninSuccessDetails* successful =
           content::Details<const GoogleServiceSigninSuccessDetails>(
@@ skipping 30 matching lines @@
         // from the sync backend.
         AuthError error(AuthError::INVALID_GAIA_CREDENTIALS);
         UpdateAuthErrorState(error);
       }
       break;
     }
     case chrome::NOTIFICATION_TOKEN_AVAILABLE: {
       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: {
       NOTREACHED();
     }
   }
 }
 
 void ProfileSyncService::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
@@ skipping 113 matching lines @@
   // See http://stackoverflow.com/questions/6224121/is-new-this-myclass-undefined-behaviour-after-directly-calling-the-destru.
   ProfileSyncService* old_this = this;
   this->~ProfileSyncService();
   new(old_this) ProfileSyncService(
       new ProfileSyncComponentsFactoryImpl(profile,
                                            CommandLine::ForCurrentProcess()),
       profile,
       signin,
       behavior);
 }