Revamp the MigrateBrowserPrefs and MigrateUserPrefs code.

chrome/browser/prefs/browser_prefs.cc

Index: chrome/browser/prefs/browser_prefs.cc
diff --git a/chrome/browser/prefs/browser_prefs.cc b/chrome/browser/prefs/browser_prefs.cc
index e733f82f18c93b36b4357b8433cde2fb1a208abf..c8a0fc655989ba9befaac2a3992b9d5f51698e08 100644
--- a/chrome/browser/prefs/browser_prefs.cc
+++ b/chrome/browser/prefs/browser_prefs.cc
@@ -163,7 +163,6 @@
 #include "chrome/browser/chromeos/extensions/echo_private_api.h"
 #include "chrome/browser/chromeos/file_system_provider/registry.h"
 #include "chrome/browser/chromeos/first_run/first_run.h"
-#include "chrome/browser/chromeos/login/default_pinned_apps_field_trial.h"
 #include "chrome/browser/chromeos/login/saml/saml_offline_signin_limiter.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
@@ -224,19 +223,6 @@
 
 namespace {
 
-enum MigratedPreferences {
-  NO_PREFS = 0,
-  DNS_PREFS = 1 << 0,
-  WINDOWS_PREFS = 1 << 1,
-};
-
-// A previous feature (see
-// chrome/browser/protector/protected_prefs_watcher.cc in source
-// control history) used this string as a prefix for various prefs it
-// registered. We keep it here for now to clear out those old prefs in
-// MigrateUserPrefs.
-const char kBackupPref[] = "backup";
-
 #if !defined(OS_ANDROID)
 // The AutomaticProfileResetter service used this preference to save that the
 // profile reset prompt had already been shown, however, the preference has been
@@ -251,8 +237,6 @@ const char kLegacyProfileResetPromptMemento[] = "profile.reset_prompt_memento";
 namespace chrome {
 
 void RegisterLocalState(PrefRegistrySimple* registry) {
-  // Prefs in Local State.
-  registry->RegisterIntegerPref(prefs::kMultipleProfilePrefMigration, 0);
 
   // Please keep this list alphabetized.
   AppListService::RegisterPrefs(registry);
@@ -329,7 +313,6 @@ void RegisterLocalState(PrefRegistrySimple* registry) {
   chromeos::DataPromoNotification::RegisterPrefs(registry);
   chromeos::DeviceOAuth2TokenService::RegisterPrefs(registry);
   chromeos::device_settings_cache::RegisterPrefs(registry);
-  chromeos::default_pinned_apps_field_trial::RegisterPrefs(registry);
   chromeos::EnableDebuggingScreenHandler::RegisterPrefs(registry);
   chromeos::language_prefs::RegisterPrefs(registry);
   chromeos::KioskAppManager::RegisterPrefs(registry);
@@ -530,13 +513,6 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 #if defined(USE_ASH)
   ash::RegisterChromeLauncherUserPrefs(registry);
 #endif
-
-  // Preferences registered only for migration (clearing or moving to a new key)
-  // go here.
-  registry->RegisterDictionaryPref(
-      kBackupPref,
-      new base::DictionaryValue(),
-      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 void RegisterUserProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
@@ -559,79 +535,45 @@ void RegisterLoginProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 }
 #endif
 
-void MigrateUserPrefs(Profile* profile) {
-  PrefService* prefs = profile->GetPrefs();
+void MigrateObsoleteBrowserPrefs(Profile* profile, PrefService* local_state) {
+// This method should be periodically pruned of year+ old migrations.

[gab, 2015/03/02 19:28:09]

Indent.

[rkaplow, 2015/03/02 19:36:35]

I agree, but what happened here was git cl format actually puts it here.
I suspect it is getting confused by the #ifdef below.

I would prefer to put it like this, as otherwise the git cl format will be
attempting to switch it back. I'd suggest filing a bug to have it handle this
case correctly.

[gab, 2015/03/02 20:05:08]

Actually I think this highlights that the comment should really be outside the
method (which hints about implementation details for the entire method) rather
than inside (which would hint that it's a comment about the next statement).

I debated that in my initial review but figured it was okay, if the format tool
insists, it hints further that it doesn't belong there IMO.

If you want to leave it there, please indent it despite the tool's desire and
file a bug with the clang-format team for Chromium (there is a chromium-dev
thread somewhere on how to do that).

[rkaplow, 2015/03/02 20:21:35]

Done.

 
-  // Cleanup prefs from now-removed protector feature.
-  prefs->ClearPref(kBackupPref);
+// Added 05/2014.

[gab, 2015/03/02 19:28:09]

Comment inside ifdef (and thus indented).

[rkaplow, 2015/03/02 19:36:35]

Done.

+#if defined(TOOLKIT_VIEWS)
+  MigrateBrowserTabStripPrefs(local_state);
+#endif
 
+// Added 08/2014.

[gab, 2015/03/02 19:28:09]

Comment inside ifdef (and thus indented).

[rkaplow, 2015/03/02 19:36:35]

Done.

 #if !defined(OS_ANDROID)
-  // Migrate kNetworkPredictionEnabled to kNetworkPredictionOptions when not on
-  // Android.  On Android, platform-specific code performs preference migration.
-  // TODO(bnc): https://crbug.com/401970  Remove migration code one year after
-  // M38.
-  chrome_browser_net::MigrateNetworkPredictionUserPrefs(prefs);
+  local_state->ClearPref(kLegacyProfileResetPromptMemento);
 #endif
+}
 
-  PromoResourceService::MigrateUserPrefs(prefs);
-  translate::TranslatePrefs::MigrateUserPrefs(prefs, prefs::kAcceptLanguages);
+void MigrateObsoleteProfilePrefs(Profile* profile) {
+  // This method should be periodically pruned of year+ old migrations.
+  PrefService* profile_prefs = profile->GetPrefs();

[gab, 2015/03/02 20:05:08]

Actually these two lines particularly highlight what I'm trying to say above,
as-is it looks like the comment on 553 is about the statement on 554 which it's
not.

I'm strongly in favor of moving the comment out of the method (but keeping it in
the impl (not the header) as it's really an implementation detail).

[rkaplow, 2015/03/02 20:21:35]

Done.

 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
-  autofill::AutofillManager::MigrateUserPrefs(prefs);
+  // Added 06/2014.
+  autofill::AutofillManager::MigrateUserPrefs(profile_prefs);
 #endif  // defined(OS_MACOSX) && !defined(OS_IOS)
 
-#if defined(OS_CHROMEOS) && defined(ENABLE_APP_LIST)
-  MigrateGoogleNowPrefs(profile);
-#endif
-}
-
-void MigrateBrowserPrefs(Profile* profile, PrefService* local_state) {
-  // Copy pref values which have been migrated to user_prefs from local_state,
-  // or remove them from local_state outright, if copying is not required.
-  int current_version =
-      local_state->GetInteger(prefs::kMultipleProfilePrefMigration);
-  PrefRegistrySimple* registry = static_cast<PrefRegistrySimple*>(
-      local_state->DeprecatedGetPrefRegistry());
-
-  if (!(current_version & DNS_PREFS)) {
-    registry->RegisterListPref(prefs::kDnsStartupPrefetchList);
-    local_state->ClearPref(prefs::kDnsStartupPrefetchList);
-
-    registry->RegisterListPref(prefs::kDnsHostReferralList);
-    local_state->ClearPref(prefs::kDnsHostReferralList);
-
-    current_version |= DNS_PREFS;
-    local_state->SetInteger(prefs::kMultipleProfilePrefMigration,
-                            current_version);
-  }
-
-  PrefService* user_prefs = profile->GetPrefs();
-  if (!(current_version & WINDOWS_PREFS)) {
-    registry->RegisterDictionaryPref(prefs::kBrowserWindowPlacement);
-    if (local_state->HasPrefPath(prefs::kBrowserWindowPlacement)) {
-      const PrefService::Preference* pref =
-          local_state->FindPreference(prefs::kBrowserWindowPlacement);
-      DCHECK(pref);
-      user_prefs->Set(prefs::kBrowserWindowPlacement,
-                      *(pref->GetValue()));
-    }
-    local_state->ClearPref(prefs::kBrowserWindowPlacement);
-
-    current_version |= WINDOWS_PREFS;
-    local_state->SetInteger(prefs::kMultipleProfilePrefMigration,
-                            current_version);
-  }
+  // Added 07/2014.
+  translate::TranslatePrefs::MigrateUserPrefs(profile_prefs,
+                                              prefs::kAcceptLanguages);
 
 #if !defined(OS_ANDROID)
-  local_state->ClearPref(kLegacyProfileResetPromptMemento);
-#endif
-
-#if defined(OS_CHROMEOS)
-  chromeos::default_pinned_apps_field_trial::MigratePrefs(local_state);
+  // Added 08/2014.
+  // Migrate kNetworkPredictionEnabled to kNetworkPredictionOptions when not on
+  // Android.  On Android, platform-specific code performs preference migration.
+  // TODO(bnc): https://crbug.com/401970  Remove migration code one year after
+  // M38.
+  chrome_browser_net::MigrateNetworkPredictionUserPrefs(profile_prefs);
 #endif
 
-#if defined(TOOLKIT_VIEWS)
-  MigrateBrowserTabStripPrefs(local_state);
+#if defined(OS_CHROMEOS) && defined(ENABLE_APP_LIST)
+  // Added 02/2015.
+  MigrateGoogleNowPrefs(profile);
 #endif
 }