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) {
+TEST_F(TemplateURLServiceSyncTest, IsLocalTemplateURLBetter) {
+  // Test some edge cases of this function.
+  const struct {
+    time_t local_time;
+    time_t sync_time;
+    bool local_is_default;
+    bool local_created_by_policy;
+    bool expected_result;
+  } test_cases[] = {
+    // Sync is better by timestamp but local is Default.
+    {10, 100, true, false, true},
+    // Sync is better by timestamp but local is Create by Policy.
+    {10, 100, false, true, true},
+    // Tie. Sync wins.
+    {100, 100, false, false, false},
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) {
+    TemplateURL* local_turl = CreateTestTemplateURL(
+        ASCIIToUTF16("localkey"), "www.local.com", "localguid",
+        test_cases[i].local_time, true, test_cases[i].local_created_by_policy);
+    model()->Add(local_turl);
+    if (test_cases[i].local_is_default)
+      model()->SetDefaultSearchProvider(local_turl);
+
+    scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(
+          ASCIIToUTF16("synckey"), "www.sync.com", "syncguid",
+          test_cases[i].sync_time));
+    EXPECT_EQ(test_cases[i].expected_result,
+        model()->IsLocalTemplateURLBetter(local_turl, sync_turl.get()));
+
+    // Undo the changes.
+    if (test_cases[i].local_is_default)
+      model()->SetDefaultSearchProvider(NULL);
+    model()->Remove(local_turl);
+  }
+}
+
+TEST_F(TemplateURLServiceSyncTest, ResolveSyncKeywordConflict) {
   // 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));
+  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));
+  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));
+  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));
+  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.

[Nicolas Zea, 2012/08/06 19:35:23]

In this case, the reason we push the local copy upstream is to update the last
modified time only right?

[SteveT, 2012/08/07 00:17:09]

That is the idea, and this should still happen after our refactor which removes
the original MergeSyncAndLocalDuplicates method. We update the timestamp so that
the next time this item is brought in during MergeDataAndStartSyncing, we don't
have to make any changes (since GUIDs and last_modified will match between the
local and sync data).

   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());

[Nicolas Zea, 2012/08/06 19:35:23]

check key2 url?

[SteveT, 2012/08/07 00:17:09]

Done.

-  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"));

[Nicolas Zea, 2012/08/06 19:35:23]

check processor didn't receive anything for either key1 or aaa

[SteveT, 2012/08/07 00:17:09]

Done.

 
   // 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);

[Nicolas Zea, 2012/08/06 19:35:23]

Why does the synced other turl get updated? Doesn't it already have a keyword
"sync2", and a later modified time (150)?

[SteveT, 2012/08/07 00:17:09]

This particular test sets the autogenerate_keyword field of the TemplateURLs to
true, which forces the TemplateURLService to generate a new keyword for the
synced-in entries.

I don't fully recall why this didn't just try to keep the keyword it had
originally if that was unique to the system. Looking into it a bit more, the
autogenerate_keyword field is something that Peter deprecated a few releases
back. He mentions that we can eventually remove uses of this field from the code
after we've shipped a few stable releases (which is roughly now, since M21 is
stable and this change to autogenerate_keyword was made in M19 or so... though I
haven't fully thought out how the throttling of M20 users might affect this).

I can inquire with Peter about the details of this change and we can remove that
field (and therefore this test) in a separate change?

[Nicolas Zea, 2012/08/07 17:45:43]

SG

   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());

[Nicolas Zea, 2012/08/06 19:35:23]

check url too? Also that the processor received an update for "key1" with the
new url (whatever.com)?

[SteveT, 2012/08/07 00:17:09]

Done.

-  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()));
 }