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

chrome/browser/autocomplete/search_provider.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/base64.h"
@@ skipping 363 matching lines @@
 void SearchProvider::UpdateMatches() {
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
   if (!matches_.empty() &&
       (default_results_.HasServerProvidedScores() ||
        keyword_results_.HasServerProvidedScores())) {
     // 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();
     }
     if (IsTopMatchSearchWithURLInput()) {
       // Disregard the suggested search and verbatim relevances if the input
       // type is URL and the top match is a highly-ranked search suggestion.
       // For example, prevent a search for "foo.com" from outranking another
       // provider's navigation for "foo.com" or "foo.com/url_from_history".
       ApplyCalculatedSuggestRelevance(&keyword_results_.suggest_results);
       ApplyCalculatedSuggestRelevance(&default_results_.suggest_results);
       default_results_.verbatim_relevance = -1;
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
     if (FindTopMatch() == matches_.end()) {
       // Guarantee that SearchProvider returns a legal default match.  (The
       // omnibox always needs at least one legal default match, and it relies
       // on SearchProvider to always return one.)
       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();
 }
 
 void SearchProvider::Run() {
   // Start a new request with the current input.
   suggest_results_pending_ = 0;
   time_suggest_request_sent_ = base::TimeTicks::Now();
 
   default_fetcher_.reset(CreateSuggestFetcher(kDefaultProviderURLFetcherID,
       providers_.GetDefaultProviderURL(), input_));
@@ skipping 265 matching lines @@
       keyword_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
 
   bool relevance_from_server;
   int verbatim_relevance = GetVerbatimRelevance(&relevance_from_server);
   int did_not_accept_default_suggestion =
       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);
     SearchSuggestionParser::SuggestResult verbatim(
         trimmed_verbatim, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
         trimmed_verbatim, base::string16(), base::string16(), base::string16(),
         base::string16(), 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);

[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...)
     // Note: in this provider, SEARCH_OTHER_ENGINE must correspond
     // to the keyword verbatim search query.  Do not create other matches
     // of type SEARCH_OTHER_ENGINE.
     if (keyword_url &&
         (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION)) {
       bool keyword_relevance_from_server;
       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);
         SearchSuggestionParser::SuggestResult verbatim(
             trimmed_verbatim, AutocompleteMatchType::SEARCH_OTHER_ENGINE,
             trimmed_verbatim, base::string16(), base::string16(),
             base::string16(), base::string16(), std::string(), std::string(),
             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);
       }
     }
   }
   AddHistoryResultsToMap(keyword_history_results_, true,
                          did_not_accept_keyword_suggestion, &map);
   AddHistoryResultsToMap(default_history_results_, false,
                          did_not_accept_default_suggestion, &map);
 
   AddSuggestResultsToMap(keyword_results_.suggest_results,
                          keyword_results_.metadata, &map);
@@ skipping 47 matching lines @@
                       base::TimeTicks::Now() - start_time);
 }
 
 ACMatches::const_iterator SearchProvider::FindTopMatch() const {
   ACMatches::const_iterator it = matches_.begin();
   while ((it != matches_.end()) && !it->allowed_to_be_default_match)
     ++it;
   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) &&
       (first_match != matches_.end()) &&
       (first_match->relevance > CalculateRelevanceForVerbatim()) &&
       (first_match->type != AutocompleteMatchType::NAVSUGGEST) &&
       (first_match->type != AutocompleteMatchType::NAVSUGGEST_PERSONALIZED);
 }
 
 void SearchProvider::AddNavigationResultsToMatches(
@@ skipping 39 matching lines @@
     if ((scored_results.front().relevance() < 1200) ||
         !HasMultipleWords(scored_results.front().suggestion()))
       scored_results.clear();  // Didn't detect the case above, score normally.
   }
   if (scored_results.empty())
     scored_results = ScoreHistoryResults(results, prevent_inline_autocomplete,
                                          input_multiple_words, input_text,
                                          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);
 }
 
 SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
     const HistoryResults& results,
     bool base_prevent_inline_autocomplete,
     bool input_multiple_words,
     const base::string16& input_text,
@@ skipping 101 matching lines @@
     last_relevance = i->relevance();
   }
 
   return scored_results;
 }
 
 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 {
   // Use the suggested verbatim relevance score if it is non-negative (valid),
   // if inline autocomplete isn't prevented (always show verbatim on backspace),
   // and if it won't suppress verbatim, leaving no default provider matches.
   // Otherwise, if the default provider returned no matches and was still able
   // to suppress verbatim, the user would have no search/nav matches and may be
   // left unable to search using their default provider from the omnibox.
   // Check for results on each verbatim calculation, as results from older
@@ skipping 218 matching lines @@
   last_answer_seen_.query_type = match->answer_type;
 }
 
 void SearchProvider::DoAnswersQuery(const AutocompleteInput& input) {
   // If the query text starts with trimmed input, this is valid prefetch data.
   prefetch_data_ = StartsWith(last_answer_seen_.full_query_text,
                               base::CollapseWhitespace(input.text(), false),
                               false) ?
       last_answer_seen_ : AnswersQueryData();
 }