Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

chrome/browser/search_engines/template_url_service_unittest.cc

Index: chrome/browser/search_engines/template_url_service_unittest.cc
diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc
index ddc06380b068dc5a15b115a81b7277584f0d54a4..6424ac41218296d13d5a3d425fddcc083b4dd5a0 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -28,10 +28,12 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/search_engines/keyword_web_data_service.h"
+#include "components/search_engines/search_engines_pref_names.h"
 #include "components/search_engines/search_host_to_urls_map.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -186,6 +188,8 @@ class TemplateURLServiceTest : public testing::Test {
       bool safe_for_autoreplace,
       int prepopulate_id);
 
+  void SetOverridenEngines();

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Comment what this does.

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

+
   // Helper methods to make calling TemplateURLServiceTestUtil methods less
   // visually noisy in the test code.
   void VerifyObserverCount(int expected_changed_count);
@@ -292,6 +296,32 @@ std::unique_ptr<TemplateURL> TemplateURLServiceTest::CreatePreloadedTemplateURL(
   return base::MakeUnique<TemplateURL>(data);
 }
 
+void TemplateURLServiceTest::SetOverridenEngines() {
+  // Set custom search engine as default through overrides.
+  auto prefs = test_util()->profile()->GetTestingPrefService();
+  prefs->SetUserPref(prefs::kSearchProviderOverridesVersion,
+                     new base::FundamentalValue(1));

[Peter Kasting, 2017/02/02 00:04:26]

Nit: Not for this CL, but it'd be nice if this API took a scoped_ptr to indicate
ownership transfer.

[Alexander Yashkin, 2017/02/03 14:44:26]

Agree.

+  auto overrides_list = base::MakeUnique<base::ListValue>();
+  auto entry = base::MakeUnique<base::DictionaryValue>();
+
+  entry->SetString("name", "override_name");
+  entry->SetString("keyword", "override_keyword");
+  entry->SetString("search_url", "http://override.com/s?q={searchTerms}");
+  entry->SetString("favicon_url", "http://override.com/favicon.ico");
+  entry->SetString("encoding", "UTF-8");
+  entry->SetInteger("id", 1001);
+  entry->SetString("suggest_url",
+                   "http://override.com/suggest?q={searchTerms}");
+  entry->SetString("instant_url",
+                   "http://override.com/instant?q={searchTerms}");
+  auto alternate_urls = base::MakeUnique<base::ListValue>();
+  alternate_urls->AppendString("http://override.com/alternate?q={searchTerms}");
+  entry->Set("alternate_urls", std::move(alternate_urls));
+  overrides_list->Append(std::move(entry));
+
+  prefs->SetUserPref(prefs::kSearchProviderOverrides, overrides_list.release());
+}
+
 void TemplateURLServiceTest::VerifyObserverCount(int expected_changed_count) {
   EXPECT_EQ(expected_changed_count, test_util_->GetObserverCount());
   test_util_->ResetObserverCount();
@@ -893,6 +923,85 @@ TEST_F(TemplateURLServiceTest, RepairSearchEnginesWithManagedDefault) {
   ExpectSimilar(expected_managed_default.get(), actual_managed_default);
 }
 
+TEST_F(TemplateURLServiceTest, RepairPrepopulatedEnginesUpdatesSyncGuid) {

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Maybe add a sentence above this declaration as to the purpose of the
overall testcase?  I would love to see this on all tests, honestly.

[Alexander Yashkin, 2017/02/03 14:44:25]

Done

+  test_util()->VerifyLoad();
+
+  // Expect no synced DSE guid until user selected something or sync come in.

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Maybe "The synced DSE GUID should be empty until the user selects something
or there is sync activity."?

[Alexander Yashkin, 2017/02/03 14:44:26]

Done, thanks.

+  EXPECT_TRUE(test_util()
+                  ->profile()
+                  ->GetTestingPrefService()
+                  ->GetString(prefs::kSyncedDefaultSearchProviderGUID)
+                  .empty());
+
+  // Google engine must exist.
+  TemplateURL* google =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.com"));
+  ASSERT_TRUE(google);
+  // Add third-party default search engine.

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Blank line above this.

It also seems a little odd to use "third-party" here, where I assume you just
mean "user-provided".  ("third-party" sounds to me like something that's
sideloaded.)  (2 places)

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

+  TemplateURL* user_dse = AddKeywordWithDate(

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Probably not for this CL, but it'd be nice if this function had fewer args
overall (I think the last param is never set by any caller?), and more defaulted
args, not only so that calls would be shorter, but so I wouldn't have to wonder
about whether callsites that pass "UTF-8" vs. "UTF-8;UTF-16" are intentionally
picking one vs. the other or just want a "sane default".

[Alexander Yashkin, 2017/02/03 14:44:26]

Agree.

+      "user_dse", "user_dse.com", "http://www.user_dse.com/s?q={searchTerms}",
+      std::string(), std::string(), std::string(), true, "UTF-8", Time(),
+      Time(), Time());
+  model()->SetUserSelectedDefaultSearchProvider(user_dse);
+  EXPECT_EQ(user_dse, model()->GetDefaultSearchProvider());
+
+  // Check that user DSE guid is stored in kSyncedDefaultSearchProviderGUID.
+  EXPECT_EQ(user_dse->sync_guid(),
+            test_util()->profile()->GetTestingPrefService()->GetString(
+                prefs::kSyncedDefaultSearchProviderGUID));
+
+  model()->RepairPrepopulatedSearchEngines();
+
+  // Check that Google is default after repair.
+  ASSERT_EQ(google, model()->GetDefaultSearchProvider());
+  // Check that google guid is stored in kSyncedDefaultSearchProviderGUID.
+  const std::string dse_guid =
+      test_util()->profile()->GetTestingPrefService()->GetString(
+          prefs::kSyncedDefaultSearchProviderGUID);
+  EXPECT_EQ(google->sync_guid(), dse_guid);
+  EXPECT_EQ(google->keyword(),
+            model()->GetTemplateURLForGUID(dse_guid)->keyword());
+}
+
+TEST_F(TemplateURLServiceTest,
+       RepairPrepopulatedEnginesWithOverridesUpdatesSyncGuid) {
+  SetOverridenEngines();

[Peter Kasting, 2017/02/02 00:04:26]

Nit: "overridden" has two 'd's (many places)

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

+  test_util()->VerifyLoad();
+
+  TemplateURL* overriden_engine =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("override_keyword"));
+  ASSERT_TRUE(overriden_engine);
+
+  TemplateURL* dse = model()->GetDefaultSearchProvider();

[Peter Kasting, 2017/02/02 00:04:27]

Nit: Can inline below

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

+  ASSERT_EQ(overriden_engine, dse);

[Peter Kasting, 2017/02/02 00:04:26]

Nit: Can maybe be EXPECT?

[Alexander Yashkin, 2017/02/03 14:44:26]

Done

+
+  // Add third-party default search engine.
+  TemplateURL* user_dse = AddKeywordWithDate(
+      "user_dse", "user_dse.com", "http://www.user_dse.com/s?q={searchTerms}",
+      std::string(), std::string(), std::string(), true, "UTF-8", Time(),
+      Time(), Time());
+  model()->SetUserSelectedDefaultSearchProvider(user_dse);
+  EXPECT_EQ(user_dse, model()->GetDefaultSearchProvider());
+
+  // Check that user DSE guid is stored in kSyncedDefaultSearchProviderGUID.
+  EXPECT_EQ(user_dse->sync_guid(),
+            test_util()->profile()->GetTestingPrefService()->GetString(
+                prefs::kSyncedDefaultSearchProviderGUID));
+
+  model()->RepairPrepopulatedSearchEngines();
+
+  // Check that overriden engine is returned as default after repair.
+  ASSERT_EQ(overriden_engine, model()->GetDefaultSearchProvider());
+  // Check that overriden_engine guid is stored in
+  // kSyncedDefaultSearchProviderGUID.
+  const std::string dse_guid =
+      test_util()->profile()->GetTestingPrefService()->GetString(
+          prefs::kSyncedDefaultSearchProviderGUID);
+  EXPECT_EQ(overriden_engine->sync_guid(), dse_guid);
+  EXPECT_EQ(overriden_engine->keyword(),
+            model()->GetTemplateURLForGUID(dse_guid)->keyword());
+}
+
 TEST_F(TemplateURLServiceTest, UpdateKeywordSearchTermsForURL) {
   struct TestData {
     const std::string url;