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..950e4dd6d06cf9f6b2afc0fec2a59e0fb10945fb 100644
--- a/chrome/browser/search_engines/template_url_service_unittest.cc
+++ b/chrome/browser/search_engines/template_url_service_unittest.cc
@@ -28,10 +28,13 @@
 #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_engines_test_util.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 +189,9 @@ class TemplateURLServiceTest : public testing::Test {
       bool safe_for_autoreplace,
       int prepopulate_id);
 
+  // Set custom search engine as default through overrides pref.

[vasilii, 2017/02/13 14:37:54]

Does it overwrite the user prefs?

[Alexander Yashkin, 2017/02/14 12:04:43]

Where is user pref kSearchProviderOverrides that can contain set of prepopulated
engines to use instead of prepopulated engines hardcoded in chromium in
template_url_prepopulate_data.cc and chosen for user country.

This mechanism is useful for people who want to create their own chrome
distributive with custom set of search engines to be used as default and
fallback.

No, it does not override user selected DSE pref.
Maybe better comment would be "// Set custom search engine as default fallback
through overrides pref." 
I have updated comment.

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

[vasilii, 2017/02/13 14:37:54]

I'd move this definition down to where it's used.

[Peter Kasting, 2017/02/13 22:24:57]

This, and adjust the other declarations and blank lines so the variable
declarations and uses are grouped a bit more:

  auto entry = ...;
  entry->...;
  ...

  auto overrides_list = ...;
  overrides_list->...;

  auto prefs = ...;
  prefs->...;
  prefs->SetUserPref(prefs::kSearchProviderOverrides, overrides_list.release());

[Alexander Yashkin, 2017/02/14 12:04:43]

Done

+  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 +925,154 @@ TEST_F(TemplateURLServiceTest, RepairSearchEnginesWithManagedDefault) {
   ExpectSimilar(expected_managed_default.get(), actual_managed_default);
 }
 
+// Test checks that RepairPrepopulatedEngines correctly updates sync guid for

[Peter Kasting, 2017/02/13 22:24:57]

Nit: Test checks -> Checks (3 places)

[Alexander Yashkin, 2017/02/14 12:04:43]

Done

