[SEC] Preserve Play API search engine when updating prepopulated search engines

This CL adjusts the logic of merging prepopulated data into search
engines. The goal is to preserve Play API search engine record.The
behavior should be:
- When new prepopulated engine matches Play API engine, merge them.
- When prepopulated Play API engine gets updated, apply updates.
- When prepopulated Play API engine gets removed, keep the record.

BUG=1002271
R=pkasting@chromium.org,wylieb@chromium.org

Change-Id: I75822512bde4024f32af24565d66d87158732cbf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1814642Reviewed-by: Kevin Bailey <krb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698634}

components/search_engines/template_url.h

@@ -651,6 +651,7 @@ class TemplateURL {
   base::Time last_visited() const { return data_.last_visited; }
 
   bool created_by_policy() const { return data_.created_by_policy; }
+  bool created_from_play_api() const { return data_.created_from_play_api; }
 
   int usage_count() const { return data_.usage_count; }
 

components/search_engines/template_url_service_util_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
 #include "components/search_engines/search_terms_data.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
@@ -18,13 +19,16 @@ namespace {
 
 std::unique_ptr<TemplateURLData> CreatePrepopulateTemplateURLData(
     int prepopulate_id,
-    const std::string& keyword,
-    TemplateURLID id) {
-  std::unique_ptr<TemplateURLData> data(new TemplateURLData);
-  data->prepopulate_id = prepopulate_id;
-  data->SetKeyword(base::ASCIIToUTF16(keyword));
-  data->id = id;
-  return data;
+    const std::string& keyword) {
+  return std::make_unique<TemplateURLData>(
+      base::ASCIIToUTF16("Search engine name"), base::ASCIIToUTF16(keyword),
+      "https://search.url", nullptr /* suggest_url */, nullptr /* image_url */,
+      nullptr /* new_tab_url */, nullptr /* contextual_search_url */,
+      nullptr /* logo_url */, nullptr /* doodle_url */,
+      nullptr /* search_url_post_params */,
+      nullptr /* suggest_url_post_params */,
+      nullptr /* image_url_post_params */, nullptr /* favicon_url */, "UTF-8",
+      base::ListValue() /* alternate_urls_list */, prepopulate_id);
 }
 
 // Creates a TemplateURL with default values except for the prepopulate ID,
@@ -33,9 +37,13 @@ std::unique_ptr<TemplateURLData> CreatePrepopulateTemplateURLData(
 std::unique_ptr<TemplateURL> CreatePrepopulateTemplateURL(
     int prepopulate_id,
     const std::string& keyword,
-    TemplateURLID id) {
-  return std::make_unique<TemplateURL>(
-      *CreatePrepopulateTemplateURLData(prepopulate_id, keyword, id));
+    TemplateURLID id,
+    bool is_play_api_turl = false) {
+  std::unique_ptr<TemplateURLData> data =
+      CreatePrepopulateTemplateURLData(prepopulate_id, keyword);
+  data->id = id;
+  data->created_from_play_api = is_play_api_turl;
+  return std::make_unique<TemplateURL>(*data);
 }
 
 }  // namespace
@@ -44,10 +52,9 @@ TEST(TemplateURLServiceUtilTest, RemoveDuplicatePrepopulateIDs) {
   std::vector<std::unique_ptr<TemplateURLData>> prepopulated_turls;
   TemplateURLService::OwnedTemplateURLVector local_turls;
 
-  prepopulated_turls.push_back(
-      CreatePrepopulateTemplateURLData(1, "winner4", 1));
-  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(2, "xxx", 2));
-  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(3, "yyy", 3));
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(1, "winner4"));
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(2, "xxx"));
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(3, "yyy"));
 
   // Create a sets of different TURLs grouped by prepopulate ID. Each group
   // will test a different heuristic of RemoveDuplicatePrepopulateIDs.
@@ -89,3 +96,43 @@ TEST(TemplateURLServiceUtilTest, RemoveDuplicatePrepopulateIDs) {
                                  base::CompareCase::SENSITIVE));
   }
 }
