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

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index bf49576093038dd465cd265dc23fb206c6212e35..e2dd28b3604291dee5d676ec1442a539efde06b1 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -381,10 +381,11 @@ 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) &&

[msw, 2014/08/15 18:32:00]

nit: remove extra parens here and below; "!= NULL" may not be needed.

[Mark P, 2014/08/15 18:47:08]

Prefer to keep all of those, which (slightly) seem to be the convention in this
file.

+        (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 there at least one match that's allowed to be the

[msw, 2014/08/15 18:32:00]

nit: "necessary, so", "there is", and consider: "if necessary, so at least one
match is allowed to be default".

[Mark P, 2014/08/15 18:47:07]

Done.

+      // default match.
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
@@ -406,24 +407,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,

[msw, 2014/08/15 18:32:00]

I'm a little worried that this might no longer be enforced, can you point me at
the test for this behavior? (I'll look after sending comments too)

[msw, 2014/08/15 18:38:24]

Actually, your changes to CreateSearchSuggestion definitely continue enforcement
of this constraint, so I'm no longer worried about that, but I'm still looking
to ensure that this behavior is tested.

[Mark P, 2014/08/15 18:47:07]

There is a link in the code review description to the tests that were added /
modified when we put in this constraint.

[msw, 2014/08/15 19:06:06]

Indeed, but I was looking for the specific test/case for this. It appears to be
tested sufficiently in SearchProviderTest.KeywordVerbatim.

-    // 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();
 }
 
@@ -709,6 +697,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);
@@ -719,10 +708,9 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
         verbatim_relevance, relevance_from_server, false,
         trimmed_verbatim);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
-                  false, &map);
+                  false, keyword_url != NULL, &map);

[msw, 2014/08/15 18:32:01]

nit: "!= NULL" may not be needed.

[Mark P, 2014/08/15 18:47:08]

Acknowledged.

However, I don't want readers to think we're passing in keyword_url to this
function.

   }
   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...)
@@ -744,7 +732,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);
       }
     }
   }
@@ -812,21 +800,6 @@ ACMatches::const_iterator SearchProvider::FindTopMatch() const {
   return it;
 }
 
-bool SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {

[msw, 2014/08/15 18:32:00]

Remove the decl in search_provider.h

[Mark P, 2014/08/15 18:47:07]

Done.

-  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) &&
@@ -886,7 +859,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);

[msw, 2014/08/15 18:32:01]

nit: "!= NULL" may not be needed.

[Mark P, 2014/08/15 18:47:07]

same reply as above.

   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
                       base::TimeTicks::Now() - start_time);
@@ -1008,8 +982,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);

[msw, 2014/08/15 18:32:00]

nit: "!= NULL" may not be needed.

[Mark P, 2014/08/15 18:47:07]

same reply as above

+  }
 }
 
 int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {