Rewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_DELETEs to Sync.

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 453 matching lines @@
 
   // 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,
-      "http://key1.com", std::string(), 9000);
-  model()->Add(original_turl);
-  scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(original_turl_keyword,
-      "http://new.com", "remote", 8999));
-  syncer::SyncChangeList changes;
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_NE(original_turl_keyword, sync_turl->keyword());
-  EXPECT_EQ(original_turl_keyword, original_turl->keyword());
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // Sync is newer.  Original TemplateURL keyword is uniquified.  A SyncChange
-  // is added (which in a normal run would be deleted by PruneSyncChanges() when
-  // the local GUID doesn't appear in the sync GUID list).  Also ensure that
-  // this does not change the safe_for_autoreplace flag or the TemplateURLID in
-  // the original.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-                                        "http://key1.com", "local", 9000);
-  model()->Add(original_turl);
-  TemplateURLID original_id = original_turl->id();
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        std::string(), 9001));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_EQ(original_turl_keyword, sync_turl->keyword());
-  EXPECT_NE(original_turl_keyword, original_turl->keyword());
-  EXPECT_FALSE(original_turl->safe_for_autoreplace());
-  EXPECT_EQ(original_id, original_turl->id());
-  EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("local", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // Equal times. Same result as above. Sync left alone, original uniquified so
-  // sync_turl can fit.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-                                        "http://key1.com", "local2", 9000);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        std::string(), 9000));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_EQ(original_turl_keyword, sync_turl->keyword());
-  EXPECT_NE(original_turl_keyword, original_turl->keyword());
-  EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("local2", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // Sync is newer, but original TemplateURL is created by policy, so it wins.
-  // Sync keyword is uniquified, and a syncer::SyncChange is added.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", std::string(), 9000, false, true);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        "remote2", 9999));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_NE(original_turl_keyword, sync_turl->keyword());
-  EXPECT_EQ(original_turl_keyword, original_turl->keyword());
-  EXPECT_EQ(NULL, model()->GetTemplateURLForKeyword(sync_turl->keyword()));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote2", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-}
-
-TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictBothAutoreplace) {
-  // This tests cases where both the sync and the local TemplateURL are marked
-  // safe_for_autoreplace.
-
-  // Create a keyword that conflicts, and make it older.  SyncChange is added,
-  // function returns false.
-  string16 original_turl_keyword = ASCIIToUTF16("key1");
-  TemplateURL* original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", std::string(), 9000, true);
-  model()->Add(original_turl);
-  scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(original_turl_keyword,
-      "http://new.com", "remote", 8999, true));
-  syncer::SyncChangeList changes;
-  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                   original_turl, &changes));
-  EXPECT_EQ(original_turl,
-            model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // Sync is newer.  Original TemplateURL is removed from the model.  A
-  // syncer::SyncChange is added (which in a normal run would be deleted by
-  // PruneSyncChanges() when the local GUID doesn't appear in the sync GUID
-  // list).
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", "local", 9000, true);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        std::string(), 9001, true));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_EQ(original_turl_keyword, sync_turl->keyword());
-  EXPECT_TRUE(model()->GetTemplateURLs().empty());
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("local", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-
-  // Equal times. Same result as above. Sync left alone, original removed so
-  // sync_turl can fit.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", "local2", 9000, true);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        std::string(), 9000, true));
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_EQ(original_turl_keyword, sync_turl->keyword());
-  EXPECT_TRUE(model()->GetTemplateURLs().empty());
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("local2", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-
-  // Sync is newer, but original TemplateURL is created by policy, so it wins.
-  // syncer::SyncChange is added, function returns false.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", std::string(), 9000, true, true);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        "remote2", 9999, true));
-  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                   original_turl, &changes));
-  EXPECT_EQ(original_turl,
-            model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote2", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-}
-
-TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictOneAutoreplace) {
-  // This tests cases where either the sync or the local TemplateURL is marked
-  // safe_for_autoreplace, but the other is not.  Basically, we run the same
-  // tests as in SyncKeywordConflictBothAutoreplace, but mark the keywords so as
-  // to reverse the outcome of each test.
-
-  // Create a keyword that conflicts, and make it older.  Normally the local
-  // TemplateURL would win this.
-  string16 original_turl_keyword = ASCIIToUTF16("key1");
-  TemplateURL* original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", "local", 9000, true);
-  model()->Add(original_turl);
-  scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(original_turl_keyword,
-      "http://new.com", std::string(), 8999));
-  syncer::SyncChangeList changes;
-  EXPECT_TRUE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                  original_turl, &changes));
-  EXPECT_EQ(original_turl_keyword, sync_turl->keyword());
-  EXPECT_TRUE(model()->GetTemplateURLs().empty());
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("local", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-
-  // Sync is newer.  Normally the sync TemplateURL would win this.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", std::string(), 9000);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        "remote", 9001, true));
-  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                   original_turl, &changes));
-  EXPECT_EQ(original_turl,
-            model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // Equal times. Same result as above.
-  original_turl = CreateTestTemplateURL(original_turl_keyword,
-      "http://key1.com", std::string(), 9000);
-  model()->Add(original_turl);
-  sync_turl.reset(CreateTestTemplateURL(original_turl_keyword, "http://new.com",
-                                        "remote2", 9000, true));
-  EXPECT_FALSE(model()->ResolveSyncKeywordConflict(sync_turl.get(),
-                                                   original_turl, &changes));
-  EXPECT_EQ(original_turl,
-            model()->GetTemplateURLForKeyword(original_turl_keyword));
-  ASSERT_EQ(1U, changes.size());
-  EXPECT_EQ("remote2", GetGUID(changes[0].sync_data()));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, changes[0].change_type());
-  changes.clear();
-  model()->Remove(original_turl);
-
-  // We don't run the "created by policy" test since URLs created by policy are
-  // never safe_for_autoreplace.
-}
-
-TEST_F(TemplateURLServiceSyncTest, FindDuplicateOfSyncTemplateURL) {
-  TemplateURL* original_turl =
-      CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com");
-  model()->Add(original_turl);
-
-  // No matches at all.
-  scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(ASCIIToUTF16("key2"),
-                                                          "http://key2.com"));
-  EXPECT_EQ(NULL, model()->FindDuplicateOfSyncTemplateURL(*sync_turl));
-
-  // URL matches, but not keyword. No dupe.
-  sync_turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"),
-                                        "http://key1.com"));
-  EXPECT_EQ(NULL, model()->FindDuplicateOfSyncTemplateURL(*sync_turl));
-
-  // Keyword matches, but not URL. No dupe.
-  sync_turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key1"),
-                                        "http://key2.com"));
-  EXPECT_EQ(NULL, model()->FindDuplicateOfSyncTemplateURL(*sync_turl));
-
-  // Duplicate.
-  sync_turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key1"),
-                                        "http://key1.com"));
-  const TemplateURL* dupe_turl =
-      model()->FindDuplicateOfSyncTemplateURL(*sync_turl);
-  ASSERT_TRUE(dupe_turl);
-  EXPECT_EQ(dupe_turl->keyword(), sync_turl->keyword());
-  EXPECT_EQ(dupe_turl->url(), sync_turl->url());
-}
-
-TEST_F(TemplateURLServiceSyncTest, MergeSyncAndLocalURLDuplicates) {
-  TemplateURL* original_turl = CreateTestTemplateURL(ASCIIToUTF16("key1"),
-      "http://key1.com", std::string(), 9000);
-  model()->Add(original_turl);
-  TemplateURL* sync_turl = CreateTestTemplateURL(ASCIIToUTF16("key1"),
-      "http://key1.com", std::string(), 9001);
-  std::string original_guid = original_turl->sync_guid();
-  syncer::SyncChangeList changes;
-
-  // The sync TemplateURL is newer. It should replace the original TemplateURL
-  // and a syncer::SyncChange should be added to the list.
-  // Note that MergeSyncAndLocalURLDuplicates takes ownership of sync_turl.
-  model()->MergeSyncAndLocalURLDuplicates(sync_turl, original_turl, &changes);
-  TemplateURL* result = model()->GetTemplateURLForKeyword(ASCIIToUTF16("key1"));
-  ASSERT_TRUE(result);
-  EXPECT_EQ(9001, result->last_modified().ToTimeT());
-  EXPECT_EQ(1U, changes.size());
-  // We expect a change to delete the local entry.
-  syncer::SyncChange change = changes.at(0);
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE, change.change_type());
-  EXPECT_EQ(original_guid,
-            change.sync_data().GetSpecifics().search_engine().sync_guid());
-  changes.clear();
-
-  // The sync TemplateURL is older. The existing TemplateURL should win and a
-  // syncer::SyncChange should be added to the list.
-  TemplateURL* sync_turl2 = CreateTestTemplateURL(ASCIIToUTF16("key1"),
-      "http://key1.com", std::string(), 8999);
-  std::string sync_guid = sync_turl2->sync_guid();
-  model()->MergeSyncAndLocalURLDuplicates(sync_turl2, sync_turl, &changes);
-  result = model()->GetTemplateURLForKeyword(ASCIIToUTF16("key1"));
-  ASSERT_TRUE(result);
-  EXPECT_EQ(9001, result->last_modified().ToTimeT());
-  EXPECT_EQ(1U, changes.size());
-  // We expect a change to update the sync entry.
-  change = changes.at(0);
-  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change.change_type());
-  EXPECT_EQ(sync_guid,
-            change.sync_data().GetSpecifics().search_engine().sync_guid());
-}
-
 TEST_F(TemplateURLServiceSyncTest, StartSyncEmpty) {
   model()->MergeDataAndStartSyncing(
       syncer::SEARCH_ENGINES, syncer::SyncDataList(),
       PassProcessor(), CreateAndPassSyncErrorFactory());
 
   EXPECT_EQ(0U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
   EXPECT_EQ(0U, processor()->change_list_size());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeIntoEmpty) {
@@ skipping 117 matching lines @@
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"),
       "http://expected.com", "bbb", 100));  // keyword conflict
 
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"),
                                      "http://unique.com", "ccc"));  // add
 
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES,
                                     CreateInitialSyncData(), PassProcessor(),
                                     CreateAndPassSyncErrorFactory());
 
