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

chrome/browser/search_engines/template_url_service_sync_unittest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/macros.h"
@@ skipping 42 matching lines @@
 // Extract the keyword from a search engine syncer::SyncData.
 std::string GetKeyword(const syncer::SyncData& sync_data) {
   return sync_data.GetSpecifics().search_engine().keyword();
 }
 
 // Much like TemplateURLService::CreateSyncDataFromTemplateURL(), but allows the
 // caller to override the keyword, URL, or GUID fields with empty strings, in
 // order to create custom data that should be handled specially when synced to a
 // client.
 syncer::SyncData CreateCustomSyncData(const TemplateURL& turl,
-                              bool autogenerate_keyword,
-                              const std::string& url,
-                              const std::string& sync_guid) {
+                                      bool autogenerate_keyword,
+                                      const std::string& url,
+                                      const std::string& sync_guid,
+                                      int prepopulate_id = -1) {

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

Nit: Can you do this, or does the compiler barf?

  int prepopulate_id = turl.prepopulate_id()) {

(I never used to think this was legal but I guess at least certain kinds of
function calls during default arguments are allowed?  Not sure you can use
|turl| in the expression though.)

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

I'd like to do this, but the compiler complains about a default argument
referring to a parameter.

   sync_pb::EntitySpecifics specifics;
   sync_pb::SearchEngineSpecifics* se_specifics =
       specifics.mutable_search_engine();
   se_specifics->set_short_name(base::UTF16ToUTF8(turl.short_name()));
   se_specifics->set_keyword(
       autogenerate_keyword ? std::string() : base::UTF16ToUTF8(turl.keyword()));
   se_specifics->set_favicon_url(turl.favicon_url().spec());
   se_specifics->set_url(url);
   se_specifics->set_safe_for_autoreplace(turl.safe_for_autoreplace());
   se_specifics->set_originating_url(turl.originating_url().spec());
   se_specifics->set_date_created(turl.date_created().ToInternalValue());
   se_specifics->set_input_encodings(
       base::JoinString(turl.input_encodings(), ";"));
   se_specifics->set_show_in_default_list(turl.show_in_default_list());
   se_specifics->set_suggestions_url(turl.suggestions_url());
-  se_specifics->set_prepopulate_id(turl.prepopulate_id());
+  se_specifics->set_prepopulate_id(prepopulate_id == -1 ? turl.prepopulate_id()
+                                                        : prepopulate_id);
   se_specifics->set_autogenerate_keyword(autogenerate_keyword);
   se_specifics->set_instant_url(turl.instant_url());
   se_specifics->set_last_modified(turl.last_modified().ToInternalValue());
   se_specifics->set_sync_guid(sync_guid);
   return syncer::SyncData::CreateLocalData(turl.sync_guid(),  // Must be valid!
                                    se_specifics->keyword(), specifics);
 }
 
 
 // TestChangeProcessor --------------------------------------------------------
@@ skipping 110 matching lines @@
   std::unique_ptr<syncer::SyncChangeProcessor> PassProcessor();
   std::unique_ptr<syncer::SyncErrorFactory> CreateAndPassSyncErrorFactory();
 
   // Creates a TemplateURL with some test values. The caller owns the returned
   // TemplateURL*.
   TemplateURL* CreateTestTemplateURL(const base::string16& keyword,
                                      const std::string& url,
                                      const std::string& guid = std::string(),
                                      time_t last_mod = 100,
                                      bool safe_for_autoreplace = false,
-                                     bool created_by_policy = false) const;
+                                     bool created_by_policy = false,
+                                     int prepopulate_id = 999999) const;
 
   // Verifies the two TemplateURLs are equal.
   // TODO(stevet): Share this with TemplateURLServiceTest.
   void AssertEquals(const TemplateURL& expected,
                     const TemplateURL& actual) const;
 
   // Expect that two syncer::SyncDataLists have equal contents, in terms of the
   // sync_guid, keyword, and url fields.
   void AssertEquals(const syncer::SyncDataList& data1,
                     const syncer::SyncDataList& data2) const;
