Add non-blocking sync code to ProfileSyncService

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 ce3c016cf1023ad396eb407c79382abf96ca9d8b..dede48bdcd2c6b37125f58d4c77a16defc534e5d 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -79,6 +79,7 @@
 #include "sync/internal_api/public/configure_reason.h"
 #include "sync/internal_api/public/http_bridge_network_resources.h"
 #include "sync/internal_api/public/network_resources.h"
+#include "sync/internal_api/public/sync_core_proxy.h"
 #include "sync/internal_api/public/sync_encryption_handler.h"
 #include "sync/internal_api/public/util/experiments.h"
 #include "sync/internal_api/public/util/sync_string_conversions.h"
@@ -347,10 +348,18 @@ void ProfileSyncService::UnregisterAuthNotifications() {
 void ProfileSyncService::RegisterDataTypeController(
     DataTypeController* data_type_controller) {
   DCHECK_EQ(data_type_controllers_.count(data_type_controller->type()), 0U);
+  DCHECK(!GetRegisteredNonBlockingDataTypes().Has(
+      data_type_controller->type()));
   data_type_controllers_[data_type_controller->type()] =
       data_type_controller;
 }
 
+void ProfileSyncService::RegisterNonBlockingType(syncer::ModelType type) {
+  DCHECK_EQ(data_type_controllers_.count(type), 0U);
+  DCHECK(!GetRegisteredNonBlockingDataTypes().Has(type));
+  off_thread_types_.Put(type);

[Nicolas Zea, 2014/04/02 23:19:05]

Presumably this is where we could kick off the startup for this type if it's
preferred? Add a comment?

[rlarocque, 2014/04/03 01:03:46]

Nope, not here.  Registration is something that should happen with every init of
the ProfileSyncService.  This function is equivalent to
RegisterDataTypeController.

Startup will be much more complicated.  We might need to add an extra class to
track all the state involved.  

We can't start up until both the type and the backend are ready.  These are two
independent events that could happen in any order.  We'll also need some
edge-triggered logic to handle types being preferred or un-preferred.  So there
will be at least three different code paths that can trigger a startup.  

+}
+
 browser_sync::SessionModelAssociator*
     ProfileSyncService::GetSessionModelAssociatorDeprecated() {
   if (!IsSessionsDataTypeControllerRunning())
@@ -543,7 +552,8 @@ void ProfileSyncService::InitializeBackend(bool delete_stale_data) {
 bool ProfileSyncService::IsEncryptedDatatypeEnabled() const {
   if (encryption_pending())
     return true;
-  const syncer::ModelTypeSet preferred_types = GetPreferredDataTypes();
+  const syncer::ModelTypeSet preferred_types =
+      GetPreferredDirectoryDataTypes();

[Nicolas Zea, 2014/04/02 23:19:05]

Why not GetPreferredDataTypes? Encryption isn't something tied to directory vs
nonblocking, right?

[rlarocque, 2014/04/03 01:03:46]

Good point.  Fixed.

   const syncer::ModelTypeSet encrypted_types = GetEncryptedDataTypes();
   DCHECK(encrypted_types.Has(syncer::PASSWORDS));
   return !Intersection(preferred_types, encrypted_types).Empty();
@@ -574,7 +584,7 @@ void ProfileSyncService::OnDataTypeRequestsSyncStartup(
     return;
   }
 
-  if (!GetPreferredDataTypes().Has(type)) {
+  if (!GetPreferredDirectoryDataTypes().Has(type)) {

[Nicolas Zea, 2014/04/02 23:19:05]

Here too. If a nonblocking type requests sync startup, shouldn't we start up?

[rlarocque, 2014/04/03 01:03:46]

Maybe.  The non-blocking types won't be as badly affected by deferred init as
directory types.  I don't foresee them needing to request sync startup.

I guess it does not harm to support it, though.  I've updated it to
GetPreferredTypes() for now.

     // We can get here as datatype SyncableServices are typically wired up
     // to the native datatype even if sync isn't enabled.
     DVLOG(1) << "Dropping sync startup request because type "
@@ -936,6 +946,8 @@ void ProfileSyncService::OnBackendInitialized(
 
   backend_initialized_ = true;
 
+  backend_->SetPreferredNonBlockingTypes(GetPreferredNonBlockingDataTypes());

[Nicolas Zea, 2014/04/02 23:19:05]

It's not clear to me why the backend needs to know about the preferred
nonblocking types

[rlarocque, 2014/04/03 01:03:46]

I was surprised to learn about this, too.

We want non-blocking types to receive invalidations like any other type.  But
the set of registered ObjectIds lives in the SyncBackendHost.  This call lets
the SyncBackendHost know that it should register these types to receive
invalidations.

There's probably a better solution than this.  Maybe invalidations should be
separated from the SyncBackendHost.  But that would be problematic, since the
SyncBackendHost manages the SyncBackendHostCore, which is where those
invalidations need to be forwarded eventually.  

I'm interested in fixing this, but I can't think of any simple way to do it.

[Nicolas Zea, 2014/04/03 17:52:15]

But why is this different from directory types? Do we care about registering for
invalidations before we actually start syncing the non-blocking type? Otherwise,
can't we wait to register for invalidations for that type until we're actually
syncing it?

[rlarocque, 2014/04/03 19:57:03]

Directory types register for invalidations once they're configured.  Now that we
have reliable invaliations, it's debatable whether or not it's necessary to wait
until configuration is done before registering them.  However, that's the way
it's always been, and there is some logic to it.  

Non-blocking types would have some difficulty registering only after they've
configured.  Non-blocking types' concept of configuration is less well-defined. 
There's currently no plan post a task to the SyncBackendHost / UI thread to
signal that their configuration is complete, like
FinishConfigureDataTypesOnFrontendLoop does now for directory types.  

I guess it would make more sense to delay the registration of the non-blocking
types until all the conditions for sync are met (ie. type is preferred, backend
is initialized, and model type code has indicated it is ready).  We'd still need
a separate hook into the SyncBackendHost, though.

+
   sync_js_controller_.AttachJsBackend(js_backend);
   debug_info_listener_ = debug_info_listener;
 
@@ -943,6 +955,8 @@ void ProfileSyncService::OnBackendInitialized(
     backend_->SetForwardProtocolEvents(true);
   }
 
+  syncer::SyncCoreProxy sync_core_proxy_ = backend_->GetSyncCoreProxy();
+
   // If we have a cached passphrase use it to decrypt/encrypt data now that the
   // backend is initialized. We want to call this before notifying observers in
   // case this operation affects the "passphrase required" status.
@@ -1217,7 +1231,7 @@ void ProfileSyncService::OnPassphraseRequired(
            << syncer::PassphraseRequiredReasonToString(reason);
   passphrase_required_reason_ = reason;
 
-  const syncer::ModelTypeSet types = GetPreferredDataTypes();
+  const syncer::ModelTypeSet types = GetPreferredDirectoryDataTypes();
   if (data_type_manager_) {
     // Reconfigure without the encrypted types (excluded implicitly via the
     // failed datatypes handler).
@@ -1245,7 +1259,7 @@ void ProfileSyncService::OnPassphraseAccepted() {
 
   // Make sure the data types that depend on the passphrase are started at
   // this time.
-  const syncer::ModelTypeSet types = GetPreferredDataTypes();
+  const syncer::ModelTypeSet types = GetPreferredDirectoryDataTypes();
   if (data_type_manager_) {
     // Re-enable any encrypted types if necessary.
     data_type_manager_->Configure(types,
@@ -1660,12 +1674,15 @@ void ProfileSyncService::ChangePreferredDataTypes(
   sync_prefs_.SetPreferredDataTypes(registered_types,
                                     registered_preferred_types);
 
+  if (backend_.get() && backend_initialized_)
+    backend_->SetPreferredNonBlockingTypes(GetPreferredNonBlockingDataTypes());
+
   // Now reconfigure the DTM.
   ReconfigureDatatypeManager();
 }
 
 syncer::ModelTypeSet ProfileSyncService::GetActiveDataTypes() const {
-  const syncer::ModelTypeSet preferred_types = GetPreferredDataTypes();
+  const syncer::ModelTypeSet preferred_types = GetPreferredDirectoryDataTypes();

[Nicolas Zea, 2014/04/02 23:19:05]

ActiveTypes only cares about whether they're running or not, not whether they're
directory or nonblocking. Why change this?

[rlarocque, 2014/04/03 01:03:46]

I thought this was used only for construction of the about:sync status, and that
it was related to DTC status.  Looks like that was wrong.

I've changed it back.

By the way, what is this actually used for?

[Nicolas Zea, 2014/04/03 17:52:15]

Anything that cares about what types are actually running. This affects sync's
internal error handling as well as UI that cares of sync types (like those using
favicons or tab data).

   const syncer::ModelTypeSet failed_types =
       failed_data_types_handler_.GetFailedTypes();
   return Difference(preferred_types, failed_types);
@@ -1678,7 +1695,27 @@ syncer::ModelTypeSet ProfileSyncService::GetPreferredDataTypes() const {
   return preferred_types;
 }
 
+syncer::ModelTypeSet
+ProfileSyncService::GetPreferredDirectoryDataTypes() const {
+  const syncer::ModelTypeSet registered_directory_types =
+      GetRegisteredDirectoryDataTypes();
+  const syncer::ModelTypeSet preferred_types =
+      sync_prefs_.GetPreferredDataTypes(registered_directory_types);
+  return preferred_types;
+}
+
+syncer::ModelTypeSet
+ProfileSyncService::GetPreferredNonBlockingDataTypes() const {
+  return sync_prefs_.GetPreferredDataTypes(GetRegisteredNonBlockingDataTypes());
+}
+
 syncer::ModelTypeSet ProfileSyncService::GetRegisteredDataTypes() const {
+  return Union(GetRegisteredDirectoryDataTypes(),
+               GetRegisteredNonBlockingDataTypes());
+}
+
+syncer::ModelTypeSet
+ProfileSyncService::GetRegisteredDirectoryDataTypes() const {
   syncer::ModelTypeSet registered_types;
   // The data_type_controllers_ are determined by command-line flags; that's
   // effectively what controls the values returned here.
@@ -1690,6 +1727,11 @@ syncer::ModelTypeSet ProfileSyncService::GetRegisteredDataTypes() const {
   return registered_types;
 }
 
+syncer::ModelTypeSet
+ProfileSyncService::GetRegisteredNonBlockingDataTypes() const {
+  return off_thread_types_;
+}
+
 bool ProfileSyncService::IsUsingSecondaryPassphrase() const {
   syncer::PassphraseType passphrase_type = GetPassphraseType();
   return passphrase_type == syncer::FROZEN_IMPLICIT_PASSPHRASE ||
@@ -1711,7 +1753,8 @@ bool ProfileSyncService::IsCryptographerReady(
 
 void ProfileSyncService::ConfigurePriorityDataTypes() {
   const syncer::ModelTypeSet priority_types =
-      Intersection(GetPreferredDataTypes(), syncer::PriorityUserTypes());
+      Intersection(GetPreferredDirectoryDataTypes(),
+                   syncer::PriorityUserTypes());
   if (!priority_types.Empty()) {
     const syncer::ConfigureReason reason = HasSyncSetupCompleted() ?
         syncer::CONFIGURE_REASON_RECONFIGURATION :
@@ -1749,7 +1792,7 @@ void ProfileSyncService::ConfigureDataTypeManager() {
                        base::Unretained(this))));
   }
 
-  const syncer::ModelTypeSet types = GetPreferredDataTypes();
+  const syncer::ModelTypeSet types = GetPreferredDirectoryDataTypes();
   syncer::ConfigureReason reason = syncer::CONFIGURE_REASON_UNKNOWN;
   if (!HasSyncSetupCompleted()) {
     reason = syncer::CONFIGURE_REASON_NEW_CLIENT;
@@ -1866,6 +1909,9 @@ base::Value* ProfileSyncService::GetTypeStatusMap() const {
     } else if (throttled_types.Has(type)) {
       type_status->SetString("status", "warning");
       type_status->SetString("value", "Throttled");
+    } else if (GetRegisteredNonBlockingDataTypes().Has(type)) {
+      type_status->SetString("status", "ok");
+      type_status->SetString("value", "Non-Blocking");
     } else if (active_types.Has(type)) {
       type_status->SetString("status", "ok");
       type_status->SetString("value", "Active: " +