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

chrome/browser/search_engines/template_url_service_sync_unittest.cc

Index: chrome/browser/search_engines/template_url_service_sync_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_service_sync_unittest.cc b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
index ca3904c163a4ecf0693fb2a13a0ef0769f64f318..4ebb75f0b68f4dc18bd9cf7e1c296a7b91d02b17 100644
--- a/chrome/browser/search_engines/template_url_service_sync_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_sync_unittest.cc
@@ -60,9 +60,10 @@ std::string GetKeyword(const syncer::SyncData& sync_data) {
 // 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();
@@ -78,7 +79,8 @@ syncer::SyncData CreateCustomSyncData(const TemplateURL& turl,
       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());
@@ -209,7 +211,8 @@ class TemplateURLServiceSyncTest : public testing::Test {
                                      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.
@@ -296,7 +299,8 @@ TemplateURL* TemplateURLServiceSyncTest::CreateTestTemplateURL(
     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);
@@ -306,7 +310,7 @@ TemplateURL* TemplateURLServiceSyncTest::CreateTestTemplateURL(
   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);
@@ -854,13 +858,15 @@ TEST_F(TemplateURLServiceSyncTest, MergeAddFromNewerSyncData) {
   // 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,
@@ -1142,7 +1148,7 @@ TEST_F(TemplateURLServiceSyncTest, AutogeneratedKeywordMigrated) {
   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,
@@ -2221,6 +2227,88 @@ TEST_F(TemplateURLServiceSyncTest, MergeEditedPrepopulatedEngine) {
   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));