Omnibox: Make the Controller Reorder Matches for Inlining

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/callback.h"
@@ skipping 289 matching lines @@
       match.contents_class.push_back(
           ACMatchClassification(input_position, ACMatchClassification::NONE));
       size_t next_fragment_position = input_position + input_text.length();
       if (next_fragment_position < query_string.length()) {
         match.contents_class.push_back(
             ACMatchClassification(next_fragment_position,
                                   ACMatchClassification::MATCH));
       }
     }
   } else {
-    // Otherwise, we're dealing with the "default search" result which has no
-    // completion.
+    // Otherwise, |match| is a verbatim (what-you-typed) match, either for the
+    // default provider or a keyword search provider.
     match.contents_class.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
+    match.allowed_to_be_default_match = true;
   }
 
   // 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
   // suggestion, non-Search results will suddenly appear.
   if (input.type() == AutocompleteInput::FORCED_QUERY)
     match.fill_into_edit.assign(ASCIIToUTF16("?"));
   if (is_keyword)
     match.fill_into_edit.append(match.keyword + char16(' '));
   if (!input.prevent_inline_autocomplete() &&
       StartsWith(query_string, input_text, false)) {
     match.inline_autocompletion = query_string.substr(input_text.length());
+    match.allowed_to_be_default_match = true;
   }
   match.fill_into_edit.append(query_string);
 
   const TemplateURLRef& search_url = template_url->url_ref();
   DCHECK(search_url.SupportsReplacement());
   match.search_terms_args.reset(
       new TemplateURLRef::SearchTermsArgs(query_string));
   match.search_terms_args->original_query = input_text;
   match.search_terms_args->accepted_suggestion = accepted_suggestion;
   match.search_terms_args->omnibox_start_margin = omnibox_start_margin;
@@ skipping 169 matching lines @@
   if (input.text().empty()) {
     // User typed "?" alone.  Give them a placeholder result indicating what
     // this syntax does.
     if (default_provider) {
       AutocompleteMatch match;
       match.provider = this;
       match.contents.assign(l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE));
       match.contents_class.push_back(
           ACMatchClassification(0, ACMatchClassification::NONE));
       match.keyword = providers_.default_provider();
+      match.allowed_to_be_default_match = true;
       matches_.push_back(match);
     }
     Stop(false);
     return;
   }
 
   input_ = input;
 
   DoHistoryQuery(minimal_changes);
   StartOrStopSuggestQuery(minimal_changes);
@@ skipping 533 matching lines @@
   return matches_.front().relevance <
       CalculateRelevanceForVerbatimIgnoringKeywordModeState();
 }
 
 bool SearchProvider::IsTopMatchSearchWithURLInput() const {
   return input_.type() == AutocompleteInput::URL &&
          matches_.front().relevance > CalculateRelevanceForVerbatim() &&
          matches_.front().type != AutocompleteMatchType::NAVSUGGEST;
 }
 
-bool SearchProvider::IsTopMatchNotInlinable() const {
-  // Note: this test assumes the SEARCH_OTHER_ENGINE match corresponds to
-  // the verbatim search query on the keyword engine.  SearchProvider should
-  // not create any other match of type SEARCH_OTHER_ENGINE.
-  return
-      matches_.front().type != AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED &&
-      matches_.front().type != AutocompleteMatchType::SEARCH_OTHER_ENGINE &&
-      matches_.front().inline_autocompletion.empty() &&
-      matches_.front().fill_into_edit != input_.text();
+bool SearchProvider::LacksValidDefaultMatch(

[Peter Kasting, 2013/08/06 22:56:16]

Nit: I think this function's body would read more clearly if you reversed it to
be "HasValidDefaultMatch()".  I found myself repeatedly confused about how we
were returning a bool indicating the lack of something.

[Mark P, 2013/08/07 00:44:31]

I chose Lacks...() for consistency with the other constraint functions.  In all
those cases, returning true means the constraint is violated.  Frankly, I think
some of the other names are confusing because of the way they're negated, but
that's outside the scope of this change (which is big enough already).

[Peter Kasting, 2013/08/07 00:49:53]

I don't see any particular benefit to being consistent in that sense.

I'd make this function Has...(), then have a followup change that reverses
whichever other functions would make more sense reversed.  But if you wanted to
do it all at once that'd be OK too; I just don't really want to leave this
function this way long term because my brain hurts.

[Mark P, 2013/08/07 00:58:52]

Done.

+    bool omnibox_will_reorder_for_legal_default_match) const {
+  // If the omnibox will reorder matches to put something allowed to be the
+  // default match first, then lacking a valid default match means there is
+  // no match allowed to be default.  If the omnibox will not reorder matches,
+  // then lacking a valid default match means the top match is not allowed
+  // to be the default match.

[Peter Kasting, 2013/08/06 22:56:16]

Nit: This comment is somewhat confusing.  How about:

  One of the SearchProvider matches may need to be the overall default.  If
AutocompleteResult is allowed to reorder matches, this means we simply need at
least one match in the list to be |allowed_to_be_default|.  If no reordering is
possible, however, then our first match needs to set this flag.

[Mark P, 2013/08/07 00:44:31]

Yes, that is clearer.  Used that comment.

+  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
+       ++it) {
+    if (it->allowed_to_be_default_match)
+      return false;
+    if (!omnibox_will_reorder_for_legal_default_match)
+      return true;
+  }
+  return true;
 }
 
 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 (IsTopMatchNavigationInKeywordMode()) {
       // Correct the suggested relevance scores if the top match is a
       // navigation in keyword mode, since inlining a navigation match
       // would break the user out of keyword mode.  By the way, if the top
       // match is a non-keyword match (query or navsuggestion) in keyword
       // mode, the user would also break out of keyword mode.  However,
       // that situation is impossible given the current scoring paradigm
       // and the fact that only one search engine (Google) provides suggested
       // relevance scores at this time.
       DemoteKeywordNavigationMatchesPastTopQuery();
       ConvertResultsToAutocompleteMatches();
       DCHECK(!IsTopMatchNavigationInKeywordMode());
     }
