Sync: Add DeviceInfo's ChangeProcessor

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 cd565da73b8dfbff1f47c12d807b58dd5bf00451..797567a74785706b7354c3ca7732fe26ca3d2e5a 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -16,7 +16,6 @@
 #include "base/file_util.h"
 #include "base/location.h"
 #include "base/metrics/histogram.h"
-#include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/timer.h"
 #include "base/tracked_objects.h"
@@ -31,6 +30,8 @@
 #include "chrome/browser/sync/glue/chrome_sync_notification_bridge.h"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/sync_backend_registrar.h"
+#include "chrome/browser/sync/glue/synced_device_tracker.h"
+#include "chrome/browser/sync/glue/device_info.h"

[Nicolas Zea, 2012/11/16 19:42:27]

abc order

[rlarocque, 2012/11/17 00:07:21]

That file was included twice, actually.  Must have been a bad merge.  Fixed.

 #include "chrome/browser/sync/invalidations/invalidator_storage.h"
 #include "chrome/browser/sync/sync_prefs.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -166,9 +167,12 @@ class SyncBackendHost::Core
   // reencrypt everything.
   void DoEnableEncryptEverything();
 
-  // Called to load sync encryption state and re-encrypt any types
-  // needing encryption as necessary.
-  void DoAssociateNigori();
+  // Called to perform tasks which require the control data to be downloaded.
+  // This includes refreshing encryption, setting up the device info change
+  // processor, etc.
+  void DoInitialProcessControlTypes();
+
+  void FinishInitialProcessControlTypes();

[Nicolas Zea, 2012/11/16 19:42:27]

Comment about why these two are separated?

[rlarocque, 2012/11/17 00:07:21]

Done.

 
   // The shutdown order is a bit complicated:
   // 1) From |sync_thread_|, invoke the syncapi Shutdown call to do
@@ -203,6 +207,10 @@ class SyncBackendHost::Core
 
   syncer::SyncManager* sync_manager() { return sync_manager_.get(); }
 
+  SyncedDeviceTracker* synced_device_tracker() {
+    return synced_device_tracker_.get();
+  }
+
   // Delete the sync data folder to cleanup backend data.  Happens the first
   // time sync is enabled for a user (to prevent accidentally reusing old
   // sync databases), as well as shutdown when you're no longer syncing.
@@ -255,6 +263,9 @@ class SyncBackendHost::Core
   // Our encryptor, which uses Chrome's encryption functions.
   ChromeEncryptor encryptor_;
 
+  // A special ChangeProcessor that tracks the DEVICE_INFO type for us.
+  scoped_ptr<SyncedDeviceTracker> synced_device_tracker_;
+
   // The top-level syncapi entry point.  Lives on the sync thread.
   scoped_ptr<syncer::SyncManager> sync_manager_;
 
@@ -761,6 +772,10 @@ void SyncBackendHost::GetModelSafeRoutingInfo(
   }
 }
 
