Revert of Use the DefaultSearchManager as the exclusive authority on DSE,...

Revert of Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data. (https://codereview.chromium.org/268643002/)

Reason for revert:
Unfortunately I have to revert this change because this is in the way of reverting https://codereview.chromium.org/270533007/.

The above change is causing some use after free failures on Mac. Please see this: http://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/759/steps/unit_tests/logs/stdio

Original issue's description:
> Use the DefaultSearchManager as the exclusive authority on DSE, ignoring Web Data.
> 
> BUG=365762
> R=engedy@chromium.org, jochen@chromium.org, pkasting@chromium.org, zea@chromium.org
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269396

TBR=pkasting@chromium.org,zea@chromium.org,engedy@chromium.org,jochen@chromium.org,erikwright@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=365762

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

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

chrome/browser/profile_resetter/automatic_profile_resetter_delegate.cc

@@ -41,52 +41,40 @@ namespace {
 
 scoped_ptr<base::DictionaryValue> BuildSubTreeFromTemplateURL(
     const TemplateURL* template_url) {
+  scoped_ptr<base::DictionaryValue> tree(new base::DictionaryValue);
+  tree->SetString("search_url", template_url->url());
   // If this value contains a placeholder in the pre-populated data, it will
   // have been replaced as it was loaded into a TemplateURL.
   // BuildSubTreeFromTemplateURL works with TemplateURL (not TemplateURLData)
   // in order to maintain this behaviour.
   // TODO(engedy): Confirm the expected behaviour and convert to use
   // TemplateURLData if possible."
-  scoped_ptr<base::DictionaryValue> tree(new base::DictionaryValue);
-  tree->SetString("name", template_url->short_name());
-  tree->SetString("short_name", template_url->short_name());
-  tree->SetString("keyword", template_url->keyword());
-  tree->SetString("search_url", template_url->url());
-  tree->SetString("url", template_url->url());
-  tree->SetString("suggestions_url", template_url->suggestions_url());
+  tree->SetString("search_terms_replacement_key",
+                  template_url->search_terms_replacement_key());
+  tree->SetString("suggest_url", template_url->suggestions_url());
   tree->SetString("instant_url", template_url->instant_url());
   tree->SetString("image_url", template_url->image_url());
   tree->SetString("new_tab_url", template_url->new_tab_url());
   tree->SetString("search_url_post_params",
                   template_url->search_url_post_params());
-  tree->SetString("suggestions_url_post_params",
+  tree->SetString("suggest_url_post_params",
                   template_url->suggestions_url_post_params());
   tree->SetString("instant_url_post_params",
                   template_url->instant_url_post_params());
   tree->SetString("image_url_post_params",
                   template_url->image_url_post_params());
-  base::ListValue* alternate_urls = new base::ListValue;
-  alternate_urls->AppendStrings(template_url->alternate_urls());
-  tree->Set("alternate_urls", alternate_urls);
-  tree->SetString("favicon_url", template_url->favicon_url().spec());
-  tree->SetString("originating_url", template_url->originating_url().spec());
-  tree->SetBoolean("safe_for_autoreplace",
-                   template_url->safe_for_autoreplace());
+  tree->SetString("icon_url", template_url->favicon_url().spec());
+  tree->SetString("name", template_url->short_name());
+  tree->SetString("keyword", template_url->keyword());
   base::ListValue* input_encodings = new base::ListValue;
   input_encodings->AppendStrings(template_url->input_encodings());
-  tree->Set("input_encodings", input_encodings);
+  tree->Set("encodings", input_encodings);
   tree->SetString("id", base::Int64ToString(template_url->id()));
-  tree->SetString("date_created",
-                  base::Int64ToString(
-                      template_url->date_created().ToInternalValue()));
-  tree->SetString("last_modified",
-                  base::Int64ToString(
-                      template_url->last_modified().ToInternalValue()));
-  tree->SetBoolean("created_by_policy", template_url->created_by_policy());
-  tree->SetInteger("usage_count", template_url->usage_count());
-  tree->SetInteger("prepopulate_id", template_url->prepopulate_id());
-  tree->SetString("search_terms_replacement_key",
-                  template_url->search_terms_replacement_key());
+  tree->SetString("prepopulate_id",
+                  base::IntToString(template_url->prepopulate_id()));
+  base::ListValue* alternate_urls = new base::ListValue;
+  alternate_urls->AppendStrings(template_url->alternate_urls());
+  tree->Set("alternate_urls", alternate_urls);
   return tree.Pass();
 }
 

chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h

@@ -66,8 +66,15 @@ class AutomaticProfileResetterDelegate {
 
   // Returns attributes of the search engine currently set as the default (or
   // an empty dictionary if there is none).
-  // The dictionary is in the same format as persisted to preferences by
-  // DefaultSearchManager::SetUserSelectedDefaultSearchEngine.
+  // The returned attributes correspond in meaning and format to the user
+  // preferences stored by TemplateURLService::SaveDefaultSearchProviderToPrefs,
+  // and will be named after the second path name segment of the respective
+  // preference (i.e. the part after "default_search_provider.").
+  // Note that:
+  //  1.) the "enabled" attribute will not be present, as it is not technically
+  //      an attribute of a search provider,
+  //  2.) "encodings" will be a list of strings, in contrast to a single string
+  //      with tokens delimited by semicolons, but the order will be the same.
   virtual scoped_ptr<base::DictionaryValue>
       GetDefaultSearchProviderDetails() const = 0;
 

chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc

@@ -24,7 +24,6 @@
 #include "chrome/browser/google/google_util.h"
 #include "chrome/browser/profile_resetter/brandcoded_default_settings.h"
 #include "chrome/browser/profile_resetter/profile_reset_global_error.h"
-#include "chrome/browser/search_engines/default_search_manager.h"
 #include "chrome/browser/search_engines/template_url_prepopulate_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
@@ -92,10 +91,16 @@ class MockCallbackTarget {
 
 // Returns the details of the default search provider from |prefs| in a format
 // suitable for usage as |expected_details| in ExpectDetailsMatch().
-const base::DictionaryValue* GetDefaultSearchProviderDetailsFromPrefs(
+scoped_ptr<base::DictionaryValue> GetDefaultSearchProviderDetailsFromPrefs(
     const PrefService* prefs) {
-  return prefs->GetDictionary(
-      DefaultSearchManager::kDefaultSearchProviderDataPrefName);
+  const char kDefaultSearchProviderPrefix[] = "default_search_provider";
+  scoped_ptr<base::DictionaryValue> pref_values_with_path_expansion(
+      prefs->GetPreferenceValues());
+  const base::DictionaryValue* dsp_details = NULL;
+  EXPECT_TRUE(pref_values_with_path_expansion->GetDictionary(
+      kDefaultSearchProviderPrefix, &dsp_details));
+  return scoped_ptr<base::DictionaryValue>(
+      dsp_details ? dsp_details->DeepCopy() : new base::DictionaryValue);
 }
 
 // Verifies that the |details| of a search engine as provided by the delegate
@@ -114,9 +119,24 @@ void ExpectDetailsMatch(const base::DictionaryValue& expected_details,
     const base::Value* actual_value = NULL;
     ASSERT_TRUE(details.Get(it.key(), &actual_value));
 
-    // Ignore ID as it is dynamically assigned by the TemplateURLService.
-    // last_modified may get updated during a run, so ignore value differences.
-    if (it.key() != "id" && it.key() != "last_modified") {
+    if (it.key() == "id") {
+      // Ignore ID as it is dynamically assigned by the TemplateURLService.
+    } else if (it.key() == "encodings") {
+      // Encoding list is stored in Prefs as a single string with tokens
+      // delimited by semicolons.
+      std::string expected_encodings;
+      ASSERT_TRUE(expected_value->GetAsString(&expected_encodings));
+      const base::ListValue* actual_encodings_list = NULL;
+      ASSERT_TRUE(actual_value->GetAsList(&actual_encodings_list));
+      std::vector<std::string> actual_encodings_vector;
+      for (base::ListValue::const_iterator it = actual_encodings_list->begin();
+           it != actual_encodings_list->end(); ++it) {
+        std::string encoding;
+        ASSERT_TRUE((*it)->GetAsString(&encoding));
+        actual_encodings_vector.push_back(encoding);
+      }
+      EXPECT_EQ(expected_encodings, JoinString(actual_encodings_vector, ';'));
+    } else {
       // Everything else is the same format.
       EXPECT_TRUE(actual_value->Equals(expected_value))
           << "Expected: " << *expected_value << ". Actual: " << *actual_value;
@@ -359,8 +379,8 @@ TEST_F(AutomaticProfileResetterDelegateTest,
   ASSERT_TRUE(prefs);
   scoped_ptr<base::DictionaryValue> dsp_details(
       resetter_delegate()->GetDefaultSearchProviderDetails());
-  const base::DictionaryValue* expected_dsp_details =
-      GetDefaultSearchProviderDetailsFromPrefs(prefs);
+  scoped_ptr<base::DictionaryValue> expected_dsp_details(
+      GetDefaultSearchProviderDetailsFromPrefs(prefs));
 
   ExpectDetailsMatch(*expected_dsp_details, *dsp_details);
   EXPECT_FALSE(resetter_delegate()->IsDefaultSearchProviderManaged());
@@ -433,8 +453,8 @@ TEST_F(AutomaticProfileResetterDelegateTest,
 
     PrefService* prefs = profile()->GetPrefs();
     ASSERT_TRUE(prefs);
-    const base::DictionaryValue* expected_dsp_details =
-        GetDefaultSearchProviderDetailsFromPrefs(prefs);
+    scoped_ptr<base::DictionaryValue> expected_dsp_details(
+        GetDefaultSearchProviderDetailsFromPrefs(prefs));
     ExpectDetailsMatch(*expected_dsp_details, *details);
   }
 }

chrome/browser/search_engines/default_search_manager.cc

@@ -128,8 +128,7 @@ TemplateURLData* DefaultSearchManager::GetDefaultSearchEngine(
 
   if (source)
     *source = FROM_FALLBACK;
-  return TemplateURLService::fallback_search_engines_disabled() ?
-      NULL : fallback_default_search_.get();
+  return fallback_default_search_.get();
 }
 
 DefaultSearchManager::Source

chrome/browser/search_engines/default_search_pref_migration.cc

@@ -13,7 +13,6 @@
 #include "chrome/browser/search_engines/default_search_manager.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_service.h"
-#include "chrome/common/pref_names.h"
 
 namespace {
 
@@ -29,25 +28,6 @@ scoped_ptr<TemplateURLData> LoadDefaultSearchProviderFromPrefs(
       scoped_ptr<TemplateURLData>() : legacy_dse_from_prefs.Pass();
 }
 
-void ClearDefaultSearchProviderFromLegacyPrefs(PrefService* prefs) {
-  prefs->ClearPref(prefs::kDefaultSearchProviderName);
-  prefs->ClearPref(prefs::kDefaultSearchProviderKeyword);
-  prefs->ClearPref(prefs::kDefaultSearchProviderSearchURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderSuggestURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderInstantURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderImageURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderNewTabURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderSearchURLPostParams);
-  prefs->ClearPref(prefs::kDefaultSearchProviderSuggestURLPostParams);
-  prefs->ClearPref(prefs::kDefaultSearchProviderInstantURLPostParams);
-  prefs->ClearPref(prefs::kDefaultSearchProviderImageURLPostParams);
-  prefs->ClearPref(prefs::kDefaultSearchProviderIconURL);
-  prefs->ClearPref(prefs::kDefaultSearchProviderEncodings);
-  prefs->ClearPref(prefs::kDefaultSearchProviderPrepopulateID);
-  prefs->ClearPref(prefs::kDefaultSearchProviderAlternateURLs);
-  prefs->ClearPref(prefs::kDefaultSearchProviderSearchTermsReplacementKey);
-}
-
 void MigrateDefaultSearchPref(PrefService* pref_service) {
   DCHECK(pref_service);
 
@@ -75,7 +55,8 @@ void MigrateDefaultSearchPref(PrefService* pref_service) {
     }
   }
 
-  ClearDefaultSearchProviderFromLegacyPrefs(pref_service);
+  // TODO(erikwright): Clear the legacy value when the modern value is the
+  // authority.
 }
 
 void OnPrefsInitialized(PrefService* pref_service,

chrome/browser/search_engines/template_url_service_sync_unittest.cc

@@ -230,7 +230,6 @@ TemplateURLServiceSyncTest::TemplateURLServiceSyncTest()
           sync_processor_.get())) {}
 
 void TemplateURLServiceSyncTest::SetUp() {
-  TemplateURLService::set_fallback_search_engines_disabled(true);
   test_util_a_.SetUp();
   // Use ChangeToLoadState() instead of VerifyLoad() so we don't actually pull
   // in the prepopulate data, which the sync tests don't care about (and would
@@ -245,7 +244,6 @@ void TemplateURLServiceSyncTest::SetUp() {
 
 void TemplateURLServiceSyncTest::TearDown() {
   test_util_a_.TearDown();
-  TemplateURLService::set_fallback_search_engines_disabled(false);
 }
 
 scoped_ptr<syncer::SyncChangeProcessor>
@@ -1636,6 +1634,41 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultAlreadySetOnStartup) {
   ASSERT_EQ(default_search, model()->GetDefaultSearchProvider());
 }
 
+TEST_F(TemplateURLServiceSyncTest, NewDefaultIsAlreadySynced) {
+  // Ensure that if the synced DSP pref changed to another synced entry (as
+  // opposed to coming in as a new entry), it gets reset correctly.
+  // Start by setting kSyncedDefaultSearchProviderGUID to the entry that should
+  // end up as the default. Note that this must be done before the initial
+  // entries are added as otherwise this call will set the DSP immediately.
+  profile_a()->GetTestingPrefService()->SetString(
+      prefs::kSyncedDefaultSearchProviderGUID, "key2");
+
+  syncer::SyncDataList initial_data = CreateInitialSyncData();
+  // Ensure that our candidate default supports replacement.
+  scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key2"),
+      "http://key2.com/{searchTerms}", "key2", 90));
+  initial_data[1] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl);
+  for (syncer::SyncDataList::const_iterator iter = initial_data.begin();
+      iter != initial_data.end(); ++iter) {
+    TemplateURL* converted = Deserialize(*iter);
+    model()->Add(converted);
+  }
+
+  // Set the initial default to something other than the desired default.
+  model()->SetUserSelectedDefaultSearchProvider(
+      model()->GetTemplateURLForGUID("key1"));
+
+  // Merge in the same data (i.e. already synced entries).
+  model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
+      PassProcessor(), CreateAndPassSyncErrorFactory());
+
+  EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  TemplateURL* current_default = model()->GetDefaultSearchProvider();
+  ASSERT_TRUE(current_default);
+  EXPECT_EQ("key2", current_default->sync_guid());
+  EXPECT_EQ(ASCIIToUTF16("key2"), current_default->keyword());
+}
+
 TEST_F(TemplateURLServiceSyncTest, SyncWithManagedDefaultSearch) {
   // First start off with a few entries and make sure we can set an unmanaged
   // default search provider.
@@ -2216,25 +2249,3 @@ TEST_F(TemplateURLServiceSyncTest, MergeNonEditedPrepopulatedEngine) {
   EXPECT_EQ(default_turl->short_name, result_turl->short_name());
   EXPECT_EQ(default_turl->url(), result_turl->url());
 }
-
-TEST_F(TemplateURLServiceSyncTest, GUIDUpdatedOnDefaultSearchChange) {
-  const char kGUID[] = "initdefault";
-  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"),
-                                     "http://thewhat.com/{searchTerms}",
-                                     kGUID));
-  model()->SetUserSelectedDefaultSearchProvider(
-      model()->GetTemplateURLForGUID(kGUID));
-
-  const TemplateURL* default_search = model()->GetDefaultSearchProvider();
-  ASSERT_TRUE(default_search);
-
-  const char kNewGUID[] = "newdefault";
-  model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"),
-                                     "http://thewhat.com/{searchTerms}",
-                                     kNewGUID));
-  model()->SetUserSelectedDefaultSearchProvider(
-      model()->GetTemplateURLForGUID(kNewGUID));
-
-  EXPECT_EQ(kNewGUID, profile_a()->GetTestingPrefService()->GetString(
-      prefs::kSyncedDefaultSearchProviderGUID));
-}

chrome/browser/search_engines/template_url_service_test_util.cc

@@ -6,11 +6,9 @@
 
 #include "base/bind.h"
 #include "base/run_loop.h"
-#include "base/strings/string_split.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/google/google_url_tracker.h"
-#include "chrome/browser/search_engines/default_search_manager.h"
 #include "chrome/browser/search_engines/search_terms_data.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
@@ -146,56 +144,55 @@ void TemplateURLServiceTestUtilBase::SetManagedDefaultSearchPreferences(
     const std::string& encodings,
     const std::string& alternate_url,
     const std::string& search_terms_replacement_key) {
-  TestingPrefServiceSyncable* pref_service = profile()->GetTestingPrefService();
-  scoped_ptr<base::DictionaryValue> value(new base::DictionaryValue);
-  if (!enabled) {
-    value->SetBoolean(DefaultSearchManager::kDisabledByPolicy, true);
-    pref_service->SetManagedPref(
-        DefaultSearchManager::kDefaultSearchProviderDataPrefName,
-        value.release());
-    return;
-  }
-
-  EXPECT_FALSE(keyword.empty());
-  EXPECT_FALSE(search_url.empty());
-  value->Set(DefaultSearchManager::kShortName,
-             base::Value::CreateStringValue(name));
-  value->Set(DefaultSearchManager::kKeyword,
-             base::Value::CreateStringValue(keyword));
-  value->Set(DefaultSearchManager::kURL,
-             base::Value::CreateStringValue(search_url));
-  value->Set(DefaultSearchManager::kSuggestionsURL,
-             base::Value::CreateStringValue(suggest_url));
-  value->Set(DefaultSearchManager::kFaviconURL,
-             base::Value::CreateStringValue(icon_url));
-  value->Set(DefaultSearchManager::kSearchTermsReplacementKey,
-             base::Value::CreateStringValue(search_terms_replacement_key));
-
-  std::vector<std::string> encodings_items;
-  base::SplitString(encodings, ';', &encodings_items);
-  scoped_ptr<base::ListValue> encodings_list(new base::ListValue);
-  for (std::vector<std::string>::const_iterator it = encodings_items.begin();
-       it != encodings_items.end();
-       ++it) {
-    encodings_list->AppendString(*it);
+  if (enabled) {
+    EXPECT_FALSE(keyword.empty());
+    EXPECT_FALSE(search_url.empty());
   }
-  value->Set(DefaultSearchManager::kInputEncodings, encodings_list.release());
-
+  TestingPrefServiceSyncable* pref_service = profile()->GetTestingPrefService();
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderEnabled,
+                               base::Value::CreateBooleanValue(enabled));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderName,
+                               base::Value::CreateStringValue(name));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderKeyword,
+                               base::Value::CreateStringValue(keyword));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderSearchURL,
+                               base::Value::CreateStringValue(search_url));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderSuggestURL,
+                               base::Value::CreateStringValue(suggest_url));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderIconURL,
+                               base::Value::CreateStringValue(icon_url));
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderEncodings,
+                               base::Value::CreateStringValue(encodings));
   scoped_ptr<base::ListValue> alternate_url_list(new base::ListValue());
   if (!alternate_url.empty())
     alternate_url_list->Append(base::Value::CreateStringValue(alternate_url));