-  // The dupe results in a merge. The other two should be added to the model.
-  EXPECT_EQ(5U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  // The dupe and conflict results in merges, as local values are always merged
+  // with sync values if there is a keyword conflict. The unique keyword should
+  // be added.
+  EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
 
   // The key1 duplicate results in the local copy winning. Ensure that Sync's
   // copy was not added, and the local copy is pushed upstream to Sync as an
   // update. The local copy should have received the sync data's GUID.
   EXPECT_TRUE(model()->GetTemplateURLForGUID("key1"));
   // Check changes for the UPDATE.
   ASSERT_TRUE(processor()->contains_guid("key1"));
   syncer::SyncChange key1_change = processor()->change_for_guid("key1");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, key1_change.change_type());
+  // The local sync_guid should no longer be found.
   EXPECT_FALSE(model()->GetTemplateURLForGUID("aaa"));
 
-  // The key2 keyword conflict results in the local copy winning, so ensure it
-  // retains the original keyword, and that an update to the sync copy is pushed
-  // upstream to Sync. Both TemplateURLs should be found locally, however.
-  const TemplateURL* key2 = model()->GetTemplateURLForGUID("bbb");
-  EXPECT_TRUE(key2);
+  // The key2 keyword conflict results in a merge, with the values of the local
+  // copy winning, so ensure it retains the original URL, and that an update to
+  // the sync guid is pushed upstream to Sync.
+  const TemplateURL* key2 = model()->GetTemplateURLForGUID("key2");
+  ASSERT_TRUE(key2);
   EXPECT_EQ(ASCIIToUTF16("key2"), key2->keyword());
