Omnibox: Strip Excess Whitespace from Queries

In SearchProvider, strip redundant interior whitespace and
leading/trailing whitespace in queries.  The changes here affect
both invisible parts (the destination URL), semi-visible parts (the
fill_into_edit text), and visible parts (the rendering the dropdown).

I think this encompasses all the cases we want.  The only possible
additional extension I know of is to remove consecutive interior
whitespace before sending an input to the search history database.
I decided not to bother doing that.

BUG=99239

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

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

4 files changed, 67 insertions, 35 deletions

diff --git a/chrome/browser/autocomplete/base_search_provider.cc b/chrome/browser/autocomplete/base_search_provider.cc
index 06e3cf3..6f4bb80 100644
--- a/chrome/browser/autocomplete/base_search_provider.cc
+++ b/chrome/browser/autocomplete/base_search_provider.cc
@@ -448,8 +448,10 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
   if (!suggestion.annotation().empty())
     match.description = suggestion.annotation();
 
+  // suggestion.match_contents() should have already been collapsed.
   match.allowed_to_be_default_match =
-      (input.text() == suggestion.match_contents());
+      (base::CollapseWhitespace(input.text(), false) ==
+       suggestion.match_contents());
 
   // When the user forced a query, we need to make sure all the fill_into_edit
   // values preserve that property.  Otherwise, if the user starts editing a
@@ -795,6 +797,8 @@ bool BaseSearchProvider::ParseSuggestResults(const base::Value& root_val,
   const bool allow_navsuggest = input.type() != AutocompleteInput::FORCED_QUERY;
   const std::string languages(
       profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
+  const base::string16& trimmed_input =
+      base::CollapseWhitespace(input.text(), false);
   for (size_t index = 0; results_list->GetString(index, &suggestion); ++index) {
     // Google search may return empty suggestions for weird input characters,
     // they make no sense at all and can cause problems in our code.
@@ -843,9 +847,11 @@ bool BaseSearchProvider::ParseSuggestResults(const base::Value& root_val,
 
       // TODO(kochi): Improve calculator suggestion presentation.
       results->suggest_results.push_back(SuggestResult(
-          suggestion, match_type, match_contents, match_contents_prefix,
-          annotation, suggest_query_params, deletion_url, is_keyword_result,
-          relevance, relevances != NULL, should_prefetch, input.text()));
+          base::CollapseWhitespace(suggestion, false), match_type,
+          base::CollapseWhitespace(match_contents, false),
+          match_contents_prefix, annotation, suggest_query_params,
+          deletion_url, is_keyword_result, relevance, relevances != NULL,
+          should_prefetch, trimmed_input));
     }
   }
   SortResults(is_keyword_result, relevances, results);
diff --git a/chrome/browser/autocomplete/base_search_provider.h b/chrome/browser/autocomplete/base_search_provider.h
index bc77f13..3c51658 100644
--- a/chrome/browser/autocomplete/base_search_provider.h
+++ b/chrome/browser/autocomplete/base_search_provider.h
@@ -196,6 +196,7 @@ class BaseSearchProvider : public AutocompleteProvider,
     // The contents to be displayed as prefix of match contents.
     // Used for postfix suggestions to display a leading ellipsis (or some
     // equivalent character) to indicate omitted text.
+    // Only used to pass this information to about:omnibox's "Additional Info".
     base::string16 match_contents_prefix_;
 
     // Optional annotation for the |match_contents_| for disambiguation.
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 1b77f34..70044e0 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -822,11 +822,13 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
   if (verbatim_relevance > 0) {
+    const base::string16& trimmed_verbatim =
+        base::CollapseWhitespace(input_.text(), false);
     SuggestResult verbatim(
-        input_.text(), AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
-        input_.text(), base::string16(), base::string16(), std::string(),
+        trimmed_verbatim, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
+        trimmed_verbatim, base::string16(), base::string16(), std::string(),
         std::string(), false, verbatim_relevance, relevance_from_server, false,
-        input_.text());
+        trimmed_verbatim);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
                   false, &map);
   }
