Add confidence to AutocompleteMatch.

Add confidence to AutocompleteMatch.

Calculated by each provider using whatever measure makes sense and stored as a float in the range [0, 1]. Will be used for determining benefit of using instant, prerender, DNS preconnect to increase success rate and avoid wasted resources.

Currently fed into UMA to validate experiment.

BUG=None
TEST=None


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

[dominich, 2011-08-09 20:22:49]

There are some TODOs still in there while I determine the best way to calculate
the confidence for certain providers, but this is the approach I'm considering.

I've also added histograms to start gathering data.

[Peter Kasting, 2011-08-09 20:53:00]

If all you care about is the confidence of the top match, you don't need to
implement it for the HCP, HQP, Shortcuts, and Builtins providers, as currently
those providers can never produce the default match.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:46: namespace {
Nit: Don't split the anonymous namespace into two pieces.

I think CompareMatchRelevance can be a member of MatchReference (maybe declared
with a similar signature to operator>() and called via an adapter?  If not, then
at least as a static two-arg member).

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:209: AutocompleteMatch
match = ResultToMatch(result_refs[i]);
Nit: I prefer constructor form to assignment form when referring to heavyweight
classes (I know, the compiler optimizes one to the other)

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:224: AutocompleteMatch
match(this, match_reference.relevance,
Nit: This wrapping would be OK:

  AutocompleteMatch match(this, match_reference.relevance,
      match_reference.confidence, true, MatchInTitle(result) ?
      AutocompleteMatch::HISTORY_TITLE : AutocompleteMatch::HISTORY_BODY);

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:284: // TODO(dominic):
Include typed count?
If you're only going to use one, prefer typed_count to visit_count, for two
reasons:

* It's a better indicator of what the user wants to open given that he is typing
in the address bar (users tend to open certain URLs by typing and others by e.g.
bookmarks, so visit_count is a good indicator of overall interest but a bad one
for specifically omnibox interest)

* Since the DB query is sorted by typed_count, the results you get back may be
effectively a random selection as far as visit_counts are concerned (meaning
many high-visit_count-URLs may be present in one query and absent in a similar
one), leading to wild swings in confidence for the same result across distinct
queries

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:295: // Add 0.5 if the
URL is bookmarked to get a final range of [0, 1].
I think this gives far too much weight to bookmarking.

For example, users who don't use bookmarks at all will never get higher than a
50% confidence rating on a URL.  This seems obviously wrong.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider.h (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.h:12: struct
MatchReference;
Nit: Shouldn't this be moved inside HistoryContentsProvider if subclasses need
it, rather than made global?

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_quick_provider.cc:213: it != matches.end();
++it) {
Nit: No need for {}

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_quick_provider.cc:218: return
static_cast<float>(match.raw_score) / denominator;
mrossetti might have good feedback on this algorithm.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:272: // TODO(dominich):
Should this be history_matches.size() - first_match - i?
No, it should not.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:311: // TODO(dominich):
Check that 100% confidence makes sense for exact input.
It doesn't.  This will mean the what-you-typed match is always scored as 100%,
which is definitely wrong sometimes.  For example, if we promote a match for
inline autocompletion, that match will outrank this one, which suggests it
should have higher confidence as well.  Or if the user types some characters and
then pastes, then the "ctrl" keydown during pasting will add ".com" to the end
of the current typing and this will then score that at 100%.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:487: // TODO(dominich): Take
into account typed count?
See comments in HCP for why typed_count is better than visit_count.

You also don't check some important clues like IsHostOnly().

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:492: it != matches.end();
++it) {
Nit: No need for {}

[dominich, 2011-08-09 21:43:43]

mrossetti: pkasting suggested you might be interested in the scoring algorithms
in this CL.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:46: namespace {
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: Don't split the anonymous namespace into two pieces.
> 
> I think CompareMatchRelevance can be a member of MatchReference (maybe
declared
> with a similar signature to operator>() and called via an adapter?  If not,
then
> at least as a static two-arg member).

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:209: AutocompleteMatch
match = ResultToMatch(result_refs[i]);
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: I prefer constructor form to assignment form when referring to
heavyweight
> classes (I know, the compiler optimizes one to the other)

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:224: AutocompleteMatch
match(this, match_reference.relevance,
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: This wrapping would be OK:
> 
>   AutocompleteMatch match(this, match_reference.relevance,
>       match_reference.confidence, true, MatchInTitle(result) ?
>       AutocompleteMatch::HISTORY_TITLE : AutocompleteMatch::HISTORY_BODY);

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:284: // TODO(dominic):
Include typed count?
On 2011/08/09 20:53:00, Peter Kasting wrote:
> If you're only going to use one, prefer typed_count to visit_count, for two
> reasons:
> 
> * It's a better indicator of what the user wants to open given that he is
typing
> in the address bar (users tend to open certain URLs by typing and others by
e.g.
> bookmarks, so visit_count is a good indicator of overall interest but a bad
one
> for specifically omnibox interest)
> 
> * Since the DB query is sorted by typed_count, the results you get back may be
> effectively a random selection as far as visit_counts are concerned (meaning
> many high-visit_count-URLs may be present in one query and absent in a similar
> one), leading to wild swings in confidence for the same result across distinct
> queries

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.cc:295: // Add 0.5 if the
URL is bookmarked to get a final range of [0, 1].
On 2011/08/09 20:53:00, Peter Kasting wrote:
> I think this gives far too much weight to bookmarking.
> 
> For example, users who don't use bookmarks at all will never get higher than a
> 50% confidence rating on a URL.  This seems obviously wrong.

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_contents_provider.h (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_contents_provider.h:12: struct
MatchReference;
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: Shouldn't this be moved inside HistoryContentsProvider if subclasses need
> it, rather than made global?

I went back and forward on this a couple of times. I think you're right to keep
the scope limited.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_quick_provider.cc:213: it != matches.end();
++it) {
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: No need for {}

I prefer to keep this as the for loop introduction spans multiple lines.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_quick_provider.cc:218: return
static_cast<float>(match.raw_score) / denominator;
On 2011/08/09 20:53:00, Peter Kasting wrote:
> mrossetti might have good feedback on this algorithm.

Adding to review line.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:272: // TODO(dominich):
Should this be history_matches.size() - first_match - i?
On 2011/08/09 20:53:00, Peter Kasting wrote:
> No, it should not.

Done.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:311: // TODO(dominich):
Check that 100% confidence makes sense for exact input.
On 2011/08/09 20:53:00, Peter Kasting wrote:
> It doesn't.  This will mean the what-you-typed match is always scored as 100%,
> which is definitely wrong sometimes.  For example, if we promote a match for
> inline autocompletion, that match will outrank this one, which suggests it
> should have higher confidence as well.  Or if the user types some characters
and
> then pastes, then the "ctrl" keydown during pasting will add ".com" to the end
> of the current typing and this will then score that at 100%.

You're mixing up the notion of rank and confidence. While they can be mixed it's
not clear to me that they should be. The rank is a cross-provider measure while
the confidence is a per-provider measure. It also is a suggestion of what we
should do with the result.

100% confident here means that /if/ the what-you-type match is the top result
then we should definitely apply instant or prerender to it.

The issue of the pasting is an interesting one but I'm not sure how we can avoid
that and still keep the confidence here relatively high.

I will set the confidence to 0 here for the first pass of stats so it doesn't
pollute them.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:487: // TODO(dominich): Take
into account typed count?
On 2011/08/09 20:53:00, Peter Kasting wrote:
> See comments in HCP for why typed_count is better than visit_count.
> 
> You also don't check some important clues like IsHostOnly().

It looks like CompareHistoryMatch is a good starting point for a more inclusive
measure.

I would like to get in a simple (naive) version with histograms and then try
different calculations for each provider to get the best results, ie increase
the proportion of successful prerenders.

http://codereview.chromium.org/7607007/diff/1/chrome/browser/autocomplete/his...
chrome/browser/autocomplete/history_url_provider.cc:492: it != matches.end();
++it) {
On 2011/08/09 20:53:00, Peter Kasting wrote:
> Nit: No need for {}

See comment on HCP as to why I prefer to keep this.

[Peter Kasting, 2011-08-09 21:52:38]

LGTM

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_contents_provider.cc:270: // - It's a better
indicator of what the user wants to open given that they
Nit: To save copy+pasting, you might want to just say "see comments on
HistoryURLProvider::CalculateConfidence() for why we use typed_count rather than
visit_count here."

[mrossetti, 2011-08-09 23:40:42]

I'm not really sure what 'confidence' means. Does it mean, "I'm sure this is a
good, solid match."? If so, the calculation:

match.raw_score / denominator

used throughout seems a poor discriminator. Depending on the provider, all three
of the matches could have equally high scores but with this formula none will
ever get a better confidence than 0.33. On the other hand, imagine that the
provider vended one of these high-scoring results and two very low scoring
results — in this case that high-scoring result that got a 0.33 confidence in
the first scenario would have a much high confidence in the second scenario,
everything else being equal.

Could you perhaps describe what 'confidence' means?

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_contents_provider.cc:200: }
Perhaps calculate sum of scores here.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_contents_provider.h (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_contents_provider.h:79: const
history::QueryResults& results) const;
Same suggestion as for the HistoryQuickProvider: calculate the sum of the scores
in the ConvertResults function instead of recalculating it here for each match.
Or do you imagine that the confidence calculation might transmogrify into
something more complex?

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_quick_provider.cc:121: }
You could calculate the denominator here and save it as an instance variable.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_quick_provider.cc:212: for
(ScoredHistoryMatches::const_iterator it = matches.begin();
Why not just calculate the 'denominator' up in DoAutocomplete() once all
qualifying matches have been collected?

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_quick_provider.h (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_quick_provider.h:55: const
history::ScoredHistoryMatches& history_matches,
Document what |history_matches| is.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_url_provider.cc:485: const
history::HistoryMatches& matches) {
I think this should just be the precalculated sum of scores.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_url_provider.cc:500: for
(history::HistoryMatches::const_iterator it = matches.begin();
Same thought as with the HistoryQuickProvider: why not calculate the sum once
the asynch pass has been completed rather than having to recalculate it for each
match.

[dominich.google, 2011-08-09 23:49:39]

Good point, I probably should explain a bit more of the background.

The confidence will be used to determine if the top result should be shown
instantly, prerendered, or we should just do a DNS preconnect. We only want
to use these resources when we're sure that the user is planning on taking
the suggestion.

It might make sense that a given result has a higher confidence in one
situation than another, as it will depend on the user's behaviour. I do
agree though that the algorithms to calculate the confidence may be too
simplistic at this point, but I expect more factors to go in once I've shown
that this is a reasonable method for prerendering more intelligently.

On Tue, Aug 9, 2011 at 4:40 PM, <mrossetti@chromium.org> wrote:

> I'm not really sure what 'confidence' means. Does it mean, "I'm sure this
> is a
> good, solid match."? If so, the calculation:
>
> match.raw_score / denominator
>
> used throughout seems a poor discriminator. Depending on the provider, all
> three
> of the matches could have equally high scores but with this formula none
> will
> ever get a better confidence than 0.33. On the other hand, imagine that the
> provider vended one of these high-scoring results and two very low scoring
> results — in this case that high-scoring result that got a 0.33 confidence
> in
> the first scenario would have a much high confidence in the second
> scenario,
> everything else being equal.
>
> Could you perhaps describe what 'confidence' means?
>
>
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**contents_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.cc>
> File chrome/browser/autocomplete/**history_contents_provider.cc (right):
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**contents_provider.cc#**newcode200<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.cc#newcode200>
> chrome/browser/autocomplete/**history_contents_provider.cc:**200: }
> Perhaps calculate sum of scores here.
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**contents_provider.h<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.h>
> File chrome/browser/autocomplete/**history_contents_provider.h (right):
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**contents_provider.h#newcode79<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.h#newcode79>
> chrome/browser/autocomplete/**history_contents_provider.h:**79: const
> history::QueryResults& results) const;
> Same suggestion as for the HistoryQuickProvider: calculate the sum of
> the scores in the ConvertResults function instead of recalculating it
> here for each match. Or do you imagine that the confidence calculation
> might transmogrify into something more complex?
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**quick_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc>
> File chrome/browser/autocomplete/**history_quick_provider.cc (right):
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**quick_provider.cc#newcode121<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc#newcode121>
> chrome/browser/autocomplete/**history_quick_provider.cc:121: }
> You could calculate the denominator here and save it as an instance
> variable.
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**quick_provider.cc#newcode212<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc#newcode212>
> chrome/browser/autocomplete/**history_quick_provider.cc:212: for
> (ScoredHistoryMatches::const_**iterator it = matches.begin();
> Why not just calculate the 'denominator' up in DoAutocomplete() once all
> qualifying matches have been collected?
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**quick_provider.h<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.h>
> File chrome/browser/autocomplete/**history_quick_provider.h (right):
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**quick_provider.h#newcode55<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.h#newcode55>
> chrome/browser/autocomplete/**history_quick_provider.h:55: const
> history::ScoredHistoryMatches& history_matches,
> Document what |history_matches| is.
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**url_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc>
> File chrome/browser/autocomplete/**history_url_provider.cc (right):
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**url_provider.cc#newcode485<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc#newcode485>
> chrome/browser/autocomplete/**history_url_provider.cc:485: const
> history::HistoryMatches& matches) {
> I think this should just be the precalculated sum of scores.
>
> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>
browser/autocomplete/history_**url_provider.cc#newcode500<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc#newcode500>
> chrome/browser/autocomplete/**history_url_provider.cc:500: for
> (history::HistoryMatches::**const_iterator it = matches.begin();
> Same thought as with the HistoryQuickProvider: why not calculate the sum
> once the asynch pass has been completed rather than having to
> recalculate it for each match.
>
>
>
http://codereview.chromium.**org/7607007/<http://codereview.chromium.org/7607...
>

[mrossetti, 2011-08-10 16:06:16]

LGTM then, understanding that this is an experiment. I have some concerns
about having two different scoring mechanisms and would suggest that we
consider usage of match attribute flags for indicating a match's worthiness
for things such as default, in-line autocomplete, instant, etc. rather than
relying on scores boundaries for such.

On Tue, Aug 9, 2011 at 4:49 PM, Dominic Hamon <dominich@google.com> wrote:

> Good point, I probably should explain a bit more of the background.
>
> The confidence will be used to determine if the top result should be shown
> instantly, prerendered, or we should just do a DNS preconnect. We only want
> to use these resources when we're sure that the user is planning on taking
> the suggestion.
>
> It might make sense that a given result has a higher confidence in one
> situation than another, as it will depend on the user's behaviour. I do
> agree though that the algorithms to calculate the confidence may be too
> simplistic at this point, but I expect more factors to go in once I've shown
> that this is a reasonable method for prerendering more intelligently.
>
> On Tue, Aug 9, 2011 at 4:40 PM, <mrossetti@chromium.org> wrote:
>
>> I'm not really sure what 'confidence' means. Does it mean, "I'm sure this
>> is a
>> good, solid match."? If so, the calculation:
>>
>> match.raw_score / denominator
>>
>> used throughout seems a poor discriminator. Depending on the provider, all
>> three
>> of the matches could have equally high scores but with this formula none
>> will
>> ever get a better confidence than 0.33. On the other hand, imagine that
>> the
>> provider vended one of these high-scoring results and two very low scoring
>> results — in this case that high-scoring result that got a 0.33 confidence
>> in
>> the first scenario would have a much high confidence in the second
>> scenario,
>> everything else being equal.
>>
>> Could you perhaps describe what 'confidence' means?
>>
>>
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**contents_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.cc>
>> File chrome/browser/autocomplete/**history_contents_provider.cc (right):
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**contents_provider.cc#**newcode200<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.cc#newcode200>
>> chrome/browser/autocomplete/**history_contents_provider.cc:**200: }
>> Perhaps calculate sum of scores here.
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**contents_provider.h<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.h>
>> File chrome/browser/autocomplete/**history_contents_provider.h (right):
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**contents_provider.h#newcode79<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_contents_provider.h#newcode79>
>> chrome/browser/autocomplete/**history_contents_provider.h:**79: const
>> history::QueryResults& results) const;
>> Same suggestion as for the HistoryQuickProvider: calculate the sum of
>> the scores in the ConvertResults function instead of recalculating it
>> here for each match. Or do you imagine that the confidence calculation
>> might transmogrify into something more complex?
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**quick_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc>
>> File chrome/browser/autocomplete/**history_quick_provider.cc (right):
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**quick_provider.cc#newcode121<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc#newcode121>
>> chrome/browser/autocomplete/**history_quick_provider.cc:121: }
>> You could calculate the denominator here and save it as an instance
>> variable.
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**quick_provider.cc#newcode212<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.cc#newcode212>
>> chrome/browser/autocomplete/**history_quick_provider.cc:212: for
>> (ScoredHistoryMatches::const_**iterator it = matches.begin();
>> Why not just calculate the 'denominator' up in DoAutocomplete() once all
>> qualifying matches have been collected?
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**quick_provider.h<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.h>
>> File chrome/browser/autocomplete/**history_quick_provider.h (right):
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**quick_provider.h#newcode55<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_quick_provider.h#newcode55>
>> chrome/browser/autocomplete/**history_quick_provider.h:55: const
>> history::ScoredHistoryMatches& history_matches,
>> Document what |history_matches| is.
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**url_provider.cc<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc>
>> File chrome/browser/autocomplete/**history_url_provider.cc (right):
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**url_provider.cc#newcode485<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc#newcode485>
>> chrome/browser/autocomplete/**history_url_provider.cc:485: const
>> history::HistoryMatches& matches) {
>> I think this should just be the precalculated sum of scores.
>>
>> http://codereview.chromium.**org/7607007/diff/18/chrome/**
>>
browser/autocomplete/history_**url_provider.cc#newcode500<http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/history_url_provider.cc#newcode500>
>> chrome/browser/autocomplete/**history_url_provider.cc:500: for
>> (history::HistoryMatches::**const_iterator it = matches.begin();
>> Same thought as with the HistoryQuickProvider: why not calculate the sum
>> once the asynch pass has been completed rather than having to
>> recalculate it for each match.
>>
>>
>>
http://codereview.chromium.**org/7607007/<http://codereview.chromium.org/7607...
>>
>
>

[dominich, 2011-08-10 16:10:13]

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_contents_provider.cc:270: // - It's a better
indicator of what the user wants to open given that they
On 2011/08/09 21:52:38, Peter Kasting wrote:
> Nit: To save copy+pasting, you might want to just say "see comments on
> HistoryURLProvider::CalculateConfidence() for why we use typed_count rather
than
> visit_count here."

I have kept these separate because the details may change. I did consider moving
this calculation to the base HistoryProvider class as a default implementation
but the derived classes use slightly different types. Perhaps this is something
that could be cleaned up later.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_contents_provider.h (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_contents_provider.h:79: const
history::QueryResults& results) const;
On 2011/08/09 23:40:43, mrossetti wrote:
> Same suggestion as for the HistoryQuickProvider: calculate the sum of the
scores
> in the ConvertResults function instead of recalculating it here for each
match.
> Or do you imagine that the confidence calculation might transmogrify into
> something more complex?

There's a very good chance the details of the calculation will change. This
first version is deliberately naive to see if it alone can improve the success
rate of prerendering from the omnibox.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_quick_provider.cc:121: }
On 2011/08/09 23:40:43, mrossetti wrote:
> You could calculate the denominator here and save it as an instance variable.

See other comment regarding potential algorithm changes.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_quick_provider.h (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_quick_provider.h:55: const
history::ScoredHistoryMatches& history_matches,
On 2011/08/09 23:40:43, mrossetti wrote:
> Document what |history_matches| is.

Done.

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
File chrome/browser/autocomplete/history_url_provider.cc (right):

http://codereview.chromium.org/7607007/diff/18/chrome/browser/autocomplete/hi...
chrome/browser/autocomplete/history_url_provider.cc:500: for
(history::HistoryMatches::const_iterator it = matches.begin();
On 2011/08/09 23:40:43, mrossetti wrote:
> Same thought as with the HistoryQuickProvider: why not calculate the sum once
> the asynch pass has been completed rather than having to recalculate it for
each
> match.

Once the algorithm for the calculation settles to a stable state (the one that
gives the best prediction rate for prerender) I will optimize.

[tburkard, 2011-08-10 17:51:09]

LGTM

[commit-bot: I haz the power, 2011-08-11 20:44:34]

Change committed as 96446