[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..245c622d4a14e6c7c1ea2c7e5d449101abe7648c 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -2210,6 +2210,14 @@ bool TemplateURLService::IsLocalTemplateURLBetter(
          local_turl== GetDefaultSearchProvider();
 }
 
+bool TemplateURLService::IsLocalPrepopulatedTemplateURLBetter(
+    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();
+}
+
 void TemplateURLService::ResolveSyncKeywordConflict(
     TemplateURL* unapplied_sync_turl,
     TemplateURL* applied_sync_turl,
@@ -2264,6 +2272,29 @@ void TemplateURLService::MergeInSyncTemplateURL(
       FindNonExtensionTemplateURLForKeyword(sync_turl->keyword());
   bool should_add_sync_turl = true;
 
+  if (!conflicting_turl) {

[Peter Kasting, 2016/06/14 19:21:26]

Nit: Just move the body of this conditional onto the } else { arm of the
conditional below.

If you do this you'll need to tweak the comment above that conditional, e.g.:

  // Resolve conflicts with local TemplateURLs.
  if (conflicting_turl) {
    // Modify |conflicting_turl| to make room for |sync_turl|.
    ...
  } else {
    // ...

[Patrick Noland, 2016/06/14 22:55:24]

Done.

+    TemplateURL* conflicting_prepopulated_turl =

[Peter Kasting, 2016/06/14 19:21:26]

Nit: Consider a comment up here that summarizes this whole block

[Nicolas Zea, 2016/06/14 21:19:44]

Seconded. This is fairly subtle, so would be good to explain what's intended.

[Patrick Noland, 2016/06/14 22:55:24]

Done.

+        FindPrepopulatedTemplateURL(sync_turl->prepopulate_id());
+
+    if (conflicting_prepopulated_turl &&
+        !IsFromSync(conflicting_prepopulated_turl, sync_data) &&
+        !IsLocalPrepopulatedTemplateURLBetter(conflicting_prepopulated_turl,
+                                              sync_turl)) {
+      std::string guid = conflicting_prepopulated_turl->sync_guid();
+      if (conflicting_prepopulated_turl == default_search_provider_) {
+        ApplyDefaultSearchChange(&sync_turl->data(),

[Nicolas Zea, 2016/06/14 21:19:44]

Why don't we remove the conflicting prepopulated turl here too?

[Patrick Noland, 2016/06/14 22:55:24]

The logic in ApplyDefaultSearchChange effectively merges sync_turl into the
default instead of removing then adding. I think this is because calling
Remove() on the default isn't supposed to happen, maybe as a way of ensuring
there's always a default. 

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

[Peter Kasting, 2016/06/14 19:21:26]

Nit: Break after ( instead

[Patrick Noland, 2016/06/14 22:55:24]

Done.

+                                            1);
+      }
+      local_data->erase(guid);
+    }

[Nicolas Zea, 2016/06/14 21:19:44]

To be clear, in the else clause here we're explicitly deciding to let both the
local and the sync items live side by side, despite conflicting?

[Patrick Noland, 2016/06/14 22:55:24]

Yes. I've added a comment to this effect. The likely scenario in which this
would happen is one where the user has modified the keyword on both clients, but
to two different values e.g. "g1" and "g2." I'm not sure how to pick a winner in
this scenario, so it seemed safer to just preserve both. 

+  }
+
   // 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