@@ -844,11 +846,13 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       const int keyword_verbatim_relevance =
           GetKeywordVerbatimRelevance(&keyword_relevance_from_server);
       if (keyword_verbatim_relevance > 0) {
+        const base::string16& trimmed_verbatim =
+            base::CollapseWhitespace(keyword_input_.text(), false);
         SuggestResult verbatim(
-            keyword_input_.text(), AutocompleteMatchType::SEARCH_OTHER_ENGINE,
-            keyword_input_.text(), base::string16(), base::string16(),
+            trimmed_verbatim, AutocompleteMatchType::SEARCH_OTHER_ENGINE,
+            trimmed_verbatim, base::string16(), base::string16(),
             std::string(), std::string(), true, keyword_verbatim_relevance,
-            keyword_relevance_from_server, false, keyword_input_.text());
+            keyword_relevance_from_server, false, trimmed_verbatim);
         AddMatchToMap(verbatim, std::string(),
                       did_not_accept_keyword_suggestion, false, &map);
       }
@@ -1061,12 +1065,18 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
   const bool prevent_search_history_inlining =
       OmniboxFieldTrial::SearchHistoryPreventInlining(
           input_.current_page_classification());
+  const base::string16& trimmed_input =
+      base::CollapseWhitespace(input_text, false);
   for (HistoryResults::const_iterator i(results.begin()); i != results.end();
        ++i) {
+    const base::string16& trimmed_suggestion =
+        base::CollapseWhitespace(i->term, false);
+
     // Don't autocomplete multi-word queries that have only been seen once
     // unless the user has typed more than one word.
     bool prevent_inline_autocomplete = base_prevent_inline_autocomplete ||
-        (!input_multiple_words && (i->visits < 2) && HasMultipleWords(i->term));
+        (!input_multiple_words && (i->visits < 2) &&
+         HasMultipleWords(trimmed_suggestion));
 
     // Don't autocomplete search terms that would normally be treated as URLs
     // when typed. For example, if the user searched for "google.com" and types
@@ -1081,9 +1091,10 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
     //  * When the user has typed the whole term, the "what you typed" history
     //    match will outrank us for URL-like inputs anyway, so we need not do
     //    anything special.
-    if (!prevent_inline_autocomplete && classifier && (i->term != input_text)) {
+    if (!prevent_inline_autocomplete && classifier &&
+        (trimmed_suggestion != trimmed_input)) {
       AutocompleteMatch match;
-      classifier->Classify(i->term, false, false,
+      classifier->Classify(trimmed_suggestion, false, false,
                            input_.current_page_classification(), &match, NULL);
       prevent_inline_autocomplete =
           !AutocompleteMatch::IsSearchType(match.type);
@@ -1093,9 +1104,9 @@ SearchProvider::SuggestResults SearchProvider::ScoreHistoryResults(
         i->time, is_keyword, !prevent_inline_autocomplete,
         prevent_search_history_inlining);
     scored_results.push_back(SuggestResult(
-        i->term, AutocompleteMatchType::SEARCH_HISTORY, i->term,
-        base::string16(), base::string16(), std::string(), std::string(),
-        is_keyword, relevance, false, false, input_text));
+        trimmed_suggestion, AutocompleteMatchType::SEARCH_HISTORY,
+        trimmed_suggestion, base::string16(), base::string16(), std::string(),
+        std::string(), is_keyword, relevance, false, false, trimmed_input));
   }
 
   // History returns results sorted for us.  However, we may have docked some
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc b/chrome/browser/autocomplete/search_provider_unittest.cc
index ae57ff8..968f531 100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -364,7 +364,8 @@ void SearchProviderTest::QueryForInputAndSetWYTMatch(
   ASSERT_GE(provider_->matches().size(), 1u);
   EXPECT_TRUE(FindMatchWithDestination(GURL(
       default_t_url_->url_ref().ReplaceSearchTerms(
-          TemplateURLRef::SearchTermsArgs(text))),
+          TemplateURLRef::SearchTermsArgs(base::CollapseWhitespace(
+              text, false)))),
       wyt_match));
 }
 
@@ -741,10 +742,12 @@ TEST_F(SearchProviderTest, AutocompleteMultipleVisitsImmediately) {
   EXPECT_TRUE(wyt_match.allowed_to_be_default_match);
 }
 
-// Autocompletion should work at a word boundary after a space.
+// Autocompletion should work at a word boundary after a space, and should
+// offer a suggestion for the trimmed search query.
 TEST_F(SearchProviderTest, AutocompleteAfterSpace) {
-  GURL term_url(AddSearchToHistory(default_t_url_, ASCIIToUTF16("two searches"),
-                                   2));
+  AddSearchToHistory(default_t_url_, ASCIIToUTF16("two  searches "), 2);
+  GURL suggested_url(default_t_url_->url_ref().ReplaceSearchTerms(
+      TemplateURLRef::SearchTermsArgs(ASCIIToUTF16("two searches"))));
   profile_.BlockUntilHistoryProcessesPendingRequests();
 
   AutocompleteMatch wyt_match;
@@ -752,9 +755,11 @@ TEST_F(SearchProviderTest, AutocompleteAfterSpace) {
                                                       &wyt_match));
   ASSERT_EQ(2u, provider_->matches().size());
   AutocompleteMatch term_match;
