[SEC] Handle merging with existing search engines

This CL adjusts logic in TemplateUrlServiceAndroid::SetPlayAPISearchEngine
to merge search engine data when search engine with matchiung keyword
already exists.

Additionally this CL adjusts merging logic during prepopulated list
update to preserve Play API search engine data. This is important since
search url might contain attribution parameters. We might decide to take
different approach with providing attribution info, but before that
happens preserving Play API data is the safest approach.

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

Change-Id: I596a6df724cfc73801663ca7d97074ba69e856c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838534
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Kevin Bailey <krb@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703394}

chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java

@@ -53,6 +53,8 @@ public class TemplateUrlServiceTest {
     private static final String PREFETCH_PARAMETER = "pf";
     private static final String PREFETCH_VALUE = "c";
 
+    private static final String PLAY_API_SEARCH_URL = "https://play.search.engine?q={searchTerms}";
+
     @Test
     @SmallTest
     @Feature({"ContextualSearch"})
@@ -270,30 +272,45 @@ public class TemplateUrlServiceTest {
     @Test
     @SmallTest
     @Feature({"SearchEngines"})
-    public void testSetPlayAPISearchEngine() {
+    public void testSetPlayAPISearchEngine_CreateNew() {
+        final TemplateUrlService templateUrlService = waitForTemplateUrlServiceToLoad();
+
+        // Adding Play API search engine should succeed.
+        Assert.assertTrue(TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
+            @Override
+            public Boolean call() {
+                return templateUrlService.setPlayAPISearchEngine(
+                        "SearchEngine1", "keyword1", PLAY_API_SEARCH_URL, "https://fav.icon");
+            }
+        }));
+        TemplateUrl defaultSearchEngine =
+                TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<TemplateUrl>() {
+                    @Override
+                    public TemplateUrl call() {
+                        return templateUrlService.getDefaultSearchEngineTemplateUrl();
+                    }
+                });
+        Assert.assertEquals("keyword1", defaultSearchEngine.getKeyword());
+        Assert.assertTrue(defaultSearchEngine.getIsPrepopulated());
+        Assert.assertEquals(PLAY_API_SEARCH_URL, defaultSearchEngine.getURL());
+    }
+
+    @Test
+    @SmallTest
+    @Feature({"SearchEngines"})
+    public void testSetPlayAPISearchEngine_UpdateExisting() {
         final TemplateUrlService templateUrlService = waitForTemplateUrlServiceToLoad();
 
         // Add regular search engine. It will be used to test conflict with Play API search engine.
         TestThreadUtils.runOnUiThreadBlocking(
                 () -> { templateUrlService.addSearchEngineForTesting("keyword1", 0); });
 
-        // Adding Play API search engine with the same keyword should fail.
-        Assert.assertFalse(
-                TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
-                    @Override
-                    public Boolean call() {
-                        return templateUrlService.setPlayAPISearchEngine("SearchEngine1",
-                                "keyword1", "https://search.engine?q={searchTerms}",
-                                "https://fav.icon");
-                    }
-                }));
-
-        // Adding Play API search engine with unique keyword should succeed.
+        // Adding Play API search engine with the same keyword should succeed.
         Assert.assertTrue(TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
             @Override
             public Boolean call() {
-                return templateUrlService.setPlayAPISearchEngine("SearchEngine2", "keyword2",
-                        "https://search.engine?q={searchTerms}", "https://fav.icon");
+                return templateUrlService.setPlayAPISearchEngine(
+                        "SearchEngine1", "keyword1", PLAY_API_SEARCH_URL, "https://fav.icon");
             }
         }));
         TemplateUrl defaultSearchEngine =
@@ -303,19 +320,20 @@ public class TemplateUrlServiceTest {
                         return templateUrlService.getDefaultSearchEngineTemplateUrl();
                     }
                 });
-        Assert.assertEquals("keyword2", defaultSearchEngine.getKeyword());
+        Assert.assertEquals("keyword1", defaultSearchEngine.getKeyword());
         Assert.assertTrue(defaultSearchEngine.getIsPrepopulated());
+        Assert.assertEquals(PLAY_API_SEARCH_URL, defaultSearchEngine.getURL());
 
         // Adding Play API search engine again should fail.
         Assert.assertFalse(
                 TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<Boolean>() {
                     @Override
                     public Boolean call() {
-                        return templateUrlService.setPlayAPISearchEngine("SearchEngine3",
-                                "keyword3", "https://search.engine?q={searchTerms}",
-                                "https://fav.icon");
+                        return templateUrlService.setPlayAPISearchEngine("SearchEngine2",
+                                "keyword2", PLAY_API_SEARCH_URL, "https://fav.icon");
                     }
                 }));
