Always allow exact match non-substituting keywords.

Always allow exact match non-substituting keywords.

BUG=70807
TEST=See bug report.

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

[Peter Kasting, 2011-01-25 23:20:44]

LGTM.  Is it possible to regression test this?

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
chrome/browser/autocomplete/keyword_provider.cc:373: if
(!allow_exact_keyword_match && supports_replacement)
Nit: This is minor, but I wonder if it might not be clearer to do:

  if (!supports_replacement || (allow_exact_keyword_match && prefer_keyword))
    return 1500;
  return (allow_exact_keyword_match && (type == Autocomplete::QUERY)) ?
      1450 : 1100;

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
chrome/browser/autocomplete/keyword_provider.h:118: bool prefer_keyword,
Nit: Super minor, but it might be better to swap |prefer_keyword| and
|support_replacement| as that groups together the flags about the keyword itself
and then the flags about the current autocomplete system state.

[James Su, 2011-01-26 00:53:31]

A test has been added.

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
chrome/browser/autocomplete/keyword_provider.cc:373: if
(!allow_exact_keyword_match && supports_replacement)
On 2011/01/25 23:20:44, Peter Kasting wrote:
> Nit: This is minor, but I wonder if it might not be clearer to do:
> 
>   if (!supports_replacement || (allow_exact_keyword_match && prefer_keyword))
>     return 1500;
>   return (allow_exact_keyword_match && (type == Autocomplete::QUERY)) ?
>       1450 : 1100;

Done.

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/6334017/diff/1/chrome/browser/autocomplete/key...
chrome/browser/autocomplete/keyword_provider.h:118: bool prefer_keyword,
On 2011/01/25 23:20:44, Peter Kasting wrote:
> Nit: Super minor, but it might be better to swap |prefer_keyword| and
> |support_replacement| as that groups together the flags about the keyword
itself
> and then the flags about the current autocomplete system state.

Done.

[Peter Kasting, 2011-01-26 00:58:07]

LGTM

http://codereview.chromium.org/6334017/diff/6001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc (right):

http://codereview.chromium.org/6334017/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:794: //
http://crbug.com/70807
Nit: Probably no need for this comment, other tests don't have one.

http://codereview.chromium.org/6334017/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:813: //
non-default substituting keyword shouldn't be matched by default.
Nit: non -> Non

http://codereview.chromium.org/6334017/diff/6001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc:836: // We
always allow exact match non-substituting keywords.
Nit: match -> matches for