-  EXPECT_TRUE(model()->GetTemplateURLForGUID("key2"));
   // Check changes for the UPDATE.
   ASSERT_TRUE(processor()->contains_guid("key2"));
   syncer::SyncChange key2_change = processor()->change_for_guid("key2");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, key2_change.change_type());
-  EXPECT_EQ("key2.com", GetKeyword(key2_change.sync_data()));
+  EXPECT_EQ("key2", GetKeyword(key2_change.sync_data()));
+  EXPECT_EQ("http://expected.com", GetURL(key2_change.sync_data()));
+  // The local sync_guid should no longer be found.
+  EXPECT_FALSE(model()->GetTemplateURLForGUID("bbb"));
 
   // The last TemplateURL should have had no conflicts and was just added. It
   // should not have replaced the third local TemplateURL.
   EXPECT_TRUE(model()->GetTemplateURLForGUID("ccc"));
   EXPECT_TRUE(model()->GetTemplateURLForGUID("key3"));
 
-  // Two UPDATEs and two ADDs.
-  EXPECT_EQ(4U, processor()->change_list_size());
-  // Two ADDs should be pushed up to Sync.
-  ASSERT_TRUE(processor()->contains_guid("bbb"));
-  EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
-            processor()->change_for_guid("bbb").change_type());
+  // Two UPDATEs and one ADD.
+  EXPECT_EQ(3U, processor()->change_list_size());
+  // One ADDs should be pushed up to Sync.
   ASSERT_TRUE(processor()->contains_guid("ccc"));
   EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
             processor()->change_for_guid("ccc").change_type());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
