Some refactorings to facilitate a larger change to TemplateURLService.

chrome/browser/search_engines/default_search_pref_migration.cc

Index: chrome/browser/search_engines/default_search_pref_migration.cc
diff --git a/chrome/browser/search_engines/default_search_pref_migration.cc b/chrome/browser/search_engines/default_search_pref_migration.cc
index 7516bfc9c2ad1a2c779ff171f103a157ed27090d..5a33f5532ad8ee91d17e1a8dcf142024fec84f93 100644
--- a/chrome/browser/search_engines/default_search_pref_migration.cc
+++ b/chrome/browser/search_engines/default_search_pref_migration.cc
@@ -6,68 +6,58 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/search_engines/default_search_manager.h"
 #include "chrome/browser/search_engines/template_url.h"
-#include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 
 namespace {
 
-void MigrateDefaultSearchPref(PrefService* pref_service) {
-  DefaultSearchManager default_search_manager(
-      pref_service, DefaultSearchManager::ObserverCallback());
-
-  if (default_search_manager.GetDefaultSearchEngineSource() ==
-      DefaultSearchManager::FROM_USER) {
-    return;
-  }
-
+// Loads the user-selected DSE (if there is one, and it's not masked by policy
+// or an extension) from legacy preferences.
+scoped_ptr<TemplateURLData> LoadDefaultSearchProviderFromPrefs(
+    PrefService* pref_service) {
   scoped_ptr<TemplateURLData> legacy_dse_from_prefs;
   bool legacy_is_managed = false;
-  bool has_legacy_dse_from_prefs =
-      TemplateURLService::LoadDefaultSearchProviderFromPrefs(
-          pref_service, &legacy_dse_from_prefs, &legacy_is_managed);
+  TemplateURLService::LoadDefaultSearchProviderFromPrefs(
+      pref_service, &legacy_dse_from_prefs, &legacy_is_managed);

[Peter Kasting, 2014/05/08 17:36:43]

Nit: It would be nice to cleanup this function to return a scoped_ptr<> instead
of taking it as an outparam (the second arg).

[erikwright (departed), 2014/05/08 18:24:20]

It returns a bool at the moment, which indicates a succesful/failed load. A
successful load can still produce a NULL TemplateURL.

It's used in that format in TemplateURLService as of this CL, but will move here
before it's all over, at which time it should be as you say.

+  if (legacy_is_managed)
+    legacy_dse_from_prefs.reset();
+  return legacy_dse_from_prefs.Pass();

[Peter Kasting, 2014/05/08 17:36:43]

Nit: Shorter:

  return legacy_is_managed ?
      scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();

[erikwright (departed), 2014/05/08 18:24:20]

Done.

+}
 
-  if (!has_legacy_dse_from_prefs) {
-    // The DSE is undefined. Nothing to migrate.
-    return;
-  }
-  if (!legacy_dse_from_prefs) {
-    // The DSE is defined as NULL. This can only really be done via policy.
-    // Policy-defined values will be automatically projected into the new
-    // format. Even if the user did somehow set this manually we do not have a
-    // way to migrate it.
-    return;
-  }
-  if (legacy_is_managed) {
-    // The DSE is policy-managed, not user-selected. It will automatically be
-    // projected into the new location.
-    return;
-  }
+void MigrateDefaultSearchPref(PrefService* pref_service) {
+  DCHECK(pref_service);
 
-  // If the pre-populated DSE matches the DSE from prefs we assume it is not a
-  // user-selected value.
-  scoped_ptr<TemplateURLData> prepopulated_dse(
-      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(pref_service));
-  if (prepopulated_dse &&
-      legacy_dse_from_prefs->prepopulate_id ==
-          prepopulated_dse->prepopulate_id) {
+  scoped_ptr<TemplateURLData> legacy_dse_from_prefs =
+      LoadDefaultSearchProviderFromPrefs(pref_service);
+  if (!legacy_dse_from_prefs)
     return;
-  }
-
-  UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);
 
-  // This looks like a user-selected value, so let's migrate it. Subsequent
-  // changes to this value will be automatically stored in the correct location.
-  default_search_manager.SetUserSelectedDefaultSearchEngine(
-      *legacy_dse_from_prefs);
+  DefaultSearchManager default_search_manager(
+      pref_service, DefaultSearchManager::ObserverCallback());
+  DefaultSearchManager::Source modern_source;
+  TemplateURLData* modern_value =
+      default_search_manager.GetDefaultSearchEngine(&modern_source);
+  if (modern_source == DefaultSearchManager::FROM_FALLBACK) {
+    // |modern_value| is the prepopulated default. If it matches the legacy DSE
+    // we assume it is not a user-selected value.
+    if (!modern_value ||
+        legacy_dse_from_prefs->prepopulate_id != modern_value->prepopulate_id) {
+      // This looks like a user-selected value, so let's migrate it.
+      // TODO(erikwright): Remove this migration logic when this stat approaches
+      // zero.
+      UMA_HISTOGRAM_BOOLEAN("Search.MigratedPrefToDictionaryValue", true);
+      default_search_manager.SetUserSelectedDefaultSearchEngine(
+          *legacy_dse_from_prefs);
+    }
+  }
 
   // TODO(erikwright): Clear the legacy value when the modern value is the
-  // authority. Don't forget to do this even if we don't migrate (because we
-  // migrated prior to implementing the clear.
+  // authority.
 }
 
 void OnPrefsInitialized(PrefService* pref_service,