+
+// Tests correct interaction of Play API search engine during prepopulated list
+// update.
+TEST(TemplateURLServiceUtilTest, MergeEnginesFromPrepopulateData_PlayAPI) {
+  std::vector<std::unique_ptr<TemplateURLData>> prepopulated_turls;
+  TemplateURLService::OwnedTemplateURLVector local_turls;
+
+  // Start with single search engine created from Play API data.
+  local_turls.push_back(CreatePrepopulateTemplateURL(0, "play", 1, true));
+
+  // Test that prepopulated search engine with matching keyword is merged with
+  // Play API search engine.
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(1, "play"));
+  MergeEnginesFromPrepopulateData(nullptr, &prepopulated_turls, &local_turls,
+                                  nullptr, nullptr);
+  ASSERT_EQ(local_turls.size(), 1U);
+  // Merged search engine should have both Play API flag and valid
+  // prepopulate_id.
+  EXPECT_TRUE(local_turls[0]->created_from_play_api());
+  EXPECT_EQ(local_turls[0]->prepopulate_id(), 1);
+
+  // Test that merging prepopulated search engine with matching prepopulate_id
+  // updates keyword of Play API search engine.
+  prepopulated_turls.clear();
+  prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(1, "play2"));
+  MergeEnginesFromPrepopulateData(nullptr, &prepopulated_turls, &local_turls,
+                                  nullptr, nullptr);
+  ASSERT_EQ(local_turls.size(), 1U);
+  EXPECT_TRUE(local_turls[0]->created_from_play_api());
+  EXPECT_EQ(local_turls[0]->keyword(), base::ASCIIToUTF16("play2"));
+
+  // Test that removing search engine from prepopulated list doesn't delete Play
+  // API search engine record.
+  prepopulated_turls.clear();
+  MergeEnginesFromPrepopulateData(nullptr, &prepopulated_turls, &local_turls,
+                                  nullptr, nullptr);
+  ASSERT_EQ(local_turls.size(), 1U);
+  EXPECT_TRUE(local_turls[0]->created_from_play_api());
+  EXPECT_EQ(local_turls[0]->prepopulate_id(), 0);
+}

components/search_engines/util.cc

