[Sync] Support open tabs experiment enabling before sync setup completion.

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2011 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 <algorithm>
 #include <cstddef>
 #include <map>
 #include <set>
@@ skipping 589 matching lines @@
                    GetSessionModelAssociator()->AsWeakPtr()));
   }
   VLOG(2) << "Notifying observers sync cycle completed";
   NotifyObservers();
 }
 
 // TODO(sync): eventually support removing datatypes too.
 void ProfileSyncService::OnDataTypesChanged(
     const syncable::ModelTypeSet& to_add) {
   // We don't bother doing anything if the migrator is busy.
-  if (migrator_->state() != browser_sync::BackendMigrator::IDLE) {
+  if (migrator_.get() &&

[akalin, 2011/10/15 01:21:55]

can remove the _.get() check, or convert to CHECK

[Nicolas Zea, 2011/10/15 02:06:27]

This is getting called before the frontend is notified of the backend
initializing (else the sync setup screen shows before we enable the type), so
these conditions have to stay.

[akalin, 2011/10/18 19:13:42]

Can you add a comment here to that effect?

+      migrator_->state() != browser_sync::BackendMigrator::IDLE) {
     VLOG(1) << "Dropping OnDataTypesChanged due to migrator busy.";
     return;
   }
 
   VLOG(2) << "OnDataTypesChanged called with types: "
           << syncable::ModelTypeSetToString(to_add);
 
   syncable::ModelTypeSet registered_types;
   GetRegisteredDataTypes(&registered_types);
 
   syncable::ModelTypeSet to_register;
   std::set_difference(to_add.begin(), to_add.end(),
                       registered_types.begin(), registered_types.end(),
                       std::inserter(to_register, to_register.end()));
 
   VLOG(2) << "Enabling types: " << syncable::ModelTypeSetToString(to_register);
 
   for (syncable::ModelTypeSet::const_iterator it = to_register.begin();
        it != to_register.end(); ++it) {
-    // Received notice to enable session sync. Check if sessions are
+    // Received notice to enable experimental type. Check if the type is
     // registered, and if not register a new datatype controller.
     RegisterNewDataType(*it);
-    // Enable the about:flags switch for sessions so we don't have to always
-    // perform this reconfiguration. Once we set this, sessions will remain
-    // registered, so we will no longer go down this code path.
+    // Enable the about:flags switch for the experimental type so we don't have
+    // to always perform this reconfiguration. Once we set this, the type will
+    // remain registered on restart, so we will no longer go down this code
+    // path.
     std::string experiment_name = GetExperimentNameForDataType(*it);
     if (experiment_name.empty())
       continue;
     about_flags::SetExperimentEnabled(g_browser_process->local_state(),
                                       experiment_name,
                                       true);
   }
 
   // Check if the user has "Keep Everything Synced" enabled. If so, we want
-  // to turn on sessions if it's not already on. Otherwise we leave it off.
-  // Note: if sessions are already registered, we don't turn it on. This
+  // to turn on all experimental types if they're not already on. Otherwise we
+  // leave them off.
+  // Note: if any types are already registered, we don't turn them on. This
   // covers the case where we're already in the process of reconfiguring
-  // to turn sessions on.
+  // to turn an experimental type on.
   if (sync_prefs_.HasKeepEverythingSynced()) {
     // Mark all data types as preferred.
     sync_prefs_.SetPreferredDataTypes(registered_types, registered_types);
 
-    if (!to_register.empty()) {
+    // Only automatically turn on types if we have already finished set up.
+    // Otherwise, just leave the experimental types on by default.
+    if (!to_register.empty() && HasSyncSetupCompleted() && migrator_.get()) {

[akalin, 2011/10/15 01:21:55]

can remove migrator_.get() check

[akalin, 2011/10/15 01:21:55]

do we still need the HasSyncSetupCompleted check?

[Nicolas Zea, 2011/10/15 02:06:27]

see above.

[Nicolas Zea, 2011/10/15 02:06:27]

Theoretically the migrator check covers this case, and vice versa, but I'd
rather not make assumptions about both of them being there in case something
changes in the future. The main point here is that we don't want to reconfigure
if the user hasn't finished setting up yet, so HasSyncSetupCompleted is the main
condition.

       VLOG(1) << "Dynamically enabling new datatypes: "
               << syncable::ModelTypeSetToString(to_register);
       OnMigrationNeededForTypes(to_register);
     }
   }
 }
 
 void ProfileSyncService::UpdateAuthErrorState(
     const GoogleServiceAuthError& error) {
   last_auth_error_ = error;
@@ skipping 858 matching lines @@
             << "Unrecoverable error.";
   } else {
     VLOG(0) << "ConfigureDataTypeManager not invoked because backend is not "
             << "initialized";
   }
 }
 
 const FailedDatatypesHandler& ProfileSyncService::failed_datatypes_handler() {
   return failed_datatypes_handler_;
 }