Prohibit keywords with spaces in settings UI and during import.

During import, such keywords are ignored.

Bug: 6867
Change-Id: I7f6955629bca92506c4be4dba398577a07483580
Fixed: 6867
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1937583Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Commit-Queue: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719847}

chrome/browser/importer/profile_writer.cc

@@ -303,6 +303,12 @@ void ProfileWriter::AddKeywords(
     if (model->GetTemplateURLForKeyword(turl->keyword()) != nullptr)
       continue;
 
+    // The omnibox doesn't properly handle search keywords with whitespace,
+    // so skip importing them.
+    if (turl->keyword().find_first_of(base::kWhitespaceUTF16) !=
+        base::string16::npos)
+      continue;
+
     // For search engines if there is already a keyword with the same
     // host+path, we don't import it. This is done to avoid both duplicate
     // search providers (such as two Googles, or two Yahoos) as well as making

chrome/browser/importer/profile_writer_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/importer/importer_unittest_utils.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/importer/imported_bookmark_entry.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -29,8 +30,8 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using bookmarks::BookmarkModel;
-using bookmarks::UrlAndTitle;
 using bookmarks::TitledUrlMatch;
+using bookmarks::UrlAndTitle;
 
 class TestProfileWriter : public ProfileWriter {
  public:
@@ -110,6 +111,11 @@ class ProfileWriterTest : public testing::Test {
     loop.Run();
   }
 
+  // Creates a TemplateURL from the provided data.
+  std::unique_ptr<TemplateURL> CreateTemplateURL(const std::string& keyword,
+                                                 const std::string& url,
+                                                 const std::string& short_name);
+
  protected:
   std::vector<ImportedBookmarkEntry> bookmarks_;
   history::URLRows pages_;
@@ -190,6 +196,17 @@ TEST_F(ProfileWriterTest, CheckBookmarksAfterWritingDataTwice) {
   VerifyBookmarksCount(bookmarks_record, bookmark_model, 2);
 }
 
+std::unique_ptr<TemplateURL> ProfileWriterTest::CreateTemplateURL(
+    const std::string& keyword,
+    const std::string& url,
+    const std::string& short_name) {
+  TemplateURLData data;
+  data.SetKeyword(base::ASCIIToUTF16(keyword));
+  data.SetShortName(base::ASCIIToUTF16(short_name));
+  data.SetURL(TemplateURLRef::DisplayURLToURLRef(base::ASCIIToUTF16(url)));
+  return std::make_unique<TemplateURL>(data);
+}
+
 // Verify that history entires are not duplicated when added twice.
 TEST_F(ProfileWriterTest, CheckHistoryAfterWritingDataTwice) {
   TestingProfile profile;
@@ -208,3 +225,36 @@ TEST_F(ProfileWriterTest, CheckHistoryAfterWritingDataTwice) {
   VerifyHistoryCount(&profile);
   EXPECT_EQ(original_history_count, history_count_);
 }
+
+TEST_F(ProfileWriterTest, AddKeywords) {
+  TestingProfile profile;
+  ASSERT_TRUE(profile.CreateHistoryService(true, false));
+  TemplateURLServiceFactory::GetInstance()->SetTestingFactoryAndUse(
+      &profile,
+      base::BindRepeating(&TemplateURLServiceFactory::BuildInstanceFor));
+
+  TemplateURLService::OwnedTemplateURLVector keywords;
+  keywords.push_back(CreateTemplateURL("key1", "http://key1.com", "n1"));
+  // This entry will not be added since it has the same key as an existing
+  // keyword.
+  keywords.push_back(CreateTemplateURL("key1", "http://key1_1.com", "n1_1"));
+  keywords.push_back(CreateTemplateURL("key2", "http://key2.com", "n2"));
+  // This entry will not be added since the keyword contains spaces.
+  keywords.push_back(CreateTemplateURL("key 3", "http://key3.com", "n3"));
+
+  auto profile_writer = base::MakeRefCounted<TestProfileWriter>(&profile);
+  profile_writer->AddKeywords(std::move(keywords), false);
+
+  TemplateURLService* turl_model =
+      TemplateURLServiceFactory::GetForProfile(&profile);
+  auto turls = turl_model->GetTemplateURLs();
+  EXPECT_EQ(turls.size(), 2u);
+
+  EXPECT_EQ(turls[0]->keyword(), base::ASCIIToUTF16("key1"));
+  EXPECT_EQ(turls[0]->url(), "http://key1.com");
+  EXPECT_EQ(turls[0]->short_name(), base::ASCIIToUTF16("n1"));
+
+  EXPECT_EQ(turls[1]->keyword(), base::ASCIIToUTF16("key2"));
+  EXPECT_EQ(turls[1]->url(), "http://key2.com");
+  EXPECT_EQ(turls[1]->short_name(), base::ASCIIToUTF16("n2"));
+}

chrome/browser/ui/search_engines/edit_search_engine_controller.cc

@@ -72,6 +72,13 @@ bool EditSearchEngineController::IsKeywordValid(
       base::CollapseWhitespace(keyword_input, true));
   if (keyword_input_trimmed.empty())
     return false;  // Do not allow empty keyword.
+
+  // The omnibox doesn't properly handle search keywords with whitespace,
+  // so do not allow such keywords.
+  if (keyword_input_trimmed.find_first_of(base::kWhitespaceUTF16) !=
+      base::string16::npos)
+    return false;
+
   const TemplateURL* turl_with_keyword =
       TemplateURLServiceFactory::GetForProfile(profile_)->
       GetTemplateURLForKeyword(keyword_input_trimmed);