Add a declarative way in c/b/s/sync_prefs.cc to say which data types have prefs

chrome/browser/sync/sync_prefs.cc

Index: chrome/browser/sync/sync_prefs.cc
diff --git a/chrome/browser/sync/sync_prefs.cc b/chrome/browser/sync/sync_prefs.cc
index b66d7c04b6e663b8f84f2b2537047c969b3d1182..ddf01f77d34c0f943378a292714105a25ee65fa8 100644
--- a/chrome/browser/sync/sync_prefs.cc
+++ b/chrome/browser/sync/sync_prefs.cc
@@ -21,6 +21,7 @@ SyncPrefObserver::~SyncPrefObserver() {}
 SyncPrefs::SyncPrefs(PrefService* pref_service)
     : pref_service_(pref_service) {
   if (pref_service_) {
+    RegisterDeterminedTypes();

[akalin, 2012/02/29 05:33:03]

this doesn't need to be in the if statement

[not at google - send to devlin, 2012/03/01 03:14:40]

Done.

     RegisterPreferences();
     // Watch the preference that indicates sync is managed so we can take
     // appropriate action.
@@ -135,17 +136,15 @@ syncable::ModelTypeSet SyncPrefs::GetPreferredDataTypes(
     return registered_types;
   }
 
-  // Remove autofill_profile since it's controlled by autofill, and
-  // search_engines since it's controlled by preferences (see code below).
   syncable::ModelTypeSet user_selectable_types(registered_types);
   DCHECK(!user_selectable_types.Has(syncable::NIGORI));
-  user_selectable_types.Remove(syncable::AUTOFILL_PROFILE);
-  user_selectable_types.Remove(syncable::SEARCH_ENGINES);
 
-  // Remove app_notifications since it's controlled by apps (see
-  // code below).
-  // TODO(akalin): Centralize notion of all user selectable data types.
-  user_selectable_types.Remove(syncable::APP_NOTIFICATIONS);
+  // Remove any data types whose prefs depend on other types (will be added

[akalin, 2012/02/29 05:33:03]

with ApplyTypeDependencies as described in the comment below, this can just be:

return ApplyTypeDependencies(registered_types, GetRawPreferredTypes());

where GetRawPreferredTypes() just loops over ModelTypeSet::All() and checks
GetDataTypePreferred() for each one and returns the result.

+  // below if applicable).
+  for (TypesDeterminedByMap::const_iterator i = types_determined_by_.begin();
+      i != types_determined_by_.end(); ++i) {
+    user_selectable_types.Remove(i->first);
+  }
 
   syncable::ModelTypeSet preferred_types;
 
@@ -156,26 +155,14 @@ syncable::ModelTypeSet SyncPrefs::GetPreferredDataTypes(
     }
   }
 
-  // Group the enabled/disabled state of autofill_profile with autofill, and
-  // search_engines with preferences (since only autofill and preferences are
-  // shown on the UI).
-  if (registered_types.Has(syncable::AUTOFILL) &&
-      registered_types.Has(syncable::AUTOFILL_PROFILE) &&
-      GetDataTypePreferred(syncable::AUTOFILL)) {
-    preferred_types.Put(syncable::AUTOFILL_PROFILE);
-  }
-  if (registered_types.Has(syncable::PREFERENCES) &&
-      registered_types.Has(syncable::SEARCH_ENGINES) &&
-      GetDataTypePreferred(syncable::PREFERENCES)) {
-    preferred_types.Put(syncable::SEARCH_ENGINES);
-  }
-
-  // Set app_notifications to the same enabled/disabled state as
-  // apps (since only apps is shown on the UI).
-  if (registered_types.Has(syncable::APPS) &&
-      registered_types.Has(syncable::APP_NOTIFICATIONS) &&
-      GetDataTypePreferred(syncable::APPS)) {
-    preferred_types.Put(syncable::APP_NOTIFICATIONS);
+  // Add any determined types if what they're determined by has been added.
+  for (TypesDeterminedByMap::const_iterator i = types_determined_by_.begin();
+      i != types_determined_by_.end(); ++i) {
+    if (registered_types.Has(i->first) &&
+        registered_types.Has(i->second) &&
+        GetDataTypePreferred(i->second)) {
+      preferred_types.Put(i->first);
+    }
   }
 
   return preferred_types;
@@ -188,34 +175,17 @@ void SyncPrefs::SetPreferredDataTypes(
   CHECK(pref_service_);
   DCHECK(registered_types.HasAll(preferred_types));
   syncable::ModelTypeSet preferred_types_with_dependents(preferred_types);
-  // Set autofill_profile to the same enabled/disabled state as
-  // autofill (since only autofill is shown in the UI).
-  if (registered_types.Has(syncable::AUTOFILL) &&
-      registered_types.Has(syncable::AUTOFILL_PROFILE)) {
-    if (preferred_types_with_dependents.Has(syncable::AUTOFILL)) {
-      preferred_types_with_dependents.Put(syncable::AUTOFILL_PROFILE);
-    } else {
-      preferred_types_with_dependents.Remove(syncable::AUTOFILL_PROFILE);
-    }
-  }
-  // Set app_notifications to the same enabled/disabled state as
-  // apps (since only apps is shown in the UI).
-  if (registered_types.Has(syncable::APPS) &&
-      registered_types.Has(syncable::APP_NOTIFICATIONS)) {
-    if (preferred_types_with_dependents.Has(syncable::APPS)) {
-      preferred_types_with_dependents.Put(syncable::APP_NOTIFICATIONS);
-    } else {
-      preferred_types_with_dependents.Remove(syncable::APP_NOTIFICATIONS);
-    }
-  }
-  // Set search_engines to the same enabled/disabled state as
-  // preferences (since only preferences is shown in the UI).
-  if (registered_types.Has(syncable::PREFERENCES) &&
-      registered_types.Has(syncable::SEARCH_ENGINES)) {
-    if (preferred_types_with_dependents.Has(syncable::PREFERENCES)) {
-      preferred_types_with_dependents.Put(syncable::SEARCH_ENGINES);
-    } else {
-      preferred_types_with_dependents.Remove(syncable::SEARCH_ENGINES);
+
+  // Set the pref of any determined types to the pref of what they are
+  // determined by.
+  for (TypesDeterminedByMap::const_iterator i = types_determined_by_.begin();
+      i != types_determined_by_.end(); ++i) {

[akalin, 2012/02/29 05:33:03]

this is almost the same logic as in GetPreferredDataTypes; we probably should
consolidate the code (I prefer the GetPreferredDataTypes version).

How about a function like:

// With the given set of registered types, applies type dependencies to |types|
and returns the result.  |types| should be a subset of |registered_types|. The
return value is not necessarily a superset of |types|, but it's a subset of
|registered_types|.
ModelTypeSet ApplyTypeDependencies(ModelTypeSet registered_types, ModelTypeSet
types) {
   ...
}

Then this can be:

SetRawPreferredTypes(ApplyTypeDependencies(registered_types, preferred_types));

+    if (registered_types.Has(i->first) &&
+        registered_types.Has(i->second)) {
+      if (preferred_types.Has(i->second))
+        preferred_types_with_dependents.Put(i->first);
+      else
+        preferred_types_with_dependents.Remove(i->first);
     }
   }
 
@@ -405,6 +375,14 @@ const char* GetPrefNameForDataType(syncable::ModelType data_type) {
 
 }  // namespace
 
+void SyncPrefs::RegisterDeterminedTypes() {
+  types_determined_by_[syncable::APP_NOTIFICATIONS]  = syncable::APPS;
+  types_determined_by_[syncable::APP_SETTINGS]       = syncable::APPS;
+  types_determined_by_[syncable::AUTOFILL_PROFILE]   = syncable::AUTOFILL;
+  types_determined_by_[syncable::EXTENSION_SETTINGS] = syncable::EXTENSIONS;
+  types_determined_by_[syncable::SEARCH_ENGINES]     = syncable::PREFERENCES;
+}
+
 void SyncPrefs::RegisterPreferences() {
   DCHECK(non_thread_safe_.CalledOnValidThread());
   CHECK(pref_service_);
@@ -459,18 +437,20 @@ void SyncPrefs::RegisterPreferences() {
   // SESSIONS - all previously launched data types are treated as if they are
   // already acknowledged.
   syncable::ModelTypeSet model_set;
-  model_set.Put(syncable::BOOKMARKS);
-  model_set.Put(syncable::PREFERENCES);
-  model_set.Put(syncable::PASSWORDS);
-  model_set.Put(syncable::AUTOFILL_PROFILE);
+  model_set.Put(syncable::APPS);
+  model_set.Put(syncable::APP_SETTINGS);

[akalin, 2012/02/29 05:33:03]

don't add new types -- see comment above definition of model_set.

in fact, probably best to leave in original order.

[not at google - send to devlin, 2012/03/01 03:14:40]

Oh, right. I was wondering what that comment meant. It's pretty vague i.e. not
clear what model_set actually does.

The comment should say like "Do not add new data types to this set. We will
start prompting people..."

And then "model_set" should be called, like, whatever it's supposed to do.

Anyway, change reverted (though I prefer it sorted...)

   model_set.Put(syncable::AUTOFILL);
-  model_set.Put(syncable::THEMES);
+  model_set.Put(syncable::AUTOFILL_PROFILE);
+  model_set.Put(syncable::BOOKMARKS);
   model_set.Put(syncable::EXTENSIONS);
+  model_set.Put(syncable::EXTENSION_SETTINGS);
   model_set.Put(syncable::NIGORI);
+  model_set.Put(syncable::PASSWORDS);
+  model_set.Put(syncable::PREFERENCES);
   model_set.Put(syncable::SEARCH_ENGINES);
-  model_set.Put(syncable::APPS);
-  model_set.Put(syncable::TYPED_URLS);
   model_set.Put(syncable::SESSIONS);
+  model_set.Put(syncable::THEMES);
+  model_set.Put(syncable::TYPED_URLS);
   pref_service_->RegisterListPref(prefs::kSyncAcknowledgedSyncTypes,
                                   syncable::ModelTypeSetToValue(model_set),
                                   PrefService::UNSYNCABLE_PREF);