Omnibox: Better Enforce Suggest Relevance Constraints in Keyword Mode (issue 11953016)

[mpea...@chromium.org]

Reviewers: Bart N., msw, Peter Kasting,

Message:

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/keyword_provider.cc
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/keyword_provider.cc#newcode353
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/keyword_provider.h
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/11953016/diff/1/chrome/browser/autocomplete/keyword_provider.h#newcode96
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.

Description:
Omnibox: Better Enforce Suggest Relevance Constraints in Keyword Mode

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

BUG=171104


Please review this at https://codereview.chromium.org/11953016/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/autocomplete/keyword_provider.h
M chrome/browser/autocomplete/keyword_provider.cc
M chrome/browser/autocomplete/search_provider.h
M chrome/browser/autocomplete/search_provider.cc

[pkas...@chromium.org]

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/autocomplete/search_provider.cc
File chrome/browser/autocomplete/search_provider.cc (right):

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

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

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplete/search_provider.h#newcode256
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/autocomplete/search_provider.h#newcode258
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.

https://codereview.chromium.org/11953016/

[pkas...@chromium.org]

https://codereview.chromium.org/11953016/

[mpea...@chromium.org]

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

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplete/search_provider.cc#newcode853
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/autocomplete/search_provider.h
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/11953016/diff/5001/chrome/browser/autocomplete/search_provider.h#newcode256
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/autocomplete/search_provider.h#newcode258
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.

https://codereview.chromium.org/11953016/

[pkas...@chromium.org]

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.

https://codereview.chromium.org/11953016/

[mpea...@chromium.org]

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

--mark


https://codereview.chromium.org/11953016/

[ba...@chromium.org]

Mark, please wait for my feedback before submitting.

https://codereview.chromium.org/11953016/

[ba...@chromium.org]

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?

https://codereview.chromium.org/11953016/

[mpea...@chromium.org]

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

[ba...@chromium.org]

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.

https://codereview.chromium.org/11953016/

[m...@chromium.org]

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/autocomplete/search_provider.cc
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomplete/search_provider.cc#newcode859
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/autocomplete/search_provider.cc#newcode882
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/autocomplete/search_provider.h
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/11953016/diff/11001/chrome/browser/autocomplete/search_provider.h#newcode254
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/autocomplete/search_provider.h#newcode256

chrome/browser/autocomplete/search_provider.h:256: // testing these

constraints if has a higher relevance than the best
nit: "if it"?

https://codereview.chromium.org/11953016/

[mpea...@chromium.org]

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


https://codereview.chromium.org/11953016/