Sync: Add a SyncTypePreferenceProvider interface.

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 69bc581d9abeccfa8233afa56100ae147dc40ac7..fda9acc450725320e8a45eef47a8fb09cc4b690d 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -51,6 +51,7 @@
 #include "chrome/browser/sync/glue/synced_device_tracker.h"
 #include "chrome/browser/sync/glue/typed_url_data_type_controller.h"
 #include "chrome/browser/sync/profile_sync_components_factory_impl.h"
+#include "chrome/browser/sync/profile_sync_service_data_type_provider.h"
 #include "chrome/browser/sync/sessions/notification_service_sessions_router.h"
 #include "chrome/browser/sync/sessions/sessions_sync_manager.h"
 #include "chrome/browser/sync/supervised_user_signin_manager_wrapper.h"
@@ -1934,7 +1935,7 @@ void ProfileSyncService::ConfigureDataTypeManager() {
     types = syncer::BackupTypes();
     reason = syncer::CONFIGURE_REASON_BACKUP_ROLLBACK;
   } else {
-    types = GetPreferredDirectoryDataTypes();
+    types = Union(GetPreferredDirectoryDataTypes(), GetRequiredDataTypes());
     if (!HasSyncSetupCompleted()) {
       reason = syncer::CONFIGURE_REASON_NEW_CLIENT;
     } else if (restart) {
@@ -2293,6 +2294,24 @@ void ProfileSyncService::RemoveTypeDebugInfoObserver(
   }
 }
 
+void ProfileSyncService::AddDataTypeProvider(
+    ProfileSyncServiceDataTypeProvider* provider) {
+  DCHECK(!HasDataTypeProvider(provider)) << "Providers may only be added once!";
+  data_type_providers_.push_back(provider);
+}
+
+void ProfileSyncService::RemoveDataTypeProvider(
+    ProfileSyncServiceDataTypeProvider* provider) {
+  data_type_providers_.remove(provider);

[Bernhard Bauer, 2014/07/30 12:38:51]

DCHECK the result?

[Marc Treib, 2014/07/30 13:10:39]

There is no result :P
We could DCHECK(HasDataTypeProvider(..)) similar to above, if
RemoveDataTypeProvider for a non-registered value should be forbidden. Here I've
mirrored the behavior of ObserverLists, which do allow this.

[Bernhard Bauer, 2014/07/30 14:13:20]

Yeah, I meant checking whether there is one registered. It just seems weird to
disallow adding a data type provider twice, but allow removing it twice.

[Marc Treib, 2014/08/07 13:05:55]

Done.

+}
+
+bool ProfileSyncService::HasDataTypeProvider(
+    ProfileSyncServiceDataTypeProvider* provider) const {
+  return std::find(data_type_providers_.begin(),
+                   data_type_providers_.end(),
+                   provider) != data_type_providers_.end();
+}
+
 namespace {
 
 class GetAllNodesRequestHelper
@@ -2481,6 +2500,21 @@ void ProfileSyncService::ReconfigureDatatypeManager() {
   }
 }
 
+syncer::ModelTypeSet ProfileSyncService::GetRequiredDataTypes() {
+  syncer::ModelTypeSet types;
+  std::list<ProfileSyncServiceDataTypeProvider*>::iterator it =
+      data_type_providers_.begin();
+  while (it != data_type_providers_.end()) {
+    // Dereference and increment the iterator *before* calling the callback,
+    // because the provider might remove itself from the list in the callback.

[Bernhard Bauer, 2014/07/30 12:38:51]

I think an easier solution here would be to require the data type provider not
to remove themselves from the list while it's being called back. No need to make
our lives harder than necessary :)

[Marc Treib, 2014/07/30 13:10:39]

I guess we could. OTOH, this is a fairly common pattern when iterating over STL
containers and I don't mind it.

[Bernhard Bauer, 2014/07/30 14:13:20]

Not as common as a standard for loop ;-)

If you keep it this way, what if the callback removes the *next* data type
observer from the list? Basically, if you want to handle all the edge cases,
you're going to end up reimplementing ObserverList, for a use case that doesn't
even currently exist.

[Marc Treib, 2014/08/07 13:05:55]

Alright. Bonus side-effect: It doesn't have to be a std::list anymore.

+    ProfileSyncServiceDataTypeProvider* provider = *it;
+    ++it;
+    types.PutAll(provider->GetRequiredSyncDataTypes());
+  }
+  // TODO(treib): Resolve pref groups?

[Marc Treib, 2014/07/30 11:42:50]

It'd be easiest to expose SyncPrefs::ResolvePrefGroups and explicitly call that
here. Would that be okay, or does anyone have a better idea?

[Bernhard Bauer, 2014/07/30 12:38:51]

Do we actually need that? Required data types could include their own
dependencies.

[Marc Treib, 2014/07/30 13:10:39]

We could also specify the types explicitly, but I think that would often result
in duplicating the pref groups. (e.g. APPS implies APP_SETTINGS,
APP_NOTIFICATIONS and APP_LIST. Do we really want to duplicate this in our
code?)

[Bernhard Bauer, 2014/07/30 14:13:20]

At the very least we would need to change the concept of "pref groups", as these
data types here are not enabled by user choice.

+  return types;
+}
+
 const FailedDataTypesHandler& ProfileSyncService::failed_data_types_handler()
     const {
   return failed_data_types_handler_;