[search_engines] Make prepopulated engines outrank autogenerated engines

Currently, prepopulated engines don't outrank other autogenerated
engines, and that was a contributory factory in the bug linked below
that clobbered users' default search engines on Canary.

This CL fixes that problem, and adds a test that fails before this CL,
and passes afterwards. (To prevent this condition from regressing.)

After this CL there will be more follow-on work to make sure we never
remove prepopulated engines during any deduplication.

This CL also incidentally improves some naming consistency among our
unit tests.

Bug: 1164024
Change-Id: I6f90668abad02ae53adfbd0c8c739e07247a4a7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617865
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842158}

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -1732,7 +1732,7 @@ TEST_F(TemplateURLServiceTest, DefaultExtensionEnginePersistsBeforeLoad) {
 // Checks that correct priority is applied when resolving conflicts between the
 // omnibox extension, search engine extension and user search engines with same
 // keyword.
-TEST_F(TemplateURLServiceTest, CheckEnginesWithSameKeywords) {
+TEST_F(TemplateURLServiceTest, KeywordConflictNonReplaceableEngines) {
   test_util()->VerifyLoad();
   // TemplateURLData used for user engines.
   std::unique_ptr<TemplateURLData> turl_data =
@@ -1845,7 +1845,7 @@ TEST_F(TemplateURLServiceTest, DefaultSearchProviderKeywordConflictReentrancy) {
          "behavior on ApplyDefaultSearchChangeNoMetrics.";
 }
 
-TEST_F(TemplateURLServiceTest, ConflictingReplaceableEnginesShouldOverwrite) {
+TEST_F(TemplateURLServiceTest, ReplaceableEngineUpdateHandlesKeywordConflicts) {
   test_util()->VerifyLoad();
   // Add 2 replaceable user engine with different keywords.
   TemplateURL* user1 =
@@ -1869,6 +1869,33 @@ TEST_F(TemplateURLServiceTest, ConflictingReplaceableEnginesShouldOverwrite) {
   EXPECT_EQ(user2, model()->GetTemplateURLForKeyword(ASCIIToUTF16("user2")));
 }
 
+// Verifies that we favor prepopulated engines over other safe_for_autoreplace()
+// engines, even if they are newer. https://crbug.com/1164024
+TEST_F(TemplateURLServiceTest, KeywordConflictFavorsPrepopulatedEngines) {
+  test_util()->VerifyLoad();
+
+  // Add prepopulated engine with prepopulate_id == 42.
+  TemplateURL* prepopulated = model()->Add(CreateKeywordWithDate(
+      model(), "prepopulated", "common_keyword", "http://test1", std::string(),
+      std::string(), std::string(), true, 42, "UTF-8",
+      base::Time::FromTimeT(0)));
+  ASSERT_TRUE(prepopulated);
+  TemplateURLData prepopulated_data = prepopulated->data();
+
+  // Add a newer autogenerated engine with the same keyword.
+  TemplateURL* newer_autogenerated_engine = AddKeywordWithDate(
+      "autogenerated", "common_keyword", "http://test2", std::string(),
+      std::string(), std::string(), true, "UTF-8", base::Time::FromTimeT(20));
+
+  // Verify that the prepopulated engine was added, and the newer autogenerated
+  // engine was discarded. Also check that data has not changed.
+  EXPECT_EQ(nullptr, newer_autogenerated_engine);
+  EXPECT_EQ(prepopulated,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword")));
+  EXPECT_TRUE(TemplateURL::MatchesData(prepopulated, &prepopulated_data,
+                                       model()->search_terms_data()));
+}
+
 TEST_F(TemplateURLServiceTest, CheckNonreplaceableEnginesKeywordsConflicts) {
   test_util()->VerifyLoad();
 

components/search_engines/template_url.cc

@@ -1365,6 +1365,8 @@ bool TemplateURL::IsBetterThanEngineWithConflictingKeyword(
         !engine->safe_for_autoreplace(),
         // Prefer engines created by Play API.
         engine->created_from_play_api(),
+        // Favor prepopulated engines over other auto-generated engines.
+        engine->prepopulate_id() > 0,
         // More recently modified engines or created engines win.
         engine->last_modified(), engine->date_created(),
         // TODO(tommycli): This should be a tie-breaker than provides a total

components/search_engines/template_url_service.h

@@ -459,7 +459,7 @@ class TemplateURLService : public WebDataServiceConsumer,
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, AddOmniboxExtensionKeyword);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, ExtensionsWithSameKeywords);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
-                           CheckEnginesWithSameKeywords);
+                           KeywordConflictNonReplaceableEngines);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest, LastVisitedTimeUpdate);
   FRIEND_TEST_ALL_PREFIXES(TemplateURLServiceTest,
                            RepairPrepopulatedSearchEngines);