[sync] Search engine shortcuts get underscores at the end after sync

components/search_engines/template_url_service.cc

Index: components/search_engines/template_url_service.cc
diff --git a/components/search_engines/template_url_service.cc b/components/search_engines/template_url_service.cc
index 65a7581695de8fc197a9cda0a25a8e6655f70d18..942496d92ac2d3f3ad4aad50dde6a8082ff57011 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -2203,11 +2203,12 @@ base::string16 TemplateURLService::UniquifyKeyword(const TemplateURL& turl,
 
 bool TemplateURLService::IsLocalTemplateURLBetter(
     const TemplateURL* local_turl,
-    const TemplateURL* sync_turl) {
+    const TemplateURL* sync_turl,
+    bool prefer_local_default) {
   DCHECK(GetTemplateURLForGUID(local_turl->sync_guid()));
   return local_turl->last_modified() > sync_turl->last_modified() ||
          local_turl->created_by_policy() ||
-         local_turl== GetDefaultSearchProvider();
+         (prefer_local_default && local_turl == GetDefaultSearchProvider());
 }
 
 void TemplateURLService::ResolveSyncKeywordConflict(
@@ -2264,11 +2265,9 @@ void TemplateURLService::MergeInSyncTemplateURL(
       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|.
+  // Resolve conflicts with local TemplateURLs.
   if (conflicting_turl) {
+    // Modify |conflicting_turl| to make room for |sync_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
@@ -2306,6 +2305,38 @@ void TemplateURLService::MergeInSyncTemplateURL(
       // Remove the entry from the local data so it isn't pushed up to Sync.
       local_data->erase(guid);
     }
+  } else {
+    // Check for a turl with a conflicting prepopulate_id.

[Peter Kasting, 2016/06/14 23:48:39]

Nit: I generally try to say TemplateURL instead of turl in comments.

Maybe add the sentence "This detects the case where the user changes a
prepopulated engine's keyword on one client.  We want to reflect this sort of
change to that prepopulated URL on other clients instead of assuming that the
changed TemplateURL is a new entity."

+    TemplateURL* conflicting_prepopulated_turl =
+        FindPrepopulatedTemplateURL(sync_turl->prepopulate_id());
+
+    // If there's a conflicting prepopulated turl that's not a keyword conflict
+    // then |sync_turl| has likely been user-modified. We give |sync_turl|
+    // precdence if it's been modified more recently and the local turl isn't

[Peter Kasting, 2016/06/14 23:48:39]

Nit: precedence

+    // yet known to sync. This can override the default search engine, since if
+    // it didn't, we could end up with two distinct default search engines.

[Peter Kasting, 2016/06/14 23:48:39]

Nit: This sentence seems a bit misleading (TemplateURLService can't actually
have multiple default search engines).  Maybe something like this comment for
the whole thing here?

"If we found a conflict, and the sync entity is better, apply the remote changes
locally.  We will consider the sync entity better even if the local one is the
current default, since in this case being default does not necessarily mean the
user explicitly set this TemplateURL as default. If we didn't do this, changes
to the keywords of prepopulated default engines would never be applied to other
clients.

"If we can't safely replace the local entry with the synced one (or merge the
relevant changes in), we give up and leave both."

+    // If we can't safely remove or merge the local turl, we'll leave both turls
+    // alone.
+    if (conflicting_prepopulated_turl &&
+        !IsFromSync(conflicting_prepopulated_turl, sync_data) &&
+        !IsLocalTemplateURLBetter(conflicting_prepopulated_turl, sync_turl,
+                                  false)) {
+      // Remove or modify |conflicting_prepopulated_turl| to make room for
+      // |sync_turl|.

[Peter Kasting, 2016/06/14 23:48:39]

Nit: Is this comment strictly accurate for the first conditional arm?  Maybe
verbs like "Merge" or "Replace" would be more accurate in describing what ends
up happening here.

But if you apply my comment suggestion above, you can probably just drop this
comment as redundant.

+      std::string guid = conflicting_prepopulated_turl->sync_guid();
+      if (conflicting_prepopulated_turl == default_search_provider_) {
+        ApplyDefaultSearchChange(&sync_turl->data(),
+                                 DefaultSearchManager::FROM_USER);
+        merge_result->set_num_items_modified(
+            merge_result->num_items_modified() + 1);
+      } else {
+        Remove(conflicting_prepopulated_turl);
+        merge_result->set_num_items_deleted(merge_result->num_items_deleted() +
+                                            1);
+      }
+      // Remove the local data so it isn't written to sync.
+      local_data->erase(guid);
+    }
   }
 
   if (should_add_sync_turl) {