Use Instant suggested match type for Instant temporary text.

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 be43ae17d8b4fdff159ffe2ad5e616802da43d68..2746c9a8dabc3c82131727be3db44ea601a67798 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -124,6 +124,132 @@ const int SearchProvider::kKeywordProviderURLFetcherID = 2;
 // static
 int SearchProvider::kMinimumTimeBetweenSuggestQueriesMs = 100;
 
+// static
+SearchProvider::Providers SearchProvider::GetProviders(
+    Profile* profile,
+    const AutocompleteInput& input,
+    AutocompleteInput* keyword_input) {
+  TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(profile);
+  Providers providers(model);
+
+  const TemplateURL* keyword_provider = NULL;
+  if (keyword_input) {
+    keyword_provider = KeywordProvider::GetSubstitutingTemplateURLForInput(
+        model, keyword_input);
+    if (keyword_provider == NULL)
+      keyword_input->Clear();
+    else if (keyword_input->text().empty())
+      keyword_provider = NULL;  // No text after the keyword.
+  }
+
+  const TemplateURL* default_provider = model->GetDefaultSearchProvider();
+  if (default_provider && !default_provider->SupportsReplacement())
+    default_provider = NULL;
+
+  if (keyword_provider == default_provider)
+    default_provider = NULL;  // No use in querying the same provider twice.
+
+  providers.set(
+      default_provider ? default_provider->keyword() : string16(),
+      keyword_provider ? keyword_provider->keyword() : string16());
+
+  return providers;
+}
+
+// static
+AutocompleteMatch SearchProvider::CreateSearchSuggestion(
+    Profile* profile,
+    AutocompleteProvider* autocomplete_provider,
+    const Providers& search_providers,
+    const AutocompleteInput& input,
+    const string16& query_string,
+    const string16& input_text,
+    int relevance,
+    AutocompleteMatch::Type type,
+    int accepted_suggestion,
+    bool is_keyword) {
+  AutocompleteMatch match(autocomplete_provider, relevance, false, type);
+
+  // Bail out now if we don't actually have a valid provider.
+  match.keyword = is_keyword ?

[Peter Kasting, 2013/04/27 01:21:21]

The only reason this function needs |search_providers| is to choose the
appropriate keyword.  If instead you pass in the keyword, you can avoid taking a
Providers object.

To calculate this keyword, the OmniboxEditModel can just get the DSP's keyword
directly.  This allows you to avoid exposing GetProviders(), which in turn
allows you to basically revert all the Providers-related changes in this file
(and the header).

Sample code to get the DSP's keyword on the OmniboxEditModel side:

  const TemplateURL* default_provider =
      TemplateURLServiceFactory::GetForProfile(profile_)->
          GetDefaultSearchProvider();
  if (default_provider && default_provider->SupportsReplacement()) {
    ... = CreateSearchSuggestion(..., default_provider->keyword(), ...);
    ...
  } else {
    // Can't create a new search match.  Probably in this case you should just
    // use an AutocompleteMatch with an empty |destination_url| so it can't be
    // navigated; this should realistically never happen anyway.
  }

[sreeram, 2013/04/27 01:25:54]

But I do want to be able to support keyword mode. @dougw is working on
supporting keyword mode for Instant Extended, and I want to make it easy for him
when he gets to this part. Wouldn't he then have to redo some of the logic in
GetProviders() to figure out the keyword provider?

[Peter Kasting, 2013/04/27 01:37:48]

I would say, let's worry about that when we get there.  It's not 100% clear to
me that that will end up needing to run through this particular case in the
omnibox code for keyword searches.

If it does, the right answer will probably be simplified version of
GetProviders() that only returns a string16 that's the keyword for the correct
provider.  The full Providers struct, the idea of ensuring the default and
keyword providers aren't the same, etc. is all needed only by the SearchProvider
internals and doesn't ever need to be exposed publicly.

+      search_providers.keyword_provider() : search_providers.default_provider();
+  const TemplateURL* provider_url = match.GetTemplateURL(profile, false);
+  if (provider_url == NULL)
+    return match;
+
+  match.contents.assign(query_string);
+  // We do intra-string highlighting for suggestions - the suggested segment
+  // will be highlighted, e.g. for input_text = "you" the suggestion may be
+  // "youtube", so we'll bold the "tube" section: you*tube*.
+  if (input_text != query_string) {
+    size_t input_position = match.contents.find(input_text);
+    if (input_position == string16::npos) {
+      // The input text is not a substring of the query string, e.g. input
+      // text is "slasdot" and the query string is "slashdot", so we bold the
+      // whole thing.
+      match.contents_class.push_back(
+          ACMatchClassification(0, ACMatchClassification::MATCH));
+    } else {
+      // TODO(beng): ACMatchClassification::MATCH now seems to just mean
+      //             "bold" this. Consider modifying the terminology.
+      // We don't iterate over the string here annotating all matches because
+      // it looks odd to have every occurrence of a substring that may be as
+      // short as a single character highlighted in a query suggestion result,
+      // e.g. for input text "s" and query string "southwest airlines", it
+      // looks odd if both the first and last s are highlighted.
+      if (input_position != 0) {
+        match.contents_class.push_back(
+            ACMatchClassification(0, ACMatchClassification::NONE));
+      }
+      match.contents_class.push_back(
+          ACMatchClassification(input_position, ACMatchClassification::DIM));
+      size_t next_fragment_position = input_position + input_text.length();
+      if (next_fragment_position < query_string.length()) {
+        match.contents_class.push_back(
+            ACMatchClassification(next_fragment_position,
+                                  ACMatchClassification::NONE));
+      }
+    }
+  } else {
+    // Otherwise, we're dealing with the "default search" result which has no
+    // completion.
+    match.contents_class.push_back(
+        ACMatchClassification(0, ACMatchClassification::NONE));
+  }
+
+  // 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() == AutocompleteInput::FORCED_QUERY)
+    match.fill_into_edit.assign(ASCIIToUTF16("?"));
+  if (is_keyword)
+    match.fill_into_edit.append(match.keyword + char16(' '));
+  if (!input.prevent_inline_autocomplete() &&
+      StartsWith(query_string, input_text, false)) {
+    match.inline_autocomplete_offset =
+        match.fill_into_edit.length() + input_text.length();
+  }
+  match.fill_into_edit.append(query_string);
+
+  const TemplateURLRef& search_url = provider_url->url_ref();
+  DCHECK(search_url.SupportsReplacement());
+  match.search_terms_args.reset(
+      new TemplateURLRef::SearchTermsArgs(query_string));
+  match.search_terms_args->original_query = input_text;
+  match.search_terms_args->accepted_suggestion = accepted_suggestion;
+  // This is the destination URL sans assisted query stats.  This must be set
+  // so the AutocompleteController can properly de-dupe; the controller will
+  // eventually overwrite it before it reaches the user.
+  match.destination_url =
+      GURL(search_url.ReplaceSearchTerms(*match.search_terms_args.get()));
+
+  // Search results don't look like URLs.
+  match.transition = is_keyword ?
+      content::PAGE_TRANSITION_KEYWORD : content::PAGE_TRANSITION_GENERATED;
+
+  return match;
+}
+
 SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
                                Profile* profile)
     : AutocompleteProvider(listener, profile,
@@ -241,22 +367,10 @@ void SearchProvider::Start(const AutocompleteInput& input,
   }
 
   keyword_input_ = input;
-  const TemplateURL* keyword_provider =
-      KeywordProvider::GetSubstitutingTemplateURLForInput(model,
-                                                          &keyword_input_);
-  if (keyword_provider == NULL)
-    keyword_input_.Clear();
-  else if (keyword_input_.text().empty())
-    keyword_provider = NULL;
-
-  const TemplateURL* default_provider = model->GetDefaultSearchProvider();
-  if (default_provider && !default_provider->SupportsReplacement())
-    default_provider = NULL;
+  Providers providers = GetProviders(profile_, input, &keyword_input_);
 
-  if (keyword_provider == default_provider)
-    default_provider = NULL;  // No use in querying the same provider twice.
-
-  if (!default_provider && !keyword_provider) {
+  if (providers.default_provider().empty() &&
+      providers.keyword_provider().empty()) {
     // No valid providers.
     Stop(false);
     return;
@@ -264,12 +378,9 @@ void SearchProvider::Start(const AutocompleteInput& input,
 
   // If we're still running an old query but have since changed the query text
   // or the providers, abort the query.
-  string16 default_provider_keyword(default_provider ?
-      default_provider->keyword() : string16());
-  string16 keyword_provider_keyword(keyword_provider ?
-      keyword_provider->keyword() : string16());
   if (!minimal_changes ||
-      !providers_.equal(default_provider_keyword, keyword_provider_keyword)) {
+      !providers_.equal(providers.default_provider(),
+                        providers.keyword_provider())) {
     // If Instant has not come back with a suggestion, adjust the previous
     // suggestion if possible. If |instant_finalized| is true, we are looking
     // for synchronous matches only, so the suggestion is cleared.
@@ -289,12 +400,12 @@ void SearchProvider::Start(const AutocompleteInput& input,
     }
   }
 
-  providers_.set(default_provider_keyword, keyword_provider_keyword);
+  providers_ = providers;
 
   if (input.text().empty()) {
     // User typed "?" alone.  Give them a placeholder result indicating what
     // this syntax does.
-    if (default_provider) {
+    if (!providers_.default_provider().empty()) {
       AutocompleteMatch match;
       match.provider = this;
       match.contents.assign(l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE));
@@ -1326,85 +1437,12 @@ void SearchProvider::AddMatchToMap(const string16& query_string,
                                    int accepted_suggestion,
                                    bool is_keyword,
                                    MatchMap* map) {
-  AutocompleteMatch match(this, relevance, false, type);
-  std::vector<size_t> content_param_offsets;
-  // Bail out now if we don't actually have a valid provider.
-  match.keyword = is_keyword ?
-      providers_.keyword_provider() : providers_.default_provider();
-  const TemplateURL* provider_url = match.GetTemplateURL(profile_, false);
-  if (provider_url == NULL)
+  AutocompleteMatch match = CreateSearchSuggestion(profile_, this, providers_,
+      input_, query_string, input_text, relevance, type, accepted_suggestion,
+      is_keyword);
+  if (!match.destination_url.is_valid())
     return;
 
-  match.contents.assign(query_string);
-  // We do intra-string highlighting for suggestions - the suggested segment
-  // will be highlighted, e.g. for input_text = "you" the suggestion may be
-  // "youtube", so we'll bold the "tube" section: you*tube*.
-  if (input_text != query_string) {
-    size_t input_position = match.contents.find(input_text);
-    if (input_position == string16::npos) {
-      // The input text is not a substring of the query string, e.g. input
-      // text is "slasdot" and the query string is "slashdot", so we bold the
-      // whole thing.
-      match.contents_class.push_back(
-          ACMatchClassification(0, ACMatchClassification::MATCH));
-    } else {
-      // TODO(beng): ACMatchClassification::MATCH now seems to just mean
-      //             "bold" this. Consider modifying the terminology.
-      // We don't iterate over the string here annotating all matches because
-      // it looks odd to have every occurrence of a substring that may be as
-      // short as a single character highlighted in a query suggestion result,
-      // e.g. for input text "s" and query string "southwest airlines", it
-      // looks odd if both the first and last s are highlighted.
-      if (input_position != 0) {
-        match.contents_class.push_back(
-            ACMatchClassification(0, ACMatchClassification::NONE));
-      }
-      match.contents_class.push_back(
-          ACMatchClassification(input_position, ACMatchClassification::DIM));
-      size_t next_fragment_position = input_position + input_text.length();
-      if (next_fragment_position < query_string.length()) {
-        match.contents_class.push_back(
-            ACMatchClassification(next_fragment_position,
-                                  ACMatchClassification::NONE));
-      }
-    }
-  } else {
-    // Otherwise, we're dealing with the "default search" result which has no
-    // completion.
-    match.contents_class.push_back(
-        ACMatchClassification(0, ACMatchClassification::NONE));
-  }
-
-  // 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() == AutocompleteInput::FORCED_QUERY)
-    match.fill_into_edit.assign(ASCIIToUTF16("?"));
-  if (is_keyword)
-    match.fill_into_edit.append(match.keyword + char16(' '));
-  if (!input_.prevent_inline_autocomplete() &&
-      StartsWith(query_string, input_text, false)) {
-    match.inline_autocomplete_offset =
-        match.fill_into_edit.length() + input_text.length();
-  }
-  match.fill_into_edit.append(query_string);
-
-  const TemplateURLRef& search_url = provider_url->url_ref();
-  DCHECK(search_url.SupportsReplacement());
-  match.search_terms_args.reset(
-      new TemplateURLRef::SearchTermsArgs(query_string));
-  match.search_terms_args->original_query = input_text;
-  match.search_terms_args->accepted_suggestion = accepted_suggestion;
-  // This is the destination URL sans assisted query stats.  This must be set
-  // so the AutocompleteController can properly de-dupe; the controller will
-  // eventually overwrite it before it reaches the user.
-  match.destination_url =
-      GURL(search_url.ReplaceSearchTerms(*match.search_terms_args.get()));
-
-  // Search results don't look like URLs.
-  match.transition = is_keyword ?
-      content::PAGE_TRANSITION_KEYWORD : content::PAGE_TRANSITION_GENERATED;
-
   // Try to add |match| to |map|.  If a match for |query_string| is already in
   // |map|, replace it if |match| is more relevant.
   // NOTE: Keep this ToLower() call in sync with url_database.cc.