-  // GUIDs all differ, so this is data to be added from Sync, but the timestamps
+  // GUIDs all differ, so Sync may overtake some entries, but the timestamps
   // from Sync are newer. Set up the local data so that one is a dupe, one has a
   // conflicting keyword, and the last has no conflicts (a clean ADD).
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com",
                                      "aaa", 10));  // dupe
 
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"),
       "http://expected.com", "bbb", 10));  // keyword conflict
 
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"),
                                      "http://unique.com", "ccc", 10));  // add
 
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES,
                                     CreateInitialSyncData(), PassProcessor(),
                                     CreateAndPassSyncErrorFactory());
 
-  // The dupe results in a merge. The other two should be added to the model.
-  EXPECT_EQ(5U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  // The dupe and keyword conflict results in merges. The unique keyword be
+  // added to the model.
+  EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
 
   // The key1 duplicate results in Sync's copy winning. Ensure that Sync's
   // copy replaced the local copy.
   EXPECT_TRUE(model()->GetTemplateURLForGUID("key1"));
   EXPECT_FALSE(model()->GetTemplateURLForGUID("aaa"));
 
   // The key2 keyword conflict results in Sync's copy winning, so ensure it
-  // retains the original keyword. The local copy should get a uniquified
-  // keyword. Both TemplateURLs should be found locally.
+  // retains the original keyword and is added. The local copy should be
+  // removed.
   const TemplateURL* key2_sync = model()->GetTemplateURLForGUID("key2");
   ASSERT_TRUE(key2_sync);
   EXPECT_EQ(ASCIIToUTF16("key2"), key2_sync->keyword());
-  const TemplateURL* key2_local = model()->GetTemplateURLForGUID("bbb");
-  ASSERT_TRUE(key2_local);
-  EXPECT_EQ(ASCIIToUTF16("expected.com"), key2_local->keyword());
+  EXPECT_FALSE(model()->GetTemplateURLForGUID("bbb"));
 
   // The last TemplateURL should have had no conflicts and was just added. It
   // should not have replaced the third local TemplateURL.
   EXPECT_TRUE(model()->GetTemplateURLForGUID("ccc"));
   EXPECT_TRUE(model()->GetTemplateURLForGUID("key3"));
 
-  // Two ADDs.
-  EXPECT_EQ(2U, processor()->change_list_size());
-  // Two ADDs should be pushed up to Sync.
-  ASSERT_TRUE(processor()->contains_guid("bbb"));
-  EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
-            processor()->change_for_guid("bbb").change_type());
+  // One ADD.
+  EXPECT_EQ(1U, processor()->change_list_size());
+  // One ADDs should be pushed up to Sync.
   ASSERT_TRUE(processor()->contains_guid("ccc"));
   EXPECT_EQ(syncer::SyncChange::ACTION_ADD,
             processor()->change_for_guid("ccc").change_type());
 }
 
 TEST_F(TemplateURLServiceSyncTest, ProcessChangesEmptyModel) {
   // We initially have no data.
   model()->MergeDataAndStartSyncing(
       syncer::SEARCH_ENGINES, syncer::SyncDataList(),
       PassProcessor(), CreateAndPassSyncErrorFactory());
@@ skipping 129 matching lines @@
             model()->GetTemplateURLForKeyword(ASCIIToUTF16("key3")));
 
   ASSERT_TRUE(processor()->contains_guid("aaa"));
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
             processor()->change_for_guid("aaa").change_type());
   ASSERT_TRUE(processor()->contains_guid("key1"));
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
             processor()->change_for_guid("key1").change_type());
 }
 
