Migrate startup URLs pref.

chrome/browser/prefs/pref_model_associator.cc

Index: chrome/browser/prefs/pref_model_associator.cc
diff --git a/chrome/browser/prefs/pref_model_associator.cc b/chrome/browser/prefs/pref_model_associator.cc
index 28759a10b6919b681ac7fcea46e5fc8227e7fc00..635b6e0374cd2d6bbee4a3ccd1f78d97beac1279 100644
--- a/chrome/browser/prefs/pref_model_associator.cc
+++ b/chrome/browser/prefs/pref_model_associator.cc
@@ -48,6 +48,34 @@ sync_pb::PreferenceSpecifics* GetMutableSpecifics(
   }
 }
 
+// List of migrated preference name pairs. If a preference is migrated
+// (meaning renamed) adding the old and new preference names here will ensure
+// that the sync engine knows how to deal with the synced values coming in
+// with the old name. Preference migration itself doesn't happen here. It may
+// happen in session_startup_pref.cc.
+const struct MigratedPreferences {
+  const char* const old_name;
+  const char* const new_name;
+} kMigratedPreferences[] = {
+  { prefs::kURLsToRestoreOnStartupOld, prefs::kURLsToRestoreOnStartup },
+};
+
+std::string GetOldMigratedPreferenceName(const char* preference_name) {
+  for (size_t i = 0; i < arraysize(kMigratedPreferences); ++i) {
+    if (!strcmp(kMigratedPreferences[i].new_name, preference_name))
+      return kMigratedPreferences[i].old_name;
+  }
+  return std::string();
+}
+
+std::string GetNewMigratedPreferenceName(const char* old_preference_name) {
+  for (size_t i = 0; i < arraysize(kMigratedPreferences); ++i) {
+    if (!strcmp(kMigratedPreferences[i].old_name, old_preference_name))
+      return kMigratedPreferences[i].new_name;
+  }
+  return std::string();
+}
+
 }  // namespace
 
 PrefModelAssociator::PrefModelAssociator(syncer::ModelType type)
