Revamp the MigrateBrowserPrefs and MigrateUserPrefs code.

chrome/browser/prefs/browser_prefs.cc

 // Copyright 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/prefs/browser_prefs.h"
 
 #include <string>
 
 #include "base/metrics/histogram_macros.h"
 #include "base/prefs/pref_registry_simple.h"
@@ skipping 145 matching lines @@
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/app_mode/kiosk_app_manager.h"
 #include "chrome/browser/chromeos/attestation/platform_verification_flow.h"
 #include "chrome/browser/chromeos/customization/customization_document.h"
 #include "chrome/browser/chromeos/display/display_preferences.h"
 #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"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_manager.h"
 #include "chrome/browser/chromeos/login/users/avatar/user_image_sync_observer.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager_impl.h"
 #include "chrome/browser/chromeos/login/users/multi_profile_user_controller.h"
 #include "chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h"
 #include "chrome/browser/chromeos/net/proxy_config_handler.h"
 #include "chrome/browser/chromeos/policy/auto_enrollment_client.h"
@@ skipping 40 matching lines @@
 #if defined(TOOLKIT_VIEWS)
 #include "chrome/browser/ui/browser_view_prefs.h"
 #endif
 
 #if defined(USE_ASH)
 #include "chrome/browser/ui/ash/chrome_launcher_prefs.h"
 #endif
 
 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
 // renamed in Local State. We keep the name here for now so that we can clear
 // out legacy values.
 // TODO(engedy): Remove this and usages in M42 or later. See crbug.com/398813.
 const char kLegacyProfileResetPromptMemento[] = "profile.reset_prompt_memento";
 #endif
 
 }  // namespace
 
 namespace chrome {
 
 void RegisterLocalState(PrefRegistrySimple* registry) {
-  // Prefs in Local State.
-  registry->RegisterIntegerPref(prefs::kMultipleProfilePrefMigration, 0);
 
   // Please keep this list alphabetized.
   AppListService::RegisterPrefs(registry);
   browser_shutdown::RegisterPrefs(registry);
   BrowserProcessImpl::RegisterPrefs(registry);
   ChromeMetricsServiceClient::RegisterPrefs(registry);
   chrome_prefs::RegisterPrefs(registry);
   chrome_variations::VariationsService::RegisterPrefs(registry);
   component_updater::RegisterPrefsForRecoveryComponent(registry);
   component_updater::SupervisedUserWhitelistInstaller::RegisterPrefs(registry);
@@ skipping 56 matching lines @@
 #endif  // !defined(OS_CHROMEOS)
 #endif  // !defined(OS_ANDROID)
 
 #if defined(OS_CHROMEOS)
   ChromeOSMetricsProvider::RegisterPrefs(registry);
   chromeos::AudioDevicesPrefHandlerImpl::RegisterPrefs(registry);
   chromeos::ChromeUserManagerImpl::RegisterPrefs(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);
   chromeos::MultiProfileUserController::RegisterPrefs(registry);
   chromeos::HIDDetectionScreenHandler::RegisterPrefs(registry);
   chromeos::DemoModeDetector::RegisterPrefs(registry);
   chromeos::Preferences::RegisterPrefs(registry);
   chromeos::proxy_config::RegisterPrefs(registry);
   chromeos::RegisterDisplayLocalStatePrefs(registry);
   chromeos::ResetScreenHandler::RegisterPrefs(registry);
@@ skipping 180 matching lines @@
 #endif
 
 #if defined(TOOLKIT_VIEWS)
   RegisterBrowserViewProfilePrefs(registry);
   RegisterInvertBubbleUserPrefs(registry);
 #endif
 
 #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) {
   RegisterProfilePrefs(registry);
 
 #if defined(OS_CHROMEOS)
   chromeos::PowerPrefs::RegisterUserProfilePrefs(registry);
 #endif
 }
 
 void RegisterScreenshotPrefs(PrefRegistrySimple* registry) {
   registry->RegisterBooleanPref(prefs::kDisableScreenshots, false);
 }
 
 #if defined(OS_CHROMEOS)
 void RegisterLoginProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   RegisterProfilePrefs(registry);
 
   chromeos::PowerPrefs::RegisterLoginProfilePrefs(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)
+  local_state->ClearPref(kLegacyProfileResetPromptMemento);
+#endif
+}
+
+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)
+  // Added 06/2014.
+  autofill::AutofillManager::MigrateUserPrefs(profile_prefs);
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
+  // Added 07/2014.
+  translate::TranslatePrefs::MigrateUserPrefs(profile_prefs,
+                                              prefs::kAcceptLanguages);
 
 #if !defined(OS_ANDROID)
+  // 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(prefs);
+  chrome_browser_net::MigrateNetworkPredictionUserPrefs(profile_prefs);
 #endif
 
-  PromoResourceService::MigrateUserPrefs(prefs);
-  translate::TranslatePrefs::MigrateUserPrefs(prefs, prefs::kAcceptLanguages);
-
-#if defined(OS_MACOSX) && !defined(OS_IOS)
-  autofill::AutofillManager::MigrateUserPrefs(prefs);
-#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
-
 #if defined(OS_CHROMEOS) && defined(ENABLE_APP_LIST)
+  // Added 02/2015.
   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);
-  }
-
-#if !defined(OS_ANDROID)
-  local_state->ClearPref(kLegacyProfileResetPromptMemento);
-#endif
-
-#if defined(OS_CHROMEOS)
-  chromeos::default_pinned_apps_field_trial::MigratePrefs(local_state);
-#endif
-
-#if defined(TOOLKIT_VIEWS)
-  MigrateBrowserTabStripPrefs(local_state);
-#endif
-}
-
 // As part of the migration from per-profile to per-partition HostZoomMaps,
 // we need to detect if an existing per-profile set of preferences exist, and
 // if so convert them to be per-partition. We migrate any per-profile zoom
 // level prefs via zoom_level_prefs.
 // Code that updates zoom prefs in the profile prefs store has been removed,
 // so once we clear these values here, they should never get set again.
 // TODO(wjmaclean): Remove this migration machinery after histograms show
 // that an aceptable percentage of users have been migrated.
 // crbug.com/420643
 void MigrateProfileZoomLevelPrefs(Profile* profile) {
@@ skipping 34 matching lines @@
       host_zoom_dictionary, false /* sanitize_partition_host_zoom_levels */);
 
   // We're done migrating the profile per-host zoom level values, so we clear
   // them all.
   DictionaryPrefUpdate host_zoom_dictionary_update(
       prefs, prefs::kPerHostZoomLevelsDeprecated);
   host_zoom_dictionary_update->Clear();
 }
 
 }  // namespace chrome