-TEST_F(TemplateURLServiceSyncTest, RemoveUpdatedURLOnConflict) {
-  // Updating a local replaceable URL to have the same keyword as a local
-  // non-replaceable URL should result in the former being removed from the
-  // model entirely.
-  syncer::SyncDataList initial_data;
-  scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("sync"),
-      "http://sync.com", "sync", 100, true));
-  initial_data.push_back(
-      TemplateURLService::CreateSyncDataFromTemplateURL(*turl));
-  model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
-      PassProcessor(), CreateAndPassSyncErrorFactory());
-
-  TemplateURL* new_turl =
-      CreateTestTemplateURL(ASCIIToUTF16("local"), "http://local.com", "local");
-  model()->Add(new_turl);
-
-  syncer::SyncChangeList changes;
-  changes.push_back(CreateTestSyncChange(syncer::SyncChange::ACTION_UPDATE,
-      CreateTestTemplateURL(ASCIIToUTF16("local"), "http://sync.com", "sync",
-                            110, true)));
-  model()->ProcessSyncChanges(FROM_HERE, changes);
-
-  EXPECT_EQ(1U, model()->GetTemplateURLs().size());
-  EXPECT_EQ("local",
-      model()->GetTemplateURLForKeyword(ASCIIToUTF16("local"))->sync_guid());
-  EXPECT_EQ(1U, processor()->change_list_size());
-  ASSERT_TRUE(processor()->contains_guid("sync"));
-  EXPECT_EQ(syncer::SyncChange::ACTION_DELETE,
-            processor()->change_for_guid("sync").change_type());
-}
-
 TEST_F(TemplateURLServiceSyncTest, ProcessTemplateURLChange) {
   // Ensure that ProcessTemplateURLChange is called and pushes the correct
   // changes to Sync whenever local changes are made to TemplateURLs.
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES,
                                     CreateInitialSyncData(), PassProcessor(),
                                     CreateAndPassSyncErrorFactory());
 
   // Add a new search engine.
   TemplateURL* new_turl =
       CreateTestTemplateURL(ASCIIToUTF16("baidu"), "http://baidu.cn", "new");