@@ -71,15 +99,18 @@ PrefModelAssociator::~PrefModelAssociator() {
 void PrefModelAssociator::InitPrefAndAssociate(
     const syncer::SyncData& sync_pref,
     const std::string& pref_name,
-    syncer::SyncChangeList* sync_changes) {
+    syncer::SyncChangeList* sync_changes,
+    SyncDataMap* migrated_preference_list) {
   const Value* user_pref_value = pref_service_->GetUserPrefValue(
       pref_name.c_str());
   VLOG(1) << "Associating preference " << pref_name;
 
   if (sync_pref.IsValid()) {
     const sync_pb::PreferenceSpecifics& preference = GetSpecifics(sync_pref);
-    DCHECK_EQ(pref_name, preference.name());
-
+    DCHECK(pref_name == preference.name() ||
+           (IsMigratedPreference(pref_name.c_str()) &&
+            preference.name() ==
+                GetOldMigratedPreferenceName(pref_name.c_str())));
     base::JSONReader reader;
     scoped_ptr<Value> sync_value(reader.ReadToValue(preference.value()));
     if (!sync_value.get()) {
@@ -115,10 +146,45 @@ void PrefModelAssociator::InitPrefAndAssociate(
           LOG(ERROR) << "Failed to update preference.";
           return;
         }
-        sync_changes->push_back(
+
+        if (IsMigratedPreference(pref_name.c_str())) {
+          // This preference has been migrated from an old version that must be
+          // kept in sync on older versions of Chrome.
+          std::string old_pref_name =
+              GetOldMigratedPreferenceName(pref_name.c_str());
+
+          if (preference.name() == old_pref_name && migrated_preference_list) {

[Nicolas Zea, 2013/10/11 19:58:54]

I think it's invalid to call InitPrefAndAssociate with an old preference and not
have the migrated_preference_list. In that case, I'd leave the first condition,
and have DCHECK(migrated_preference_list) within the body.

[robertshield, 2013/10/11 21:47:23]

Afaict it should never happen and I'll add the DCHECK. I'm leery of leaving the
pointer dereference there in release mode without checking it because I'm not
extremely confident in my assessment that it will never happen. 

Let me know if you think I'm being overly paranoid and I'll drop the
migrated_preference_list check too.

+            // If the name the syncer has is the old pre-migration value, then
+            // it's possible the new migrated preference name hasn't been synced
+            // yet. In that case the SyncChange should be an ACTION_ADD rather
+            // than an ACTION_UPDATE. Defer the decision of whether to sync with
+            // ACTION_ADD or ACTION_UPDATE until the migrated_preferences phase.
+            (*migrated_preference_list)[pref_name] = sync_data;
+          } else {
+            DCHECK_EQ(preference.name(), pref_name);
+            sync_changes->push_back(
+                syncer::SyncChange(FROM_HERE,
+                                   syncer::SyncChange::ACTION_UPDATE,
+                                   sync_data));
+          }
+
+          syncer::SyncData old_sync_data;
+          if (!CreatePrefSyncData(old_pref_name, *new_value, &old_sync_data)) {
+            LOG(ERROR) << "Failed to update preference.";
+            return;
+          }
+          (*migrated_preference_list)[old_pref_name] = old_sync_data;
+
+          // Keep track of the name of the synced pref. This will be idempotent
+          // with the insertion of pref_name below when the migrated value has
+          // already been synced, not so when it has not.
+          synced_preferences_.insert(preference.name());

[Nicolas Zea, 2013/10/11 19:58:54]

I think rather than having this here, you need to change the one at line 217 to
use preference.name().

For example, if the migrated synced pref doesn't exist, pref_name will be for
the migrated pref, and adding it into synced_preferences_ is invalid. If you do,
at the later migrated_preferences phase you'll assume the preference exists, and
do an ACTION_UPDATE, when it should be an ACTION_ADD.

[robertshield, 2013/10/11 21:47:23]

preference.name() is only defined if sync_pref.IsValid() is true. In cases where
sync_pref is not valid (there are two such callsites) I think we should still
add pref_name and rather just make sure to not add pref_name if
preference.name() has already been added.

Did that, let me know what you think.

+        } else {
+          sync_changes->push_back(
             syncer::SyncChange(FROM_HERE,
                                syncer::SyncChange::ACTION_UPDATE,
                                sync_data));
+        }
       }
     } else if (!sync_value->IsType(Value::TYPE_NULL)) {
       // Only a server value exists. Just set the local user value.
@@ -170,6 +236,11 @@ syncer::SyncMergeResult PrefModelAssociator::MergeDataAndStartSyncing(
   syncer::SyncChangeList new_changes;
   std::set<std::string> remaining_preferences = registered_preferences_;
 
+  // Maintains a list of old migrated preference names that we wish to sync.
+  // Keep track of these in a list such that when the preference iteration
+  // loops below are complete we can go back and determine whether
+  SyncDataMap migrated_preference_list;
+
   // Go through and check for all preferences we care about that sync already
   // knows about.
   for (syncer::SyncDataList::const_iterator sync_iter =
@@ -179,19 +250,28 @@ syncer::SyncMergeResult PrefModelAssociator::MergeDataAndStartSyncing(
     DCHECK_EQ(type_, sync_iter->GetDataType());
 
     const sync_pb::PreferenceSpecifics& preference = GetSpecifics(*sync_iter);
-    const std::string& sync_pref_name = preference.name();
+    std::string sync_pref_name = preference.name();
 
     if (remaining_preferences.count(sync_pref_name) == 0) {
-      // We're not syncing this preference locally, ignore the sync data.
-      // TODO(zea): Eventually we want to be able to have the syncable service
-      // reconstruct all sync data for it's datatype (therefore having
-      // GetAllSyncData be a complete representation). We should store this data
-      // somewhere, even if we don't use it.
-      continue;
+      if (IsOldMigratedPreference(sync_pref_name.c_str())) {
+        // This old pref name is not syncable locally anymore but we accept
+        // changes from other Chrome installs of previous versions and migrate
+        // them to the new name. Note that we will be merging any differences
+        // between the new and old values and sync'ing them back.
+        sync_pref_name = GetNewMigratedPreferenceName(sync_pref_name.c_str());
+      } else {
+        // We're not syncing this preference locally, ignore the sync data.
+        // TODO(zea): Eventually we want to be able to have the syncable service
+        // reconstruct all sync data for its datatype (therefore having
+        // GetAllSyncData be a complete representation). We should store this
+        // data somewhere, even if we don't use it.
+        continue;
+      }
+    } else {
+      remaining_preferences.erase(sync_pref_name);
     }
-
-    remaining_preferences.erase(sync_pref_name);
-    InitPrefAndAssociate(*sync_iter, sync_pref_name, &new_changes);
+    InitPrefAndAssociate(*sync_iter, sync_pref_name, &new_changes,
+                         &migrated_preference_list);
   }
 
   // Go through and build sync data for any remaining preferences.
@@ -199,7 +279,21 @@ syncer::SyncMergeResult PrefModelAssociator::MergeDataAndStartSyncing(
           remaining_preferences.begin();
        pref_name_iter != remaining_preferences.end();
        ++pref_name_iter) {
-    InitPrefAndAssociate(syncer::SyncData(), *pref_name_iter, &new_changes);
+    InitPrefAndAssociate(syncer::SyncData(), *pref_name_iter, &new_changes,
+                         &migrated_preference_list);
+  }
+
+  // Now go over any migrated preference names and build sync data for them too.
+  for (SyncDataMap::const_iterator migrated_pref_iter =
+          migrated_preference_list.begin();
+       migrated_pref_iter != migrated_preference_list.end();
+       ++migrated_pref_iter) {
+    syncer::SyncChange::SyncChangeType change_type =
+        (synced_preferences_.count(migrated_pref_iter->first) == 0) ?
+            syncer::SyncChange::ACTION_ADD :
+            syncer::SyncChange::ACTION_UPDATE;
+    new_changes.push_back(
+        syncer::SyncChange(FROM_HERE, change_type, migrated_pref_iter->second));
   }
 
   // Push updates to sync.
@@ -225,7 +319,10 @@ scoped_ptr<Value> PrefModelAssociator::MergePreference(
     const std::string& name,
     const Value& local_value,
     const Value& server_value) {
-  if (name == prefs::kURLsToRestoreOnStartup) {
+  // This function special cases preferences individually, so don't attempt
+  // to merge for all migrated values.
+  if (name == prefs::kURLsToRestoreOnStartup ||
+      name == prefs::kURLsToRestoreOnStartupOld) {
     return scoped_ptr<Value>(MergeListValues(local_value, server_value)).Pass();
   }
 
@@ -321,6 +418,17 @@ Value* PrefModelAssociator::MergeDictionaryValues(
   return result;
 }
 
+// static
+bool PrefModelAssociator::IsMigratedPreference(const char* preference_name) {
+  return !GetOldMigratedPreferenceName(preference_name).empty();
+}
+
+// static
+bool PrefModelAssociator::IsOldMigratedPreference(
+    const char* old_preference_name) {
+  return !GetNewMigratedPreferenceName(old_preference_name).empty();
+}
+
 // Note: This will build a model of all preferences registered as syncable
 // with user controlled data. We do not track any information for preferences
 // not registered locally as syncable and do not inform the syncer of
@@ -388,6 +496,13 @@ syncer::SyncError PrefModelAssociator::ProcessSyncChanges(
     // Windows client, the Windows client does not support
     // kConfirmToQuitEnabled. Ignore updates from these preferences.
     const char* pref_name = name.c_str();
+    std::string new_name;
+    // We migrated this preference name, so do as if the name had not changed.
+    if (IsOldMigratedPreference(pref_name)) {
+      new_name = GetNewMigratedPreferenceName(pref_name);
+      pref_name = new_name.c_str();
+    }
+
     if (!IsPrefRegistered(pref_name))
       continue;
 
@@ -496,7 +611,7 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) {
     // Not in synced_preferences_ means no synced data. InitPrefAndAssociate(..)
     // will determine if we care about its data (e.g. if it has a default value
     // and hasn't been changed yet we don't) and take care syncing any new data.
-    InitPrefAndAssociate(syncer::SyncData(), name, &changes);
+    InitPrefAndAssociate(syncer::SyncData(), name, &changes, NULL);
   } else {
     // We are already syncing this preference, just update it's sync node.
     syncer::SyncData sync_data;
@@ -508,6 +623,24 @@ void PrefModelAssociator::ProcessPrefChange(const std::string& name) {
         syncer::SyncChange(FROM_HERE,
                            syncer::SyncChange::ACTION_UPDATE,
                            sync_data));
+    // This preference has been migrated from an old version that must be kept
+    // in sync on older versions of Chrome.
+    if (IsMigratedPreference(name.c_str())) {
+      std::string old_pref_name = GetOldMigratedPreferenceName(name.c_str());
+      if (!CreatePrefSyncData(old_pref_name,
+                              *preference->GetValue(),
+                              &sync_data)) {
+        LOG(ERROR) << "Failed to update preference.";
+        return;
+      }
+
+      syncer::SyncChange::SyncChangeType change_type =
+          (synced_preferences_.count(name) == 0) ?

[Nicolas Zea, 2013/10/11 19:58:54]

this should be old_pref_name right?

[robertshield, 2013/10/11 21:47:23]

oops, yes. good catch.

+              syncer::SyncChange::ACTION_ADD :
+              syncer::SyncChange::ACTION_UPDATE;
+      changes.push_back(
+          syncer::SyncChange(FROM_HERE, change_type, sync_data));
+    }
   }
 
   syncer::SyncError error =