Omnibox - Search Provider - Cleanup Keyword Mode's Legal Matches

chrome/browser/autocomplete/base_search_provider.cc

 // Copyright 2014 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/base_search_provider.h"
 
 #include "base/i18n/case_conversion.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ skipping 104 matching lines @@
 }
 
 // static
 AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     const base::string16& suggestion,
     AutocompleteMatchType::Type type,
     bool from_keyword_provider,
     const TemplateURL* template_url,
     const SearchTermsData& search_terms_data) {
   return CreateSearchSuggestion(
-      NULL, AutocompleteInput(), SearchSuggestionParser::SuggestResult(
+      NULL, AutocompleteInput(), from_keyword_provider,

[msw, 2014/08/15 18:32:00]

You're passing |from_keyword_provider| as |in_keyword_mode|, which seems wrong
(saying that we're in keyword mode iff the suggestion is from a keyword
provider, etc.). Please fix this, clarify the parameter names, or clarify the
reasoning for such behavior with a comment.

[Mark P, 2014/08/15 18:47:07]

I hoped the warning by this function's declaration in the header would be
enough.
// Returns a simpler AutocompleteMatch suitable for persistence like in
// ShortcutsDatabase.
// NOTE: Use with care. Most likely you want the other CreateSearchSuggestion
// with protected access.

Put a similar comment in the code.

[msw, 2014/08/15 19:06:06]

I saw the warning in the header, it's not bad, but I like your additional
specificity about it's use of default values; thanks.

+      SearchSuggestionParser::SuggestResult(
           suggestion, type, suggestion, base::string16(), base::string16(),
           base::string16(), base::string16(), std::string(), std::string(),
           from_keyword_provider, 0, false, false, base::string16()),
       template_url, search_terms_data, 0, false);
 }
 
 void BaseSearchProvider::Stop(bool clear_cached_results) {
   StopSuggest();
   done_ = true;
 
@@ skipping 72 matching lines @@
     match->RecordAdditionalInfo(BaseSearchProvider::kDeletionUrlKey,
         url.spec());
     match->deletable = true;
   }
 }
 
 // static
 AutocompleteMatch BaseSearchProvider::CreateSearchSuggestion(
     AutocompleteProvider* autocomplete_provider,
     const AutocompleteInput& input,
+    const bool in_keyword_mode,
     const SearchSuggestionParser::SuggestResult& suggestion,
     const TemplateURL* template_url,
     const SearchTermsData& search_terms_data,
     int accepted_suggestion,
     bool append_extra_query_params) {
   AutocompleteMatch match(autocomplete_provider, suggestion.relevance(), false,
                           suggestion.type());
 
   if (!template_url)
     return match;
@@ skipping 12 matching lines @@
         kACMatchPropertyContentsStartIndex,
         static_cast<int>(
             suggestion.suggestion().length() - match.contents.length()));
   }
 
   if (!suggestion.annotation().empty())
     match.description = suggestion.annotation();
 
   // suggestion.match_contents() should have already been collapsed.
   match.allowed_to_be_default_match =
+      (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       (base::CollapseWhitespace(input.text(), false) ==
        suggestion.match_contents());
 
   // When the user forced a query, we need to make sure all the fill_into_edit
   // values preserve that property.  Otherwise, if the user starts editing a
   // suggestion, non-Search results will suddenly appear.
   if (input.type() == metrics::OmniboxInputType::FORCED_QUERY)
     match.fill_into_edit.assign(base::ASCIIToUTF16("?"));
   if (suggestion.from_keyword_provider())
     match.fill_into_edit.append(match.keyword + base::char16(' '));
   if (!input.prevent_inline_autocomplete() &&
+      (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       StartsWith(suggestion.suggestion(), input.text(), false)) {
     match.inline_autocompletion =
         suggestion.suggestion().substr(input.text().length());
     match.allowed_to_be_default_match = true;
   }
   match.fill_into_edit.append(suggestion.suggestion());
 
   const TemplateURLRef& search_url = template_url->url_ref();
   DCHECK(search_url.SupportsReplacement(search_terms_data));
   match.search_terms_args.reset(
@@ skipping 130 matching lines @@
   UpdateMatches();
   if (done_ || results_updated)
     listener_->OnProviderUpdate(results_updated);
 }
 
 void BaseSearchProvider::AddMatchToMap(
     const SearchSuggestionParser::SuggestResult& result,
     const std::string& metadata,
     int accepted_suggestion,
     bool mark_as_deletable,
+    bool in_keyword_mode,
     MatchMap* map) {
   AutocompleteMatch match = CreateSearchSuggestion(
-      this, GetInput(result.from_keyword_provider()), result,
+      this, GetInput(result.from_keyword_provider()), in_keyword_mode, result,
       GetTemplateURL(result.from_keyword_provider()),
       UIThreadSearchTermsData(profile_), accepted_suggestion,
       ShouldAppendExtraParams(result));
   if (!match.destination_url.is_valid())
     return;
   match.search_terms_args->bookmark_bar_pinned =
       profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
   match.RecordAdditionalInfo(kRelevanceFromServerKey,
                              result.relevance_from_server() ? kTrue : kFalse);
   match.RecordAdditionalInfo(kShouldPrefetchKey,
@@ skipping 112 matching lines @@
 }
 
 void BaseSearchProvider::OnDeletionComplete(
     bool success, SuggestionDeletionHandler* handler) {
   RecordDeletionResult(success);
   SuggestionDeletionHandlers::iterator it = std::find(
       deletion_handlers_.begin(), deletion_handlers_.end(), handler);
   DCHECK(it != deletion_handlers_.end());
   deletion_handlers_.erase(it);
 }