Omnibox: Create Keyword Verbatim Result in Search Provider

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 765 matching lines @@
 }
 
 void SearchProvider::ConvertResultsToAutocompleteMatches() {
   // Convert all the results to matches and add them to a map, so we can keep
   // the most relevant match for each result.
   MatchMap map;
   const Time no_time;
   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() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
   if (verbatim_relevance > 0) {
     AddMatchToMap(input_.text(), input_.text(), verbatim_relevance,
                   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())
     AddMatchToMap(input_.text() + default_provider_suggestion_.text,
                   input_.text(), verbatim_relevance + 1,
                   AutocompleteMatch::SEARCH_SUGGEST,
                   did_not_accept_default_suggestion, false, &map);
 
   AddHistoryResultsToMap(keyword_history_results_, true,
                          did_not_accept_keyword_suggestion, &map);
@@ skipping 15 matching lines @@
     // |verbatim_relevance| here.
     matches_.push_back(NavigationToMatch(
         NavigationResult(GURL(UTF16ToUTF8(default_provider_suggestion_.text)),
                          string16(),
                          kNonURLVerbatimRelevance + 1),
         false));
   }
   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);
 
   // If the top match is effectively 'verbatim' but exceeds the calculated
   // verbatim relevance, and REQUESTED_URL |input_| has a |desired_tld|
   // (for example ".com" when the CTRL key is pressed for REQUESTED_URL input),
   // promote a URL_WHAT_YOU_TYPED match to the top. Otherwise, these matches can
   // stomp the HistoryURLProvider's similar transient URL_WHAT_YOU_TYPED match,
   // and CTRL+ENTER will invoke the search instead of the expected navigation.
@@ skipping 13 matching lines @@
 }
 
 bool SearchProvider::IsTopMatchScoreTooLow() const {
   return matches_.front().relevance < CalculateRelevanceForVerbatim();
 }
 
 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();
 }
 
 void SearchProvider::UpdateMatches() {
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
   if (!matches_.empty() &&
       (has_suggested_relevance_ || verbatim_relevance_ >= 0)) {
@@ skipping 11 matching lines @@
       // 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_suggest_results_, true);
       ApplyCalculatedSuggestRelevance(&default_suggest_results_, false);
       verbatim_relevance_ = -1;
       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();
     }
     DCHECK(!IsTopMatchScoreTooLow());
     DCHECK(!IsTopMatchHighRankSearchForURL());
     DCHECK(!IsTopMatchNotInlinable());
   }
 
   UpdateStarredStateOfMatches();
   UpdateDone();
@@ skipping 164 matching lines @@
 
     case AutocompleteInput::URL:
       return 850;
 
     default:
       NOTREACHED();
       return 0;
   }
 }
 
+// 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,
     bool prevent_inline_autocomplete) const {
   // The relevance of past searches falls off over time. There are two distinct
   // equations used. If the first equation is used (searches to the primary
   // provider that we want to inline autocomplete), the score starts at 1399 and
   // falls to 1300. If the second equation is used the relevance of a search 15
   // minutes ago is discounted 50 points, while the relevance of a search two
   // weeks ago is discounted 450 points.
@@ skipping 198 matching lines @@
 }
 
 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) &&
            (instant_finalized_ ||
             (!chrome::BrowserInstantController::IsInstantEnabled(profile_) &&
              !chrome::search::IsInstantExtendedAPIEnabled(profile_))));
 }