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

chrome/browser/search_engines/template_url_service_sync_unittest.cc

 // Copyright (c) 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 "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/search_engines/search_terms_data.h"
@@ skipping 420 matching lines @@
     ASSERT_FALSE(service_turl->created_by_policy());
     AssertEquals(*service_turl, *deserialized);
   }
 }
 
 TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) {
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com"));
   // 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",
                                      "xyz"));
 
   // Test a second collision. This time it should be resolved by actually
   // 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"));
 
   // 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) {
   // This tests cases where neither the sync nor the local TemplateURL are
   // marked safe_for_autoreplace.
 
   // Create a keyword that conflicts, and make it older.  Sync keyword is
   // uniquified, and a syncer::SyncChange is added.
   string16 original_turl_keyword = ASCIIToUTF16("key1");
   TemplateURL* original_turl = CreateTestTemplateURL(original_turl_keyword,
@@ skipping 1127 matching lines @@
   changes2.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_ADD,
       CreateTestTemplateURL(ASCIIToUTF16("new"), "http://new.com/{searchTerms}",
                             "newdefault")));
   model()->ProcessSyncChanges(FROM_HERE, changes2);
 
   EXPECT_EQ(5U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   ASSERT_NE(default_search, model()->GetDefaultSearchProvider());
   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"),
       "http://key1.com/{searchTerms}", "key1", 90));
   // Create a second default search provider for the
   // FindNewDefaultSearchProvider method to find.
   TemplateURLData data;
   data.short_name = ASCIIToUTF16("unittest");
   data.SetKeyword(ASCIIToUTF16("key2"));
   data.SetURL("http://key2.com/{searchTerms}");
@@ skipping 12 matching lines @@
       *turl2));
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
       PassProcessor(), CreateAndPassSyncErrorFactory());
   model()->SetDefaultSearchProvider(model()->GetTemplateURLForGUID("key1"));
   ASSERT_EQ("key1", model()->GetDefaultSearchProvider()->sync_guid());
 
   EXPECT_EQ(2U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   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;
   changes2.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_ADD,
       CreateTestTemplateURL(ASCIIToUTF16("new"), "http://new.com/{searchTerms}",
                             "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));
 }
 
 TEST_F(TemplateURLServiceSyncTest, SyncedDefaultArrivesAfterStartup) {
   // Start with the default set to something in the model before we start
   // syncing.
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("what"),
                                      "http://thewhat.com/{searchTerms}",
@@ skipping 304 matching lines @@
   // A local change to the Google base URL should update the keyword and
   // generate a sync change.
   test_util_a_.SetGoogleBaseURL(GURL("http://google.co.in/"));
   EXPECT_EQ(ASCIIToUTF16("google.co.in"), synced_turl->keyword());
   EXPECT_EQ(1U, processor()->change_list_size());
   ASSERT_TRUE(processor()->contains_guid("guid"));
   syncer::SyncChange change(processor()->change_for_guid("guid"));
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change.change_type());
   EXPECT_EQ("google.co.in", GetKeyword(change.sync_data()));
 }