Omnibox: Don't Let Users Escape Keyword Mode Accidentally

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 1289 matching lines @@
 ACMatches::const_iterator SearchProvider::FindTopMatch(
     bool autocomplete_result_will_reorder_for_default_match) const {
   if (!autocomplete_result_will_reorder_for_default_match)
     return matches_.begin();
   ACMatches::const_iterator it = matches_.begin();
   while ((it != matches_.end()) && !it->allowed_to_be_default_match)
     ++it;
   return it;
 }
 
-bool SearchProvider::IsTopMatchNavigationInKeywordMode(
+bool SearchProvider::IsTopMatchNavigation(

[msw, 2013/11/20 21:05:03]

Keep this as IsTopMatchNavigationInKeywordMode. I don't see the benefit of
changing the function from something that checks a constraint violation to
something that seems innocuous.

[Mark P, 2013/11/21 21:30:22]

Okay.  I don't feel strongly.

     bool autocomplete_result_will_reorder_for_default_match) const {
   ACMatches::const_iterator first_match =
       FindTopMatch(autocomplete_result_will_reorder_for_default_match);
-  return !providers_.keyword_provider().empty() &&
-      (first_match != matches_.end()) &&
+  return (first_match != matches_.end()) &&
       (first_match->type == AutocompleteMatchType::NAVSUGGEST);
 }
 
+bool SearchProvider::HasKeywordMatchThatCanBeDefault() const {

[msw, 2013/11/20 21:05:03]

Rename this to HasValidDefaultMatchInKeywordMode to parallel
HasValidDefaultMatch.

[Mark P, 2013/11/21 21:30:22]

Revised it to something similar to your suggested name.

+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
+  if (keyword_url == NULL)  // not in keyword mode -> say everything's okay

[msw, 2013/11/20 21:05:03]

nit: Please make this comment more formal.

[Mark P, 2013/11/21 21:30:22]

Done.

+    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::IsTopMatchScoreTooLow(
     bool autocomplete_result_will_reorder_for_default_match) const {
   // In reorder mode, there's no such thing as a score that's too low.
   if (autocomplete_result_will_reorder_for_default_match)
     return false;
 
   // Here we use CalculateRelevanceForVerbatimIgnoringKeywordModeState()
   // rather than CalculateRelevanceForVerbatim() because the latter returns
   // a very low score (250) if keyword mode is active.  This is because
   // when keyword mode is active the user probably wants the keyword matches,
@@ skipping 28 matching lines @@
     if (it->allowed_to_be_default_match)
       return true;
     if (!autocomplete_result_will_reorder_for_default_match)
       return false;
   }
   return false;
 }
 
 void SearchProvider::UpdateMatches() {
   base::TimeTicks update_matches_start_time(base::TimeTicks::Now());
+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   ConvertResultsToAutocompleteMatches();
 
+  // True if the omnibox will reorder matches as necessary to make the first

[msw, 2013/11/20 21:05:03]

nit: you don't check this outside the if block; leave it as it was.

[Mark P, 2013/11/21 21:30:22]

Good point.  That code movement comes from an earlier iteration of this change.

+  // 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());
+
   // 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.
 
-    // True if the omnibox will reorder matches as necessary to make the first
-    // 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 (IsTopMatchNavigationInKeywordMode(
-        omnibox_will_reorder_for_legal_default_match)) {
+    if ((keyword_url != NULL) &&
+        IsTopMatchNavigation(omnibox_will_reorder_for_legal_default_match)) {
       // 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.
+      // would break the user out of keyword mode.  This will only be
+      // triggered in regular (non-reorder) mode; in reorder mode,
+      // navigation matches are marked as not allowed to be the default
+      // match and hence IsTopMatchNavigation() will always return false.
+      DCHECK(!omnibox_will_reorder_for_legal_default_match);
       DemoteKeywordNavigationMatchesPastTopQuery();
       ConvertResultsToAutocompleteMatches();
-      DCHECK(!IsTopMatchNavigationInKeywordMode(
+      DCHECK(!IsTopMatchNavigation(

[msw, 2013/11/20 21:05:03]

nit: remove this DCHECK; it's redundant with the one at line 1452.

[Mark P, 2013/11/21 21:30:22]

I'd prefer to keep it to verify that
DemoteKeywordNavigationMatchesPastTopQuery() and that the constraint wasn't
actually saved by one of the other constraint checks/repairs.

[msw, 2013/11/21 22:11:11]

Good call!

[Mark P, 2013/11/22 01:17:57]

Acknowledged.

           omnibox_will_reorder_for_legal_default_match));
     }
+    if ((keyword_url != NULL) && !HasKeywordMatchThatCanBeDefault()) {

[msw, 2013/11/20 21:05:03]

Like IsTopMatchNavigation[InKeywordMode], I'd prefer this were a self-contained
constraint check, rather than checking for keyword mode outside the function.

[Mark P, 2013/11/21 21:30:22]

Okay.

+      // 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.
+      keyword_results_.verbatim_relevance = -1;
+      ConvertResultsToAutocompleteMatches();
+    }
     if (IsTopMatchScoreTooLow(omnibox_will_reorder_for_legal_default_match)) {
       // 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" (all
       // allowed to be default match) to always outrank its own lowly-ranked
       // "bar" matches that shouldn't be the default match.
       default_results_.verbatim_relevance = -1;
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
@@ skipping 16 matching lines @@
       // 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.)
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(!IsTopMatchNavigationInKeywordMode(
-        omnibox_will_reorder_for_legal_default_match));
+    DCHECK((keyword_url == NULL) ||
+        !IsTopMatchNavigation(omnibox_will_reorder_for_legal_default_match));
+    DCHECK((keyword_url == NULL) || HasKeywordMatchThatCanBeDefault());
     DCHECK(!IsTopMatchScoreTooLow(
         omnibox_will_reorder_for_legal_default_match));
     DCHECK(!IsTopMatchSearchWithURLInput(
         omnibox_will_reorder_for_legal_default_match));
     DCHECK(HasValidDefaultMatch(omnibox_will_reorder_for_legal_default_match));
   }
 
+  if ((keyword_url != NULL) && HasKeywordMatchThatCanBeDefault()) {
+    // If there is a keyword match that is allowed to be the default match,
+    // then prohibit default provider matches from being the defaul match lest

[msw, 2013/11/20 21:05:03]

nit: defaul->default

[Mark P, 2013/11/21 21:30:22]

Done.

+    // 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;
+    }
+  }
+
   base::TimeTicks update_starred_start_time(base::TimeTicks::Now());
   UpdateStarredStateOfMatches();
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateStarredTime",
                       base::TimeTicks::Now() - update_starred_start_time);
   UpdateDone();
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateMatchesTime",
                       base::TimeTicks::Now() - update_matches_start_time);
 }
 
 void SearchProvider::AddNavigationResultsToMatches(
@@ skipping 381 matching lines @@
   // Preserve the forced query '?' prefix in |match.fill_into_edit|.
   // Otherwise, user edits to a suggestion would show non-Search results.
   if (input_.type() == AutocompleteInput::FORCED_QUERY) {
     match.fill_into_edit.insert(0, ASCIIToUTF16("?"));
     if (inline_autocomplete_offset != string16::npos)
       ++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 = true;
+    // A navsuggestion can only be the default match when there is no
+    // keyword provider active, lest it appear first and break the user
+    // out of keyword mode.
+    match.allowed_to_be_default_match =
+        (providers_.GetKeywordProviderURL() == NULL);
     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 54 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);
 }