Omnibox: Create Keyword Verbatim Result in Search Provider

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 f1e02da9162e21b9aee5ff4151960043caa7cd30..49cc988a947d85868f33dae3e1b82ed147c39e82 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -783,7 +783,6 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   int did_not_accept_keyword_suggestion = keyword_suggest_results_.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
-  // Keyword what you typed results are handled by the KeywordProvider.
 
   int verbatim_relevance = GetVerbatimRelevance();
   int did_not_accept_default_suggestion = default_suggest_results_.empty() ?
@@ -794,7 +793,23 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                   AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
                   did_not_accept_default_suggestion, false, &map);
   }
-  const size_t what_you_typed_size = map.size();
+  if (!keyword_input_text_.empty()) {
+    const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
+    // We only create the verbatim search query match for a keyword
+    // if it's not an extension keyword.  Extension keywords are handled
+    // in KeywordProvider::Start().  (Extensions are complicated...)
+    // Note: in this provider, SEARCH_OTHER_ENGINE must correspond
+    // to the keyword verbatim search query.  Do not create other matches
+    // of type SEARCH_OTHER_ENGINE.
+    if (keyword_url && !keyword_url->IsExtensionKeyword()) {
+      AddMatchToMap(keyword_input_text_, keyword_input_text_,
+                    CalculateRelevanceForKeywordVerbatim(
+                        input_.type(), input_.prefer_keyword()),
+                    AutocompleteMatch::SEARCH_OTHER_ENGINE,
+                    did_not_accept_keyword_suggestion, true, &map);
+    }
+  }
+  const size_t verbatim_matches_size = map.size();
   if (!default_provider_suggestion_.text.empty() &&
       default_provider_suggestion_.type == INSTANT_SUGGESTION_SEARCH &&
       !input_.prevent_inline_autocomplete())
@@ -830,8 +845,8 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   AddNavigationResultsToMatches(keyword_navigation_results_, true);
   AddNavigationResultsToMatches(default_navigation_results_, false);
 
-  // Allow an additional match for "what you typed" if it's present.
-  const size_t max_total_matches = kMaxMatches + what_you_typed_size;
+  // Allow additional match(es) for verbatim results if present.
+  const size_t max_total_matches = kMaxMatches + verbatim_matches_size;
   std::partial_sort(matches_.begin(),
       matches_.begin() + std::min(max_total_matches, matches_.size()),
       matches_.end(), &AutocompleteMatch::MoreRelevant);
@@ -865,12 +880,17 @@ bool SearchProvider::IsTopMatchHighRankSearchForURL() const {
   return input_.type() == AutocompleteInput::URL &&
          matches_.front().relevance > CalculateRelevanceForVerbatim() &&
          (matches_.front().type == AutocompleteMatch::SEARCH_SUGGEST ||
-          matches_.front().type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED);
+          matches_.front().type == AutocompleteMatch::SEARCH_WHAT_YOU_TYPED ||
+          matches_.front().type == AutocompleteMatch::SEARCH_OTHER_ENGINE);
 }
 
 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 != AutocompleteMatch::SEARCH_WHAT_YOU_TYPED &&
          matches_.front().type != AutocompleteMatch::URL_WHAT_YOU_TYPED &&
+         matches_.front().type != AutocompleteMatch::SEARCH_OTHER_ENGINE &&
          matches_.front().inline_autocomplete_offset == string16::npos &&
          matches_.front().fill_into_edit != input_.text();
 }
@@ -902,10 +922,11 @@ void SearchProvider::UpdateMatches() {
       ConvertResultsToAutocompleteMatches();
     }
     if (IsTopMatchNotInlinable()) {
-      // 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
-      // would happen if the "bar" search match outranked all other matches.
+      // Disregard suggested relevances if the top match is not a verbatim
+      // match, 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 would happen if the "bar" search
+      // match outranked all other matches.
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
@@ -1090,6 +1111,22 @@ int SearchProvider::CalculateRelevanceForVerbatim() const {
   }
 }
 
