Omnibox: Make Exact Match Tab-to-Search Work

Tested interactively and adds unit test.  Seems to behave as expected.

BUG=47572

Review URL: https://codereview.chromium.org/440753003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287807 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autocomplete/autocomplete_controller.cc

@@ -505,6 +505,10 @@ void AutocompleteController::UpdateAssociatedKeywords(
   if (!keyword_provider_)
     return;
 
+  // Determine if the user's input is an exact keyword match.
+  base::string16 exact_keyword = keyword_provider_->GetKeywordForText(
+      TemplateURLService::CleanUserInputKeyword(input_.text()));
+
   std::set<base::string16> keywords;
   for (ACMatches::iterator match(result->begin()); match != result->end();
        ++match) {
@@ -515,18 +519,31 @@ void AutocompleteController::UpdateAssociatedKeywords(
       continue;
     }
 
+    // When the user has typed an exact keyword, we want tab-to-search on the
+    // default match to select that keyword, even if the match
+    // inline-autocompletes to a different keyword.  (This prevents inline
+    // autocompletions from blocking a user's attempts to use an explicitly-set
+    // keyword of their own creation.)  So use |exact_keyword| if it's
+    // available.
+    if (!exact_keyword.empty() && !keywords.count(exact_keyword)) {
+      keywords.insert(exact_keyword);
+      match->associated_keyword.reset(new AutocompleteMatch(
+          keyword_provider_->CreateVerbatimMatch(exact_keyword,
+                                                 exact_keyword, input_)));
+      continue;
+    }
+
+    // Otherwise, set a match's associated keyword based on the match's
+    // fill_into_edit, which should take inline autocompletions into account.
+    keyword = keyword_provider_->GetKeywordForText(match->fill_into_edit);
+
     // Only add the keyword if the match does not have a duplicate keyword with
     // a more relevant match.
-    keyword = match->associated_keyword.get() ?
-        match->associated_keyword->keyword :
-        keyword_provider_->GetKeywordForText(match->fill_into_edit);
     if (!keyword.empty() && !keywords.count(keyword)) {
       keywords.insert(keyword);
-
-      if (!match->associated_keyword.get())
-        match->associated_keyword.reset(new AutocompleteMatch(
-            keyword_provider_->CreateVerbatimMatch(match->fill_into_edit,
-                                                   keyword, input_)));
+      match->associated_keyword.reset(new AutocompleteMatch(
+          keyword_provider_->CreateVerbatimMatch(match->fill_into_edit,
+                                                 keyword, input_)));
     } else {
       match->associated_keyword.reset();
     }

chrome/browser/autocomplete/autocomplete_provider_unittest.cc

@@ -162,7 +162,7 @@ class AutocompleteProviderTest : public testing::Test,
   struct KeywordTestData {
     const base::string16 fill_into_edit;
     const base::string16 keyword;
-    const bool expected_keyword_result;
+    const base::string16 expected_associated_keyword;
   };
 
   struct AssistedQueryStatsTestData {
@@ -185,7 +185,13 @@ class AutocompleteProviderTest : public testing::Test,
   // properly collected.
   void RunTest();
 
-  void RunRedundantKeywordTest(const KeywordTestData* match_data, size_t size);
+  // Constructs an AutocompleteResult from |match_data|, sets the |controller_|
+  // to pretend it was running against input |input|, calls the |controller_|'s
+  // UpdateAssociatedKeywords, and checks that the matches have associated
+  // keywords as expected.
+  void RunKeywordTest(const base::string16& input,
+                      const KeywordTestData* match_data,
+                      size_t size);
 
   void RunAssistedQueryStatsTest(
       const AssistedQueryStatsTestData* aqs_test_data,
@@ -341,6 +347,15 @@ void AutocompleteProviderTest::ResetControllerWithKeywordProvider() {
   turl_model->Add(keyword_t_url);
   ASSERT_NE(0, keyword_t_url->id());
 
+  // Make a TemplateURL for KeywordProvider that a shorter version of the
+  // first.
+  data.short_name = base::ASCIIToUTF16("f");
+  data.SetKeyword(base::ASCIIToUTF16("f"));
+  data.SetURL("http://f.com/{searchTerms}");
+  keyword_t_url = new TemplateURL(data);
+  turl_model->Add(keyword_t_url);
+  ASSERT_NE(0, keyword_t_url->id());
+
   // Create another TemplateURL for KeywordProvider.
   data.short_name = base::ASCIIToUTF16("bar.com");
   data.SetKeyword(base::ASCIIToUTF16("bar.com"));
@@ -358,9 +373,9 @@ void AutocompleteProviderTest::RunTest() {
   RunQuery(base::ASCIIToUTF16("a"));
 }
 
-void AutocompleteProviderTest::RunRedundantKeywordTest(
-    const KeywordTestData* match_data,
-    size_t size) {
+void AutocompleteProviderTest::RunKeywordTest(const base::string16& input,
+                                              const KeywordTestData* match_data,
+                                              size_t size) {
   ACMatches matches;
   for (size_t i = 0; i < size; ++i) {
     AutocompleteMatch match;
@@ -374,11 +389,17 @@ void AutocompleteProviderTest::RunRedundantKeywordTest(
 
   AutocompleteResult result;
   result.AppendMatches(matches);
+  controller_->input_ = AutocompleteInput(
+      input, base::string16::npos, base::string16(), GURL(),
+      metrics::OmniboxEventProto::INSTANT_NTP_WITH_OMNIBOX_AS_STARTING_FOCUS,
+      false, true, true, true, ChromeAutocompleteSchemeClassifier(&profile_));
   controller_->UpdateAssociatedKeywords(&result);
 
   for (size_t j = 0; j < result.size(); ++j) {
-    EXPECT_EQ(match_data[j].expected_keyword_result,
-        result.match_at(j)->associated_keyword.get() != NULL);
+    EXPECT_EQ(match_data[j].expected_associated_keyword,
+              result.match_at(j)->associated_keyword.get() ?
+                  result.match_at(j)->associated_keyword->keyword :
+                  base::string16());
   }
 }
 
@@ -542,36 +563,74 @@ TEST_F(AutocompleteProviderTest, RedundantKeywordsIgnoredInResult) {
 
   {
     KeywordTestData duplicate_url[] = {
-      { base::ASCIIToUTF16("fo"), base::string16(), false },
-      { base::ASCIIToUTF16("foo.com"), base::string16(), true },
-      { base::ASCIIToUTF16("foo.com"), base::string16(), false }
+      { base::ASCIIToUTF16("fo"), base::string16(), base::string16() },
+      { base::ASCIIToUTF16("foo.com"), base::string16(),
+        base::ASCIIToUTF16("foo.com") },
+      { base::ASCIIToUTF16("foo.com"), base::string16(), base::string16() }
     };
 
     SCOPED_TRACE("Duplicate url");
-    RunRedundantKeywordTest(duplicate_url, ARRAYSIZE_UNSAFE(duplicate_url));
+    RunKeywordTest(base::ASCIIToUTF16("fo"), duplicate_url,
+                   ARRAYSIZE_UNSAFE(duplicate_url));
   }
 
   {
     KeywordTestData keyword_match[] = {
-      { base::ASCIIToUTF16("foo.com"), base::ASCIIToUTF16("foo.com"), false },
-      { base::ASCIIToUTF16("foo.com"), base::string16(), false }
+      { base::ASCIIToUTF16("foo.com"), base::ASCIIToUTF16("foo.com"),
+        base::string16() },
+      { base::ASCIIToUTF16("foo.com"), base::string16(), base::string16() }
     };
 
     SCOPED_TRACE("Duplicate url with keyword match");
-    RunRedundantKeywordTest(keyword_match, ARRAYSIZE_UNSAFE(keyword_match));
+    RunKeywordTest(base::ASCIIToUTF16("fo"), keyword_match,
+                   ARRAYSIZE_UNSAFE(keyword_match));
   }
 
   {
     KeywordTestData multiple_keyword[] = {
-      { base::ASCIIToUTF16("fo"), base::string16(), false },
-      { base::ASCIIToUTF16("foo.com"), base::string16(), true },
-      { base::ASCIIToUTF16("foo.com"), base::string16(), false },
-      { base::ASCIIToUTF16("bar.com"), base::string16(), true },
+      { base::ASCIIToUTF16("fo"), base::string16(), base::string16() },
+      { base::ASCIIToUTF16("foo.com"), base::string16(),
+        base::ASCIIToUTF16("foo.com") },
+      { base::ASCIIToUTF16("foo.com"), base::string16(), base::string16() },
+      { base::ASCIIToUTF16("bar.com"), base::string16(),
+        base::ASCIIToUTF16("bar.com") },
     };
 
     SCOPED_TRACE("Duplicate url with multiple keywords");
-    RunRedundantKeywordTest(multiple_keyword,
-        ARRAYSIZE_UNSAFE(multiple_keyword));
+    RunKeywordTest(base::ASCIIToUTF16("fo"), multiple_keyword,
+                   ARRAYSIZE_UNSAFE(multiple_keyword));
+  }
+}
+
+// Test that exact match keywords trump keywords associated with
+// the match.
+TEST_F(AutocompleteProviderTest, ExactMatchKeywords) {
+  ResetControllerWithKeywordProvider();
+
+  {
+    KeywordTestData keyword_match[] = {
+      { base::ASCIIToUTF16("foo.com"), base::string16(),
+        base::ASCIIToUTF16("foo.com") }
+    };
+
+    SCOPED_TRACE("keyword match as usual");
+    RunKeywordTest(base::ASCIIToUTF16("fo"), keyword_match,
+                   ARRAYSIZE_UNSAFE(keyword_match));
+  }
+
+  // The same result set with an input of "f" (versus "fo") should get
+  // a different associated keyword because "f" is an exact match for
+  // a keyword and that should trump the keyword normally associated with
+  // this match.
+  {
+    KeywordTestData keyword_match[] = {
+      { base::ASCIIToUTF16("foo.com"), base::string16(),
+        base::ASCIIToUTF16("f") }
+    };
+
+    SCOPED_TRACE("keyword exact match");
+    RunKeywordTest(base::ASCIIToUTF16("f"), keyword_match,
+                   ARRAYSIZE_UNSAFE(keyword_match));
   }
 }
 

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -56,16 +56,26 @@ namespace {
 
 const char kSearchKeyword[] = "foo";
 const char kSearchKeyword2[] = "footest.com";
-const wchar_t kSearchKeywordKeys[] = { ui::VKEY_F, ui::VKEY_O, ui::VKEY_O, 0 };
+const ui::KeyboardCode kSearchKeywordKeys[] = {
+  ui::VKEY_F, ui::VKEY_O, ui::VKEY_O, ui::VKEY_UNKNOWN
+};
+const ui::KeyboardCode kSearchKeywordPrefixKeys[] = {
+  ui::VKEY_F, ui::VKEY_O, ui::VKEY_UNKNOWN
+};
+const ui::KeyboardCode kSearchKeywordCompletionKeys[] = {
+  ui::VKEY_O, ui::VKEY_UNKNOWN
+};
 const char kSearchURL[] = "http://www.foo.com/search?q={searchTerms}";
 const char kSearchShortName[] = "foo";
 const char kSearchText[] = "abc";
-const wchar_t kSearchTextKeys[] = { ui::VKEY_A, ui::VKEY_B, ui::VKEY_C, 0 };
+const ui::KeyboardCode kSearchTextKeys[] = {
+  ui::VKEY_A, ui::VKEY_B, ui::VKEY_C, ui::VKEY_UNKNOWN
+};
 const char kSearchTextURL[] = "http://www.foo.com/search?q=abc";
 
 const char kInlineAutocompleteText[] = "def";
-const wchar_t kInlineAutocompleteTextKeys[] = {
-  ui::VKEY_D, ui::VKEY_E, ui::VKEY_F, 0
+const ui::KeyboardCode kInlineAutocompleteTextKeys[] = {
+  ui::VKEY_D, ui::VKEY_E, ui::VKEY_F, ui::VKEY_UNKNOWN
 };
 
 // Hostnames that shall be blocked by host resolver.
@@ -167,9 +177,9 @@ class OmniboxViewTest : public InProcessBrowserTest,
     SendKeyForBrowser(browser(), key, modifiers);
   }
 
-  void SendKeySequence(const wchar_t* keys) {
-    for (; *keys; ++keys)
-      ASSERT_NO_FATAL_FAILURE(SendKey(static_cast<ui::KeyboardCode>(*keys), 0));
+  void SendKeySequence(const ui::KeyboardCode* keys) {
+    for (; *keys != ui::VKEY_UNKNOWN; ++keys)
+      ASSERT_NO_FATAL_FAILURE(SendKey(*keys, 0));
   }
 
   bool SendKeyAndWait(const Browser* browser,
@@ -563,7 +573,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DesiredTLD) {
   ASSERT_TRUE(popup_model);
 
   // Test ctrl-Enter.
-  const wchar_t kKeys[] = { ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, 0 };
+  const ui::KeyboardCode kKeys[] = {
+    ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, ui::VKEY_UNKNOWN
+  };
   ASSERT_NO_FATAL_FAILURE(SendKeySequence(kKeys));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(popup_model->IsOpen());
@@ -600,7 +612,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DesiredTLDWithTemporaryText) {
   template_url_service->Add(new TemplateURL(data));
 
   // Send "ab", so that an "abc" entry appears in the popup.
-  const wchar_t kSearchTextPrefixKeys[] = { ui::VKEY_A, ui::VKEY_B, 0 };
+  const ui::KeyboardCode kSearchTextPrefixKeys[] = {
+    ui::VKEY_A, ui::VKEY_B, ui::VKEY_UNKNOWN
+  };
   ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchTextPrefixKeys));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(popup_model->IsOpen());
@@ -665,7 +679,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, DISABLED_EnterToSearch) {
   EXPECT_EQ(kSearchTextURL, url.spec());
 
   // Test that entering a single character then Enter performs a search.
-  const wchar_t kSearchSingleCharKeys[] = { ui::VKEY_Z, 0 };
+  const ui::KeyboardCode kSearchSingleCharKeys[] = {
+    ui::VKEY_Z, ui::VKEY_UNKNOWN
+  };
   chrome::FocusLocationBar(browser());
   EXPECT_TRUE(omnibox_view->IsSelectAll());
   ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchSingleCharKeys));
@@ -929,8 +945,8 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_AcceptKeywordBySpace) {
   ASSERT_NO_FATAL_FAILURE(
       AddHistoryEntry(kHistoryFoobar, Time::Now() - TimeDelta::FromHours(1)));
 
-  // Type "foo" to trigger inline autocomplete.
-  ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys));
+  // Type "fo" to trigger inline autocomplete.
+  ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordPrefixKeys));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
   ASSERT_NE(search_keyword, omnibox_view->GetText());
@@ -939,6 +955,15 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_AcceptKeywordBySpace) {
   ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
   ASSERT_TRUE(omnibox_view->model()->keyword().empty());
 
+  // Add the "o".  Inline autocompletion should still happen, but now we
+  // should also get a keyword hint because we've typed a keyword exactly.
+  ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordCompletionKeys));
+  ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+  ASSERT_TRUE(omnibox_view->model()->popup_model()->IsOpen());
+  ASSERT_NE(search_keyword, omnibox_view->GetText());
+  ASSERT_TRUE(omnibox_view->model()->is_keyword_hint());
+  ASSERT_FALSE(omnibox_view->model()->keyword().empty());
+
   // Trigger keyword mode by space.
   ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0));
   ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
@@ -1229,7 +1254,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, MAYBE_TabTraverseResultsTest) {
   ASSERT_TRUE(popup_model);
 
   // Input something to trigger results.
-  const wchar_t kKeys[] = { ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, 0 };
+  const ui::KeyboardCode kKeys[] = {
+    ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, ui::VKEY_UNKNOWN
+  };
   ASSERT_NO_FATAL_FAILURE(SendKeySequence(kKeys));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(popup_model->IsOpen());
@@ -1700,7 +1727,9 @@ IN_PROC_BROWSER_TEST_F(OmniboxViewTest, CtrlArrowAfterArrowSuggestions) {
   ASSERT_TRUE(popup_model);
 
   // Input something to trigger results.
-  const wchar_t kKeys[] = { ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, 0 };
+  const ui::KeyboardCode kKeys[] = {
+    ui::VKEY_B, ui::VKEY_A, ui::VKEY_R, ui::VKEY_UNKNOWN
+  };
   ASSERT_NO_FATAL_FAILURE(SendKeySequence(kKeys));
   ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
   ASSERT_TRUE(popup_model->IsOpen());