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

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

SearchProvider enforces that if the user is in keyword mode, the
only suggestions allowed to be the default match are keyword mode
suggestions, lest they break the user out of keyword mode.

Previously, this constraint was applied with an after-the-fact
correction to allowed_to_be_default_match.  This change sets
allowed_to_be_default_match correctly when the AutocompleteMatches
are created.

All tests pass.  (And yes, this constraint enforcement is tested.
Here you can see the tests that were added when the constraint was put
in place: https://codereview.chromium.org/67693004 .)

BUG=398135
R=msw@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289981

[Mark P, 2014-08-15 18:10:01]

Please take a look.

I love refactoring when there's good test coverage.  It makes me confident that
I've done things right.

--mark

[msw, 2014-08-15 18:32:01]

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:125: NULL,
AutocompleteInput(), from_keyword_provider,
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.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:418: // If there is a keyword
match that is allowed to be the default match,
I'm a little worried that this might no longer be enforced, can you point me at
the test for this behavior? (I'll look after sending comments too)

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:815: bool
SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {
Remove the decl in search_provider.h

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:384: if
((providers_.GetKeywordProviderURL() != NULL) &&
nit: remove extra parens here and below; "!= NULL" may not be needed.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:387: // if necessary so there at
least one match that's allowed to be the
nit: "necessary, so", "there is", and consider: "if necessary, so at least one
match is allowed to be default".

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:711: false, keyword_url != NULL,
&map);
nit: "!= NULL" may not be needed.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:863:
providers_.GetKeywordProviderURL() != NULL, map);
nit: "!= NULL" may not be needed.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:987:
providers_.GetKeywordProviderURL() != NULL, map);
nit: "!= NULL" may not be needed.

[msw, 2014-08-15 18:38:24]

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:418: // If there is a keyword
match that is allowed to be the default match,
On 2014/08/15 18:32:00, msw wrote:
> I'm a little worried that this might no longer be enforced, can you point me
at
> the test for this behavior? (I'll look after sending comments too)

Actually, your changes to CreateSearchSuggestion definitely continue enforcement
of this constraint, so I'm no longer worried about that, but I'm still looking
to ensure that this behavior is tested.

[Mark P, 2014-08-15 18:47:08]

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:125: NULL,
AutocompleteInput(), from_keyword_provider,
On 2014/08/15 18:32:00, msw wrote:
> 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.

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.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:418: // If there is a keyword
match that is allowed to be the default match,
On 2014/08/15 18:32:00, msw wrote:
> I'm a little worried that this might no longer be enforced, can you point me
at
> the test for this behavior? (I'll look after sending comments too)

There is a link in the code review description to the tests that were added /
modified when we put in this constraint.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:815: bool
SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {
On 2014/08/15 18:32:00, msw wrote:
> Remove the decl in search_provider.h

Done.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:384: if
((providers_.GetKeywordProviderURL() != NULL) &&
On 2014/08/15 18:32:00, msw wrote:
> nit: remove extra parens here and below; "!= NULL" may not be needed.

Prefer to keep all of those, which (slightly) seem to be the convention in this
file.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:387: // if necessary so there at
least one match that's allowed to be the
On 2014/08/15 18:32:00, msw wrote:
> nit: "necessary, so", "there is", and consider: "if necessary, so at least one
> match is allowed to be default".

Done.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:711: false, keyword_url != NULL,
&map);
On 2014/08/15 18:32:01, msw wrote:
> nit: "!= NULL" may not be needed.

Acknowledged.

However, I don't want readers to think we're passing in keyword_url to this
function.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:863:
providers_.GetKeywordProviderURL() != NULL, map);
On 2014/08/15 18:32:01, msw wrote:
> nit: "!= NULL" may not be needed.

same reply as above.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:987:
providers_.GetKeywordProviderURL() != NULL, map);
On 2014/08/15 18:32:00, msw wrote:
> nit: "!= NULL" may not be needed.

same reply as above

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

LGTM with updated CreateSearchSuggestion comment and a nit.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/base_search_provider.cc:125: NULL,
AutocompleteInput(), from_keyword_provider,
On 2014/08/15 18:47:07, Mark P wrote:
> On 2014/08/15 18:32:00, msw wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/476263002/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/search_provider.cc:418: // If there is a keyword
match that is allowed to be the default match,
On 2014/08/15 18:47:07, Mark P wrote:
> On 2014/08/15 18:32:00, msw wrote:
> > I'm a little worried that this might no longer be enforced, can you point me
> at
> > the test for this behavior? (I'll look after sending comments too)
> 
> There is a link in the code review description to the tests that were added /
> modified when we put in this constraint.
> 

Indeed, but I was looking for the specific test/case for this. It appears to be
tested sufficiently in SearchProviderTest.KeywordVerbatim.

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:113: // This wrapper uses a
number of default values that may or may not be
This comment is valuable, but belongs integrated with the comment in the header,
or within the body of this function. (perhaps a little bit of both: 1st sentence
to header, modified 2nd sentence to the body here, remove 3rd sentence).

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:234: // and others defined in the
code (that don't need helper functions).
nit: "  // and other constraints enforced without the use of helper functions."

[Mark P, 2014-08-15 19:14:49]

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:113: // This wrapper uses a
number of default values that may or may not be
On 2014/08/15 19:06:06, msw wrote:
> This comment is valuable, but belongs integrated with the comment in the
header,
> or within the body of this function. (perhaps a little bit of both: 1st
sentence
> to header, modified 2nd sentence to the body here, remove 3rd sentence).

Done.

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/476263002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:234: // and others defined in the
code (that don't need helper functions).
On 2014/08/15 19:06:06, msw wrote:
> nit: "  // and other constraints enforced without the use of helper
functions."

Done.

[Mark P, 2014-08-15 19:41:25]

Committed patchset #3 manually as 289981 (presubmit successful).