Omnibox: Better Enforce Suggest Relevance Constraints in Keyword Mode

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 713af35fd7e0e65b091f63f1af5cbb4e3990949e..3397aa56d68565e7b0ff4713215118feb7ca8de8 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -846,19 +846,29 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
     matches_.resize(max_total_matches);
 }
 
-bool SearchProvider::IsTopMatchScoreTooLow() const {
-  return matches_.front().relevance < CalculateRelevanceForVerbatim();
+bool SearchProvider::IsTopMatchScoreTooLow(
+    int keyword_search_what_you_typed_relevance) const {
+  return std::max(matches_.front().relevance,
+                  keyword_search_what_you_typed_relevance) <
+      CalculateRelevanceForVerbatim();
 }
 
-bool SearchProvider::IsTopMatchHighRankSearchForURL() const {
+bool SearchProvider::IsTopMatchHighRankSearchForURL(
+    int keyword_search_what_you_typed_relevance) const {
   return input_.type() == AutocompleteInput::URL &&
-         matches_.front().relevance > CalculateRelevanceForVerbatim() &&
-         (matches_.front().type == AutocompleteMatch::SEARCH_SUGGEST ||
-          matches_.front().type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED);
+         ((keyword_search_what_you_typed_relevance >

[msw, 2013/01/23 19:15:19]

Shouldn't this only kick in when keyword_search_what_you_typed_relevance >
CalculateRelevanceForVerbatim(). Also, can this be re-written a little cleaner
with a const bool or two, or an early return or two?

+           matches_.front().relevance) ||
+          (matches_.front().relevance > CalculateRelevanceForVerbatim() &&
+           (matches_.front().type == AutocompleteMatch::SEARCH_SUGGEST ||
+            matches_.front().type ==
+            AutocompleteMatch::SEARCH_WHAT_YOU_TYPED)));
 }
 
-bool SearchProvider::IsTopMatchNotInlinable() const {
-  return matches_.front().type != AutocompleteMatch::SEARCH_WHAT_YOU_TYPED &&
+bool SearchProvider::IsTopMatchNotInlinable(
+    int keyword_search_what_you_typed_relevance) const {
+  return (keyword_search_what_you_typed_relevance <
+          matches_.front().relevance) &&
+         matches_.front().type != AutocompleteMatch::SEARCH_WHAT_YOU_TYPED &&
          matches_.front().type != AutocompleteMatch::URL_WHAT_YOU_TYPED &&
          matches_.front().inline_autocomplete_offset == string16::npos &&
          matches_.front().fill_into_edit != input_.text();
@@ -867,12 +877,17 @@ bool SearchProvider::IsTopMatchNotInlinable() const {
 void SearchProvider::UpdateMatches() {
   ConvertResultsToAutocompleteMatches();
 
+  const int keyword_search_what_you_typed_relevance =
+      providers_.keyword_provider().empty() ? 0 :
+      KeywordProvider::CalculateRelevance(input_.type(), true, true,

[msw, 2013/01/23 19:15:19]

It seems odd that you're just passing in true for |complete| and
|supports_replacement|, don't you need actual values to get the correct score?

+          input_.prefer_keyword(), input_.allow_exact_keyword_match());
+
   // Check constraints that may be violated by suggested relevances.
   if (!matches_.empty() &&
       (has_suggested_relevance_ || verbatim_relevance_ >= 0)) {
     // These two blocks attempt to repair undesriable behavior by suggested
     // relevances with minimal impact, preserving other suggested relevances.
-    if (IsTopMatchScoreTooLow()) {
+    if (IsTopMatchScoreTooLow(keyword_search_what_you_typed_relevance)) {
       // 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
@@ -880,7 +895,8 @@ void SearchProvider::UpdateMatches() {
       verbatim_relevance_ = -1;
       ConvertResultsToAutocompleteMatches();
     }
-    if (IsTopMatchHighRankSearchForURL()) {
+    if (IsTopMatchHighRankSearchForURL(
+        keyword_search_what_you_typed_relevance)) {
       // 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
@@ -890,7 +906,7 @@ void SearchProvider::UpdateMatches() {
       verbatim_relevance_ = -1;
       ConvertResultsToAutocompleteMatches();
     }
-    if (IsTopMatchNotInlinable()) {
+    if (IsTopMatchNotInlinable(keyword_search_what_you_typed_relevance)) {
       // Disregard suggested relevances if the top match is not SWYT, inlinable,
       // or URL_WHAT_YOU_TYPED (which may be top match regardless of inlining).
       // For example, input "foo" should not invoke a search for "bar", which
@@ -898,9 +914,10 @@ void SearchProvider::UpdateMatches() {
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(!IsTopMatchScoreTooLow());
-    DCHECK(!IsTopMatchHighRankSearchForURL());
-    DCHECK(!IsTopMatchNotInlinable());
+    DCHECK(!IsTopMatchScoreTooLow(keyword_search_what_you_typed_relevance));
+    DCHECK(!IsTopMatchHighRankSearchForURL(
+        keyword_search_what_you_typed_relevance));
+    DCHECK(!IsTopMatchNotInlinable(keyword_search_what_you_typed_relevance));
   }
 
   UpdateStarredStateOfMatches();