Make SyncBackendRegistrar aware of loaded data

chrome/browser/sync/glue/sync_backend_host.cc

Index: chrome/browser/sync/glue/sync_backend_host.cc
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 206773525ca0e2aeb2069095ce3d93b190ece7a0..21f13c2fc78166099d9b2b5ebcb8c1d34159d9c0 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -87,7 +87,8 @@ class SyncBackendHost::Core
       const syncer::sessions::SyncSessionSnapshot& snapshot) OVERRIDE;
   virtual void OnInitializationComplete(
       const syncer::WeakHandle<syncer::JsBackend>& js_backend,
-      bool success) OVERRIDE;
+      bool success,
+      syncer::ModelTypeSet restored_types) OVERRIDE;
   virtual void OnConnectionStatusChange(
       syncer::ConnectionStatus status) OVERRIDE;
   virtual void OnPassphraseRequired(
@@ -365,7 +366,6 @@ void SyncBackendHost::Initialize(
     SyncFrontend* frontend,
     const syncer::WeakHandle<syncer::JsEventHandler>& event_handler,
     const GURL& sync_service_url,
-    syncer::ModelTypeSet initial_types,
     const SyncCredentials& credentials,
     bool delete_sync_data_folder,
     syncer::SyncManagerFactory* sync_manager_factory,
@@ -378,14 +378,7 @@ void SyncBackendHost::Initialize(
   frontend_ = frontend;
   DCHECK(frontend);
 
-  syncer::ModelTypeSet initial_types_with_nigori(initial_types);
-  CHECK(sync_prefs_.get());
-  if (sync_prefs_->HasSyncSetupCompleted()) {
-    initial_types_with_nigori.Put(syncer::NIGORI);
-  }
-
-  registrar_.reset(new SyncBackendRegistrar(initial_types_with_nigori,
-                                            name_,
+  registrar_.reset(new SyncBackendRegistrar(name_,
                                             profile_,
                                             sync_thread_.message_loop()));
   syncer::ModelSafeRoutingInfo routing_info;
@@ -592,6 +585,14 @@ void SyncBackendHost::ConfigureDataTypes(
     NigoriState nigori_state,
     const base::Callback<void(syncer::ModelTypeSet)>& ready_task,
     const base::Callback<void()>& retry_callback) {
+  // Only one configure is allowed at a time.  This is guaranteed by our
+  // callers.  The SyncBackendHost requests one  configure as the backend is
+  // initializing and waits for it to complete.  After initialization, all
+  // configurations will pass through the DataTypeMnaager, which is careful to
+  // never send a new configure request until the current request succeeds.

[Nicolas Zea, 2012/07/20 21:35:39]

I think it'd be useful to also mention that the backend doesn't know about any
newly supported types until after the backend is initialized. During
initialization time the backend only knows about pre-existing types.

Also probably worth adding a TODO that once we stop purging all partial types,
we're going to need to change this to not attempt to download anything other
than the nigori as part of initialization.

[rlarocque, 2012/07/20 23:20:05]

Why add that comment here?  Is the configuration for newly supported types
handled differently?  
 
Why put that comment here, as opposed to
HandleInitializationCompleteOnFrontendLoop()?

+
+  DCHECK_GT(initialization_state_, NOT_INITIALIZED);
+
   syncer::ModelTypeSet types_to_add_with_nigori = types_to_add;
   syncer::ModelTypeSet types_to_remove_with_nigori = types_to_remove;
   if (nigori_state == WITH_NIGORI) {
@@ -601,53 +602,34 @@ void SyncBackendHost::ConfigureDataTypes(
     types_to_add_with_nigori.Remove(syncer::NIGORI);
     types_to_remove_with_nigori.Put(syncer::NIGORI);
   }
-  // Only one configure is allowed at a time (DataTypeManager handles user
-  // changes that happen while the syncer is reconfiguring, and will only
-  // trigger another call to ConfigureDataTypes once the current reconfiguration
-  // completes).
-  DCHECK_GT(initialization_state_, NOT_INITIALIZED);
 
-  // The new set of enabled types is types_to_add_with_nigori + the
-  // previously enabled types (on restart, the preferred types are already
-  // enabled) - types_to_remove_with_nigori. After reconfiguring the registrar,
-  // the new routing info will reflect the set of enabled types.
+  // The SyncBackendRegistrar's routing info will be updated by removing the
+  // types_to_remove_with_nigori from the list then adding

[Nicolas Zea, 2012/07/20 21:35:39]

it's actually the other way around (add, then remove)

[rlarocque, 2012/07/23 20:50:30]

Done.

+  // types_to_add_with_nigori.  Any types which have are not in either of those

[Nicolas Zea, 2012/07/20 21:35:39]

which are not

[rlarocque, 2012/07/23 20:50:30]

Done.

+  // sets will remain untouched.
+  //
+  // Types which were not in previously in the list are not cached locally, so

[Nicolas Zea, 2012/07/20 21:35:39]

not in the list previously

[rlarocque, 2012/07/23 20:50:30]

Done.

+  // we must ask the syncer to download them.
+  //
+  // Note that the SyncBackendRegistrar's state was initially derrived from the
+  // types marked initial_sync_ended when the sync database was loaded.
+
+  syncer::ModelTypeSet types_to_download = registrar_->ConfigureDataTypes(
+      types_to_add_with_nigori, types_to_remove_with_nigori);
+  if (!types_to_download.Empty())
+    types_to_download.Put(syncer::NIGORI);
+
   syncer::ModelSafeRoutingInfo routing_info;
-  registrar_->ConfigureDataTypes(types_to_add_with_nigori,
-                                 types_to_remove_with_nigori);
   registrar_->GetModelSafeRoutingInfo(&routing_info);
-  const syncer::ModelTypeSet enabled_types =
-      GetRoutingInfoTypes(routing_info);
-
-  // Figure out which types need to actually be downloaded. We pass those on
-  // to the syncer while it's in configuration mode so that they can be
-  // downloaded before we perform association. Once we switch to normal mode
-  // downloads will get applied normally and hit the datatype's change
-  // processor.
-  // A datatype is in need of downloading if any of the following are true:
-  // 1. it's enabled and initial_sync_ended is false (initial_sync_ended is
-  //    set after applying updates, and hence is a more conservative measure
-  //    than having a non-empty progress marker, which is set during
-  //    StoreTimestamps).
-  // 2. the type is NIGORI, and any other datatype is being downloaded (nigori
-  //    is always included if we download a datatype).
-  // TODO(sync): consider moving this logic onto the sync thread (perhaps
-  // as part of SyncManager::ConfigureSyncer).
-  syncer::ModelTypeSet initial_sync_ended_types =
-      core_->sync_manager()->InitialSyncEndedTypes();
-  initial_sync_ended_types.RetainAll(enabled_types);
-  syncer::ModelTypeSet types_to_config =
-      Difference(enabled_types, initial_sync_ended_types);
-  if (!types_to_config.Empty() && enabled_types.Has(syncer::NIGORI))
-    types_to_config.Put(syncer::NIGORI);
 
   SDVLOG(1) << "Types "
-            << syncer::ModelTypeSetToString(types_to_config)
+            << syncer::ModelTypeSetToString(types_to_download)
             << " added; calling DoConfigureSyncer";
   // TODO(zea): figure out how to bypass this call if no types are being
   // configured and GetKey is not needed. For now we rely on determining the
   // need for GetKey as part of the SyncManager::ConfigureSyncer logic.
   RequestConfigureSyncer(reason,
-                         types_to_config,
+                         types_to_download,
                          routing_info,
                          ready_task,
                          retry_callback);
@@ -674,6 +656,11 @@ syncer::UserShare* SyncBackendHost::GetUserShare() const {
   return core_->sync_manager()->GetUserShare();
 }
 
+// Bypasses the initialized() check.
+syncer::UserShare* SyncBackendHost::GetUserShareForTest() const {

[Nicolas Zea, 2012/07/20 21:35:39]

I'd almost rather just get rif of DCHECK(initialized()) than have this extra
ForTest method. If we're not initialized, the above method is going to crash
anyways right?

[rlarocque, 2012/07/20 23:20:05]

The intent of this CL is to ensure that the ProfileSyncService can never get a
hold of an improperly initialized UserShare object.  Allowing the
ProfileSyncService to access the UserShare before OnBackendInitialize makes it
harder to enforce this.

I must admit that the ProfileSyncServiceTest code does provide a fairly strong
practical argument for allowing it.  On the other hand, that work has been done,
so the argument that it lets us avoid updating a bunch of test code carries less
weight now.  

+  return core_->sync_manager()->GetUserShare();
+}
+
 SyncBackendHost::Status SyncBackendHost::GetDetailedStatus() {
   DCHECK(initialized());
   return core_->sync_manager()->GetDetailedStatus();
@@ -764,6 +751,13 @@ void SyncBackendHost::FinishConfigureDataTypesOnFrontendLoop(
     ready_task.Run(failed_configuration_types);
 }
 
+void SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop(
+    const syncer::WeakHandle<syncer::JsBackend>& js_backend, bool success,
+    syncer::ModelTypeSet restored_types) {
+  registrar_->SetInitialTypes(restored_types);
+  HandleInitializationCompletedOnFrontendLoop(js_backend, success);
+}
+
 SyncBackendHost::DoInitializeOptions::DoInitializeOptions(
     MessageLoop* sync_loop,
     SyncBackendRegistrar* registrar,
@@ -835,12 +829,13 @@ void SyncBackendHost::Core::OnSyncCycleCompleted(
 
 void SyncBackendHost::Core::OnInitializationComplete(
     const syncer::WeakHandle<syncer::JsBackend>& js_backend,
-    bool success) {
+    bool success,
+    const syncer::ModelTypeSet restored_types) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   host_.Call(
       FROM_HERE,
-      &SyncBackendHost::HandleInitializationCompletedOnFrontendLoop,
-      js_backend, success);
+      &SyncBackendHost::HandleSyncManagerInitializationOnFrontendLoop,
+      js_backend, success, restored_types);
 
   if (success) {
     // Initialization is complete, so we can schedule recurring SaveChanges.
@@ -971,7 +966,6 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.service_url.SchemeIsSecure(),
       BrowserThread::GetBlockingPool(),
       options.make_http_bridge_factory_fn.Run().Pass(),
-      options.routing_info,
       options.workers,
       options.extensions_activity_monitor,
       options.registrar /* as SyncManager::ChangeDelegate */,
@@ -1168,14 +1162,6 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
     return;
   }
 
-  // If setup has completed, start off in DOWNLOADING_NIGORI so that
-  // we start off by refreshing nigori.
-  CHECK(sync_prefs_.get());
-  if (sync_prefs_->HasSyncSetupCompleted() &&
-      initialization_state_ < DOWNLOADING_NIGORI) {
-    initialization_state_ = DOWNLOADING_NIGORI;
-  }
-
   // Run initialization state machine.
   switch (initialization_state_) {
     case NOT_INITIALIZED: