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

This rewrite should make MergeDataAndStartSyncing more robust, resulting in fewer underscore dupes and hopefully fewer SyncErrors, as we are more careful about when we send ACTION_UPDATEs during initial merge.

Update unit tests to reflect these changes.

BUG=140732
TEST=Sync two clients with different search engine entries and ensure that they end up in sync. Try to add, update, or remove entries from one client and expect the same to occur on the other.


Review URL: https://chromiumcodereview.appspot.com/10826309

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

3 files changed, 448 insertions, 502 deletions

diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index d3dce0e..22d77b7 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -194,8 +194,15 @@ std::string OldBaseURLSearchTermsData::GoogleBaseURLValue() const {
   return old_base_url;
 }
 
-}  // namespace
+// Returns true if |turl|'s GUID is not found inside |sync_data|. This is to be
+// used in MergeDataAndStartSyncing to differentiate between TemplateURLs from
+// Sync and TemplateURLs that were initially local, assuming |sync_data| is the
+// |initial_sync_data| parameter.
+bool IsFromSync(const TemplateURL* turl, const SyncDataMap& sync_data) {
+  return !!sync_data.count(turl->sync_guid());
+}
 
+}  // namespace
 
 class TemplateURLService::LessWithPrefix {
  public:
@@ -1012,17 +1019,19 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         LOG(ERROR) << "Trying to add an existing TemplateURL.";
         continue;
       }
-      std::string guid = turl->sync_guid();
-      if (!existing_keyword_turl || ResolveSyncKeywordConflict(turl.get(),
-          existing_keyword_turl, &new_changes)) {
-        // Force the local ID to kInvalidTemplateURLID so we can add it.
-        TemplateURLData data(turl->data());
-        data.id = kInvalidTemplateURLID;
-        Add(new TemplateURL(profile_, data));
-
-        // Possibly set the newly added |turl| as the default search provider.
-        SetDefaultSearchProviderIfNewlySynced(guid);
+      const std::string guid = turl->sync_guid();
+      if (existing_keyword_turl) {
+        // Resolve any conflicts so we can safely add the new entry.
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
       }
+      // Force the local ID to kInvalidTemplateURLID so we can add it.
+      TemplateURLData data(turl->data());
+      data.id = kInvalidTemplateURLID;
+      Add(new TemplateURL(profile_, data));
+
+      // Possibly set the newly added |turl| as the default search provider.
+      SetDefaultSearchProviderIfNewlySynced(guid);
     } else if (iter->change_type() == syncer::SyncChange::ACTION_UPDATE) {
       if (!existing_turl) {
         NOTREACHED() << "Unexpected sync change state.";
@@ -1032,16 +1041,11 @@ syncer::SyncError TemplateURLService::ProcessSyncChanges(
         LOG(ERROR) << "Trying to update a non-existent TemplateURL.";
         continue;
       }
-      // Possibly resolve a keyword conflict if they have the same keywords but
-      // are not the same entry.
-      if (existing_keyword_turl && (existing_keyword_turl != existing_turl) &&
-          !ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
-                                      &new_changes)) {
-        // Note that because we're processing changes, this Remove() call won't
-        // generate an ACTION_DELETE; but ResolveSyncKeywordConflict() did
-        // already, so we should be OK.
-        Remove(existing_turl);
-        continue;
+      if (existing_keyword_turl && (existing_keyword_turl != existing_turl)) {
+        // Resolve any conflicts with other entries so we can safely update the
+        // keyword.
+        ResolveSyncKeywordConflict(turl.get(), existing_keyword_turl,
+                                   &new_changes);
       }
       UIThreadSearchTermsData search_terms_data(existing_turl->profile());
       if (UpdateNoNotify(existing_turl, *turl, search_terms_data))
@@ -1129,6 +1133,7 @@ syncer::SyncError TemplateURLService::MergeDataAndStartSyncing(
     }
 
     if (local_turl) {
+      DCHECK(IsFromSync(local_turl, sync_data_map));
       // This local search engine is already synced. If the timestamp differs
       // from Sync, we need to update locally or to the cloud. Note that if the
       // timestamps are equal, we touch neither.
@@ -1151,36 +1156,12 @@ syncer::SyncError TemplateURLService::MergeDataAndStartSyncing(
       }
       local_data_map.erase(iter->first);
     } else {
-      // The search engine from the cloud has not been synced locally, but there
-      // might be a local search engine that is a duplicate that needs to be
-      // merged.
-      TemplateURL* dupe_turl = FindDuplicateOfSyncTemplateURL(*sync_turl);
-      if (dupe_turl) {
-        // Merge duplicates and remove the processed local TURL from the map.
-        std::string old_guid = dupe_turl->sync_guid();
-        MergeSyncAndLocalURLDuplicates(sync_turl.release(), dupe_turl,
-                                       &new_changes);
-        local_data_map.erase(old_guid);
-      } else {
-        std::string guid = sync_turl->sync_guid();
-        // Keyword conflict is possible in this case. Resolve it first before
-        // adding the new TemplateURL. Note that we don't remove the local TURL
-        // from local_data_map in this case as it may still need to be pushed to
-        // the cloud.  We also explicitly don't resolve conflicts against
-        // extension keywords; see comments in ProcessSyncChanges().
-        TemplateURL* existing_keyword_turl =
-            FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
-        if (!existing_keyword_turl || ResolveSyncKeywordConflict(
-            sync_turl.get(), existing_keyword_turl, &new_changes)) {
-          // Force the local ID to kInvalidTemplateURLID so we can add it.
-          TemplateURLData data(sync_turl->data());
-          data.id = kInvalidTemplateURLID;
-          Add(new TemplateURL(profile_, data));
-
-          // Possibly set the newly added |turl| as the default search provider.
-          SetDefaultSearchProviderIfNewlySynced(guid);
-        }
-      }
+      // The search engine from the cloud has not been synced locally. Merge it
+      // into our local model. This will handle any conflicts with local (and
+      // already-synced) TemplateURLs. It will prefer to keep entries from Sync
+      // over not-yet-synced TemplateURLs.
+      MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes,
+                             &local_data_map);
     }
   }
 
@@ -2324,125 +2305,129 @@ string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
   return keyword_candidate;
 }
 
-bool TemplateURLService::ResolveSyncKeywordConflict(
-    TemplateURL* sync_turl,
-    TemplateURL* local_turl,
+bool TemplateURLService::IsLocalTemplateURLBetter(
+    const TemplateURL* local_turl,
+    const TemplateURL* sync_turl) {
+  DCHECK(GetTemplateURLForGUID(local_turl->sync_guid()));
+  return local_turl->last_modified() > sync_turl->last_modified() ||
+         local_turl->created_by_policy() ||
+         local_turl== GetDefaultSearchProvider();
+}
+
+void TemplateURLService::ResolveSyncKeywordConflict(
+    TemplateURL* unapplied_sync_turl,
+    TemplateURL* applied_sync_turl,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
-  DCHECK(sync_turl);
-  DCHECK(local_turl);
-  DCHECK(sync_turl->sync_guid() != local_turl->sync_guid());
-  DCHECK(!local_turl->IsExtensionKeyword());
+  DCHECK(unapplied_sync_turl);
+  DCHECK(applied_sync_turl);
   DCHECK(change_list);
-
-  const bool local_is_better =
-      (local_turl->last_modified() > sync_turl->last_modified()) ||
-      local_turl->created_by_policy() ||
-      (local_turl == GetDefaultSearchProvider());
-  const bool can_replace_local = CanReplace(local_turl);
-  if (CanReplace(sync_turl) && (local_is_better || !can_replace_local)) {
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    return false;
-  }
-  if (can_replace_local) {
-    // Since we're processing sync changes, the upcoming Remove() won't generate
-    // an ACTION_DELETE.  We need to do it manually to keep the server in sync
-    // with us.  Note that if we're being called from
-    // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
-    // than having just been brought down, then this is wrong, because the
-    // server doesn't yet know about this entity; but in this case,
-    // PruneSyncChanges() will prune out the ACTION_DELETE we create here.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    Remove(local_turl);
-  } else if (local_is_better) {
-    string16 new_keyword = UniquifyKeyword(*sync_turl, false);
-    DCHECK(!GetTemplateURLForKeyword(new_keyword));
-    sync_turl->data_.SetKeyword(new_keyword);
-    // If we update the cloud TURL, we need to push an update back to sync
-    // informing it that something has changed.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*sync_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
+  DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
+  DCHECK(!applied_sync_turl->IsExtensionKeyword());
+
+  // Both |unapplied_sync_turl| and |applied_sync_turl| are known to Sync, so
+  // don't delete either of them. Instead, determine which is "better" and
+  // uniquify the other one, sending an update to the server for the updated
+  // entry.
+  const bool applied_turl_is_better =
+      IsLocalTemplateURLBetter(applied_sync_turl, unapplied_sync_turl);
+  TemplateURL* loser = applied_turl_is_better ?
+      unapplied_sync_turl : applied_sync_turl;
+  string16 new_keyword = UniquifyKeyword(*loser, false);
+  DCHECK(!GetTemplateURLForKeyword(new_keyword));
+  if (applied_turl_is_better) {
+    // Just set the keyword of |unapplied_sync_turl|. The caller is responsible
+    // for adding or updating unapplied_sync_turl in the local model.
+    unapplied_sync_turl->data_.SetKeyword(new_keyword);
   } else {
-    string16 new_keyword = UniquifyKeyword(*local_turl, false);
-    TemplateURLData data(local_turl->data());
+    // Update |applied_sync_turl| in the local model with the new keyword.
+    TemplateURLData data(applied_sync_turl->data());
     data.SetKeyword(new_keyword);
-    TemplateURL new_turl(local_turl->profile(), data);
-    UIThreadSearchTermsData search_terms_data(local_turl->profile());
-    if (UpdateNoNotify(local_turl, new_turl, search_terms_data))
+    TemplateURL new_turl(applied_sync_turl->profile(), data);
+    UIThreadSearchTermsData search_terms_data(applied_sync_turl->profile());
+    if (UpdateNoNotify(applied_sync_turl, new_turl, search_terms_data))
       NotifyObservers();
-    // Since we're processing sync changes, the UpdateNoNotify() above didn't
-    // generate an ACTION_UPDATE.  We need to do it manually to keep the server
-    // in sync with us.  Note that if we're being called from
-    // MergeDataAndStartSyncing(), and this TemplateURL was pre-existing rather
-    // than having just been brought down, then this is wrong, because the
-    // server won't know about this entity until it processes the ACTION_ADD our
-    // caller will later generate; but in this case, PruneSyncChanges() will
-    // prune out the ACTION_UPDATE we create here.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
   }
-  return true;
-}
-
-TemplateURL* TemplateURLService::FindDuplicateOfSyncTemplateURL(
-    const TemplateURL& sync_turl) {
-  TemplateURL* existing_turl = GetTemplateURLForKeyword(sync_turl.keyword());
-  return existing_turl && (existing_turl->url() == sync_turl.url()) ?
-      existing_turl : NULL;
+  // The losing TemplateURL should have their keyword updated. Send a change to
+  // the server to reflect this change.
+  syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*loser);
+  change_list->push_back(syncer::SyncChange(FROM_HERE,
+      syncer::SyncChange::ACTION_UPDATE,
+      sync_data));
 }
 
-void TemplateURLService::MergeSyncAndLocalURLDuplicates(
+void TemplateURLService::MergeInSyncTemplateURL(
     TemplateURL* sync_turl,
-    TemplateURL* local_turl,
-    syncer::SyncChangeList* change_list) {
-  DCHECK(loaded_);
+    const SyncDataMap& sync_data,
+    syncer::SyncChangeList* change_list,
+    SyncDataMap* local_data) {
   DCHECK(sync_turl);
-  DCHECK(local_turl);
-  DCHECK(change_list);
-  scoped_ptr<TemplateURL> scoped_sync_turl(sync_turl);
-  if (sync_turl->last_modified() > local_turl->last_modified()) {
-    // Fully replace local_url with Sync's copy. Note that because use Add
-    // rather than ResetTemplateURL, |sync_url| is added with a fresh
-    // TemplateURLID. We don't need to sync the new ID back to the server since
-    // it's only relevant locally.
-    bool delete_default = (local_turl == GetDefaultSearchProvider());
-    DCHECK(!delete_default || !is_default_search_managed_);
-    if (delete_default)
-      default_search_provider_ = NULL;
-
-    // See comments in ResolveSyncKeywordConflict() regarding generating an
-    // ACTION_DELETE manually since Remove() won't do it.
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_DELETE,
-                                              sync_data));
-    Remove(local_turl);
+  DCHECK(!GetTemplateURLForGUID(sync_turl->sync_guid()));
+  DCHECK(IsFromSync(sync_turl, sync_data));
+
+  TemplateURL* conflicting_turl =
+      FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
+  bool should_add_sync_turl = true;
+
+  // If there was no TemplateURL in the local model that conflicts with
+  // |sync_turl|, skip the following preparation steps and just add |sync_turl|
+  // directly. Otherwise, modify |conflicting_turl| to make room for
+  // |sync_turl|.
+  if (conflicting_turl) {
+    if (IsFromSync(conflicting_turl, sync_data)) {
+      // |conflicting_turl| is already known to Sync, so we're not allowed to
+      // remove it. In this case, we want to uniquify the worse one and send an
+      // update for the changed keyword to sync. We can reuse the logic from
+      // ResolveSyncKeywordConflict for this.
+      ResolveSyncKeywordConflict(sync_turl, conflicting_turl, change_list);
+    } else {
+      // |conflicting_turl| is not yet known to Sync. If it is better, then we
+      // want to transfer its values up to sync. Otherwise, we remove it and
+      // allow the entry from Sync to overtake it in the model.
+      const std::string guid = conflicting_turl->sync_guid();
+      if (IsLocalTemplateURLBetter(conflicting_turl, sync_turl)) {
+        ResetTemplateURLGUID(conflicting_turl, sync_turl->sync_guid());
+        syncer::SyncData sync_data =
+            CreateSyncDataFromTemplateURL(*conflicting_turl);
+        change_list->push_back(syncer::SyncChange(
+            FROM_HERE, syncer::SyncChange::ACTION_UPDATE, sync_data));
+        if (conflicting_turl == GetDefaultSearchProvider() &&
+            !pending_synced_default_search_) {
+          // If we're not waiting for the Synced default to come in, we should
+          // override the pref with our new GUID. If we are waiting for the
+          // arrival of a synced default, setting the pref here would cause us
+          // to lose the GUID we are waiting on.
+          PrefService* prefs = GetPrefs();
+          if (prefs) {
+            prefs->SetString(prefs::kSyncedDefaultSearchProviderGUID,
+                             conflicting_turl->sync_guid());
+          }
+        }
+        // Note that in this case we do not add the Sync TemplateURL to the
+        // local model, since we've effectively "merged" it in by updating the
+        // local conflicting entry with its sync_guid.
+        should_add_sync_turl = false;
+      } else {
+        // We guarantee that this isn't the local search provider. Otherwise,
+        // local would have won.
+        DCHECK(conflicting_turl != GetDefaultSearchProvider());
+        Remove(conflicting_turl);
+      }
+      // This TemplateURL was either removed or overwritten in the local model.
+      // Remove the entry from the local data so it isn't pushed up to Sync.
+      local_data->erase(guid);
+    }
+  }
 
+  if (should_add_sync_turl) {
+    const std::string guid = sync_turl->sync_guid();
     // Force the local ID to kInvalidTemplateURLID so we can add it.
-    sync_turl->data_.id = kInvalidTemplateURLID;
-    Add(scoped_sync_turl.release());
-    if (delete_default)
-      SetDefaultSearchProvider(sync_turl);
-  } else {
-    // Change the local TURL's GUID to the server's GUID and push an update to
-    // Sync. This ensures that the rest of local_url's fields are sync'd up to
-    // the server, and the next time local_url is synced, it is recognized by
-    // having the same GUID.
-    ResetTemplateURLGUID(local_turl, sync_turl->sync_guid());
-    syncer::SyncData sync_data = CreateSyncDataFromTemplateURL(*local_turl);
-    change_list->push_back(syncer::SyncChange(FROM_HERE,
-                                              syncer::SyncChange::ACTION_UPDATE,
-                                              sync_data));
+    TemplateURLData data(sync_turl->data());
+    data.id = kInvalidTemplateURLID;
+    Add(new TemplateURL(profile_, data));
+
+    // Possibly set the newly added |turl| as the default search provider.
+    SetDefaultSearchProviderIfNewlySynced(guid);
   }
 }
 
diff --git a/chrome/browser/search_engines/template_url_service.h b/chrome/browser/search_engines/template_url_service.h
index b4641ea..b89bd62 100644
--- a/chrome/browser/search_engines/template_url_service.h
+++ b/chrome/browser/search_engines/template_url_service.h
@@ -352,17 +352,11 @@ class TemplateURLService : public WebDataServiceConsumer,
                            CreateTemplateURLFromSyncData);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, UniquifyKeyword);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           SyncKeywordConflictNeitherAutoreplace);
+                           ResolveSyncKeywordConflict);
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, PreSyncDeletes);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           SyncKeywordConflictBothAutoreplace);
-  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           SyncKeywordConflictOneAutoreplace);
-  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           FindDuplicateOfSyncTemplateURL);
-  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           MergeSyncAndLocalURLDuplicates);
-  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest,
-                           PreSyncDeletes);
+                           IsLocalTemplateURLBetter);
+  FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceSyncTest, MergeInSyncTemplateURL);
 
   friend class TemplateURLServiceTestUtil;
 
@@ -532,49 +526,46 @@ class TemplateURLService : public WebDataServiceConsumer,
   // execute the special character appending functionality.
   string16 UniquifyKeyword(const TemplateURL& turl, bool force);
 
-  // Given a TemplateURL from Sync (cloud) and a local, non-extension
-  // TemplateURL with the same keyword, selects "better" and "worse" entries:
-  //   * If one of the TemplateURLs is replaceable and the other is not, the
-  //     non-replaceable entry is better.
-  //   * Otherwise, if |local_turl| was created by policy, is the default
-  //     provider, or was modified more recently, it is better.
-  //   * Otherwise |sync_turl| is better.
-  // Then resolves the conflict:
-  //   * If the "worse" entry is |sync_turl|, and it is replaceable, add a
-  //     syncer::SyncChange to delete it, and return false.
-  //   * If the "worse" entry is |local_turl|, and it is replaceable, remove it
-  //     from the service and return true.
-  //   * Otherwise, uniquify the keyword of the "worse" entry.  If it is
-  //     |local_turl|, update it within the service.  Add a SyncChange to update
-  //     things (always for |sync_turl|, sometimes for |local_turl|; see
-  //     comments in implementation), and return true.
-  // When the function returns true, callers can then go ahead and add or update
-  // |sync_turl| within the service.  If it returns false, callers must not add
-  // the |sync_turl|, and must Remove() the |sync_turl| if it was being updated.
-  // (Be careful; calling Remove() could add an ACTION_DELETE sync change, which
-  // this function has already done.  Make sure to avoid duplicates.)
-  //
-  // Note that we never call this for conflicts with extension keywords because
-  // other code (e.g. AddToMaps()) is responsible for correctly prioritizing
-  // extension- vs. non-extension-based TemplateURLs with the same keyword.
-  bool ResolveSyncKeywordConflict(TemplateURL* sync_turl,
-                                  TemplateURL* local_turl,
+  // Returns true iff |local_turl| is considered "better" than |sync_turl| for
+  // the purposes of resolving conflicts. |local_turl| must be a TemplateURL
+  // known to the local model (though it may already be synced), and |sync_turl|
+  // is a new TemplateURL known to Sync but not yet known to the local model.
+  // The criteria for if |local_turl| is better than |sync_turl| is whether any
+  // of the following are true:
+  //  * |local_turl|'s last_modified timestamp is newer than sync_turl.
+  //  * |local_turl| is created by policy.
+  //  * |local_turl| is the local default search provider.
+  bool IsLocalTemplateURLBetter(const TemplateURL* local_turl,
+                                const TemplateURL* sync_turl);
+
+  // Given two synced TemplateURLs with a conflicting keyword, one of which
+  // needs to be added to or updated in the local model (|unapplied_sync_turl|)
+  // and one which is already known to the local model (|applied_sync_turl|),
+  // prepares the local model so that |unapplied_sync_turl| can be added to it,
+  // or applied as an update to an existing TemplateURL.
+  // Since both entries are known to Sync and one of their keywords will change,
+  // an ACTION_UPDATE will be appended to |change_list| to reflect this change.
+  // Note that |applied_sync_turl| must not be an extension keyword.
+  void ResolveSyncKeywordConflict(TemplateURL* unapplied_sync_turl,
+                                  TemplateURL* applied_sync_turl,
                                   syncer::SyncChangeList* change_list);
 
-  // Returns a TemplateURL from the service that has the same keyword and search
-  // URL as |sync_turl|, if it exists.
-  TemplateURL* FindDuplicateOfSyncTemplateURL(const TemplateURL& sync_turl);
-
-  // Given a TemplateURL from the cloud and a local matching duplicate found by
-  // FindDuplicateOfSyncTemplateURL, merges the two. If |sync_turl| is newer,
-  // this replaces |local_turl| with |sync_turl| using the service's Remove and
-  // Add. If |local_turl| is newer, this replaces |sync_turl| with |local_turl|
-  // through through adding appropriate SyncChanges to |change_list|. This
-  // method takes ownership of |sync_turl|, and adds it to the model if it is
-  // newer, so the caller must release it if need be.
-  void MergeSyncAndLocalURLDuplicates(TemplateURL* sync_turl,
-                                      TemplateURL* local_turl,
-                                      syncer::SyncChangeList* change_list);
+  // Adds |sync_turl| into the local model, possibly removing or updating a
+  // local TemplateURL to make room for it. This expects |sync_turl| to be a new
+  // entry from Sync, not currently known to the local model. |sync_data| should
+  // be a SyncDataMap where the contents are entries initially known to Sync
+  // during MergeDataAndStartSyncing.
+  // Any necessary updates to Sync will be appended to |change_list|. This can
+  // include updates on local TemplateURLs, if they are found in |sync_data|.
+  // |initial_data| should be a SyncDataMap of the entries known to the local
+  // model during MergeDataAndStartSyncing. If |sync_turl| replaces a local
+  // entry, that entry is removed from |initial_data| to prevent it from being
+  // sent up to Sync.
+  // This should only be called from MergeDataAndStartSyncing.
+  void MergeInSyncTemplateURL(TemplateURL* sync_turl,
+                              const SyncDataMap& sync_data,
+                              syncer::SyncChangeList* change_list,
+                              SyncDataMap* local_data);
 
   // Checks a newly added TemplateURL from Sync by its sync_guid and sets it as
   // the default search provider if we were waiting for it.
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 fd034ee..c4295e6 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -471,7 +471,45 @@ TEST_F(TemplateURLServiceSyncTest, UniquifyKeyword) {
   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.
 
@@ -484,8 +522,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
   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());
@@ -505,8 +542,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
   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());
