Some refactorings to facilitate a larger change to TemplateURLService.

BUG=365762
R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269310

Review URL: https://codereview.chromium.org/270533007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269723 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/search_engines/default_search_manager.cc

@@ -99,7 +99,7 @@ void DefaultSearchManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(
       kDefaultSearchProviderDataPrefName,
-      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+      user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 // static

chrome/browser/search_engines/default_search_pref_migration.cc

@@ -6,68 +6,57 @@
 
 #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);
+  return legacy_is_managed ?
+      scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();
+}
 
-  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;
-  }
 
+  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);
-
-  // 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);
+    }
+  }
 
   // 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,

chrome/browser/search_engines/template_url_service.cc

@@ -51,6 +51,7 @@
 #include "sync/protocol/search_engine_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "url/gurl.h"
 
 typedef SearchHostToURLsMap::TemplateURLSet TemplateURLSet;
 typedef TemplateURLService::SyncDataMap SyncDataMap;
@@ -265,8 +266,7 @@ TemplateURLService::TemplateURLService(Profile* profile)
       pending_synced_default_search_(false),
       dsp_change_origin_(DSP_CHANGE_OTHER),
       default_search_manager_(
-          new DefaultSearchManager(GetPrefs(),
-                                   DefaultSearchManager::ObserverCallback())) {
+          GetPrefs(), DefaultSearchManager::ObserverCallback()) {
   DCHECK(profile_);
   Init(NULL, 0);
 }
