Omnibox - Search Provider - Cleanup Keyword Mode's Legal Matches

SearchProvider enforces that if the user is in keyword mode, the
only suggestions allowed to be the default match are keyword mode
suggestions, lest they break the user out of keyword mode.

Previously, this constraint was applied with an after-the-fact
correction to allowed_to_be_default_match.  This change sets
allowed_to_be_default_match correctly when the AutocompleteMatches
are created.

All tests pass.  (And yes, this constraint enforcement is tested.
Here you can see the tests that were added when the constraint was put
in place: https://codereview.chromium.org/67693004 .)

BUG=398135
R=msw@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#289981}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289981 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 34 insertions, 50 deletions

diff --git a/chrome/browser/autocomplete/base_search_provider.cc b/chrome/browser/autocomplete/base_search_provider.cc
index 29a6ac0..c2a68bf 100644
--- a/chrome/browser/autocomplete/base_search_provider.cc
+++ b/chrome/browser/autocomplete/base_search_provider.cc
@@ -116,8 +116,11 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     bool from_keyword_provider,
     const TemplateURL* template_url,
     const SearchTermsData& search_terms_data) {
+  // This call uses a number of default values.  For instance, it assumes that
+  // if this match is from a keyword provider than the user is in keyword mode.
   return CreateSearchSuggestion(
-      NULL, AutocompleteInput(), SearchSuggestionParser::SuggestResult(
+      NULL, AutocompleteInput(), from_keyword_provider,
+      SearchSuggestionParser::SuggestResult(
           suggestion, type, suggestion, base::string16(), base::string16(),
           base::string16(), base::string16(), std::string(), std::string(),
           from_keyword_provider, 0, false, false, base::string16()),
@@ -208,6 +211,7 @@ void BaseSearchProvider::SetDeletionURL(const std::string& deletion_url,
 AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     AutocompleteProvider* autocomplete_provider,
     const AutocompleteInput& input,
+    const bool in_keyword_mode,
     const SearchSuggestionParser::SuggestResult& suggestion,
     const TemplateURL* template_url,
     const SearchTermsData& search_terms_data,
@@ -240,6 +244,7 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
 
   // suggestion.match_contents() should have already been collapsed.
   match.allowed_to_be_default_match =
+      (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       (base::CollapseWhitespace(input.text(), false) ==
        suggestion.match_contents());
 
@@ -251,6 +256,7 @@ AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
   if (suggestion.from_keyword_provider())
     match.fill_into_edit.append(match.keyword + base::char16(' '));
   if (!input.prevent_inline_autocomplete() &&
+      (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       StartsWith(suggestion.suggestion(), input.text(), false)) {
     match.inline_autocompletion =
         suggestion.suggestion().substr(input.text().length());
@@ -370,9 +376,10 @@ void BaseSearchProvider::AddMatchToMap(
     const std::string& metadata,
     int accepted_suggestion,
     bool mark_as_deletable,
+    bool in_keyword_mode,
     MatchMap* map) {
   AutocompleteMatch match = CreateSearchSuggestion(
-      this, GetInput(result.from_keyword_provider()), result,
+      this, GetInput(result.from_keyword_provider()), in_keyword_mode, result,
       GetTemplateURL(result.from_keyword_provider()),
       template_url_service_->search_terms_data(), accepted_suggestion,
       ShouldAppendExtraParams(result));
diff --git a/chrome/browser/autocomplete/base_search_provider.h b/chrome/browser/autocomplete/base_search_provider.h
index 0f3b05c..66f26aa 100644
--- a/chrome/browser/autocomplete/base_search_provider.h
+++ b/chrome/browser/autocomplete/base_search_provider.h
@@ -57,7 +57,8 @@ class BaseSearchProvider : public AutocompleteProvider {
   static bool ShouldPrefetch(const AutocompleteMatch& match);
 
   // Returns a simpler AutocompleteMatch suitable for persistence like in
-  // ShortcutsDatabase.
+  // ShortcutsDatabase.  This wrapper function uses a number of default values
+  // that may or may not be appropriate for your needs.
   // NOTE: Use with care. Most likely you want the other CreateSearchSuggestion
   // with protected access.
   static AutocompleteMatch CreateSearchSuggestion(
@@ -113,6 +114,8 @@ class BaseSearchProvider : public AutocompleteProvider {
   //
   // |input| is also necessary for various other details, like whether we should
   // allow inline autocompletion and what the transition type should be.
+  // |in_keyword_mode| helps guarantee a non-keyword suggestion does not
+  // appear as the default match when the user is in keyword mode.
   // |accepted_suggestion| is used to generate Assisted Query Stats.
   // |append_extra_query_params| should be set if |template_url| is the default
   // search engine, so the destination URL will contain any
@@ -120,6 +123,7 @@ class BaseSearchProvider : public AutocompleteProvider {
   static AutocompleteMatch CreateSearchSuggestion(
       AutocompleteProvider* autocomplete_provider,
       const AutocompleteInput& input,
+      const bool in_keyword_mode,
       const SearchSuggestionParser::SuggestResult& suggestion,
       const TemplateURL* template_url,
       const SearchTermsData& search_terms_data,
@@ -180,11 +184,14 @@ class BaseSearchProvider : public AutocompleteProvider {
   // |metadata| and |accepted_suggestion| are used for generating an
   // AutocompleteMatch.
   // |mark_as_deletable| indicates whether the match should be marked deletable.
+  // |in_keyword_mode| helps guarantee a non-keyword suggestion does not
+  // appear as the default match when the user is in keyword mode.
   // NOTE: Any result containing a deletion URL is always marked deletable.
   void AddMatchToMap(const SearchSuggestionParser::SuggestResult& result,
                      const std::string& metadata,
                      int accepted_suggestion,
                      bool mark_as_deletable,
+                     bool in_keyword_mode,
                      MatchMap* map);
 
   // Parses results from the suggest server and updates the appropriate suggest
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 802b879..64eec0e 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -401,10 +401,10 @@ void SearchProvider::UpdateMatches() {
     // These blocks attempt to repair undesirable behavior by suggested
     // relevances with minimal impact, preserving other suggested relevances.
 
-    if (!HasKeywordDefaultMatchInKeywordMode()) {
+    if ((providers_.GetKeywordProviderURL() != NULL) &&
+        (FindTopMatch() == matches_.end())) {
       // In keyword mode, disregard the keyword verbatim suggested relevance
-      // if necessary so there at least one keyword match that's allowed to
-      // be the default match.
+      // if necessary, so at least one match is allowed to be default.
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
@@ -426,24 +426,11 @@ void SearchProvider::UpdateMatches() {
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(HasKeywordDefaultMatchInKeywordMode());
     DCHECK(!IsTopMatchSearchWithURLInput());
     DCHECK(FindTopMatch() != matches_.end());
   }
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Omnibox.SearchProviderMatches", matches_.size(), 1, 6, 7);
-
-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
-  if ((keyword_url != NULL) && HasKeywordDefaultMatchInKeywordMode()) {
-    // If there is a keyword match that is allowed to be the default match,
-    // then prohibit default provider matches from being the default match lest
-    // such matches cause the user to break out of keyword mode.
-    for (ACMatches::iterator it = matches_.begin(); it != matches_.end();
-         ++it) {
-      if (it->keyword != keyword_url->keyword())
-        it->allowed_to_be_default_match = false;
-    }
-  }
   UpdateDone();
 }
 
@@ -730,6 +717,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       default_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   if (verbatim_relevance > 0) {
     const base::string16& trimmed_verbatim =
         base::CollapseWhitespace(input_.text(), false);
@@ -755,10 +743,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
         answer_type, std::string(), std::string(), false, verbatim_relevance,
         relevance_from_server, false, trimmed_verbatim);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
-                  false, &map);
+                  false, keyword_url != NULL, &map);
   }
   if (!keyword_input_.text().empty()) {
-    const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
@@ -780,7 +767,7 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
             true, keyword_verbatim_relevance, keyword_relevance_from_server,
             false, trimmed_verbatim);
         AddMatchToMap(verbatim, std::string(),
-                      did_not_accept_keyword_suggestion, false, &map);
+                      did_not_accept_keyword_suggestion, false, true, &map);
       }
     }
   }
@@ -848,21 +835,6 @@ ACMatches::const_iterator SearchProvider::FindTopMatch() const {
   return it;
 }
 
-bool SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {
-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
-  // If the user is not in keyword mode, return true to say that this
-  // constraint is not violated.
-  if (keyword_url == NULL)
-    return true;
-  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
-       ++it) {
-    if ((it->keyword == keyword_url->keyword()) &&
-        it->allowed_to_be_default_match)
-      return true;
-  }
-  return false;
-}
-
 bool SearchProvider::IsTopMatchSearchWithURLInput() const {
   ACMatches::const_iterator first_match = FindTopMatch();
   return (input_.type() == metrics::OmniboxInputType::URL) &&
@@ -922,7 +894,8 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
                                          is_keyword);
   for (SearchSuggestionParser::SuggestResults::const_iterator i(
            scored_results.begin()); i != scored_results.end(); ++i) {
-    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true, map);
+    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true,
+                  providers_.GetKeywordProviderURL() != NULL, map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
                       base::TimeTicks::Now() - start_time);
@@ -1044,8 +1017,10 @@ void SearchProvider::AddSuggestResultsToMap(
     const SearchSuggestionParser::SuggestResults& results,
     const std::string& metadata,
     MatchMap* map) {
-  for (size_t i = 0; i < results.size(); ++i)
-    AddMatchToMap(results[i], metadata, i, false, map);
+  for (size_t i = 0; i < results.size(); ++i) {
+    AddMatchToMap(results[i], metadata, i, false,
+                  providers_.GetKeywordProviderURL() != NULL, map);
+  }
 }
 
 int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {
diff --git a/chrome/browser/autocomplete/search_provider.h b/chrome/browser/autocomplete/search_provider.h
index d7be6bf..7140c5c 100644
--- a/chrome/browser/autocomplete/search_provider.h
+++ b/chrome/browser/autocomplete/search_provider.h
@@ -229,9 +229,9 @@ class SearchProvider : public BaseSearchProvider,
   // be chosen as default.
   ACMatches::const_iterator FindTopMatch() const;
 
-  // Checks if suggested relevances violate certain expected constraints.
-  // See UpdateMatches() for the use and explanation of these constraints.
-  bool HasKeywordDefaultMatchInKeywordMode() const;
+  // Checks if suggested relevances violate an expected constraint.
+  // See UpdateMatches() for the use and explanation of this constraint
+  // and other constraints enforced without the use of helper functions.
   bool IsTopMatchSearchWithURLInput() const;
 
   // Converts an appropriate number of navigation results in
diff --git a/chrome/browser/autocomplete/zero_suggest_provider.cc b/chrome/browser/autocomplete/zero_suggest_provider.cc
index 7cc12e8..a71472b 100644
--- a/chrome/browser/autocomplete/zero_suggest_provider.cc
+++ b/chrome/browser/autocomplete/zero_suggest_provider.cc
@@ -273,7 +273,7 @@ void ZeroSuggestProvider::AddSuggestResultsToMap(
     const SearchSuggestionParser::SuggestResults& results,
     MatchMap* map) {
   for (size_t i = 0; i < results.size(); ++i)
-    AddMatchToMap(results[i], std::string(), i, false, map);
+    AddMatchToMap(results[i], std::string(), i, false, false, map);
 }
 
 AutocompleteMatch ZeroSuggestProvider::NavigationToMatch(
diff --git a/chrome/browser/autocomplete/zero_suggest_provider.h b/chrome/browser/autocomplete/zero_suggest_provider.h
index 5c7b9f7..0f4c093 100644
--- a/chrome/browser/autocomplete/zero_suggest_provider.h
+++ b/chrome/browser/autocomplete/zero_suggest_provider.h
@@ -4,12 +4,7 @@
 //
 // This file contains the zero-suggest autocomplete provider. This experimental
 // provider is invoked when the user focuses in the omnibox prior to editing,
-// and generates search query suggestions based on the current URL. To enable
-// this provider, point --experimental-zero-suggest-url-prefix at an
-// appropriate suggestion service.
-//
-// HUGE DISCLAIMER: This is just here for experimenting and will probably be
-// deleted entirely as we revise how suggestions work with the omnibox.
+// and generates search query suggestions based on the current URL.
 
 #ifndef CHROME_BROWSER_AUTOCOMPLETE_ZERO_SUGGEST_PROVIDER_H_
 #define CHROME_BROWSER_AUTOCOMPLETE_ZERO_SUGGEST_PROVIDER_H_