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

components/search_engines/template_url_service.cc

 // Copyright 2014 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 "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 2192 matching lines @@
 
 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();
 }
 
+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,
     syncer::SyncChangeList* change_list) {
   DCHECK(loaded_);
   DCHECK(unapplied_sync_turl);
   DCHECK(applied_sync_turl);
   DCHECK(change_list);
   DCHECK_EQ(applied_sync_turl->keyword(), unapplied_sync_turl->keyword());
   DCHECK_EQ(TemplateURL::NORMAL, applied_sync_turl->GetType());
@@ skipping 34 matching lines @@
     SyncDataMap* local_data,
     syncer::SyncMergeResult* merge_result) {
   DCHECK(sync_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 (!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
   // |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.
@@ skipping 157 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }