Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)

chrome/browser/search_engines/template_url_service_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 "components/search_engines/template_url_service.h"
 
 #include <stddef.h>
 
 #include <memory>
 #include <utility>
@@ skipping 10 matching lines @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_test_util.h"
 #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"
 
 using base::ASCIIToUTF16;
 using base::Time;
 using base::TimeDelta;
 
 namespace {
 
 // A prepopulated ID to set for engines we want to show in the default list.
@@ skipping 128 matching lines @@
                                   Time last_modified,
                                   Time last_visited);
 
   // Verifies the two TemplateURLs are equal.
   void AssertEquals(const TemplateURL& expected, const TemplateURL& actual);
 
   // Verifies the two timestamps are equal, within the expected degree of
   // precision.
   void AssertTimesEqual(const base::Time& expected, const base::Time& actual);
 
   // Create an URL that appears to have been prepopulated, but won't be in the

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

Nit: While here, can you fix "Create" to "Creates"?

   // current data.
   std::unique_ptr<TemplateURL> CreatePreloadedTemplateURL(
       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);
   void VerifyObserverFired();
   TemplateURLServiceTestUtil* test_util() { return test_util_.get(); }
   TemplateURLService* model() { return test_util_->model(); }
   const SearchTermsData& search_terms_data() {
     return model()->search_terms_data();
   }
 
@@ skipping 86 matching lines @@
   data.favicon_url = GURL("http://favicon.url");
   data.safe_for_autoreplace = safe_for_autoreplace;
   data.input_encodings.push_back("UTF-8");
   data.date_created = Time::FromTimeT(100);
   data.last_modified = Time::FromTimeT(100);
   data.last_visited = Time::FromTimeT(100);
   data.prepopulate_id = prepopulate_id;
   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();
 }
 
 void TemplateURLServiceTest::VerifyObserverFired() {
   EXPECT_LE(1, test_util_->GetObserverCount());
   test_util_->ResetObserverCount();
 }
 
@@ skipping 581 matching lines @@
   ExpectSimilar(expected_managed_default.get(), actual_managed_default);
 
   // The following call has no effect on the managed search engine.
   model()->RepairPrepopulatedSearchEngines();
 
   EXPECT_TRUE(model()->is_default_search_managed());
   actual_managed_default = model()->GetDefaultSearchProvider();
   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;
     const base::string16 term;
   } data[] = {
     { "http://foo/", base::string16() },
     { "http://foo/foo?q=xx", base::string16() },
     { "http://x/bar?q=xx", base::string16() },
     { "http://x/foo?y=xx", base::string16() },
     { "http://x/foo?q=xx", ASCIIToUTF16("xx") },
@@ skipping 636 matching lines @@
   TemplateURL* update_url =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));
   const base::Time update_last_modified = update_url->last_modified();
   model()->SetUserSelectedDefaultSearchProvider(update_url);
   TemplateURL* reloaded_url =
       model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));
   const base::Time reloaded_last_modified = reloaded_url->last_modified();
   EXPECT_NE(original_last_modified, reloaded_last_modified);
   EXPECT_EQ(update_last_modified, reloaded_last_modified);
 }