+SyncedDeviceTracker* SyncBackendHost::GetSyncedDeviceTrackerForTest() {
+  return core_->synced_device_tracker();
+}
+
 void SyncBackendHost::InitCore(const DoInitializeOptions& options) {
   sync_thread_.message_loop()->PostTask(FROM_HERE,
       base::Bind(&SyncBackendHost::Core::DoInitialize, core_.get(), options));
@@ -1079,7 +1094,6 @@ void SyncBackendHost::Core::DoInitialize(const DoInitializeOptions& options) {
       options.service_url.host() + options.service_url.path(),
       options.service_url.EffectiveIntPort(),
       options.service_url.SchemeIsSecure(),
-      BrowserThread::GetBlockingPool(),
       options.make_http_bridge_factory_fn.Run().Pass(),
       options.workers,
       options.extensions_activity_monitor,
@@ -1143,14 +1157,6 @@ void SyncBackendHost::Core::DoStartSyncing(
   sync_manager_->StartSyncingNormally(routing_info);
 }
 
-void SyncBackendHost::Core::DoAssociateNigori() {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  sync_manager_->GetEncryptionHandler()->Init();
-  host_.Call(FROM_HERE,
-             &SyncBackendHost::HandleInitializationCompletedOnFrontendLoop,
-             true);
-}
-
 void SyncBackendHost::Core::DoSetEncryptionPassphrase(
     const std::string& passphrase,
     bool is_explicit) {
@@ -1159,6 +1165,39 @@ void SyncBackendHost::Core::DoSetEncryptionPassphrase(
       passphrase, is_explicit);
 }
 
+void SyncBackendHost::Core::DoInitialProcessControlTypes() {
+  DCHECK_EQ(MessageLoop::current(), sync_loop_);
+
+  DVLOG(1) << "Initilalizing Control Types";
+
+  // Initialize encryption.
+  sync_manager_->GetEncryptionHandler()->Init();
+
+  // Initialize device info.
+  synced_device_tracker_.reset(
+      new SyncedDeviceTracker(sync_manager_->cache_guid()));
+  if (sync_manager_->GetUserShare()) { // NULL in some tests.
+    DVLOG(1) << "Initializing DeviceInfo type";
+    registrar_->ActivateDataType(syncer::DEVICE_INFO,

[Nicolas Zea, 2012/11/16 19:42:27]

Why not activate the datatype after initialization succeeds? Or is it that we
expect it to never fail?

[rlarocque, 2012/11/17 00:07:21]

It made more sense to me to do it after the activation was complete.  I see
InitLocalDeviceInfo() as an operation that can only be performed on an active
data type, in the same way that only an active BookmarkChangeProcessor can
respond to bookmark change notifications.

Also, I think it relies on some the ChangeProcessor::share_handle() function,
which I believe won't work until the type is activated.

Maybe InitLocalDeviceInfo() is a bad name?

[Nicolas Zea, 2012/11/17 00:31:01]

InitLocalDeviceInfo seems to me like an "AssociateModels" step, which typically
is what must succeed before we activate a datatype. If there's a chance of that
failing we shouldn't be auto-activating the type.

At the moment, it seems that's not the case. But in case we start doing
something fancier in the future, I think I'd prefer moving ActivateDatatype to
DoFinishInitialProcessControlTypes, which you could eventually extend with a
success bool if you need to.

Given that it's both (effectively) a model associator and a change processor, I
think it's fine to pass in the user share into the device tracker directly at
construction time (which would happen here). This is also how the sync
encryption handler works.

+                                 syncer::GROUP_PASSIVE,
+                                 synced_device_tracker_.get(),
+                                 sync_manager_->GetUserShare());
+    synced_device_tracker_->InitLocalDeviceInfo(
+        base::Bind(&SyncBackendHost::Core::FinishInitialProcessControlTypes,
+                   this));
+  } else {
+    DVLOG(1) << "Skipping initialization of DeviceInfo";
+    FinishInitialProcessControlTypes();
+  }
+}
+
+void SyncBackendHost::Core::FinishInitialProcessControlTypes() {

[Nicolas Zea, 2012/11/16 19:42:27]

I think the naming convention prefers DoFinishInitialProcessControlTypes?
Although, given that this is only calling
HandleInitializationCompletedOnFrontendLoop, DoHandleInitialization... might be
more appropriate.

[rlarocque, 2012/11/17 00:07:21]

I think you're right.  I renamed it.

+  host_.Call(
+      FROM_HERE,
+      &SyncBackendHost::HandleInitializationCompletedOnFrontendLoop,
+      true);
+}
+
 void SyncBackendHost::Core::DoSetDecryptionPassphrase(
     const std::string& passphrase) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
@@ -1182,6 +1221,10 @@ void SyncBackendHost::Core::DoStopSyncManagerForShutdown(
 
 void SyncBackendHost::Core::DoShutdown(bool sync_disabled) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
+  // It's safe to do this even if the type was never activated.
+  registrar_->DeactivateDataType(syncer::DEVICE_INFO);
+  synced_device_tracker_.reset();
+
   DoDestroySyncManager();
 
   chrome_sync_notification_bridge_ = NULL;
@@ -1327,7 +1370,7 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
       // trigger migration.  That would be disastrous, so we must rely on the
       // sync manager to ensure that this type never has both progress markers
       // and !initial_sync_ended.
-      initialization_state_ = DOWNLOADING_NIGORI;
+      initialization_state_ = DOWNLOADING_CONTROL_TYPES;
       ConfigureDataTypes(
           syncer::CONFIGURE_REASON_NEW_CLIENT,
           syncer::ModelTypeSet(syncer::ControlTypes()),
@@ -1340,16 +1383,17 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop(
           base::Bind(&SyncBackendHost::OnNigoriDownloadRetry,
                      weak_ptr_factory_.GetWeakPtr()));
       break;
-    case DOWNLOADING_NIGORI:
-      initialization_state_ = ASSOCIATING_NIGORI;
-      // Triggers OnEncryptedTypesChanged() and OnEncryptionComplete()
-      // if necessary.
-      sync_thread_.message_loop()->PostTask(
-          FROM_HERE,
-          base::Bind(&SyncBackendHost::Core::DoAssociateNigori,
-                     core_.get()));
+    case DOWNLOADING_CONTROL_TYPES:
+      initialization_state_ = PROCESSING_CONTROL_TYPES;
+      // Updates encryption and other metadata.  Will call
+      // OnEncryptedTypesChanged() and OnEncryptionComplete() if necessary.
+      InitialProcessControlTypes(
+          base::Bind(
+              &SyncBackendHost::
+                  HandleInitializationCompletedOnFrontendLoop,
+              weak_ptr_factory_.GetWeakPtr(), true));
       break;
-    case ASSOCIATING_NIGORI:
+    case PROCESSING_CONTROL_TYPES:
       initialization_state_ = INITIALIZED;
       // Now that we've downloaded the nigori node, we can see if there are any
       // experimental types to enable. This should be done before we inform
@@ -1528,6 +1572,16 @@ void SyncBackendHost::HandleNigoriConfigurationCompletedOnFrontendLoop(
       failed_configuration_types.Empty());
 }
 
+void SyncBackendHost::InitialProcessControlTypes(
+    const base::Closure& done_callback) {
+  DCHECK_EQ(MessageLoop::current(), frontend_loop_);
+  // Then forward the request to the sync thread.
+  sync_thread_.message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncBackendHost::Core::DoInitialProcessControlTypes,
+                 core_.get()));
+}
+
 #undef SDVLOG
 
 #undef SLOG