@@ -525,8 +561,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
   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));
@@ -543,8 +578,7 @@ TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
   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()));
@@ -555,212 +589,6 @@ TEST_F(TemplateURLServiceSyncTest, SyncKeywordConflictNeitherAutoreplace) {
   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(),
@@ -898,8 +726,10 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) {
                                     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
@@ -909,39 +739,40 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromOlderSyncData) {
   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"));
+  EXPECT_EQ("http://expected.com", key2->url());
   // 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",
@@ -957,35 +788,33 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
                                     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"));
+  EXPECT_FALSE(processor()->contains_guid("key1"));
+  EXPECT_FALSE(processor()->contains_guid("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());
@@ -1135,37 +964,6 @@ TEST_F(TemplateURLServiceSyncTest, ProcessChangesWithConflictsLocalWins) {
             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.
@@ -1291,8 +1089,9 @@ TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordConflicts) {
   // 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");
@@ -1302,34 +1101,46 @@ TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordConflicts) {
       "{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) {
@@ -1358,6 +1169,7 @@ TEST_F(TemplateURLServiceSyncTest, TwoAutogeneratedKeywordsUsingGoogleBaseURL) {
   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");
@@ -1836,8 +1648,11 @@ TEST_F(TemplateURLServiceSyncTest, SyncMergeDeletesDefault) {
 TEST_F(TemplateURLServiceSyncTest, LocalDefaultWinsConflict) {
   // We expect that the local default always wins keyword conflict resolution.
   const string16 keyword(ASCIIToUTF16("key1"));
+  const std::string url("http://whatever.com/{searchTerms}");
   TemplateURL* default_turl = CreateTestTemplateURL(keyword,
-      "http://whatever.com/{searchTerms}", "whateverguid", 10);
+                                                    url,
+                                                    "whateverguid",
+                                                    10);
   model()->Add(default_turl);
   model()->SetDefaultSearchProvider(default_turl);
 
@@ -1851,16 +1666,24 @@ TEST_F(TemplateURLServiceSyncTest, LocalDefaultWinsConflict) {
   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, its 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());
+  EXPECT_EQ(url, winner->url());
+  ASSERT_TRUE(processor()->contains_guid("key1"));
+  EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
+            processor()->change_for_guid("key1").change_type());
+  EXPECT_EQ(url, GetURL(processor()->change_for_guid("key1").sync_data()));
+
+  // 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) {
@@ -2012,3 +1835,150 @@ TEST_F(TemplateURLServiceSyncTest, SyncBaseURLs) {
   EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE, change.change_type());
   EXPECT_EQ("google.co.in", GetKeyword(change.sync_data()));
 }
+
+TEST_F(TemplateURLServiceSyncTest, MergeInSyncTemplateURL) {
+  // An enumeration used to indicate which TemplateURL test value is expected
+  // for a particular test result.
+  enum ExpectedTemplateURL {
+    LOCAL,
+    SYNC,
+    BOTH,
+    NEITHER,
+  };
+
+  // Sets up and executes a MergeInSyncTemplateURL test given a number of
+  // expected start and end states:
+  //  * |conflict_winner| denotes which TemplateURL should win the
+  //    conflict.
+  //  * |synced_at_start| denotes which of the TemplateURLs should known
+  //    to Sync.
+  //  * |update_sent| denotes which TemplateURL should have an
+  //    ACTION_UPDATE sent to the server after the merge.
+  //  * |turl_uniquified| denotes which TemplateURL should have its
+  //    keyword updated after the merge.
+  //  * |present_in_model| denotes which TemplateURL should be found in
+  //    the model after the merge.
+  //  * If |keywords_conflict| is true, the TemplateURLs are set up with
+  //    the same keyword.
+  const struct {
+    ExpectedTemplateURL conflict_winner;
+    ExpectedTemplateURL synced_at_start;
+    ExpectedTemplateURL update_sent;
+    ExpectedTemplateURL turl_uniquified;
+    ExpectedTemplateURL present_in_model;
+    bool keywords_conflict;
+  } test_cases[] = {
+    // Both are synced and the new sync entry is better: Local is uniquified and
+    // UPDATE sent. Sync is added.
+    {SYNC, BOTH, LOCAL, LOCAL, BOTH, true},
+    // Both are synced and the local entry is better: Sync is uniquified and
+    // added to the model. An UPDATE is sent for it.
+    {LOCAL, BOTH, SYNC, SYNC, BOTH, true},
+    // Local was not known to Sync and the new sync entry is better: Sync is
+    // added. Local is removed. No updates.
+    {SYNC, SYNC, NEITHER, NEITHER, SYNC, true},
+    // Local was not known to sync and the local entry is better: Local is
+    // updated with sync GUID, Sync is not added. UPDATE sent for Sync.
+    {LOCAL, SYNC, SYNC, NEITHER, SYNC, true},
+    // No conflicting keyword. Both should be added with their original
+    // keywords, with no updates sent. Note that MergeDataAndStartSyncing is
+    // responsible for creating the ACTION_ADD for the local TemplateURL.
+    {NEITHER, SYNC, NEITHER, NEITHER, BOTH, false},
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) {
+    // Assert all the valid states of ExpectedTemplateURLs.
+    ASSERT_FALSE(test_cases[i].conflict_winner == BOTH);
+    ASSERT_FALSE(test_cases[i].synced_at_start == NEITHER);
+    ASSERT_FALSE(test_cases[i].synced_at_start == LOCAL);
+    ASSERT_FALSE(test_cases[i].update_sent == BOTH);
+    ASSERT_FALSE(test_cases[i].turl_uniquified == BOTH);
+    ASSERT_FALSE(test_cases[i].present_in_model == NEITHER);
+
+    const string16 local_keyword = ASCIIToUTF16("localkeyword");
+    const string16 sync_keyword = test_cases[i].keywords_conflict ?
+        local_keyword : ASCIIToUTF16("synckeyword");
+    const std::string local_url = "www.localurl.com";
+    const std::string sync_url = "www.syncurl.com";
+    const time_t local_last_modified = 100;
+    const time_t sync_last_modified =
+        test_cases[i].conflict_winner == SYNC ? 110 : 90;
+    const std::string local_guid = "local_guid";
+    const std::string sync_guid = "sync_guid";
+
+    // Initialize expectations.
+    string16 expected_local_keyword = local_keyword;
+    string16 expected_sync_keyword = sync_keyword;
+
+    // Create the data and run the actual test.
+    TemplateURL* local_turl = CreateTestTemplateURL(
+        local_keyword, local_url, local_guid, local_last_modified);
+    model()->Add(local_turl);
+    scoped_ptr<TemplateURL> sync_turl(CreateTestTemplateURL(
+        sync_keyword, sync_url, sync_guid, sync_last_modified));
+
+    SyncDataMap sync_data;
+    if (test_cases[i].synced_at_start == SYNC ||
+        test_cases[i].synced_at_start == BOTH) {
+      sync_data[sync_turl->sync_guid()] =
+          TemplateURLService::CreateSyncDataFromTemplateURL(*sync_turl);
+    }
+    if (test_cases[i].synced_at_start == BOTH) {
+      sync_data[local_turl->sync_guid()] =
+          TemplateURLService::CreateSyncDataFromTemplateURL(*local_turl);
+    }
+    SyncDataMap initial_data;
+    initial_data[local_turl->sync_guid()] =
+        TemplateURLService::CreateSyncDataFromTemplateURL(*local_turl);
+
+    syncer::SyncChangeList change_list;
+    model()->MergeInSyncTemplateURL(sync_turl.get(), sync_data, &change_list,
+                                    &initial_data);
+
+    // Check for expected updates, if any.
+    std::string expected_update_guid;
+    if (test_cases[i].update_sent == LOCAL)
+      expected_update_guid = local_guid;
+    else if (test_cases[i].update_sent == SYNC)
+      expected_update_guid = sync_guid;
+    if (!expected_update_guid.empty()) {
+      ASSERT_EQ(1U, change_list.size());
+      EXPECT_EQ(expected_update_guid, GetGUID(change_list[0].sync_data()));
+      EXPECT_EQ(syncer::SyncChange::ACTION_UPDATE,
+                change_list[0].change_type());
+    } else {
+      EXPECT_EQ(0U, change_list.size());
+    }
+
+    // Adjust the expectations based on the expectation enums.
+    if (test_cases[i].turl_uniquified == LOCAL) {
+      DCHECK(test_cases[i].keywords_conflict);
+      expected_local_keyword = ASCIIToUTF16("localkeyword_");
+    }
+    if (test_cases[i].turl_uniquified == SYNC) {
+      DCHECK(test_cases[i].keywords_conflict);
+      expected_sync_keyword = ASCIIToUTF16("localkeyword_");
+    }
+
+    // Check for TemplateURLs expected in the model. Note that this is checked
+    // by GUID rather than the initial pointer, as a merge could occur (the
+    // Sync TemplateURL overtakes the local one). Also remove the present
+    // TemplateURL when done so the next test case starts with a clean slate.
+    if (test_cases[i].present_in_model == LOCAL ||
+        test_cases[i].present_in_model == BOTH) {
+      ASSERT_TRUE(model()->GetTemplateURLForGUID(local_guid));
+      EXPECT_EQ(expected_local_keyword, local_turl->keyword());
+      EXPECT_EQ(local_url, local_turl->url());
+      EXPECT_EQ(local_last_modified, local_turl->last_modified().ToTimeT());
+      model()->Remove(model()->GetTemplateURLForGUID(local_guid));
+    }
+    if (test_cases[i].present_in_model == SYNC ||
+        test_cases[i].present_in_model == BOTH) {
+      ASSERT_TRUE(model()->GetTemplateURLForGUID(sync_guid));
+      EXPECT_EQ(expected_sync_keyword, sync_turl->keyword());
+      EXPECT_EQ(sync_url, sync_turl->url());
+      EXPECT_EQ(sync_last_modified, sync_turl->last_modified().ToTimeT());
+      model()->Remove(model()->GetTemplateURLForGUID(sync_guid));
+    }
+  }  // for
+}