Omnibox: Better Enforce Suggest Relevance Constraints in Keyword Mode

Omnibox: Better Enforce Suggest Relevance Constraints in Keyword Mode

TEST=doesn't check.  Also, all the inline completion suggestions look
sane.

BUG=171104

[Mark P, 2013-01-21 20:18:17]

I think this is right thing to do.  This is a bit messy, mainly because
KeywordProvider is separate from SearchProvider.  Sadly, that means I had to
make its CalculateRelevance public.  (Ideally I'd make the function visible only
to SearchProvider.  However, it doesn't seem possible to make private
_functions_ available to a friend, only friend member _variables_.)

Alternative implementation suggestions encouraged.

Sticking with this implementation, I know I made a number of decisions about
wrapping and indentation that you may question.  If you prefer a different
choice, tell me and I'll reformat the code.

thanks,
mark

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/k...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/k...
chrome/browser/autocomplete/keyword_provider.cc:353: int
KeywordProvider::CalculateRelevance(AutocompleteInput::Type type,
This is an exact cut-and-paste.

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/k...
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/k...
chrome/browser/autocomplete/keyword_provider.h:96: // Determines the relevance
for some input, given its type, whether the user
This is an exact cut-and-paste.

[Peter Kasting, 2013-01-21 20:22:57]

msw might be better at reviewing this than me?  My brain doesn't seem to want to
understand the larger picture of what this patch is doing.

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.cc:853:
CalculateRelevanceForVerbatim();
Nit: Indent 4

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.h:256: // testing these constraints
if it's high-ranked than the best result
Nit: high-ranked -> ranked higher?

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.h:258: bool IsTopMatchScoreTooLow(
Nit: We may want to comment what some of these test in the context of the larger
set of constraints we're aiming to enforce.  For example, "high rank search" is
a little unclear.

[Mark P, 2013-01-21 20:57:45]

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.cc:853:
CalculateRelevanceForVerbatim();
On 2013/01/21 20:22:57, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.h:256: // testing these constraints
if it's high-ranked than the best result
On 2013/01/21 20:22:57, Peter Kasting wrote:
> Nit: high-ranked -> ranked higher?

Rephrased for precision and readability.

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider.h:258: bool IsTopMatchScoreTooLow(
On 2013/01/21 20:22:57, Peter Kasting wrote:
> Nit: We may want to comment what some of these test in the context of the
larger
> set of constraints we're aiming to enforce.  For example, "high rank search"
is
> a little unclear.

These are explained in UpdateMatch().  (Note the comment above "See
UpdateMatches() for the use and explanation of these constraints.")

If the explanations there are no enough, please tell me.

[Peter Kasting, 2013-01-21 21:14:26]

I guess I'm just sort of confused now that the keyword provider is factored in,
because the functions that previously did one thing now seem to be checking for
one of a couple possibilities, so it's harder to see how the comments in
UpdateMatch() apply to the keyword cases.

I could be just confused today.

Separately, I wonder if some of the comments in UpdateMatch() should move to the
header file on the various functions.

[Mark P, 2013-01-21 21:31:26]

On 2013/01/21 21:14:26, Peter Kasting wrote:
> I guess I'm just sort of confused now that the keyword provider is factored
in,
> because the functions that previously did one thing now seem to be checking
for
> one of a couple possibilities, so it's harder to see how the comments in
> UpdateMatch() apply to the keyword cases.
> 
> I could be just confused today.
> 
> Separately, I wonder if some of the comments in UpdateMatch() should move to
the
> header file on the various functions.

I'll give it more thought this week, hopefully after Mike's had a chance to pipe
up.

--mark

[Bart N., 2013-01-22 16:38:56]

Mark, please wait for my feedback before submitting.

[Bart N., 2013-01-22 19:02:10]

On 2013/01/22 16:38:56, Bart N. wrote:
> Mark, please wait for my feedback before submitting.
I've looked at this change and I'm deeply concerned design-wise. The
high-coupling/low-cohesion between KP and SP is what worries me the most. While
this is something probably not easy to fix right away, but can we at least keep
it in mind going on forward?

[Mark P, 2013-01-23 16:59:08]

I'm going to re-think this change.  I'm going to try to rewrite it so the
verbatim search for a keyword search provider is generated by SearchProvider,
not KeywordProvider.  I'll leave KeywordProvider to solely provide suggestions
for bare keywords.

I'll update this thread once I know how clean this alternate strategy is.

--mark


On 2013/01/22 19:02:10, Bart N. wrote:
> On 2013/01/22 16:38:56, Bart N. wrote:
> > Mark, please wait for my feedback before submitting.
> I've looked at this change and I'm deeply concerned design-wise. The
> high-coupling/low-cohesion between KP and SP is what worries me the most.
While
> this is something probably not easy to fix right away, but can we at least
keep
> it in mind going on forward?

[Bart N., 2013-01-23 17:07:59]

On 2013/01/23 16:59:08, Mark P wrote:
> I'm going to re-think this change.  I'm going to try to rewrite it so the
> verbatim search for a keyword search provider is generated by SearchProvider,
> not KeywordProvider.  I'll leave KeywordProvider to solely provide suggestions
> for bare keywords.
That sounds like a much better approach! +1.

[msw, 2013-01-23 19:15:19]

Agreed, if you have SearchProvider generate the Keyword verbatim result, you
shouldn't need to adjust the constraint checking. That said, I don't know too
much about KeywordProvider.

Another strategy that might warrant consideration (but could be tough/bad) is:
Move constraint checking to AutocompleteController and check actual top match
violations (not the conditions that might cause violations). Then, let
AutocompleteController regenerate SearchProvider matches without suggested
relevances as needed.

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:859:
((keyword_search_what_you_typed_relevance >
Shouldn't this only kick in when keyword_search_what_you_typed_relevance >
CalculateRelevanceForVerbatim(). Also, can this be re-written a little cleaner
with a const bool or two, or an early return or two?

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:882:
KeywordProvider::CalculateRelevance(input_.type(), true, true,
It seems odd that you're just passing in true for |complete| and
|supports_replacement|, don't you need actual values to get the correct score?

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.h:254: // We pass in
keyword_search_what_you_typed_relevance because the
nit: vertical bars, and i prefer 'verbatim' to 'search_what_you_typed'.

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.h:256: // testing these constraints
if has a higher relevance than the best
nit: "if it"?

[Mark P, 2013-01-25 01:08:14]

On 2013/01/23 19:15:19, msw wrote:
> Agreed, if you have SearchProvider generate the Keyword verbatim result, you
> shouldn't need to adjust the constraint checking. That said, I don't know too
> much about KeywordProvider.

I'm going take this approach.  I think I'l abandon this changelist and start a
new one.

I'll only delete this one from codereview.chromium.org once it looks like the
new approach will work.  Until that time you'll have to ignore this listing on
your "to review" page. :|  Sorry.

--mark

[Mark P, 2013-02-12 00:02:00]

On 2013/01/25 01:08:14, Mark P wrote:
> On 2013/01/23 19:15:19, msw wrote:
> > Agreed, if you have SearchProvider generate the Keyword verbatim result, you
> > shouldn't need to adjust the constraint checking. That said, I don't know
too
> > much about KeywordProvider.
> 
> I'm going take this approach.  I think I'l abandon this changelist and start a
> new one.
> 
> I'll only delete this one from http://codereview.chromium.org once it looks
like the
> new approach will work.  Until that time you'll have to ignore this listing on
> your "to review" page. :|  Sorry.
> 
> --mark

The other approach works.  I'm deleting this changelist.

--mark