+// static
+int SearchProvider::CalculateRelevanceForKeywordVerbatim(
+    AutocompleteInput::Type type,
+    bool prefer_keyword) {
+  // Perhaps this should be kept similar to

[Peter Kasting, 2013/02/05 23:17:48]

I'm not a huge fan of the "perhaps" comments here and in the keyword provider. 
It's not clear to a reader whether they should or shouldn't be identical, how to
tell, if some action will or should be taken, etc.

I feel like, make a judgement call, then if necessary explain why you did it the
way you did and/or how to tell (or what to do) if problems arise.

[Mark P, 2013/02/06 01:31:59]

I don't like these comments either.  I think they add more confusion than they
remove.  Originally these functions were both uncommented; I added the comments
about how they're pretty similar but don't really have to be upon Mike's
request.  I can remove the comments if you prefer.

[Peter Kasting, 2013/02/06 02:01:41]

My hope was either:

* Make the functions match and write a comment as to why they need to be kept in
sync
* Leave the functions not matching and write a comment as to why you think not
matching is correct, or how to tell if you're wrong

It's OK to make the wrong judgment call.  I'd just rather, if there is potential
confusion here, someone _made_ a judgment call and documented it.  The comments
as-is and the functions uncommented are about the same in terms of future reader
confusion.

[Mark P, 2013/02/06 04:31:14]

My judgment call is that they don't need to match.  I can add the following
comment (if you prefer it to no comment at all and prefer it to the wishy-washy
perhaps comment):
This function bears some resemblance to KeywordProvider::CalculateRelevance()
but there is no need to keep them looking like each other.  They're independent.
 Reminder: if you change this function, you should evaluate the change as with
any other omnibox ranking change.
BTW, it's hard to explain why they don't need to match.  That's like asking me
to comment in the SearchProvider code why this relevance function doesn't look
anything like the relevance function in the HistoryURL code.  They're
independent.

What do you think of the proposed comment?

[Peter Kasting, 2013/02/06 18:47:34]

Honestly, looking at these, it sure looks to me like they're supposed to match. 
The code here looks like what you get if you take the keyword provider code and
substitute in the conditions that must be true here -- although I'm not sure
about allow_exact_keyword_match; maybe we should be checking that here?  Do we
risk triggering keyword mode when we shouldn't, with this patch?

[Mark P, 2013/02/06 18:54:11]

You are correct; that's how I made the function.  (This change is supposed to be
user-invisible, so I had to keep the scoring functions the same.)  But there is
no reason they cannot diverge in the future.  There's no reason we need to score
verbatim queries for regular keyword search engines the same as verbatim queries
for extension-based keyword search engines.  I can easily imagine wanting to
make one more important than the other.
We don't need to check it here.  We wouldn't have a keyword provider is we don't
allow_exact_keyword match.  This is enforced here:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/autocomplete...
I don't believe so.

[Peter Kasting, 2013/02/06 19:00:09]

That seems like the right comment, then:

// This function is responsible for scoring verbatim matches for non-extension
keywords.  KeywordProvider::CalculateRelevance() scores verbatim matches for
extension keywords, as well as non-verbatim keyword matches.  These are
currently in sync, but there's no reason we couldn't decide in the future to
score verbatim matches differently for extension and non-extension keywords.  If
you make such a change, however, you should update this comment to describe it,
so it's clear why the functions diverge.

[Mark P, 2013/02/06 19:50:22]

That comment sounds good to me.  I copied and pasted it with a minor edit for
clarity (in my opinion).

Also put an analogous comment in keyword_provider.cc.

+  // KeywordProvider::CalculateRelevance().  That function calculates,
+  // among other things, the verbatim query score for search keywords
+  // but only extension-based ones.  It would be a bit odd if the score
+  // for a verbatim query for an extension keyword and for a
+  // non-extension keyword differed dramatically (though no immediate
+  // harm would come from it).
+  if (prefer_keyword)
+    return 1500;
+  return type == AutocompleteInput::QUERY ? 1450 : 1100;

[Peter Kasting, 2013/02/05 23:17:48]

Nit: I'd put parens around binary subexpression for clarity

[Mark P, 2013/02/06 01:31:59]

Done.

+}
+
 int SearchProvider::CalculateRelevanceForHistory(
     const Time& time,
     bool is_keyword,