@@ -182,7 +182,8 @@ TemplateURL* FindURLByPrepopulateID(
 
 void MergeIntoPrepopulatedEngineData(const TemplateURL* original_turl,
                                      TemplateURLData* prepopulated_url) {
-  DCHECK_EQ(original_turl->prepopulate_id(), prepopulated_url->prepopulate_id);
+  DCHECK(original_turl->prepopulate_id() == 0 ||
+         original_turl->prepopulate_id() == prepopulated_url->prepopulate_id);
   if (!original_turl->safe_for_autoreplace()) {
     prepopulated_url->safe_for_autoreplace = false;
     prepopulated_url->SetKeyword(original_turl->keyword());
@@ -192,6 +193,8 @@ void MergeIntoPrepopulatedEngineData(const TemplateURL* original_turl,
   prepopulated_url->sync_guid = original_turl->sync_guid();
   prepopulated_url->date_created = original_turl->date_created();
   prepopulated_url->last_modified = original_turl->last_modified();
+  prepopulated_url->created_from_play_api =
+      original_turl->created_from_play_api();
 }
 
 ActionsFromPrepopulateData::ActionsFromPrepopulateData() {}
@@ -201,14 +204,9 @@ ActionsFromPrepopulateData::ActionsFromPrepopulateData(
 
 ActionsFromPrepopulateData::~ActionsFromPrepopulateData() {}
 
-// This is invoked when the version of the prepopulate data changes.
-// If |removed_keyword_guids| is not NULL, the Sync GUID of each item removed
-// from the DB will be added to it.  Note that this function will take
-// ownership of |prepopulated_urls| and will clear the vector.
 void MergeEnginesFromPrepopulateData(
     KeywordWebDataService* service,
     std::vector<std::unique_ptr<TemplateURLData>>* prepopulated_urls,
-    size_t default_search_index,
     TemplateURLService::OwnedTemplateURLVector* template_urls,
     TemplateURL* default_search_provider,
     std::set<std::string>* removed_keyword_guids) {
@@ -255,8 +253,13 @@ ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
     const TemplateURL* default_search_provider) {
   // Create a map to hold all provided |template_urls| that originally came from
   // prepopulate data (i.e. have a non-zero prepopulate_id()).
+  TemplateURL* play_api_turl = nullptr;
   std::map<int, TemplateURL*> id_to_turl;
   for (auto& turl : existing_urls) {
+    if (turl->created_from_play_api()) {
+      DCHECK_EQ(nullptr, play_api_turl);
+      play_api_turl = turl.get();
+    }
     int prepopulate_id = turl->prepopulate_id();
     if (prepopulate_id > 0)
       id_to_turl[prepopulate_id] = turl.get();
@@ -272,18 +275,24 @@ ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
     DCHECK_NE(0, prepopulated_id);
 
     auto existing_url_iter = id_to_turl.find(prepopulated_id);
+    TemplateURL* existing_url = nullptr;
     if (existing_url_iter != id_to_turl.end()) {
+      existing_url = existing_url_iter->second;
+      id_to_turl.erase(existing_url_iter);
+    } else if (play_api_turl &&
+               play_api_turl->keyword() == prepopulated_url->keyword()) {
+      existing_url = play_api_turl;
+    }
+
+    if (existing_url != nullptr) {
       // Update the data store with the new prepopulated data. Preserve user
       // edits to the name and keyword.
-      TemplateURL* existing_url(existing_url_iter->second);
-      id_to_turl.erase(existing_url_iter);
       MergeIntoPrepopulatedEngineData(existing_url, prepopulated_url.get());
       // Update last_modified to ensure that if this entry is later merged with
       // entries from Sync, the conflict resolution logic knows that this was
       // updated and propagates the new values to the server.
       prepopulated_url->last_modified = base::Time::Now();
-      actions.edited_engines.push_back(
-          std::make_pair(existing_url, *prepopulated_url));
+      actions.edited_engines.push_back({existing_url, *prepopulated_url});
     } else {
       actions.added_engines.push_back(*prepopulated_url);
     }
@@ -300,9 +309,18 @@ ActionsFromPrepopulateData CreateActionsFromCurrentPrepopulateData(
     if ((template_url->safe_for_autoreplace()) &&
         (!default_search_provider ||
          (template_url->prepopulate_id() !=
-             default_search_provider->prepopulate_id()) ||
-         (template_url->keyword() != default_search_provider->keyword())))
-      actions.removed_engines.push_back(template_url);
+          default_search_provider->prepopulate_id()) ||
+         (template_url->keyword() != default_search_provider->keyword()))) {
+      if (template_url->created_from_play_api()) {
+        // Don't remove the entry created from Play API. Just reset
+        // prepopulate_id for it.
+        TemplateURLData data = template_url->data();
+        data.prepopulate_id = 0;
+        actions.edited_engines.push_back({template_url, data});
+      } else {
+        actions.removed_engines.push_back(template_url);
+      }
+    }
   }
 
   return actions;
@@ -358,10 +376,8 @@ void GetSearchProvidersUsingLoadedEngines(
     std::set<std::string>* removed_keyword_guids) {
   DCHECK(template_urls);
   DCHECK(resource_keyword_version);
-  size_t default_search_index;
   std::vector<std::unique_ptr<TemplateURLData>> prepopulated_urls =
-      TemplateURLPrepopulateData::GetPrepopulatedEngines(prefs,
-                                                         &default_search_index);
+      TemplateURLPrepopulateData::GetPrepopulatedEngines(prefs, nullptr);
   RemoveDuplicatePrepopulateIDs(service, prepopulated_urls,
                                 default_search_provider, template_urls,
                                 search_terms_data, removed_keyword_guids);
@@ -369,9 +385,9 @@ void GetSearchProvidersUsingLoadedEngines(
   const int prepopulate_resource_keyword_version =
       TemplateURLPrepopulateData::GetDataVersion(prefs);
   if (*resource_keyword_version < prepopulate_resource_keyword_version) {
-    MergeEnginesFromPrepopulateData(
-        service, &prepopulated_urls, default_search_index, template_urls,
-        default_search_provider, removed_keyword_guids);
+    MergeEnginesFromPrepopulateData(service, &prepopulated_urls, template_urls,
+                                    default_search_provider,
+                                    removed_keyword_guids);
     *resource_keyword_version = prepopulate_resource_keyword_version;
   } else {
     *resource_keyword_version = 0;

components/search_engines/util.h

@@ -68,6 +68,22 @@ struct ActionsFromPrepopulateData {
   std::vector<TemplateURLData> added_engines;
 };
 
+// MergeEnginesFromPrepopulateData merges search engines from
+// |prepopulated_urls| into |template_urls|. Calls
+// CreateActionsFromCurrentPrepopulateData() to collect actions and then applies
+// them on |tempate_urls|. MergeEnginesFromPrepopulateData is invoked when the
+// version of the prepopulate data changes. If |removed_keyword_guids| is not
+// nullptr, the Sync GUID of each item removed from the DB will be added to it.
+// Note that this function will take ownership of |prepopulated_urls| and will
+// clear the vector.
+// The function is exposed in header file to provide access from unittests.
+void MergeEnginesFromPrepopulateData(
+    KeywordWebDataService* service,
+    std::vector<std::unique_ptr<TemplateURLData>>* prepopulated_urls,
+    TemplateURLService::OwnedTemplateURLVector* template_urls,
+    TemplateURL* default_search_provider,
+    std::set<std::string>* removed_keyword_guids);
+
 // Given the user's current URLs and the current set of prepopulated URLs,
 // produces the set of actions (see above) required to make the user's URLs
 // reflect the prepopulate data.  |default_search_provider| is used to avoid