@@ skipping 105 matching lines @@
   syncer::SyncChange key2_change = processor()->change_for_guid("key2");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, key2_change.change_type());
   EXPECT_EQ(google_keyword, UTF8ToUTF16(GetKeyword(key2_change.sync_data())));
 }
 
 TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordConflicts) {
   // Sync brings in some autogenerated keywords, but the generated keywords we
   // try to create conflict with ones in the model.
   string16 google_keyword(net::StripWWW(ASCIIToUTF16(GURL(
       UIThreadSearchTermsData(profile_a()).GoogleBaseURLValue()).host())));
-  TemplateURL* google = CreateTestTemplateURL(google_keyword,
-      "{google:baseURL}1/search?q={searchTerms}");
+  const std::string local_google_url =
+      "{google:baseURL}1/search?q={searchTerms}";
+  TemplateURL* google = CreateTestTemplateURL(google_keyword, local_google_url);
   model()->Add(google);
   TemplateURL* other =
       CreateTestTemplateURL(ASCIIToUTF16("other.com"), "http://other.com/foo");
   model()->Add(other);
   syncer::SyncDataList initial_data;
   scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("sync1"),
       "{google:baseURL}2/search?q={searchTerms}", "sync1", 50));
   initial_data.push_back(
       CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
+  const std::string synced_other_url = "http://other.com/search?q={searchTerms}";
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("sync2"),
-      "http://other.com/search?q={searchTerms}", "sync2", 150));
+      synced_other_url, "sync2", 150));
   initial_data.push_back(
       CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
+
+  // Before we merge the data, grab the local sync_guids so we can ensure that
+  // they've been replaced.
+  const std::string local_google_guid = google->sync_guid();
+  const std::string local_other_guid = other->sync_guid();
+
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
       PassProcessor(), CreateAndPassSyncErrorFactory());
 
   // In this case, the conflicts should be handled just like any other keyword
   // conflicts -- the later-modified TemplateURL is assumed to be authoritative.
-  EXPECT_EQ(google_keyword, google->keyword());
-  EXPECT_EQ(google_keyword + ASCIIToUTF16("_"),
-            model()->GetTemplateURLForGUID("sync1")->keyword());
-  EXPECT_EQ(ASCIIToUTF16("other.com_"), other->keyword());
+  // Since the initial TemplateURLs were local only, they should be merged with
+  // the sync TemplateURLs (GUIDs transferred over).
+  EXPECT_FALSE(model()->GetTemplateURLForGUID(local_google_guid));
+  ASSERT_TRUE(model()->GetTemplateURLForGUID("sync1"));
+  EXPECT_EQ(google_keyword, model()->GetTemplateURLForGUID("sync1")->keyword());
+  EXPECT_FALSE(model()->GetTemplateURLForGUID(local_other_guid));
+  ASSERT_TRUE(model()->GetTemplateURLForGUID("sync2"));
   EXPECT_EQ(ASCIIToUTF16("other.com"),
             model()->GetTemplateURLForGUID("sync2")->keyword());
 
   // Both synced URLs should have associated UPDATEs, since both needed their
-  // keywords to be generated (and sync1 needed conflict resolution as well).
-  EXPECT_GE(processor()->change_list_size(), 2U);
+  // keywords to be generated.
+  EXPECT_EQ(processor()->change_list_size(), 2U);
   ASSERT_TRUE(processor()->contains_guid("sync1"));
   syncer::SyncChange sync1_change = processor()->change_for_guid("sync1");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, sync1_change.change_type());
-  EXPECT_EQ(google_keyword + ASCIIToUTF16("_"),
-            UTF8ToUTF16(GetKeyword(sync1_change.sync_data())));
+  EXPECT_EQ(google_keyword, UTF8ToUTF16(GetKeyword(sync1_change.sync_data())));
+  EXPECT_EQ(local_google_url, GetURL(sync1_change.sync_data()));
   ASSERT_TRUE(processor()->contains_guid("sync2"));
   syncer::SyncChange sync2_change = processor()->change_for_guid("sync2");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, sync2_change.change_type());
   EXPECT_EQ("other.com", GetKeyword(sync2_change.sync_data()));