-  value->Set(DefaultSearchManager::kAlternateURLs,
-             alternate_url_list.release());
-
+  pref_service->SetManagedPref(prefs::kDefaultSearchProviderAlternateURLs,
+                               alternate_url_list.release());
   pref_service->SetManagedPref(
-      DefaultSearchManager::kDefaultSearchProviderDataPrefName,
-      value.release());
+      prefs::kDefaultSearchProviderSearchTermsReplacementKey,
+      base::Value::CreateStringValue(search_terms_replacement_key));
+  model()->Observe(chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED,
+                   content::NotificationService::AllSources(),
+                   content::NotificationService::NoDetails());
 }
 
 void TemplateURLServiceTestUtilBase::RemoveManagedDefaultSearchPreferences() {
   TestingPrefServiceSyncable* pref_service = profile()->GetTestingPrefService();
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderEnabled);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderName);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderKeyword);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderSearchURL);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderSuggestURL);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderIconURL);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderEncodings);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderAlternateURLs);
   pref_service->RemoveManagedPref(
-      DefaultSearchManager::kDefaultSearchProviderDataPrefName);
+      prefs::kDefaultSearchProviderSearchTermsReplacementKey);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderID);
+  pref_service->RemoveManagedPref(prefs::kDefaultSearchProviderPrepopulateID);
+  model()->Observe(chrome::NOTIFICATION_DEFAULT_SEARCH_POLICY_CHANGED,
+                   content::NotificationService::AllSources(),
+                   content::NotificationService::NoDetails());
 }
 
 TemplateURLService* TemplateURLServiceTestUtilBase::model() const {

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -187,6 +187,20 @@ class TemplateURLServiceTest : public testing::Test {
   TemplateURL* CreatePreloadedTemplateURL(bool safe_for_autoreplace,
                                           int prepopulate_id);
 
+  // Creates a TemplateURL with the same prepopulated id as a real prepopulated
+  // item. The input number determines which prepopulated item. The caller is
+  // responsible for owning the returned TemplateURL*.
+  TemplateURL* CreateReplaceablePreloadedTemplateURL(
+      bool safe_for_autoreplace,
+      size_t index_offset_from_default,
+      base::string16* prepopulated_display_url);
+
+  // Verifies the behavior of when a preloaded url later gets changed.
+  // Since the input is the offset from the default, when one passes in
+  // 0, it tests the default. Passing in a number > 0 will verify what
+  // happens when a preloaded url that is not the default gets updated.
+  void TestLoadUpdatingPreloadedURL(size_t index_offset_from_default);
+
   // Helper methods to make calling TemplateURLServiceTestUtil methods less
   // visually noisy in the test code.
   void VerifyObserverCount(int expected_changed_count);
@@ -229,21 +243,6 @@ class TemplateURLServiceTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(TemplateURLServiceTest);
 };
 
-class TemplateURLServiceWithoutFallbackTest : public TemplateURLServiceTest {
- public:
-  TemplateURLServiceWithoutFallbackTest() : TemplateURLServiceTest() {}
-
-  virtual void SetUp() OVERRIDE {
-    TemplateURLService::set_fallback_search_engines_disabled(true);
-    TemplateURLServiceTest::SetUp();
-  }
-
-  virtual void TearDown() OVERRIDE {
-    TemplateURLServiceTest::TearDown();
-    TemplateURLService::set_fallback_search_engines_disabled(false);
-  }
-};
-
 TemplateURLServiceTest::TemplateURLServiceTest() {
 }
 
@@ -307,6 +306,64 @@ TemplateURL* TemplateURLServiceTest::CreatePreloadedTemplateURL(
   return new TemplateURL(test_util_.profile(), data);
 }
 
+TemplateURL* TemplateURLServiceTest::CreateReplaceablePreloadedTemplateURL(
+    bool safe_for_autoreplace,
+    size_t index_offset_from_default,
+    base::string16* prepopulated_display_url) {
+  size_t default_search_provider_index = 0;
+  ScopedVector<TemplateURLData> prepopulated_urls =
+      TemplateURLPrepopulateData::GetPrepopulatedEngines(
+          test_util_.profile()->GetPrefs(), &default_search_provider_index);
+  EXPECT_LT(index_offset_from_default, prepopulated_urls.size());
+  size_t prepopulated_index = (default_search_provider_index +
+      index_offset_from_default) % prepopulated_urls.size();
+  TemplateURL* t_url = CreatePreloadedTemplateURL(safe_for_autoreplace,
+      prepopulated_urls[prepopulated_index]->prepopulate_id);
+  *prepopulated_display_url =
+      TemplateURL(NULL, *prepopulated_urls[prepopulated_index]).url_ref().
+          DisplayURL();
+  return t_url;
+}
+
+void TemplateURLServiceTest::TestLoadUpdatingPreloadedURL(
+    size_t index_offset_from_default) {
+  base::string16 prepopulated_url;
+  TemplateURL* t_url = CreateReplaceablePreloadedTemplateURL(false,
+      index_offset_from_default, &prepopulated_url);
+
+  base::string16 original_url = t_url->url_ref().DisplayURL();
+  std::string original_guid = t_url->sync_guid();
+  EXPECT_NE(prepopulated_url, original_url);
+
+  // Then add it to the model and save it all.
+  test_util_.ChangeModelToLoadState();
+  model()->Add(t_url);
+  const TemplateURL* keyword_url =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("unittest"));
+  ASSERT_TRUE(keyword_url != NULL);
+  EXPECT_EQ(t_url, keyword_url);
+  EXPECT_EQ(original_url, keyword_url->url_ref().DisplayURL());
+  base::RunLoop().RunUntilIdle();
+
+  // Now reload the model and verify that the merge updates the url, and
+  // preserves the sync GUID.
+  test_util_.ResetModel(true);
+  keyword_url = model()->GetTemplateURLForKeyword(ASCIIToUTF16("unittest"));
+  ASSERT_TRUE(keyword_url != NULL);
+  EXPECT_EQ(prepopulated_url, keyword_url->url_ref().DisplayURL());
+  EXPECT_EQ(original_guid, keyword_url->sync_guid());
+
+  // Wait for any saves to finish.
+  base::RunLoop().RunUntilIdle();
+
+  // Reload the model to verify that change was saved correctly.
+  test_util_.ResetModel(true);
+  keyword_url = model()->GetTemplateURLForKeyword(ASCIIToUTF16("unittest"));
+  ASSERT_TRUE(keyword_url != NULL);
+  EXPECT_EQ(prepopulated_url, keyword_url->url_ref().DisplayURL());
+  EXPECT_EQ(original_guid, keyword_url->sync_guid());
+}
+
 void TemplateURLServiceTest::VerifyObserverCount(int expected_changed_count) {
   EXPECT_EQ(expected_changed_count, test_util_.GetObserverCount());
   test_util_.ResetObserverCount();
@@ -992,11 +1049,11 @@ TEST_F(TemplateURLServiceTest, DontUpdateKeywordSearchForNonReplaceable) {
   }
 }
 