+
         defaultSearchEngine =
                 TestThreadUtils.runOnUiThreadBlockingNoException(new Callable<TemplateUrl>() {
                     @Override
@@ -323,7 +341,7 @@ public class TemplateUrlServiceTest {
                         return templateUrlService.getDefaultSearchEngineTemplateUrl();
                     }
                 });
-        Assert.assertEquals("keyword2", defaultSearchEngine.getKeyword());
+        Assert.assertEquals("keyword1", defaultSearchEngine.getKeyword());
     }
 
     private int getSearchEngineCount(final TemplateUrlService templateUrlService) {

chrome/browser/search_engines/template_url_service_unittest.cc

@@ -708,8 +708,8 @@ TEST_F(TemplateURLServiceTest, Reset) {
 
   // Make sure the mappings in the model were updated.
   ASSERT_EQ(t_url, model()->GetTemplateURLForKeyword(new_keyword));
-  ASSERT_TRUE(
-      model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")) == NULL);
+  ASSERT_EQ(nullptr,
+            model()->GetTemplateURLForKeyword(ASCIIToUTF16("keyword")));
 
   std::unique_ptr<TemplateURL> cloned_url(
       std::make_unique<TemplateURL>(t_url->data()));
@@ -723,6 +723,83 @@ TEST_F(TemplateURLServiceTest, Reset) {
   AssertTimesEqual(now, read_url->last_modified());
 }
 
+TEST_F(TemplateURLServiceTest, CreateFromPlayAPI) {
+  test_util()->VerifyLoad();
+  const size_t initial_count = model()->GetTemplateURLs().size();
+
+  const base::string16 short_name = ASCIIToUTF16("google");
+  const base::string16 keyword = ASCIIToUTF16("keyword");
+  const std::string search_url = "http://www.google.com/foo/bar";
+  const std::string favicon_url = "http://favicon.url";
+  TemplateURL* t_url = model()->CreateOrUpdateTemplateURLFromPlayAPIData(
+      short_name, keyword, search_url, favicon_url);
+  ASSERT_EQ(short_name, t_url->short_name());
+  ASSERT_EQ(keyword, t_url->keyword());
+  ASSERT_EQ(search_url, t_url->url());
+  ASSERT_EQ(GURL(favicon_url), t_url->favicon_url());
+  ASSERT_TRUE(t_url->created_from_play_api());
+  ASSERT_EQ(t_url, model()->GetTemplateURLForKeyword(keyword));
+
+  auto cloned_url = std::make_unique<TemplateURL>(t_url->data());
+
+  // Reload the model from the database and make sure the change took.
+  test_util()->ResetModel(true);
+  EXPECT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
+  const TemplateURL* read_url = model()->GetTemplateURLForKeyword(keyword);
+  ASSERT_TRUE(read_url);
+  AssertEquals(*cloned_url, *read_url);
+}
+
+TEST_F(TemplateURLServiceTest, UpdateFromPlayAPI) {
+  base::string16 keyword = ASCIIToUTF16("keyword");
+
+  // Add a new TemplateURL.
+  test_util()->VerifyLoad();
+  const size_t initial_count = model()->GetTemplateURLs().size();
+  TemplateURLData data;
+  data.SetShortName(ASCIIToUTF16("google"));
+  data.SetKeyword(keyword);
+  data.SetURL("http://www.google.com/foo/bar");
+  data.favicon_url = GURL("http://favicon.url");
+  data.date_created = Time::FromTimeT(100);
+  data.last_modified = Time::FromTimeT(100);
+  data.last_visited = Time::FromTimeT(100);
+  TemplateURL* t_url = model()->Add(std::make_unique<TemplateURL>(data));
+
+  VerifyObserverCount(1);
+  base::RunLoop().RunUntilIdle();
+
+  Time now = Time::Now();
+  auto clock = std::make_unique<base::SimpleTestClock>();
+  clock->SetNow(now);
+  model()->set_clock(std::move(clock));
+
+  // Reset the short name and url and make sure it takes.
+  const base::string16 new_short_name = ASCIIToUTF16("new_name");
+  const std::string new_search_url = "new_url";
+  const std::string new_favicon_url = "new_favicon_url";
+  TemplateURL* updated_turl = model()->CreateOrUpdateTemplateURLFromPlayAPIData(
+      new_short_name, keyword, new_search_url, new_favicon_url);
+  ASSERT_EQ(t_url, updated_turl);
+  ASSERT_EQ(new_short_name, t_url->short_name());
+  ASSERT_EQ(keyword, t_url->keyword());
+  ASSERT_EQ(new_search_url, t_url->url());
+  ASSERT_EQ(GURL(new_favicon_url), t_url->favicon_url());
+  ASSERT_TRUE(t_url->created_from_play_api());
+
+  // Make sure the mappings in the model were updated.
+  ASSERT_EQ(t_url, model()->GetTemplateURLForKeyword(keyword));
+
+  auto cloned_url = std::make_unique<TemplateURL>(t_url->data());
+
+  // Reload the model from the database and make sure the change took.
+  test_util()->ResetModel(true);
+  EXPECT_EQ(initial_count + 1, model()->GetTemplateURLs().size());
+  const TemplateURL* read_url = model()->GetTemplateURLForKeyword(keyword);
+  ASSERT_TRUE(read_url);
+  AssertEquals(*cloned_url, *read_url);
+}
+
 TEST_F(TemplateURLServiceTest, DefaultSearchProvider) {
   // Add a new TemplateURL.
   test_util()->VerifyLoad();

components/search_engines/android/template_url_service_android.cc

@@ -256,28 +256,14 @@ jboolean TemplateUrlServiceAndroid::SetPlayAPISearchEngine(
 
   base::string16 keyword =
       base::android::ConvertJavaStringToUTF16(env, jkeyword);
-  const TemplateURL* existing_turl =
-      template_url_service_->GetTemplateURLForKeyword(keyword);
-  if (existing_turl) {
-    // TODO(crbug/1002271): Implement merging Play API search engine with
-    // existing prepopulated or autogenerated search engine.
-    return false;
-  }
+  base::string16 name = base::android::ConvertJavaStringToUTF16(env, jname);
+  std::string search_url = base::android::ConvertJavaStringToUTF8(jsearch_url);
+  std::string favicon_url =
+      base::android::ConvertJavaStringToUTF8(jfavicon_url);
 
-  TemplateURLData data;
-  data.SetShortName(base::android::ConvertJavaStringToUTF16(env, jname));
-  data.SetKeyword(keyword);
-  data.SetURL(base::android::ConvertJavaStringToUTF8(jsearch_url));
-  data.favicon_url = GURL(base::android::ConvertJavaStringToUTF8(jfavicon_url));
-  data.safe_for_autoreplace = true;
-  data.created_from_play_api = true;
   TemplateURL* t_url =
-      template_url_service_->Add(std::make_unique<TemplateURL>(data));
-  if (!t_url) {
-    // TODO(crbug/1002271): Investigate and handle cases when
-    // TemplateURLService::Add() might return false.
-    return false;
-  }
+      template_url_service_->CreateOrUpdateTemplateURLFromPlayAPIData(
+          name, keyword, search_url, favicon_url);
 
   if (template_url_service_->CanMakeDefault(t_url))
     template_url_service_->SetUserSelectedDefaultSearchProvider(t_url);

components/search_engines/template_url_data.cc

@@ -49,6 +49,9 @@ TemplateURLData::TemplateURLData()
 
 TemplateURLData::TemplateURLData(const TemplateURLData& other) = default;
 
+TemplateURLData& TemplateURLData::operator=(const TemplateURLData& other) =
+    default;
+
 TemplateURLData::TemplateURLData(const base::string16& name,
                                  const base::string16& keyword,
                                  base::StringPiece search_url,
@@ -96,8 +99,7 @@ TemplateURLData::TemplateURLData(const base::string16& name,
   }
 }
 
-TemplateURLData::~TemplateURLData() {
-}
+TemplateURLData::~TemplateURLData() = default;
 
 void TemplateURLData::SetShortName(const base::string16& short_name) {
   // Remove tabs, carriage returns, and the like, as they can corrupt

components/search_engines/template_url_data.h

@@ -23,6 +23,7 @@ class ListValue;
 struct TemplateURLData {
   TemplateURLData();
   TemplateURLData(const TemplateURLData& other);
+  TemplateURLData& operator=(const TemplateURLData& other);
 
   // Creates a TemplateURLData suitable for prepopulated engines.
   // Note that unlike in the default constructor, |safe_for_autoreplace| will

components/search_engines/template_url_service.cc

@@ -617,6 +617,30 @@ void TemplateURLService::ResetTemplateURL(TemplateURL* url,
   Update(url, TemplateURL(data));
 }
 
+TemplateURL* TemplateURLService::CreateOrUpdateTemplateURLFromPlayAPIData(
+    const base::string16& title,
+    const base::string16& keyword,
+    const std::string& search_url,
+    const std::string& favicon_url) {
+  TemplateURL* existing_turl = FindNonExtensionTemplateURLForKeyword(keyword);
+  TemplateURLData data;
+  if (existing_turl)
+    data = existing_turl->data();
+  data.SetShortName(title);
+  data.SetKeyword(keyword);
+  data.SetURL(search_url);
+  data.favicon_url = GURL(favicon_url);
+  data.safe_for_autoreplace = true;
+  data.created_from_play_api = true;
+  if (existing_turl) {
+    Update(existing_turl, TemplateURL(data));
+  } else {
+    existing_turl = Add(std::make_unique<TemplateURL>(data));
+    DCHECK(existing_turl);
+  }
+  return existing_turl;
+}
+
 void TemplateURLService::UpdateProviderFavicons(
     const GURL& potential_search_url,
     const GURL& favicon_url) {

components/search_engines/template_url_service.h

@@ -254,6 +254,15 @@ class TemplateURLService : public WebDataServiceConsumer,
                         const base::string16& keyword,
                         const std::string& search_url);
 
+  // Creates TemplateURL, populating it with data from Play API. If TemplateURL
+  // with matching keyword already exists then merges Play API data into it.
+  // Sets |created_from_play_api| flag.
+  TemplateURL* CreateOrUpdateTemplateURLFromPlayAPIData(
+      const base::string16& title,
+      const base::string16& keyword,
+      const std::string& search_url,
+      const std::string& favicon_url);
+
   // Updates any search providers matching |potential_search_url| with the new
   // favicon location |favicon_url|.
   void UpdateProviderFavicons(const GURL& potential_search_url,

components/search_engines/template_url_service_util_unittest.cc

@@ -107,25 +107,28 @@ TEST(TemplateURLServiceUtilTest, MergeEnginesFromPrepopulateData_PlayAPI) {
   local_turls.push_back(CreatePrepopulateTemplateURL(0, "play", 1, true));
 
   // Test that prepopulated search engine with matching keyword is merged with
-  // Play API search engine.
+  // Play API search engine. Search URL should come from Play API search engine.
+  const std::string prepopulated_search_url = "http://prepopulated.url";
   prepopulated_turls.push_back(CreatePrepopulateTemplateURLData(1, "play"));
+  prepopulated_turls.back()->SetURL(prepopulated_search_url);
   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);
+  EXPECT_EQ(1, local_turls[0]->prepopulate_id());
+  EXPECT_NE(prepopulated_search_url, local_turls[0]->url());
 
   // Test that merging prepopulated search engine with matching prepopulate_id
-  // updates keyword of Play API search engine.
+  // preserves 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"));
+  EXPECT_EQ(local_turls[0]->keyword(), base::ASCIIToUTF16("play"));
 
   // Test that removing search engine from prepopulated list doesn't delete Play
   // API search engine record.

components/search_engines/util.cc

@@ -184,10 +184,22 @@ void MergeIntoPrepopulatedEngineData(const TemplateURL* original_turl,
                                      TemplateURLData* prepopulated_url) {
   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());
+  // When the user modified search engine's properties or search engine is
+  // imported from Play API data we need to preserve certain search engine
+  // properties from overriding with prepopulated data.
+  if (!original_turl->safe_for_autoreplace() ||
+      original_turl->created_from_play_api()) {
+    prepopulated_url->safe_for_autoreplace =
+        original_turl->safe_for_autoreplace();
     prepopulated_url->SetShortName(original_turl->short_name());
+    prepopulated_url->SetKeyword(original_turl->keyword());
+    if (original_turl->created_from_play_api()) {
+      // TODO(crbug/1002271): Search url from Play API might contain attribution
+      // info and therefore should be preserved through prepopulated data
+      // update. In the future we might decide to take different approach to
+      // pass attribution info to search providers.
+      prepopulated_url->SetURL(original_turl->url());
+    }
   }
   prepopulated_url->id = original_turl->id();
   prepopulated_url->sync_guid = original_turl->sync_guid();