Changes SearchProvider to set the description of the first consecutive

Changes SearchProvider to set the description of the first consecutive
match sharing the same template url. For example, if the results were
'A B C D' with 'A' and 'B' sharing the same provider and 'C' and 'D'
sharing the same provider, then 'A' and 'C' would get descriptions.

As part of this I'm always setting AutocompleteMatch::template_url
(previously we only set it for keywords).

BUG=88322
TEST=see bug, also covered by unit tests.
R=pkasting@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91741

[sky, 2011-07-07 00:51:19]

I looked at http://codereview.chromium.org/6731036/ . I think we could
accommodate the needs of both by expanding the enum 6731036 is adding.

  -Scott

[Peter Kasting, 2011-07-07 01:16:07]

LGTM

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete.cc:955: for
(ACProviders::const_iterator i(providers_.begin()); i != providers_.end();
Nit: Should we DCHECK that the number of matches does not change ?

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete.h:412: #ifdef UNIT_TEST
Nit: Maybe this should go between Stop() and matches()?

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_popup_model.cc:145: if (default_url &&
default_url->id() == match.template_url->id() &&
Nit: Slightly simpler conditionals by using else to avoid restating first
condition.  This also seems a little clearer to me:

// NO COMMENT HERE
if (default_url && default_url->id() == match.template_url->id()) {
  // This is using the default search provider.  Only claim we had a keyword if
in fact the user manually typed the provider's keyword.
  if (...) {
    ...
  }
} else if (TemplateURL::SupportsReplacement(match.template_url)) {
  ...
}

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_popup_model.cc:146:
autocomplete_controller()->input().text().compare(
Nit: base::StartsWith()?

Do we need to worry about upper/lowercase?  Does this correctly handle
"?google.com" as input that does not have a keyword?

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/se...
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/se...
chrome/browser/autocomplete/search_provider.cc:219: // Set the
description/description_class for the first AutocompleteMatch with
Nit: Maybe better:

// For each group of contiguous matches from the same TemplateURL, show the
provider name as a description on the first match in the group.

[sky, 2011-07-07 15:34:01]

I incorporated all your other comments.

  -Scott

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_popup_model.cc (right):

http://codereview.chromium.org/7321001/diff/14/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_popup_model.cc:146:
autocomplete_controller()->input().text().compare(
On 2011/07/07 01:16:07, Peter Kasting wrote:
> Nit: base::StartsWith()?
> 
> Do we need to worry about upper/lowercase?

As far as I can tell no. keywords are always lowercase, and if we're in keyword
mode the first part of the text is forced to lowercase.

> Does this correctly handle "?google.com" as input that does not have a
keyword?

Yes.