-  EXPECT_TRUE(FindMatchWithDestination(term_url, &term_match));
+  EXPECT_TRUE(FindMatchWithDestination(suggested_url, &term_match));
   EXPECT_GT(term_match.relevance, wyt_match.relevance);
   EXPECT_TRUE(term_match.allowed_to_be_default_match);
+  EXPECT_EQ(ASCIIToUTF16("searches"), term_match.inline_autocompletion);
+  EXPECT_EQ(ASCIIToUTF16("two searches"), term_match.fill_into_edit);
   EXPECT_TRUE(wyt_match.allowed_to_be_default_match);
 }
 
@@ -914,33 +919,41 @@ TEST_F(SearchProviderTest, KeywordVerbatim) {
                    ASCIIToUTF16("k foo") ) } },
 
     // Make sure extra whitespace after the keyword doesn't change the
-    // keyword verbatim query.
+    // keyword verbatim query.  Also verify that interior consecutive
+    // whitespace gets trimmed.
     { ASCIIToUTF16("k   foo"), 2,
       { ResultInfo(GURL("http://keyword/foo"),
                    AutocompleteMatchType::SEARCH_OTHER_ENGINE,
                    true,
                    ASCIIToUTF16("k foo")),
-        ResultInfo(GURL("http://defaultturl/k%20%20%20foo"),
+        ResultInfo(GURL("http://defaultturl/k%20foo"),
                    AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                    false,
-                   ASCIIToUTF16("k   foo")) } },
+                   ASCIIToUTF16("k foo")) } },
     // Leading whitespace should be stripped before SearchProvider gets the
     // input; hence there are no tests here about how it handles those inputs.
 
-    // But whitespace elsewhere in the query string should matter to both
-    // matches.
+    // Verify that interior consecutive whitespace gets trimmed in either case.
     { ASCIIToUTF16("k  foo  bar"), 2,
-      { ResultInfo(GURL("http://keyword/foo%20%20bar"),
+      { ResultInfo(GURL("http://keyword/foo%20bar"),
+                   AutocompleteMatchType::SEARCH_OTHER_ENGINE,
+                   true,
+                   ASCIIToUTF16("k foo bar")),
+        ResultInfo(GURL("http://defaultturl/k%20foo%20bar"),
+                   AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
+                   false,
+                   ASCIIToUTF16("k foo bar")) } },
+
+    // Verify that trailing whitespace gets trimmed.
+    { ASCIIToUTF16("k foo bar  "), 2,
+      { ResultInfo(GURL("http://keyword/foo%20bar"),
                    AutocompleteMatchType::SEARCH_OTHER_ENGINE,
                    true,
-                   ASCIIToUTF16("k foo  bar")),
-        ResultInfo(GURL("http://defaultturl/k%20%20foo%20%20bar"),
+                   ASCIIToUTF16("k foo bar")),
+        ResultInfo(GURL("http://defaultturl/k%20foo%20bar"),
                    AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                    false,
-                   ASCIIToUTF16("k  foo  bar")) } },
-    // Note in the above test case we don't test trailing whitespace because
-    // SearchProvider still doesn't handle this well.  See related bugs:
-    // 102690, 99239, 164635.
+                   ASCIIToUTF16("k foo bar")) } },
 
     // Keywords can be prefixed by certain things that should get ignored
     // when constructing the keyword match.
@@ -979,11 +992,12 @@ TEST_F(SearchProviderTest, KeywordVerbatim) {
                    AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                    true,
                    ASCIIToUTF16("k")) } },
+    // Ditto.  Trailing whitespace shouldn't make a difference.
     { ASCIIToUTF16("k "), 1,
-      { ResultInfo(GURL("http://defaultturl/k%20"),
+      { ResultInfo(GURL("http://defaultturl/k"),
                    AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
                    true,
-                   ASCIIToUTF16("k ")) } }
+                   ASCIIToUTF16("k")) } }
 
     // The fact that verbatim queries to keyword are handled by KeywordProvider
     // not SearchProvider is tested in