@@ -286,7 +286,9 @@ TemplateURLService::TemplateURLService(const Initializer* initializers,
       models_associated_(false),
       processing_syncer_changes_(false),
       pending_synced_default_search_(false),
-      dsp_change_origin_(DSP_CHANGE_OTHER) {
+      dsp_change_origin_(DSP_CHANGE_OTHER),
+      default_search_manager_(
+          GetPrefs(), DefaultSearchManager::ObserverCallback()) {
   Init(initializers, count);
 }
 
@@ -572,7 +574,7 @@ bool TemplateURLService::CanReplaceKeyword(
 void TemplateURLService::FindMatchingKeywords(
     const base::string16& prefix,
     bool support_replacement_only,
-    TemplateURLVector* matches) const {
+    TemplateURLVector* matches) {
   // Sanity check args.
   if (prefix.empty())
     return;
@@ -809,12 +811,10 @@ bool TemplateURLService::CanMakeDefault(const TemplateURL* url) {
 void TemplateURLService::SetUserSelectedDefaultSearchProvider(
     TemplateURL* url) {
   SetDefaultSearchProvider(url);
-  if (default_search_manager_) {
   if (url)
-      default_search_manager_->SetUserSelectedDefaultSearchEngine(url->data());
+    default_search_manager_.SetUserSelectedDefaultSearchEngine(url->data());
   else
-      default_search_manager_->ClearUserSelectedDefaultSearchEngine();
-  }
+    default_search_manager_.ClearUserSelectedDefaultSearchEngine();
 }
 
 TemplateURL* TemplateURLService::GetDefaultSearchProvider() {
@@ -1167,7 +1167,7 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         new_changes.push_back(syncer::SyncChange(FROM_HERE,
                                                  syncer::SyncChange::ACTION_ADD,
                                                  sync_data));
-        // Ignore the delete attempt. This means we never end up reseting the
+        // Ignore the delete attempt. This means we never end up resetting the
         // default search provider due to an ACTION_DELETE from sync.
         continue;
       }
@@ -2686,7 +2686,7 @@ void TemplateURLService::EnsureDefaultSearchProviderExists() {
 }
 
 TemplateURL* TemplateURLService::CreateTemplateURLForExtension(
-    const ExtensionKeyword& extension_keyword) const {
+    const ExtensionKeyword& extension_keyword) {
   TemplateURLData data;
   data.short_name = base::UTF8ToUTF16(extension_keyword.extension_name);
   data.SetKeyword(base::UTF8ToUTF16(extension_keyword.extension_keyword));
@@ -2699,7 +2699,7 @@ TemplateURL* TemplateURLService::CreateTemplateURLForExtension(
 
 TemplateURL* TemplateURLService::FindTemplateURLForExtension(
     const std::string& extension_id,
-    TemplateURL::Type type) const {
+    TemplateURL::Type type) {
   DCHECK_NE(TemplateURL::NORMAL, type);
   for (TemplateURLVector::const_iterator i = template_urls_.begin();
        i != template_urls_.end(); ++i) {

chrome/browser/search_engines/template_url_service.h

@@ -156,7 +156,7 @@ class TemplateURLService : public WebDataServiceConsumer,
   // TemplateURLs that support replacement are returned.
   void FindMatchingKeywords(const base::string16& prefix,
                             bool support_replacement_only,
-                            TemplateURLVector* matches) const;
+                            TemplateURLVector* matches);
 
   // Looks up |keyword| and returns the element it maps to.  Returns NULL if
   // the keyword was not found.
@@ -276,7 +276,7 @@ class TemplateURLService : public WebDataServiceConsumer,
   // revved: all existing prepopulated entries are checked against the current
   // prepopulate data, any now-extraneous safe_for_autoreplace() entries are
   // removed, any existing engines are reset to the provided data (except for
-  // user-edited names or keywords), and any new prepopulated anegines are
+  // user-edited names or keywords), and any new prepopulated engines are
   // added.
   //
   // After this, the default search engine is reset to the default entry in the
@@ -673,11 +673,11 @@ class TemplateURLService : public WebDataServiceConsumer,
 
   // Returns a new TemplateURL for the given extension.
   TemplateURL* CreateTemplateURLForExtension(
-      const ExtensionKeyword& extension_keyword) const;
+      const ExtensionKeyword& extension_keyword);
 
   // Returns the TemplateURL associated with |extension_id|, if any.
   TemplateURL* FindTemplateURLForExtension(const std::string& extension_id,
-                                           TemplateURL::Type type) const;
+                                           TemplateURL::Type type);
 
   // Finds the most recently-installed NORMAL_CONTROLLED_BY_EXTENSION engine
   // that supports replacement and wants to be default, if any.
@@ -732,7 +732,7 @@ class TemplateURLService : public WebDataServiceConsumer,
   std::vector<history::URLVisitedDetails> visits_to_add_;
 
   // Once loaded, the default search provider.  This is a pointer to a
-  // TemplateURL owned by template_urls_.
+  // TemplateURL owned by |template_urls_|.
   TemplateURL* default_search_provider_;
 
   // The initial search provider extracted from preferences. This is only valid
@@ -786,9 +786,8 @@ class TemplateURLService : public WebDataServiceConsumer,
   // Stores a list of callbacks to be run after TemplateURLService has loaded.
   base::CallbackList<void(void)> on_loaded_callbacks_;
 
-  // Helper class to manage the default search engine. This will be NULL when
-  // using the testing-specific constructor.
-  scoped_ptr<DefaultSearchManager> default_search_manager_;
+  // Helper class to manage the default search engine.
+  DefaultSearchManager default_search_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(TemplateURLService);
 };

chrome/browser/search_engines/template_url_service_sync_unittest.cc

@@ -1881,9 +1881,9 @@ TEST_F(TemplateURLServiceSyncTest, PreSyncUpdates) {
   // updated time.
   TemplateURL* added_turl = model()->GetTemplateURLForKeyword(
       ASCIIToUTF16(kNewKeyword));
+  ASSERT_TRUE(added_turl);
   base::Time new_timestamp = added_turl->last_modified();
   EXPECT_GE(new_timestamp, pre_merge_time);
-  ASSERT_TRUE(added_turl);
   std::string sync_guid = added_turl->sync_guid();
 
   // Bring down a copy of the prepopulate engine from Sync with the old values,