-    if (IsTopMatchScoreTooLow()) {
+    // True if the omnibox will reorder matches as necessary to make the top
+    // one something that is allowed to be the default match.
+    const bool omnibox_will_reorder_for_legal_default_match =
+        OmniboxFieldTrial::ReorderForLegalDefaultMatch(
+            input_.current_page_classification());
+    if (!omnibox_will_reorder_for_legal_default_match &&
+        IsTopMatchScoreTooLow()) {
       // Disregard the suggested verbatim relevance if the top score is below
       // the usual verbatim value. For example, a BarProvider may rely on
-      // SearchProvider's verbatim or inlineable matches for input "foo" to
-      // always outrank its own lowly-ranked non-inlineable "bar" match.
+      // SearchProvider's verbatim or inlineable matches for input "foo" (all
+      // allowed to be default match) to always outrank its own lowly-ranked
+      // "bar" matches that shouldn't be the default match.  This only needs
+      // to be enforced when the omnibox will not reorder results to make a
+      // legal default match first.
       default_results_.verbatim_relevance = -1;
       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 (IsTopMatchNotInlinable()) {
-      // Disregard suggested relevances if the top match is not a verbatim match
-      // or inlinable.  For example, input "foo" should not invoke a search for
-      // "bar", which would happen if the "bar" search match outranked all other
-      // matches.
+    if (LacksValidDefaultMatch(omnibox_will_reorder_for_legal_default_match)) {
+      // If the omnibox is not going to reorder results to put a legal default
+      // match at the top, then this provider needs to guarantee that its top
+      // scoring result is a legal default match (i.e., it's either a verbatim
+      // match or inlinable).  For example, input "foo" should not invoke a
+      // search for "bar", which would happen if the "bar" search match
+      // outranked all other matches.  On the other hand, if the omnibox will
+      // reorder matches as necessary to put a legal default match at the top,
+      // all we need to guarantee is 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.)

[Peter Kasting, 2013/08/06 22:56:16]

Nit: Given the comments in LacksValidDefaultMatch(), most or all of this comment
can go away.

[Mark P, 2013/08/07 00:44:31]

I know it's mostly redundant but I think it's useful here to make reading down
the list of constraints easier.  Again, it's mostly for consistency with
already-existing code.

       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
     DCHECK(!IsTopMatchNavigationInKeywordMode());
-    DCHECK(!IsTopMatchScoreTooLow());
+    DCHECK(omnibox_will_reorder_for_legal_default_match ||
+           !IsTopMatchScoreTooLow());

[Peter Kasting, 2013/08/06 22:56:16]

Nit: Indent 4, not even

[Mark P, 2013/08/07 00:44:31]

Done.

     DCHECK(!IsTopMatchSearchWithURLInput());
-    DCHECK(!IsTopMatchNotInlinable());
+    DCHECK(!LacksValidDefaultMatch(
+        omnibox_will_reorder_for_legal_default_match));
   }
 
   UpdateStarredStateOfMatches();
   UpdateDone();
 }
 
 void SearchProvider::AddNavigationResultsToMatches(
     const NavigationResults& navigation_results,
     ACMatches* matches) {
   for (NavigationResults::const_iterator it = navigation_results.begin();
@@ skipping 300 matching lines @@
   const net::FormatUrlTypes format_types =
       net::kFormatUrlOmitAll & ~(trim_http ? 0 : net::kFormatUrlOmitHTTP);
   match.fill_into_edit +=
       AutocompleteInput::FormattedStringWithEquivalentMeaning(navigation.url(),
           net::FormatUrl(navigation.url(), languages, format_types,
                          net::UnescapeRule::SPACES, NULL, NULL,
                          &inline_autocomplete_offset));
   if (!input_.prevent_inline_autocomplete() &&
       (inline_autocomplete_offset != string16::npos)) {
     DCHECK(inline_autocomplete_offset <= match.fill_into_edit.length());
+    match.allowed_to_be_default_match =
+        (inline_autocomplete_offset != string16::npos);

[Peter Kasting, 2013/08/06 22:56:16]

Eh?  The conditional above has already guaranteed this holds.

[Mark P, 2013/08/07 00:44:31]

Replaced with "true" :-).

     match.inline_autocompletion =
         match.fill_into_edit.substr(inline_autocomplete_offset);
   }
 
   match.contents = net::FormatUrl(navigation.url(), languages,
       format_types, net::UnescapeRule::SPACES, NULL, NULL, &match_start);
   // If the first match in the untrimmed string was inside a scheme that we
   // trimmed, look for a subsequent match.
   if (match_start == string16::npos)
     match_start = match.contents.find(input);
@@ skipping 53 matching lines @@
     it->set_relevance(max_query_relevance);
     it->set_relevance_from_server(relevance_from_server);
   }
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when the timer isn't running, there are no suggest queries
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }