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 237 matching lines @@
     : AutocompleteProvider(listener, profile,
           AutocompleteProvider::TYPE_SEARCH),
       providers_(TemplateURLServiceFactory::GetForProfile(profile)),
       suggest_results_pending_(0),
       field_trial_triggered_(false),
       field_trial_triggered_in_session_(false),
       omnibox_start_margin_(-1),
       prevent_search_history_inlining_(
           OmniboxFieldTrial::SearchHistoryPreventInlining()),
       disable_search_history_(
-          OmniboxFieldTrial::SearchHistoryDisable()) {
+          OmniboxFieldTrial::SearchHistoryDisable()),
+      omnibox_will_reorder_for_legal_default_match_(
+          OmniboxFieldTrial::InReorderForLegalDefaultMatchGroup()) {
 }
 
 // static
 AutocompleteMatch SearchProvider::CreateSearchSuggestion(
     AutocompleteProvider* autocomplete_provider,
     int relevance,
     AutocompleteMatch::Type type,
     const TemplateURL* template_url,
     const string16& query_string,
     const string16& input_text,
@@ skipping 39 matching lines @@
         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.
     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_autocomplete_offset =
         match.fill_into_edit.length() + 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 535 matching lines @@
 
 bool SearchProvider::IsTopMatchHighRankSearchForURL() const {
   return input_.type() == AutocompleteInput::URL &&
          matches_.front().relevance > CalculateRelevanceForVerbatim() &&
          (matches_.front().type == AutocompleteMatchType::SEARCH_SUGGEST ||
           matches_.front().type ==
               AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED ||
           matches_.front().type == AutocompleteMatchType::SEARCH_OTHER_ENGINE);
 }
 
-bool SearchProvider::IsTopMatchNotInlinable() const {

[msw, 2013/07/18 06:23:57]

This function is almost what I'd expect AutocompleteResult to check to ensure
that matches marked as allowed really are allowed.

[Mark P, 2013/07/21 20:31:05]

Comment moot given the discussion you and Peter had in autocomplete_result.cc

-  // 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_autocomplete_offset == string16::npos &&
-      matches_.front().fill_into_edit != input_.text();
+bool SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {

[msw, 2013/07/18 06:23:57]

Can you combine these two functions? It'd make more sense if the logic to check
only the top match versus checking anywhere within the set of matches were more
tightly related to the omnibox_will_reorder_for_legal_default_match_ flag that
controls this nuanced detail.

[Mark P, 2013/07/21 20:31:05]

I see your desire, but I'd like to keep them separate.  Combining them would
make something check the first if reorder is false and the latter if reorder is
true, but I like having them separate so I can write DCHECKs below that allow me
to assert the later regardless of the former.  Or maybe I could easily collect
statistics on how often the former fails when reorder is true.

[msw, 2013/07/23 21:55:32]

A combined function would check that either (1) or (2) is true:
(1) The top match may be default. <OR> (2) Some match may be default.
(1) implies (2), so if (1) or (2) is true, then (2) must be true.
DCHECK(Combined) will automatically "assert the later regardless of the former".
Combine them :)

[Mark P, 2013/07/26 16:48:13]

Please suggest a good combined name.  I can't think of one, which is why my
default is separate-makes-more-sense.

[msw, 2013/07/26 20:04:49]

LacksValidDefaultMatch or HasValidDefaultMatch seems good.

[Mark P, 2013/07/26 23:55:24]

Used the first to parallel the other constraint testing functions.

+  return !matches_.front().allowed_to_be_default_match;
+}
+
+bool SearchProvider::LacksDefaultMatch() const {
+  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
+       ++it) {
+    if (it->allowed_to_be_default_match)
+      return false;
+  }
+  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()) {
+    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 (IsTopMatchHighRankSearchForURL()) {
       // 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 (omnibox_will_reorder_for_legal_default_match_ ?
+        LacksDefaultMatch() : IsTopMatchNotAllowedToBeDefaultMatch()) {
+      // Disregard suggested relevances if either:
+      // * No matches are allowed to be the default match.  SearchProvider is
+      //   supposed to return a legal default match for any input.  We need
+      //   to make sure it does so.  This constraint should be true always,
+      //   but we only need to check it if
+      //   |omnibox_will_reorder_for_legal_default_match_|.  (If that were
+      //   false, the first part of the test above will suffice.)

[msw, 2013/07/18 06:23:57]

I don't follow "the first part of the test above will suffice", can you be more
explicit? Or move this logic into the combined LacksDefaultMatch() :
IsTopMatchNotAllowedToBeDefaultMatch() I suggested above, and I think it'll be a
lot more self-evident from the code.

[Mark P, 2013/07/21 20:31:05]

I rewrote this comment.  It was confusing because the original comment's
structure was explaining some code that was structured differently in the past
that I revised (to use the ? : construct) per Peter's suggestion.  I didn't
notice the comment became confusing as a result.

+      // * The top match is not allowed to be the default match (i.e., it's not
+      //   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.  We only care
+      //   about this condition if the omnibox itself will not reorder
+      //   matches to put a legal default match on top.
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
     DCHECK(!IsTopMatchNavigationInKeywordMode());
-    DCHECK(!IsTopMatchScoreTooLow());
+    DCHECK(omnibox_will_reorder_for_legal_default_match_ ||
+           !IsTopMatchScoreTooLow());
     DCHECK(!IsTopMatchHighRankSearchForURL());
-    DCHECK(!IsTopMatchNotInlinable());
+    DCHECK(omnibox_will_reorder_for_legal_default_match_ ||
+           !IsTopMatchNotAllowedToBeDefaultMatch());
+    DCHECK(!LacksDefaultMatch());
   }
 
   UpdateStarredStateOfMatches();
   UpdateDone();
 }
 
 void SearchProvider::AddNavigationResultsToMatches(
     const NavigationResults& navigation_results,
     ACMatches* matches) {
   for (NavigationResults::const_iterator it = navigation_results.begin();
@@ skipping 292 matching lines @@
 
   const std::string languages(
       profile_->GetPrefs()->GetString(prefs::kAcceptLanguages));
   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())
+  if (!input_.prevent_inline_autocomplete()) {
+    match.allowed_to_be_default_match =
+        (inline_autocomplete_offset != string16::npos);
     match.inline_autocomplete_offset = inline_autocomplete_offset;
+  }
   DCHECK((match.inline_autocomplete_offset == string16::npos) ||
          (match.inline_autocomplete_offset <= match.fill_into_edit.length()));
 
   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);
   // Safe if |match_start| is npos; also safe if the input is longer than the
@@ skipping 52 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);
 }