[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 2185 matching lines @@
   base::string16 keyword_candidate(turl.keyword());
   do {
     keyword_candidate.append(base::ASCIIToUTF16("_"));
   } while (GetTemplateURLForKeyword(keyword_candidate));
 
   return keyword_candidate;
 }
 
 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(
     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);
@@ skipping 36 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 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
       // update for the changed keyword to sync. We can reuse the logic from
       // ResolveSyncKeywordConflict for this.
       ResolveSyncKeywordConflict(sync_turl, conflicting_turl, change_list);
       merge_result->set_num_items_modified(
           merge_result->num_items_modified() + 1);
     } else {
       // |conflicting_turl| is not yet known to Sync. If it is better, then we
@@ skipping 17 matching lines @@
         // local would have won.
         DCHECK(conflicting_turl != GetDefaultSearchProvider());
         Remove(conflicting_turl);
         merge_result->set_num_items_deleted(
             merge_result->num_items_deleted() + 1);
       }
       // 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);
     }
+  } 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) {
     // Force the local ID to kInvalidTemplateURLID so we can add it.
     TemplateURLData data(sync_turl->data());
     data.id = kInvalidTemplateURLID;
     TemplateURL* added = new TemplateURL(data);
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_SYNC_ADD);
     if (Add(added))
@@ skipping 115 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();
   }
 }