Sync: Add a SyncTypePreferenceProvider interface.

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 2012 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 <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 33 matching lines @@
 #include "chrome/browser/sync/backend_migrator.h"
 #include "chrome/browser/sync/glue/chrome_report_unrecoverable_error.h"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/favicon_cache.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 #include "chrome/browser/sync/glue/sync_backend_host_impl.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
 #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"
 #include "chrome/browser/sync/sync_error_controller.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 1863 matching lines @@
             base::Bind(&ProfileSyncService::StartSyncingWithServer,
                        base::Unretained(this))));
   }
 
   syncer::ModelTypeSet types;
   syncer::ConfigureReason reason = syncer::CONFIGURE_REASON_UNKNOWN;
   if (backend_mode_ == BACKUP || backend_mode_ == ROLLBACK) {
     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) {
       // Datatype downloads on restart are generally due to newly supported
       // datatypes (although it's also possible we're picking up where a failed
       // previous configuration left off).
       // TODO(sync): consider detecting configuration recovery and setting
       // the reason here appropriately.
       reason = syncer::CONFIGURE_REASON_NEWLY_ENABLED_DATA_TYPE;
     } else {
@@ skipping 338 matching lines @@
 
 void ProfileSyncService::RemoveTypeDebugInfoObserver(
     syncer::TypeDebugInfoObserver* type_debug_info_observer) {
   type_debug_info_observers_.RemoveObserver(type_debug_info_observer);
   if (!type_debug_info_observers_.might_have_observers() &&
       backend_initialized_) {
     backend_->DisableDirectoryTypeDebugInfoForwarding();
   }
 }
 
+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
     : public base::RefCountedThreadSafe<GetAllNodesRequestHelper> {
  public:
   GetAllNodesRequestHelper(
       syncer::ModelTypeSet requested_types,
       const base::Callback<void(scoped_ptr<base::ListValue>)>& callback);
 
   void OnReceivedNodesForTypes(
@@ skipping 168 matching lines @@
     NotifyObservers();
 
     DVLOG(1) << "ConfigureDataTypeManager not invoked because of an "
              << "Unrecoverable error.";
   } else {
     DVLOG(0) << "ConfigureDataTypeManager not invoked because backend is not "
              << "initialized";
   }
 }
 
+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_;
 }
 
 void ProfileSyncService::OnInternalUnrecoverableError(
     const tracked_objects::Location& from_here,
     const std::string& message,
     bool delete_sync_database,
     UnrecoverableErrorReason reason) {
@@ skipping 166 matching lines @@
     browser_sync::SyncedDeviceTracker* device_tracker =
         backend_->GetSyncedDeviceTracker();
     if (device_tracker)
       device_tracker->UpdateLocalDeviceBackupTime(*last_backup_time_);
   }
 }
 
 base::Time ProfileSyncService::GetDeviceBackupTimeForTesting() const {
   return backend_->GetSyncedDeviceTracker()->GetLocalDeviceBackupTime();
 }