+  EXPECT_EQ(synced_other_url, GetURL(sync2_change.sync_data()));
 }
 
 TEST_F(TemplateURLServiceSyncTest, TwoAutogeneratedKeywordsUsingGoogleBaseURL) {
   // Sync brings in two autogenerated keywords and both use Google base URLs.
   // We make the first older so that it will get renamed once before the second
   // and then again once after (when we resolve conflicts for the second).
   syncer::SyncDataList initial_data;
   scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(ASCIIToUTF16("key1"),
       "{google:baseURL}1/search?q={searchTerms}", "key1", 50));
   initial_data.push_back(
       CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"),
       "{google:baseURL}2/search?q={searchTerms}", "key2"));
   initial_data.push_back(
       CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
       PassProcessor(), CreateAndPassSyncErrorFactory());
 
   // We should still have coalesced the updates to one each.
   string16 google_keyword(net::StripWWW(ASCIIToUTF16(GURL(
       UIThreadSearchTermsData(profile_a()).GoogleBaseURLValue()).host())));
   TemplateURL* keyword1 =
       model()->GetTemplateURLForKeyword(google_keyword + ASCIIToUTF16("_"));
   ASSERT_FALSE(keyword1 == NULL);
   EXPECT_EQ("key1", keyword1->sync_guid());
   TemplateURL* keyword2 = model()->GetTemplateURLForKeyword(google_keyword);
   ASSERT_FALSE(keyword2 == NULL);
   EXPECT_EQ("key2", keyword2->sync_guid());
+
   EXPECT_GE(processor()->change_list_size(), 2U);
   ASSERT_TRUE(processor()->contains_guid("key1"));
   syncer::SyncChange key1_change = processor()->change_for_guid("key1");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, key1_change.change_type());
   EXPECT_EQ(keyword1->keyword(),
             UTF8ToUTF16(GetKeyword(key1_change.sync_data())));
   ASSERT_TRUE(processor()->contains_guid("key2"));
   syncer::SyncChange key2_change = processor()->change_for_guid("key2");
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, key2_change.change_type());
   EXPECT_EQ(keyword2->keyword(),
@@ skipping 473 matching lines @@
   syncer::SyncDataList initial_data = CreateInitialSyncData();
   // The key1 entry should be different from the default but conflict in the
   // keyword.
   scoped_ptr<TemplateURL> turl(CreateTestTemplateURL(keyword,
       "http://key1.com/{searchTerms}", "key1", 90));
   initial_data[0] = TemplateURLService::CreateSyncDataFromTemplateURL(*turl);
 
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
       PassProcessor(), CreateAndPassSyncErrorFactory());
 
-  // The conflicting TemplateURL should be added, but it should have lost
-  // conflict resolution against the default.
-  EXPECT_EQ(4U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
-  const TemplateURL* winner = model()->GetTemplateURLForGUID("whateverguid");
+  // Since the local default was not yet synced, it should be merged with the
+  // conflicting TemplateURL. However, it's values should have been preserved
+  // since it would have won conflict resolution due to being the default.
+  EXPECT_EQ(3U, model()->GetAllSyncData(syncer::SEARCH_ENGINES).size());
+  const TemplateURL* winner = model()->GetTemplateURLForGUID("key1");
   ASSERT_TRUE(winner);
   EXPECT_EQ(model()->GetDefaultSearchProvider(), winner);
   EXPECT_EQ(keyword, winner->keyword());
-  const TemplateURL* loser = model()->GetTemplateURLForGUID("key1");
-  ASSERT_TRUE(loser);
-  EXPECT_EQ(ASCIIToUTF16("key1.com"), loser->keyword());
+
+  // There is no loser, as the two were merged together. The local sync_guid
+  // should no longer be found in the model.
+  const TemplateURL* loser = model()->GetTemplateURLForGUID("whateverguid");
+  ASSERT_FALSE(loser);
 }
 
 TEST_F(TemplateURLServiceSyncTest, DeleteBogusData) {
   // Create a couple of bogus entries to sync.
   syncer::SyncDataList initial_data;
   scoped_ptr<TemplateURL> turl(
       CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com", "key1"));
   initial_data.push_back(
       CreateCustomSyncData(*turl, false, std::string(), turl->sync_guid()));
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"), "http://key2.com"));
@@ skipping 131 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()));
 }