-TEST_F(TemplateURLServiceWithoutFallbackTest, ChangeGoogleBaseValue) {
-  // NOTE: Do not load the prepopulate data, which also has a {google:baseURL}
-  // keyword in it and would confuse this test.
+TEST_F(TemplateURLServiceTest, ChangeGoogleBaseValue) {
+  // NOTE: Do not do a VerifyLoad() here as it will load the prepopulate data,
+  // which also has a {google:baseURL} keyword in it, which will confuse this
+  // test.
   test_util_.ChangeModelToLoadState();
-
   test_util_.SetGoogleBaseURL(GURL("http://google.com/"));
   const TemplateURL* t_url = AddKeywordWithDate(
       "name", "google.com", "{google:baseURL}?q={searchTerms}", "http://sugg1",
@@ -1173,6 +1230,42 @@ TEST_F(TemplateURLServiceTest, LoadSavesPrepopulatedDefaultSearchProvider) {
   AssertEquals(*cloned_url, *default_search);
 }
 
+TEST_F(TemplateURLServiceTest, FindNewDefaultSearchProvider) {
+  // Ensure that if our service is initially empty, we don't initial have a
+  // valid new DSP.
+  EXPECT_FALSE(model()->FindNewDefaultSearchProvider());
+
+  // Add a few entries with searchTerms, but ensure only the last one is in the
+  // default list.
+  AddKeywordWithDate("name1", "key1", "http://foo1/{searchTerms}",
+                     "http://sugg1", std::string(), "http://icon1", true,
+                     "UTF-8;UTF-16", Time(), Time());
+  AddKeywordWithDate("name2", "key2", "http://foo2/{searchTerms}",
+                     "http://sugg2", std::string(), "http://icon1", true,
+                     "UTF-8;UTF-16", Time(), Time());
+  AddKeywordWithDate("name3", "key3", "http://foo1/{searchTerms}",
+                     "http://sugg3", std::string(), "http://icon3", true,
+                     "UTF-8;UTF-16", Time(), Time());
+  TemplateURLData data;
+  data.short_name = ASCIIToUTF16("valid");
+  data.SetKeyword(ASCIIToUTF16("validkeyword"));
+  data.SetURL("http://valid/{searchTerms}");
+  data.favicon_url = GURL("http://validicon");
+  data.show_in_default_list = true;
+  TemplateURL* valid_turl(new TemplateURL(test_util_.profile(), data));
+  model()->Add(valid_turl);
+  EXPECT_EQ(4U, model()->GetTemplateURLs().size());
+
+  // Request a new DSP from the service and only expect the valid one.
+  TemplateURL* new_default = model()->FindNewDefaultSearchProvider();
+  ASSERT_TRUE(new_default);
+  EXPECT_EQ(valid_turl, new_default);
+
+  // Remove the default we received and ensure that the service returns NULL.
+  model()->Remove(new_default);
+  EXPECT_FALSE(model()->FindNewDefaultSearchProvider());
+}
+
 // Make sure that the load routine doesn't delete
 // prepopulated engines that no longer exist in the prepopulate data if
 // it is the default search provider.
@@ -1218,13 +1311,37 @@ TEST_F(TemplateURLServiceTest, LoadRetainsDefaultProvider) {
   }
 }
 
+// Make sure that the load routine updates the url of a preexisting
+// default search engine provider and that the result is saved correctly.
+TEST_F(TemplateURLServiceTest, LoadUpdatesDefaultSearchURL) {
+  TestLoadUpdatingPreloadedURL(0);
+}
+
+// Make sure that the load routine updates the url of a preexisting
+// non-default search engine provider and that the result is saved correctly.
+TEST_F(TemplateURLServiceTest, LoadUpdatesSearchURL) {
+  TestLoadUpdatingPreloadedURL(1);
+}
+
 // Make sure that the load routine sets a default search provider if it was
 // missing and not managed.
 TEST_F(TemplateURLServiceTest, LoadEnsuresDefaultSearchProviderExists) {
   // Force the model to load and make sure we have a default search provider.
   test_util_.VerifyLoad();
-  EXPECT_TRUE(model()->GetDefaultSearchProvider());
+  TemplateURL* old_default = model()->GetDefaultSearchProvider();
+  EXPECT_TRUE(old_default);
 
+  // Now remove it.
+  model()->SetUserSelectedDefaultSearchProvider(NULL);
+  model()->Remove(old_default);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(model()->GetDefaultSearchProvider());
+
+  // Reset the model and load it. There should be a default search provider.
+  test_util_.ResetModel(true);
+
+  ASSERT_TRUE(model()->GetDefaultSearchProvider());
   EXPECT_TRUE(model()->GetDefaultSearchProvider()->SupportsReplacement());
 
   // Make default search provider unusable (no search terms).
@@ -1342,10 +1459,10 @@ TEST_F(TemplateURLServiceTest, TestManagedDefaultSearch) {
   EXPECT_FALSE(model()->is_default_search_managed());
   EXPECT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
 
-  // The default should now be the user preference.
+  // The default should now be the first URL added
   const TemplateURL* actual_final_managed_default =
       model()->GetDefaultSearchProvider();
-  ExpectSimilar(regular_default, actual_final_managed_default);
+  ExpectSimilar(model()->GetTemplateURLs()[0], actual_final_managed_default);
   EXPECT_EQ(actual_final_managed_default->show_in_default_list(), true);
 
   // Disable the default search provider through policy.

chrome/browser/search_engines/util.cc

@@ -98,16 +98,8 @@ void RemoveDuplicatePrepopulateIDs(
     UncheckedURLMap::iterator best = unchecked_urls.begin();
     bool matched_keyword = false;
     for (UncheckedURLMap::iterator i = unchecked_urls.begin(); i != end; ++i) {
-      // If the user-selected DSE is a prepopulated engine its properties will
-      // either come from the prepopulation origin or from the user preferences
-      // file (see DefaultSearchManager). Those properties will end up
-      // overwriting whatever we load now anyway. If we are eliminating
-      // duplicates, then, we err on the side of keeping the thing that looks
-      // more like the value we will end up with in the end.
-      if (default_search_provider &&
-          (default_search_provider->prepopulate_id() ==
-              i->second->prepopulate_id()) &&
-          default_search_provider->HasSameKeywordAs(i->second->data())) {
+      // A URL is automatically the best if it's the default search engine.
+      if (i->second == default_search_provider) {
         best = i;
         break;
       }
@@ -199,14 +191,17 @@ void MergeEnginesFromPrepopulateData(
     ScopedVector<TemplateURLData>* prepopulated_urls,
     size_t default_search_index,
     TemplateURLService::TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider,
+    TemplateURL** default_search_provider,
     std::set<std::string>* removed_keyword_guids) {
   DCHECK(service == NULL || BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(prepopulated_urls);
   DCHECK(template_urls);
+  DCHECK(default_search_provider);
 
+  int default_prepopulated_id =
+      (*prepopulated_urls)[default_search_index]->prepopulate_id;
   ActionsFromPrepopulateData actions(CreateActionsFromCurrentPrepopulateData(
-      prepopulated_urls, *template_urls, default_search_provider));
+      prepopulated_urls, *template_urls, *default_search_provider));
 
   // Remove items.
   for (std::vector<TemplateURL*>::iterator i = actions.removed_engines.begin();
@@ -215,8 +210,7 @@ void MergeEnginesFromPrepopulateData(
     TemplateURLService::TemplateURLVector::iterator j =
         std::find(template_urls->begin(), template_urls->end(), template_url);
     DCHECK(j != template_urls->end());
-    DCHECK(!default_search_provider ||
-           (*j)->prepopulate_id() != default_search_provider->prepopulate_id());
+    DCHECK(*j != *default_search_provider);
     template_urls->erase(j);
     if (service) {
       service->RemoveKeyword(template_url->id());
@@ -237,6 +231,8 @@ void MergeEnginesFromPrepopulateData(
     TemplateURLService::TemplateURLVector::iterator j = std::find(
         template_urls->begin(), template_urls->end(), existing_url.get());
     *j = new TemplateURL(profile, data);
+    if (*default_search_provider == existing_url.get())
+      *default_search_provider = *j;
   }
 
   // Add items.
@@ -246,6 +242,13 @@ void MergeEnginesFromPrepopulateData(
        ++it) {
     template_urls->push_back(new TemplateURL(profile, *it));
   }
+
+  if (!*default_search_provider) {
+    // The user had no existing default search provider, so set the
+    // default to the default prepopulated engine.
+    *default_search_provider = FindURLByPrepopulateID(*template_urls,
+                                                      default_prepopulated_id);
+  }
 }
 
 ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
@@ -299,16 +302,10 @@ ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
   // The block above removed all the URLs from the |id_to_turl| map that were
   // found in the prepopulate data.  Any remaining URLs that haven't been
   // user-edited or made default can be removed from the data store.
-  // We assume that this entry is equivalent to the DSE if its prepopulate ID
-  // and keyword both match. If the prepopulate ID _does_ match all properties
-  // will be replaced with those from |default_search_provider| anyway.
   for (IDMap::iterator i(id_to_turl.begin()); i != id_to_turl.end(); ++i) {
     TemplateURL* template_url = i->second;
     if ((template_url->safe_for_autoreplace()) &&
-        (!default_search_provider ||
-         (template_url->prepopulate_id() !=
-             default_search_provider->prepopulate_id()) ||
-         (template_url->keyword() != default_search_provider->keyword())))
+        (template_url != default_search_provider))
       actions.removed_engines.push_back(template_url);
   }
 
@@ -320,12 +317,14 @@ void GetSearchProvidersUsingKeywordResult(
     WebDataService* service,
     Profile* profile,
     TemplateURLService::TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider,
+    TemplateURL** default_search_provider,
     int* new_resource_keyword_version,
     std::set<std::string>* removed_keyword_guids) {
   DCHECK(service == NULL || BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(template_urls);
   DCHECK(template_urls->empty());
+  DCHECK(default_search_provider);
+  DCHECK(*default_search_provider == NULL);
   DCHECK_EQ(KEYWORDS_RESULT, result.GetType());
   DCHECK(new_resource_keyword_version);
 
@@ -348,6 +347,12 @@ void GetSearchProvidersUsingKeywordResult(
     template_urls->push_back(new TemplateURL(profile, *i));
   }
 
+  int64 default_search_provider_id = keyword_result.default_search_provider_id;
+  if (default_search_provider_id) {
+    *default_search_provider =
+        GetTemplateURLByID(*template_urls, default_search_provider_id);
+  }
+
   *new_resource_keyword_version = keyword_result.builtin_keyword_version;
   GetSearchProvidersUsingLoadedEngines(service, profile, template_urls,
                                        default_search_provider,
@@ -359,11 +364,12 @@ void GetSearchProvidersUsingLoadedEngines(
     WebDataService* service,
     Profile* profile,
     TemplateURLService::TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider,
+    TemplateURL** default_search_provider,
     int* resource_keyword_version,
     std::set<std::string>* removed_keyword_guids) {
   DCHECK(service == NULL || BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(template_urls);
+  DCHECK(default_search_provider);
   DCHECK(resource_keyword_version);
   PrefService* prefs = profile ? profile->GetPrefs() : NULL;
   size_t default_search_index;
@@ -371,7 +377,7 @@ void GetSearchProvidersUsingLoadedEngines(
       TemplateURLPrepopulateData::GetPrepopulatedEngines(prefs,
                                                          &default_search_index);
   RemoveDuplicatePrepopulateIDs(service, prepopulated_urls,
-                                default_search_provider, template_urls,
+                                *default_search_provider, template_urls,
                                 removed_keyword_guids);
 
   const int prepopulate_resource_keyword_version =

chrome/browser/search_engines/util.h

@@ -81,20 +81,21 @@ ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
 // prepopulated search providers to result in:
 //  * a set of template_urls (search providers). The caller owns the
 //    TemplateURL* returned in template_urls.
+//  * the default search provider (and if *default_search_provider is not NULL,
+//    it is contained in template_urls).
 //  * whether there is a new resource keyword version (and the value).
 //    |*new_resource_keyword_version| is set to 0 if no new value. Otherwise,
 //    it is the new value.
 // Only pass in a non-NULL value for service if the WebDataService should be
 // updated. If |removed_keyword_guids| is not NULL, any TemplateURLs removed
 // from the keyword table in the WebDataService will have their Sync GUIDs
-// added to it. |default_search_provider| will be used to prevent removing the
-// current user-selected DSE, regardless of changes in prepopulate data.
+// added to it.
 void GetSearchProvidersUsingKeywordResult(
     const WDTypedResult& result,
     WebDataService* service,
     Profile* profile,
     TemplateURLService::TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider,
+    TemplateURL** default_search_provider,
     int* new_resource_keyword_version,
     std::set<std::string>* removed_keyword_guids);
 
@@ -109,7 +110,7 @@ void GetSearchProvidersUsingLoadedEngines(
     WebDataService* service,
     Profile* profile,
     TemplateURLService::TemplateURLVector* template_urls,
-    TemplateURL* default_search_provider,
+    TemplateURL** default_search_provider,
     int* resource_keyword_version,
     std::set<std::string>* removed_keyword_guids);