@@ skipping 66 matching lines @@
   return std::unique_ptr<syncer::SyncErrorFactory>(
       new syncer::SyncErrorFactoryMock());
 }
 
 TemplateURL* TemplateURLServiceSyncTest::CreateTestTemplateURL(
     const base::string16& keyword,
     const std::string& url,
     const std::string& guid,
     time_t last_mod,
     bool safe_for_autoreplace,
-    bool created_by_policy) const {
+    bool created_by_policy,
+    int prepopulate_id) const {
   TemplateURLData data;
   data.SetShortName(ASCIIToUTF16("unittest"));
   data.SetKeyword(keyword);
   data.SetURL(url);
   data.favicon_url = GURL("http://favicon.url");
   data.safe_for_autoreplace = safe_for_autoreplace;
   data.date_created = Time::FromTimeT(100);
   data.last_modified = Time::FromTimeT(last_mod);
   data.created_by_policy = created_by_policy;
-  data.prepopulate_id = 999999;
+  data.prepopulate_id = prepopulate_id;
   if (!guid.empty())
     data.sync_guid = guid;
   return new TemplateURL(data);
 }
 
 void TemplateURLServiceSyncTest::AssertEquals(const TemplateURL& expected,
                                               const TemplateURL& actual) const {
   ASSERT_EQ(expected.short_name(), actual.short_name());
   ASSERT_EQ(expected.keyword(), actual.keyword());
   ASSERT_EQ(expected.url(), actual.url());
@@ skipping 527 matching lines @@
   EXPECT_EQ(0, merge_result.num_items_deleted());
   EXPECT_EQ(3, merge_result.num_items_before_association());
   EXPECT_EQ(4, merge_result.num_items_after_association());
 }
 
 TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
   // 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",
-                                     "aaa", 10));  // dupe
+                                     "aaa", 10, false, false, 111));  // dupe
 
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("key2"),
-      "http://expected.com", "bbb", 10));  // keyword conflict
+                                     "http://expected.com", "bbb", 10, false,
+                                     false, 112));  // keyword conflict
 
   model()->Add(CreateTestTemplateURL(ASCIIToUTF16("unique"),
-                                     "http://unique.com", "ccc", 10));  // add
+                                     "http://unique.com", "ccc", 10, false,
+                                     false, 113));  // add
 
   syncer::SyncMergeResult merge_result = model()->MergeDataAndStartSyncing(
       syncer::SEARCH_ENGINES,
       CreateInitialSyncData(), PassProcessor(),
       CreateAndPassSyncErrorFactory());
 
   // 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());
 
@@ skipping 261 matching lines @@
 TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordMigrated) {
   // Create a couple of sync entries with autogenerated keywords.
   syncer::SyncDataList initial_data;
   std::unique_ptr<TemplateURL> turl(
       CreateTestTemplateURL(ASCIIToUTF16("key1"), "http://key1.com", "key1"));
   initial_data.push_back(
       CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
   turl.reset(CreateTestTemplateURL(ASCIIToUTF16("key2"),
       "{google:baseURL}search?q={searchTerms}", "key2"));
   initial_data.push_back(
-      CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid()));
+      CreateCustomSyncData(*turl, true, turl->url(), turl->sync_guid(), 99));
 
   // Now try to sync the data locally.
   model()->MergeDataAndStartSyncing(syncer::SEARCH_ENGINES, initial_data,
       PassProcessor(), CreateAndPassSyncErrorFactory());
 
   // Both entries should have been added, with explicit keywords.
   TemplateURL* key1 = model()->GetTemplateURLForHost("key1.com");
   ASSERT_FALSE(key1 == NULL);
   EXPECT_EQ(ASCIIToUTF16("key1.com"), key1->keyword());
   GURL google_url(model()->search_terms_data().GoogleBaseURLValue());
@@ skipping 1058 matching lines @@
       syncer::SEARCH_ENGINES, list, PassProcessor(),
       CreateAndPassSyncErrorFactory());
 
   const TemplateURL* result_turl = model()->GetTemplateURLForGUID("default");
   EXPECT_TRUE(result_turl);
   EXPECT_EQ(ASCIIToUTF16("new_kw"), result_turl->keyword());
   EXPECT_EQ(ASCIIToUTF16("my name"), result_turl->short_name());
   EXPECT_EQ(default_turl->url(), result_turl->url());
 }
 
+TEST_F(TemplateURLServiceSyncTest, MergeConflictingPrepopulatedEngine) {
+  std::unique_ptr<TemplateURLData> default_turl(
+      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(NULL));

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

Nit: nullptr

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

Done.

+
+  TemplateURLData data(*default_turl);
+  data.safe_for_autoreplace = true;
+  data.SetKeyword(ASCIIToUTF16("old_kw"));
+  data.SetShortName(ASCIIToUTF16("my name"));
+  data.SetURL("http://wrong.url.com?q={searchTerms}");
+  data.date_created = Time::FromTimeT(50);
+  data.last_modified = Time::FromTimeT(50);
+  data.sync_guid = "default";
+  data.prepopulate_id = 1;
+  model()->Add(new TemplateURL(data));
+
+  TemplateURLData new_data(*default_turl);
+  new_data.date_created = Time::FromTimeT(100);

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

Nit: Set fields in the order they're declared within the struct

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

Done.

+  new_data.last_modified = Time::FromTimeT(100);
+  new_data.safe_for_autoreplace = false;
+  new_data.SetKeyword(ASCIIToUTF16("new_kw"));
+  new_data.SetShortName(ASCIIToUTF16("my name"));
+  new_data.SetURL("http://wrong.url.com?q={searchTerms}");
+  new_data.sync_guid = "different_guid";
+  new_data.prepopulate_id = 1;
+
+  std::unique_ptr<TemplateURL> sync_turl(new TemplateURL(new_data));

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

Nit: Use MakeUnique<> over raw new where possible

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

Done.

+  syncer::SyncDataList list;
+  list.push_back(TemplateURLService::CreateSyncDataFromTemplateURL(*sync_turl));
+  syncer::SyncMergeResult merge_result = model()->MergeDataAndStartSyncing(
+      syncer::SEARCH_ENGINES, list, PassProcessor(),
+      CreateAndPassSyncErrorFactory());
+
+  const TemplateURL* result_turl =
+      model()->GetTemplateURLForGUID("different_guid");
+  EXPECT_TRUE(result_turl);
+  EXPECT_EQ(ASCIIToUTF16("new_kw"), result_turl->keyword());
+  EXPECT_EQ(ASCIIToUTF16("my name"), result_turl->short_name());
+  EXPECT_EQ(default_turl->url(), result_turl->url());
+}
+
+TEST_F(TemplateURLServiceSyncTest,
+       MergeConflictingPrepopulatedEngineIntoDefault) {

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

All comments from above apply.

It kinda sucks that this test is long and almost entirely copy-pasted from the
one above.  Try to find ways to combine these into one test or in some other way
share all this code.

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

Done.

+  std::unique_ptr<TemplateURLData> default_turl(
+      TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(NULL));
+
+  TemplateURLData data(*default_turl);
+  data.safe_for_autoreplace = true;
+  data.SetKeyword(ASCIIToUTF16("old_kw"));
+  data.SetShortName(ASCIIToUTF16("my name"));
+  data.SetURL("http://wrong.url.com?q={searchTerms}");
+  data.date_created = Time::FromTimeT(50);
+  data.last_modified = Time::FromTimeT(50);
+  data.sync_guid = "default";
+  data.prepopulate_id = 1;
+  TemplateURL* existing_default = new TemplateURL(data);
+  model()->Add(existing_default);
+  model()->SetUserSelectedDefaultSearchProvider(existing_default);
+
+  TemplateURLData new_data(*default_turl);
+  new_data.date_created = Time::FromTimeT(100);
+  new_data.last_modified = Time::FromTimeT(100);
+  new_data.safe_for_autoreplace = false;
+  new_data.SetKeyword(ASCIIToUTF16("new_kw"));
+  new_data.SetShortName(ASCIIToUTF16("my name"));
+  new_data.SetURL("http://wrong.url.com?q={searchTerms}");
+  new_data.sync_guid = "different_guid";
+  new_data.prepopulate_id = 1;
+
+  std::unique_ptr<TemplateURL> sync_turl(new TemplateURL(new_data));
+  syncer::SyncDataList list;
+  list.push_back(TemplateURLService::CreateSyncDataFromTemplateURL(*sync_turl));
+  syncer::SyncMergeResult merge_result = model()->MergeDataAndStartSyncing(
+      syncer::SEARCH_ENGINES, list, PassProcessor(),
+      CreateAndPassSyncErrorFactory());
+
+  const TemplateURL* result_turl = model()->GetDefaultSearchProvider();
+  EXPECT_TRUE(result_turl);
+  EXPECT_EQ(ASCIIToUTF16("new_kw"), result_turl->keyword());
+  EXPECT_EQ(ASCIIToUTF16("my name"), result_turl->short_name());
+  EXPECT_EQ(default_turl->url(), result_turl->url());
+}
+
 TEST_F(TemplateURLServiceSyncTest, MergeNonEditedPrepopulatedEngine) {
   std::unique_ptr<TemplateURLData> default_turl(
       TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(NULL));
 
   TemplateURLData data(*default_turl);
   data.safe_for_autoreplace = true;  // Can be replaced with built-in values.
   data.SetKeyword(ASCIIToUTF16("new_kw"));
   data.SetShortName(ASCIIToUTF16("my name"));
   data.SetURL("http://wrong.url.com?q={searchTerms}");
   data.date_created = Time::FromTimeT(50);
@@ skipping 41 matching lines @@
 
 TEST_F(TemplateURLServiceSyncTest, NonAsciiKeywordDoesNotCrash) {
   model()->Add(CreateTestTemplateURL(UTF8ToUTF16("\xf0\xaf\xa6\x8d"),
                                      "http://key1.com"));
   syncer::SyncDataList initial_data = CreateInitialSyncData();
 
   model()->MergeDataAndStartSyncing(
       syncer::SEARCH_ENGINES, initial_data, PassProcessor(),
       CreateAndPassSyncErrorFactory());
 }