Omnibox: Don't Let Users Escape Keyword Mode Accidentally

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 2e79b08f3841cd8c1562bf669ea5ce13ea10a12b..354026a51eb808869b6f82f03ffe2debf1f09d6a 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -1307,15 +1307,33 @@ ACMatches::const_iterator SearchProvider::FindTopMatch(
   return it;
 }
 
-bool SearchProvider::IsTopMatchNavigationInKeywordMode(
+bool SearchProvider::IsTopMatchNavigation(
     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(
+    bool autocomplete_result_will_reorder_for_default_match) const {
+  // In regular (non-reorder) mode we don't care about the
+  // |allowed_to_be_default_match| state, so return true saying
+  // everything is fine.
+  if (!autocomplete_result_will_reorder_for_default_match)
+    return true;

[Peter Kasting, 2013/11/19 02:58:24]

It seems kinda wrong to do this here, because it effectively makes the function
lie.  Why shouldn't the caller check this variable and not call the function to
begin with instead?

[Mark P, 2013/11/20 19:05:52]

Good point.  It does seem wrong.  Fixed.

+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
+  if (keyword_url == NULL)  // not in keyword mode -> say everything's okay
+    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.
@@ -1364,8 +1382,15 @@ bool SearchProvider::HasValidDefaultMatch(
 
 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
+  // 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() ||
@@ -1373,26 +1398,27 @@ void SearchProvider::UpdateMatches() {
     // 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.  We only need to do this
+      // correction in regular (non-reorder) mode; in reorder mode, we rely
+      // on the fact that navigation matches are marked as not allowed to be
+      // the default match.

[Peter Kasting, 2013/11/19 02:58:24]

This comment doesn't seem to match the code?  It looks like we make the
correction in reorder mode too (and check for it below in both modes).

[Mark P, 2013/11/20 19:05:52]

Clarified comment and added DCHECK.

       DemoteKeywordNavigationMatchesPastTopQuery();
       ConvertResultsToAutocompleteMatches();
-      DCHECK(!IsTopMatchNavigationInKeywordMode(
+      DCHECK(!IsTopMatchNavigation(
           omnibox_will_reorder_for_legal_default_match));
     }
+    if ((keyword_url != NULL) && !HasKeywordMatchThatCanBeDefault(
+        omnibox_will_reorder_for_legal_default_match)) {
+      // 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
@@ -1429,7 +1455,9 @@ void SearchProvider::UpdateMatches() {
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(!IsTopMatchNavigationInKeywordMode(
+    DCHECK((keyword_url == NULL) ||
+        !IsTopMatchNavigation(omnibox_will_reorder_for_legal_default_match));
+    DCHECK((keyword_url == NULL) || HasKeywordMatchThatCanBeDefault(
         omnibox_will_reorder_for_legal_default_match));
     DCHECK(!IsTopMatchScoreTooLow(
         omnibox_will_reorder_for_legal_default_match));
@@ -1438,6 +1466,26 @@ void SearchProvider::UpdateMatches() {
     DCHECK(HasValidDefaultMatch(omnibox_will_reorder_for_legal_default_match));
   }
 
+  if ((keyword_url != NULL) && HasKeywordMatchThatCanBeDefault(
+      omnibox_will_reorder_for_legal_default_match)) {
+    // 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
+    // such matches cause the user to break out of keyword mode.
+    for (ACMatches::const_iterator keyword_match_it = matches_.begin();
+         keyword_match_it != matches_.end(); ++keyword_match_it) {
+      if (keyword_match_it->allowed_to_be_default_match &&
+          (keyword_match_it->keyword == keyword_url->keyword())) {

[Peter Kasting, 2013/11/19 02:58:24]

Nit: You know, if we had a functor that could compare match keywords against a
particular keyword, we could make this block be simpler by way of things like
std::find_if().

[Mark P, 2013/11/20 19:05:52]

I realized part of this block is redundant with
HasKeywordMatchThatCanBeDefault() so I simplified it.  Now the find_if doesn't
seem necessary here.  I thought about adding a functor for
HasKeywordMatchThatCanBeDefault() but it seemed heavy-handed--it would only get
used once, wouldn't save any net lines of code, and I'm not sure the net result
will be more readable.

+        // Found one such match.
+        for (ACMatches::iterator it = matches_.begin(); it != matches_.end();
+             ++it) {
+          if (it->keyword != keyword_url->keyword())
+            it->allowed_to_be_default_match = false;
+        }
+        break;
+      }
+    }
+  }
+
   base::TimeTicks update_starred_start_time(base::TimeTicks::Now());
   UpdateStarredStateOfMatches();
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.UpdateStarredTime",
@@ -1839,7 +1887,11 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
   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 breaks the user

[Peter Kasting, 2013/11/19 02:58:24]

Nit: breaks -> break

Also probably add a comma after "active"

[Mark P, 2013/11/20 19:05:52]

Agree with both.  Done.

+    // out of keyword mode.
+    match.allowed_to_be_default_match =
+        (providers_.GetKeywordProviderURL() == NULL);
     match.inline_autocompletion =
         match.fill_into_edit.substr(inline_autocomplete_offset);
   }