+// default search. Repair is considered a user action and new DSE must
+// be synced to other devices as well. Otherwise previous user selected engine
+// will arrive on next sync attempt.
+TEST_F(TemplateURLServiceTest, RepairPrepopulatedEnginesUpdatesSyncGuid) {
+  test_util()->VerifyLoad();
+
+  // The synced DSE GUID should be empty until the user selects something or
+  // there is sync activity.
+  EXPECT_TRUE(test_util()
+                  ->profile()
+                  ->GetTestingPrefService()
+                  ->GetString(prefs::kSyncedDefaultSearchProviderGUID)
+                  .empty());
+
+  // Google engine must exist.

[Peter Kasting, 2017/02/13 22:24:57]

Rather than looking explicitly for Google, would it work the same to just get
the current DSE at this point?

That way the test would be robust against forks/l10n changes that made Google
not the default engine in some area or distribution.

[Alexander Yashkin, 2017/02/14 12:04:43]

Done.

+  TemplateURL* google =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.com"));
+  ASSERT_TRUE(google);
+
+  // Add user provided 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 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 checks that RepairPrepopulatedEngines correctly updates sync guid for
+// default search when search engines are overridden using pref.
+TEST_F(TemplateURLServiceTest,
+       RepairPrepopulatedEnginesWithOverridesUpdatesSyncGuid) {

[vasilii, 2017/02/13 14:37:54]

I fail to understand how this test works and why it passes.
SetOverriddenEngines() overwrites the user prefs. Later in
SetUserSelectedDefaultSearchProvider() and RepairPrepopulatedSearchEngines()
they are overwritten again. How can it be that |overridden_engine| becomes the
DSE in #1010?

[Peter Kasting, 2017/02/13 22:24:57]

It sets the kSearchProviderOverrides pref, which the next two things you cite
don't touch.  Those work on other prefs.

[Alexander Yashkin, 2017/02/14 12:04:43]

I tried to describe how kSearchProviderOverrides works above in 
my reply to your comment to SetOverridenEngines.

+  SetOverriddenEngines();
+  test_util()->VerifyLoad();
+
+  // The synced DSE GUID should be empty until the user selects something or
+  // there is sync activity.
+  EXPECT_TRUE(test_util()
+                  ->profile()
+                  ->GetTestingPrefService()
+                  ->GetString(prefs::kSyncedDefaultSearchProviderGUID)
+                  .empty());
+
+  TemplateURL* overridden_engine =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("override_keyword"));
+  ASSERT_TRUE(overridden_engine);
+
+  EXPECT_EQ(overridden_engine, model()->GetDefaultSearchProvider());
+
+  // Add user provided 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 overridden engine is returned as default after repair.
+  ASSERT_EQ(overridden_engine, model()->GetDefaultSearchProvider());
+  // Check that overridden_engine guid is stored in
+  // kSyncedDefaultSearchProviderGUID.
+  const std::string dse_guid =
+      test_util()->profile()->GetTestingPrefService()->GetString(
+          prefs::kSyncedDefaultSearchProviderGUID);
+  EXPECT_EQ(overridden_engine->sync_guid(), dse_guid);
+  EXPECT_EQ(overridden_engine->keyword(),
+            model()->GetTemplateURLForGUID(dse_guid)->keyword());
+}
+
+// Test checks that RepairPrepopulatedEngines correctly updates sync guid for
+// default search when search engines is overridden by extension.
+TEST_F(TemplateURLServiceTest,
+       RepairPrepopulatedEnginesWithExtensionUpdatesSyncGuid) {
+  test_util()->VerifyLoad();
+
+  // The synced DSE GUID should be empty until the user selects something or
+  // there is sync activity.
+  EXPECT_TRUE(test_util()
+                  ->profile()
+                  ->GetTestingPrefService()
+                  ->GetString(prefs::kSyncedDefaultSearchProviderGUID)
+                  .empty());
+  // Add user provided default search engine.

[Peter Kasting, 2017/02/13 22:24:57]

Nit: Blank line above this

[Alexander Yashkin, 2017/02/14 12:04:43]

Done

+  TemplateURL* user_dse = model()->Add(
+      base::MakeUnique<TemplateURL>(*GenerateDummyTemplateURLData("user_dse")));
+  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));
+
+  // Add extension controlled default search engine.
+  std::unique_ptr<TemplateURL::AssociatedExtensionInfo> extension_info(
+      new TemplateURL::AssociatedExtensionInfo("extension"));
+  extension_info->wants_to_be_default_engine = true;
+  TemplateURL* extension_dse = model()->AddExtensionControlledTURL(
+      base::MakeUnique<TemplateURL>(
+          *GenerateDummyTemplateURLData("extension_dse"),
+          TemplateURL::NORMAL_CONTROLLED_BY_EXTENSION),
+      std::move(extension_info));
+  EXPECT_EQ(extension_dse, model()->GetDefaultSearchProvider());
+  // Check that user DSE guid is still stored in
+  // kSyncedDefaultSearchProviderGUID.
+  EXPECT_EQ(user_dse->sync_guid(),
+            test_util()->profile()->GetTestingPrefService()->GetString(
+                prefs::kSyncedDefaultSearchProviderGUID));
+
+  model()->RepairPrepopulatedSearchEngines();
+  // Check that extension engine is still default but sync guid is updated to
+  // google.

[Peter Kasting, 2017/02/13 22:24:57]

Again, I'd try to write this to check for the prior DSE instead of checking
specifically for Google.

Also, it's weird that you have a comment here which applies to all the code
below, but there are multiple blank lines in the code below.  In general, don't
have blank lines in any of the code a comment applies to.  Put a blank line
above the comment, then all the applicable code.

[Alexander Yashkin, 2017/02/14 12:04:43]

Fixed, thanks.

+  EXPECT_EQ(extension_dse, model()->GetDefaultSearchProvider());
+
+  TemplateURL* google =
+      model()->GetTemplateURLForKeyword(ASCIIToUTF16("google.com"));
+  ASSERT_TRUE(google);
+
+  EXPECT_EQ(google->sync_guid(),
+            test_util()->profile()->GetTestingPrefService()->GetString(
+                prefs::kSyncedDefaultSearchProviderGUID));
+}
+
 TEST_F(TemplateURLServiceTest, UpdateKeywordSearchTermsForURL) {
   struct TestData {
     const std::string url;