Makes the omnibox show past searches and suggestions for keywords....

Makes the omnibox show past searches and suggestions for keywords.
We talked about primary and secondary, but after doing it all I felt
keyword and default better portrayed what is going on.

After trying this out I think we need to tune relevancy. But that
can be done later

BUG=3636
TEST=make sure omnibox isn't broken.


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

[Peter Kasting, 2009-04-13 14:06:46]

This is generally good stuff.  Two substantive change suggestions:

* Rather than have the default engine's suggestions/history come in just below
the keyword's, I'd make an entirely new score section for them noticeably lower
(since I think they're of significantly worse quality).  This should actually
make your code a little easier because instead of having max_relevancy outparams
you can simply have a different set of base values in the relevancy calculation
function, which you toggle between based on whether {we have two providers and
this isn't the default}.  (Note that I still think using primary/secondary
engines instead of keyword/default would have made this less confusing, but the
difference is not large.)

* I'm concerned with the function in the keyword provider that selects the right
keyword for the input string.  It seems like it should be more generic, along
the lines of issue 9732 comment 9?

http://codereview.chromium.org/66073/diff/10/1002
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/66073/diff/10/1002#newcode133
Line 133: matches_.push_back(CreateAutocompleteMatch(model, keyword, input,
Do we need to do this anymore?  Seems like the search provider should cover
this.

http://codereview.chromium.org/66073/diff/10/1003
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/66073/diff/10/1003#newcode74
Line 74: // is valid and has a keyword.
Nit: Might be nice to have a sentence or two about how this is substantially
different from SplitKeywordFromInput() below.

http://codereview.chromium.org/66073/diff/10/1004
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/66073/diff/10/1004#newcode33
Line 33: bool IsValidTemplateURL(const TemplateURL* turl) {
Nit: Seems like multiple files use something like this.  Should we just put a
static utility function in template_url.h or something?

http://codereview.chromium.org/66073/diff/10/1004#newcode228
Line 228: // correctly set in Run.
Nit: Add comment on why we need to do this (so
ConvertResultsToAutocompleteMatches() works right).

http://codereview.chromium.org/66073/diff/10/1004#newcode321
Line 321: providers_.keyword_provider().id() ==
Nit: put () around binary operator condition (file style)

http://codereview.chromium.org/66073/diff/10/1004#newcode322
Line 322: history_request_consumer_.GetClientData(history_service, handle)) {
Nit: no {}

http://codereview.chromium.org/66073/diff/10/1004#newcode408
Line 408: // the keyword provider.
I think we should just go ahead and allow navigation results from the keyword
provider for now.  Practically speaking only Google returns them so only one of
your providers will be returning navigation results anyway.  (This will require
fixing the TODO you added at file bottom.)

http://codereview.chromium.org/66073/diff/10/1004#newcode522
Line 522: is_keyword ? 0 :static_cast<int>(default_suggest_results_.size());
Nit: space after :

http://codereview.chromium.org/66073/diff/10/1005
File chrome/browser/autocomplete/search_provider.h (right):

http://codereview.chromium.org/66073/diff/10/1005#newcode67
Line 67: // . The default provider. This corresponds to the users default search
Nit: users -> user's

http://codereview.chromium.org/66073/diff/10/1005#newcode71
Line 71: class Providers {
Nit: Maybe it's just me, but I'd prefer to have the declarations here but the
definitions be in the .cc file.

http://codereview.chromium.org/66073/diff/10/1005#newcode227
Line 227: int CalculateRelevanceForSuggestion(const SuggestResults&
suggest_results,
Please comment what the two new params mean.

[sky, 2009-04-13 23:07:24]

New snapshot uploaded.

http://codereview.chromium.org/66073/diff/10/1002
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/66073/diff/10/1002#newcode133
Line 133: matches_.push_back(CreateAutocompleteMatch(model, keyword, input,
On 2009/04/13 14:06:46, pkasting wrote:
> Do we need to do this anymore?  Seems like the search provider should cover
> this.

The SearchProvider could certainly do this, but as his code is here, I didn't
try and move it over.

http://codereview.chromium.org/66073/diff/10/1004
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/66073/diff/10/1004#newcode33
Line 33: bool IsValidTemplateURL(const TemplateURL* turl) {
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: Seems like multiple files use something like this.  Should we just put a
> static utility function in template_url.h or something?

Done.

http://codereview.chromium.org/66073/diff/10/1004#newcode228
Line 228: // correctly set in Run.
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: Add comment on why we need to do this (so
> ConvertResultsToAutocompleteMatches() works right).

Done.

http://codereview.chromium.org/66073/diff/10/1004#newcode321
Line 321: providers_.keyword_provider().id() ==
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: put () around binary operator condition (file style)

Done.

http://codereview.chromium.org/66073/diff/10/1004#newcode322
Line 322: history_request_consumer_.GetClientData(history_service, handle)) {
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: no {}

We differ on the interpretation of this rule. My understanding is I need {} here
as the clause spans multiple lines.

http://codereview.chromium.org/66073/diff/10/1004#newcode522
Line 522: is_keyword ? 0 :static_cast<int>(default_suggest_results_.size());
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: space after :

Done.

http://codereview.chromium.org/66073/diff/10/1005
File chrome/browser/autocomplete/search_provider.h (right):

http://codereview.chromium.org/66073/diff/10/1005#newcode71
Line 71: class Providers {
On 2009/04/13 14:06:46, pkasting wrote:
> Nit: Maybe it's just me, but I'd prefer to have the declarations here but the
> definitions be in the .cc file.

They're all cheap operations. I thought with cheap operations we put the
definition in the header too? The 'set' method is the longest one. I can move
that if you feel strongly.

[Peter Kasting, 2009-04-13 23:42:24]

http://codereview.chromium.org/66073/diff/10/1002
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/66073/diff/10/1002#newcode133
Line 133: matches_.push_back(CreateAutocompleteMatch(model, keyword, input,
On 2009/04/13 23:07:24, sky wrote:
> On 2009/04/13 14:06:46, pkasting wrote:
> > Do we need to do this anymore?  Seems like the search provider should cover
> > this.
> 
> The SearchProvider could certainly do this, but as his code is here, I didn't
> try and move it over.

I didn't mean "shouldn't it do this", I meant "doesn't it do this".  I thought
we got this for free with your new code.  But now that I look I see you
explicitly check for the keyword provider and don't create such a match. 
Hmmm... seems like it'd be better to just go ahead and do the same thing for
both providers there instead of what is basically a (very simple) special case.

http://codereview.chromium.org/66073/diff/10/1005
File chrome/browser/autocomplete/search_provider.h (right):

http://codereview.chromium.org/66073/diff/10/1005#newcode71
Line 71: class Providers {
On 2009/04/13 23:07:24, sky wrote:
> On 2009/04/13 14:06:46, pkasting wrote:
> > Nit: Maybe it's just me, but I'd prefer to have the declarations here but
the
> > definitions be in the .cc file.
> 
> They're all cheap operations. I thought with cheap operations we put the
> definition in the header too? The 'set' method is the longest one. I can move
> that if you feel strongly.

Yeah, sorry, I didn't look at the bodies closely enough.  I'd capitalize Set()
and move its body out, the rest are clearly fine.

http://codereview.chromium.org/66073/diff/2002/2007
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/66073/diff/2002/2007#newcode54
Line 54: // Search Primary Keyword (navigational suggestion)                   
|  800++
Nit: Keyword -> Provider (two places)

http://codereview.chromium.org/66073/diff/2002/2007#newcode138
Line 138: // keyword, then the primary provider corresponds to the default
provider.
I was thinking about this some more and wondering if maybe we want per-category
relevance pushdown.  What I mean by that is, instead of "if two providers,
default search providers' results all get scored low" we have something more
like "if we have keyword suggestions, default provider's suggestions get scored
low" and the same for the other types.  There are two reasons for this idea:

* Today, most of the keywords people use will not have suggestions, and thus
potentially useful results from the main engine will get pushed really low.  For
example, "amazon.com gi" might get "Search google for amazon.com gift
certificates" as a suggest result.

* In the future, as the keyword provider's heuristic function gets more
aggressive about returning keywords for UNKNOWN input (i.e. input that doesn't
have a second token to substitute in), "search for what you typed" results will
basically disappear any time the user has typed a word that matches a keyword. 
I already get pressure on this angle from Ben and I don't want to make things
worse here.

If this isn't clear feel free to ask me.

http://codereview.chromium.org/66073/diff/2002/2004
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/66073/diff/2002/2004#newcode75
Line 75: // extract the keyword and remaining string.
Nit: The other important difference is the insertion of the
"CleanUserInputKeyword" functionality... it still sounds like these two
functions are almost equivalent from just this comment.

http://codereview.chromium.org/66073/diff/2002/2005
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/66073/diff/2002/2005#newcode407
Line 407: is_keyword ? keyword_navigation_results_ :
Nit: If you break the first line after "?" you can fit it on two lines and have
it be just as readable :D

http://codereview.chromium.org/66073/diff/2002/2005#newcode417
Line 417: if (result_url.is_valid()) {
Nit: no {}

http://codereview.chromium.org/66073/diff/2002/2005#newcode559
Line 559: // Don't let scores go below 0 (or 275 if we have both a keyword and
default
I don't think we should force keyword provider results to be above default
provider results.  There's no reason to ensure that artificial separation if the
keyword results are really that poor.

http://codereview.chromium.org/66073/diff/2002/2005#newcode623
Line 623: DCHECK(
Nit: Maybe this would be better as something like:
DCHECK(suggestion_number < (is_keyword ?
    keyword_navigation_results_.size() :
    default_navigation_results_.size()));

http://codereview.chromium.org/66073/diff/2002/2005#newcode730
Line 730: // TODO(kochi): http://b/1170574 if we end up allowing navigation
results from
I think this comment can be removed?

http://codereview.chromium.org/66073/diff/2002/2006
File chrome/browser/autocomplete/search_provider.h (right):

http://codereview.chromium.org/66073/diff/2002/2006#newcode128
Line 128: return from_keyword_provider || (!from_keyword_provider &&
Nit: Just
return from_keyword_provider || !valid_keyword_provider();
...is equivalent.

[sky, 2009-04-14 03:59:09]

New snapshot uploaded. I didn't change a couple of things (as noted below). If
you feel strongly though, I will.

It strikes me that for relevancy it would be nice if we had a way for folks to
try out different ranking schemes. I'm not suggesting this for end users, but
rather for internal folks to help determine how we should create the scores.
Perhaps I should talk with the extensions folks to see if they have any
suggestions. It could be as simple as a file we load from to get the rankings.
This would differ from other extensions though in that I don't think we want to
lock this extension format in, it's just something we can use to help tune our
scores.

http://codereview.chromium.org/66073/diff/2002/2007
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/66073/diff/2002/2007#newcode138
Line 138: // keyword, then the primary provider corresponds to the default
provider.
On 2009/04/13 23:42:24, pkasting wrote:
> I was thinking about this some more and wondering if maybe we want
per-category
> relevance pushdown.  What I mean by that is, instead of "if two providers,
> default search providers' results all get scored low" we have something more
> like "if we have keyword suggestions, default provider's suggestions get
scored
> low" and the same for the other types.  There are two reasons for this idea:
> 
> * Today, most of the keywords people use will not have suggestions, and thus
> potentially useful results from the main engine will get pushed really low. 
For
> example, "amazon.com gi" might get "Search google for amazon.com gift
> certificates" as a suggest result.

This isn't quite true. We've been recording searches against all the other
engines for quite some time, we just haven't been showing them. So, once we turn
this on you should immediately get searches for existing engines.

You are right for newly added engines though.

> * In the future, as the keyword provider's heuristic function gets more
> aggressive about returning keywords for UNKNOWN input (i.e. input that doesn't
> have a second token to substitute in), "search for what you typed" results
will
> basically disappear any time the user has typed a word that matches a keyword.

> I already get pressure on this angle from Ben and I don't want to make things
> worse here.
> 
> If this isn't clear feel free to ask me.

My feeling is when in keyword mode we should "only" show things relevant to that
keyword, and probably what you typed for the default provider. Which this patch
that isn't always the case. For example, I have a keyword for a search that
starts with 'm'. I've noticed that after this patch I get history searches that
contain m. I find the useless.

I have no doubt we'll need to tweak relevancy scores. That is easy to do after
the fact though. Can we do that after landing this?

http://codereview.chromium.org/66073/diff/2002/2004
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/66073/diff/2002/2004#newcode75
Line 75: // extract the keyword and remaining string.
On 2009/04/13 23:42:24, pkasting wrote:
> Nit: The other important difference is the insertion of the
> "CleanUserInputKeyword" functionality... it still sounds like these two
> functions are almost equivalent from just this comment.

I took another crack at it. If you still don't like it, tell me what you want it
to say and I'll change it.

http://codereview.chromium.org/66073/diff/2002/2005
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/66073/diff/2002/2005#newcode407
Line 407: is_keyword ? keyword_navigation_results_ :
On 2009/04/13 23:42:24, pkasting wrote:
> Nit: If you break the first line after "?" you can fit it on two lines and
have
> it be just as readable :D

I find this form more readable than what you suggest.

http://codereview.chromium.org/66073/diff/2002/2005#newcode417
Line 417: if (result_url.is_valid()) {
On 2009/04/13 23:42:24, pkasting wrote:
> Nit: no {}

Done.

http://codereview.chromium.org/66073/diff/2002/2005#newcode559
Line 559: // Don't let scores go below 0 (or 275 if we have both a keyword and
default
On 2009/04/13 23:42:24, pkasting wrote:
> I don't think we should force keyword provider results to be above default
> provider results.  There's no reason to ensure that artificial separation if
the
> keyword results are really that poor.

Can I ask that you try it this way before I change it? This leads to something
like:

'Search m for XXX'
'Search Google for XXX'
'Search m for XXX YYY'

which I find just wrong. I would argue that in keyword mode we should not show
past searches for the default provider unless we have none from the keyword
provider.

http://codereview.chromium.org/66073/diff/2002/2005#newcode623
Line 623: DCHECK(
On 2009/04/13 23:42:24, pkasting wrote:
> Nit: Maybe this would be better as something like:
> DCHECK(suggestion_number < (is_keyword ?
>     keyword_navigation_results_.size() :
>     default_navigation_results_.size()));

I think you have more love for the ternary operator than the rest of the team
combined!

http://codereview.chromium.org/66073/diff/2002/2005#newcode730
Line 730: // TODO(kochi): http://b/1170574 if we end up allowing navigation
results from
On 2009/04/13 23:42:24, pkasting wrote:
> I think this comment can be removed?

Done.

http://codereview.chromium.org/66073/diff/2002/2006
File chrome/browser/autocomplete/search_provider.h (right):

http://codereview.chromium.org/66073/diff/2002/2006#newcode128
Line 128: return from_keyword_provider || (!from_keyword_provider &&
On 2009/04/13 23:42:24, pkasting wrote:
> Nit: Just
> return from_keyword_provider || !valid_keyword_provider();
> ...is equivalent.

Done.

[sky, 2009-04-14 04:08:48]

New snapshot is really up now.

[Peter Kasting, 2009-04-14 04:33:22]

http://codereview.chromium.org/66073/diff/2002/2007
File chrome/browser/autocomplete/autocomplete.h (right):

http://codereview.chromium.org/66073/diff/2002/2007#newcode138
Line 138: // keyword, then the primary provider corresponds to the default
provider.
On 2009/04/14 03:59:09, sky wrote:
> On 2009/04/13 23:42:24, pkasting wrote:
> > * Today, most of the keywords people use will not have suggestions, and thus
> > potentially useful results from the main engine will get pushed really low. 
> For
> > example, "amazon.com gi" might get "Search google for amazon.com gift
> > certificates" as a suggest result.
> 
> This isn't quite true. We've been recording searches against all the other
> engines for quite some time, we just haven't been showing them. So, once we
turn
> this on you should immediately get searches for existing engines.
> 
> You are right for newly added engines though.

Suggestions, not history (history would be another of those "sections" to do
this pushdown inside).  You're right that pretty much any commonly used keyword
will have history, but almost none will have suggestions.  Practically the main
effect of this would be to boost the query results for suggestions up to ~800,
which would put them above the history contents provider, which seems good.

> > * In the future, as the keyword provider's heuristic function gets more
> > aggressive about returning keywords for UNKNOWN input (i.e. input that
doesn't
> > have a second token to substitute in), "search for what you typed" results
> will
> > basically disappear any time the user has typed a word that matches a
keyword.
> 
> My feeling is when in keyword mode we should "only" show things relevant to
that
> keyword, and probably what you typed for the default provider. Which this
patch
> that isn't always the case. For example, I have a keyword for a search that
> starts with 'm'. I've noticed that after this patch I get history searches
that
> contain m. I find the useless.

But there are two problems with that:
1) having a keyword does not necessarily imply we're "in keyword mode"
(especially in the future, somewhat now), in the sense of showing the keyword UI
2) we already show lots of things other than the keyword results "in keyword
mode", which is to some degree sensical -- as with inline autocomplete, the edit
shows what your default action is, but you still have other choices.  (Even
without the patch you should be getting history results from your default search
provider containing m, in fact they would be scored more highly before the patch
than after it.)

> I have no doubt we'll need to tweak relevancy scores. That is easy to do after
> the fact though. Can we do that after landing this?

I fully expect we'll need to tweak after landing, I'm more concerned about how
ideas like this would to some degree necessitate rearchitecting a big chunk of
this patch.  And also that for things like the history results I mention above,
not doing this stuff now could be a regression.  And I want to know what feels
like it models the real world best.  You seem very convinced that the way you
have it is better, I feel uncertain of what way works better :/

http://codereview.chromium.org/66073/diff/2002/2005
File chrome/browser/autocomplete/search_provider.cc (right):

http://codereview.chromium.org/66073/diff/2002/2005#newcode559
Line 559: // Don't let scores go below 0 (or 275 if we have both a keyword and
default
On 2009/04/14 03:59:09, sky wrote:
> On 2009/04/13 23:42:24, pkasting wrote:
> > I don't think we should force keyword provider results to be above default
> > provider results.
> 
> Can I ask that you try it this way before I change it? This leads to something
> like:
> 
> 'Search m for XXX'
> 'Search Google for XXX'
> 'Search m for XXX YYY'
> 
> which I find just wrong. I would argue that in keyword mode we should not show
> past searches for the default provider unless we have none from the keyword
> provider.

But that's precisely how this fall off is _designed_ to work.  We already
intermingle it with all the other providers.  Besides, it seems like your
argument is one for scoring the default provider results at 0, not one for
scoring the keyword provider results at 275.  Plus it makes the code less
hackish.  It seems wrong in principle for a result to have a relevance of 275 no
matter how bad we think it is.

Practically speaking, I doubt you'll frequently notice a difference.  If you
have enough history queries that you're getting results that low on the
timescale you're unlikely to see many or any default search results anyway.  So
let's just make the algorithm simpler.

http://codereview.chromium.org/66073/diff/2002/2005#newcode623
Line 623: DCHECK(
On 2009/04/14 03:59:09, sky wrote:
> On 2009/04/13 23:42:24, pkasting wrote:
> > Nit: Maybe this would be better as something like:
> > DCHECK(suggestion_number < (is_keyword ?
> >     keyword_navigation_results_.size() :
> >     default_navigation_results_.size()));
> 
> I think you have more love for the ternary operator than the rest of the team
> combined!

You used it a lot in this patch!  And it makes the line a lot shorter and less
redundant, == more readable :(

[sky, 2009-04-14 16:47:23]

I've updated the patch as we discussed: the lower limit of 275 has been nuked.

[Peter Kasting, 2009-04-14 16:48:50]

LGTM, let's start testing it!