Made the deletion of the default search provider defensive in TemplateURLService.

chrome/browser/search_engines/template_url_service_sync_unittest.cc

Index: chrome/browser/search_engines/template_url_service_sync_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_service_sync_unittest.cc b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
index a0e3804804ab15026eb03c9a2eda9ec842773b64..7a099569f2e1425f3997ce624492cad57b7e390e 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -438,7 +438,7 @@ TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) {
   // Create a key that conflicts with something in the model.
   scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key1"),
                                                      "http://new.com", "xyz"));
-  string16 new_keyword = model()->UniquifyKeyword(*turl);
+  string16 new_keyword = model()->UniquifyKeyword(*turl, false);
   EXPECT_EQ(ASCIIToUTF16("new.com"), new_keyword);
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(new_keyword));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("new.com"), "http://new.com",
@@ -448,7 +448,7 @@ TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) {
   // modifying the original keyword, since the autogenerated keyword is already
   // used.
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://new.com"));
-  new_keyword = model()->UniquifyKeyword(*turl);
+  new_keyword = model()->UniquifyKeyword(*turl, false);
   EXPECT_EQ(ASCIIToUTF16("key1_"), new_keyword);
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(new_keyword));
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1_"), "http://new.com"));
@@ -456,9 +456,17 @@ TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) {
   // Test a third collision. This should collide on both the autogenerated
   // keyword and the first uniquification attempt.
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://new.com"));
-  new_keyword = model()->UniquifyKeyword(*turl);
+  new_keyword = model()->UniquifyKeyword(*turl, false);
   EXPECT_EQ(ASCIIToUTF16("key1__"), new_keyword);
   EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(new_keyword));
+
+  // If we force the method, it should uniquify the keyword even if it is
+  // currently unique, and skip the host-based autogenerated keyword.
+  turl.reset(
+      CreateTestTemplateURL(ASCIIToUTF16("unique"), "http://unique.com"));
+  new_keyword = model()->UniquifyKeyword(*turl, true);
+  EXPECT_EQ(ASCIIToUTF16("unique_"), new_keyword);
+  EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(new_keyword));
 }
 
 TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
@@ -1606,7 +1614,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncedDefaultGUIDArrivesFirst) {
   ASSERT_EQ("newdefault", model()->GetDefaultSearchProvider()->sync_guid());
 }
 
-TEST_F(TemplateURLServiceSyncTest, DefaultGuidDeletedAndReplaced) {
+TEST_F(TemplateURLServiceSyncTest, DefaultGuidDeletedBeforeNewDSPArrives) {
   syncer::SyncDataList initial_data;
   // The default search provider should support replacement.
   scoped_ptr<TemplateURL> turl1(CreateTestTemplateURL(ASCIIToUTF16("key1"),
@@ -1639,27 +1647,36 @@ TEST_F(TemplateURLServiceSyncTest, DefaultGuidDeletedAndReplaced) {
   const TemplateURL* default_search = model()->GetDefaultSearchProvider();
   ASSERT_TRUE(default_search);
 
-  // Delete the old default. This will change the default to the next available
-  // (turl2), but should not affect the synced preference.
-  syncer::SyncChangeList changes1;
-  changes1.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_DELETE,
-                                          turl1.release()));
-  model()->ProcessSyncChanges(FROM_HERE, changes1);
-
-  EXPECT_EQ(1U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
-  EXPECT_EQ("key2", model()->GetDefaultSearchProvider()->sync_guid());
-  EXPECT_EQ("key1", profile_a()->GetTestingPrefService()->GetString(
-      prefs::kSyncedDefaultSearchProviderGUID));
-
   // Change kSyncedDefaultSearchProviderGUID to a GUID that does not exist in
   // the model yet. Ensure that the default has not changed in any way.
   profile_a()->GetTestingPrefService()->SetString(
       prefs::kSyncedDefaultSearchProviderGUID, "newdefault");
 
-  ASSERT_EQ("key2", model()->GetDefaultSearchProvider()->sync_guid());
+  ASSERT_EQ("key1", model()->GetDefaultSearchProvider()->sync_guid());
   EXPECT_EQ("newdefault", profile_a()->GetTestingPrefService()->GetString(
       prefs::kSyncedDefaultSearchProviderGUID));
 
+  // Simulate a situation where an ACTION_DELETE on the default arrives before
+  // the new default search provider entry. This should fail to delete the
+  // target entry, and instead send up an "undelete" to the server, after
+  // further uniquifying the keyword to avoid infinite sync loops. The synced
+  // default GUID should not be changed so that when the expected default entry
+  // arrives, it can still be set as the default.
+  syncer::SyncChangeList changes1;
+  changes1.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_DELETE,
+                                          turl1.release()));
+  model()->ProcessSyncChanges(FROM_HERE, changes1);
+
+  EXPECT_TRUE(model()->GetTemplateURLForKeyword(ASCIIToUTF16("key1_")));
+  EXPECT_EQ(2U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  EXPECT_EQ("key1", model()->GetDefaultSearchProvider()->sync_guid());
+  EXPECT_EQ("newdefault", profile_a()->GetTestingPrefService()->GetString(
+      prefs::kSyncedDefaultSearchProviderGUID));
+  syncer::SyncChange undelete = processor()->change_for_guid("key1");
+  EXPECT_EQ(syncer::SyncChange::ACTION_ADD, undelete.change_type());
+  EXPECT_EQ("key1_",
+            undelete.sync_data().GetSpecifics().search_engine().keyword());

[Nicolas Zea, 2012/07/10 17:07:47]

verify that the sync guid stayed the same?

[SteveT, 2012/07/10 17:58:35]

This was done above in 1673. Did you mean something else?

[Nicolas Zea, 2012/07/10 19:10:12]

Ah, I just realized you're doing a lookup based on the guid in the first place
(line 1675). Gotcha, disregard.

+
   // Finally, bring in the expected entry with the right GUID. Ensure that
   // the default has changed to the new search engine.
   syncer::SyncChangeList changes2;
@@ -1668,7 +1685,7 @@ TEST_F(TemplateURLServiceSyncTest, DefaultGuidDeletedAndReplaced) {
                             "newdefault")));
   model()->ProcessSyncChanges(FROM_HERE, changes2);
 
-  EXPECT_EQ(2U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   EXPECT_EQ("newdefault", model()->GetDefaultSearchProvider()->sync_guid());
   EXPECT_EQ("newdefault", profile_a()->GetTestingPrefService()->GetString(
       prefs::kSyncedDefaultSearchProviderGUID));