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 761 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 TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
+  if (!keyword_input_text_.empty() && keyword_url &&

[Bart N., 2013/01/29 18:50:42]

Most of the time keyword_input_text_ is empty, so it'd be worth to optimize this
code by moving 
const TemplateURL* keyword_url = providers_.GetKeywordProviderURL()
after the if condition.

I know Peter doesn't like assignments in if clauses, but you can nest ifs if you
want.

[Mark P, 2013/01/30 19:46:21]

Done.  (Perhaps I have too much respect for optimizing compilers.)

+      !keyword_url->IsExtensionKeyword()) {

[Bart N., 2013/01/29 18:50:42]

It's not clear to me why we ignore extension keyword here. Mind adding a comment
or pointing to relevant documentation?

[Mark P, 2013/01/30 19:46:21]

Done.

+    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 what_you_typed_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,
@@ skipping 16 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.
+  // Allow additional match(es) for "what you typed" results if present.

[Bart N., 2013/01/29 18:50:42]

If I read the code correctly, we may end up with 5 matches, because
what_you_typed_size can be 2. Is this expected?

[Mark P, 2013/01/30 19:46:21]

Yes.

   const size_t max_total_matches = kMaxMatches + what_you_typed_size;

[Bart N., 2013/01/29 18:50:42]

Technically, this is not "what you typed" matches, because in case of
SEARCH_OTHER_ENGINE case you've added above, we are adding partial query
(keyword is stripped from the original query).

I'd rename this variable to something like verbatim or "trivial_matches_size"
etc.

[Mark P, 2013/01/30 19:46:21]

Done.

   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 16 matching lines @@
   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);
 }
 
 bool SearchProvider::IsTopMatchNotInlinable() const {

[Bart N., 2013/01/29 18:50:42]

Don't you want to modify this condition now to include SEARCH_OTHER_ENGINE as
well? (see http://code.google.com/p/chromium/issues/detail?id=171104#c1).

[Mark P, 2013/01/30 19:46:21]

I was thinking about doing that in another changelist, but I might as well do it
now.

   return 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();
 }
 
 void SearchProvider::UpdateMatches() {
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
@@ skipping 201 matching lines @@
 
     case AutocompleteInput::URL:
       return 850;
 
     default:
       NOTREACHED();
       return 0;
   }
 }
 
+// static
+int SearchProvider::CalculateRelevanceForKeywordVerbatim(
+    AutocompleteInput::Type type,
+    bool prefer_keyword) {
+  if (prefer_keyword)
+    return 1500;
+  return (type == AutocompleteInput::QUERY) ? 1450 : 1100;

[Bart N., 2013/01/29 18:50:42]

Nit: We don't need () around the first expression.

It looks like the only case we can beat keyword verbatim suggestion is when we
don't prefer keyword and input is non-query, e.g. URL.

BTW, under which circumstances can we get keyword verbatim and prefer_keyword is
false?

[Mark P, 2013/01/30 19:46:21]

Done.
Correct.

Though this will change in a follow-up changelist once I turn on suggested
relevance scores for keyword providers.
If you paste text into the omnibox that starts with a keyword, the omnibox will
not go into keyword mode.  This means prefer_keyword is false and yet you'll get
keyword suggestions.

+}
+
 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_))));
 }