Adding a fourth omnibox prerender experiment.

Adding a fourth omnibox prerender experiment.

This adds or updates entries in the database for the entire chain of text the user typed. For example, if the user types 'goog' and navigates to google.com, this will add or update database entries for 'go' -> google.com, 'goo' -> google.com and 'goog' -> google.com.


BUG=
TEST=


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

[dominich, 2011-11-18 21:05:26]

You know the drill:

cbentzel: prerender
pkasting: autocomplete

Thank you!

[Peter Kasting, 2011-11-18 21:17:13]

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:144: // Don't register
if we already saw this |user_text|.
Are you sure this is what you want?  It's possible for the same user_text to
have many different sets of matches in the same single navigation.  Two ways
this can happen: OnChanged() can get called repeatedly as more providers return
async results to the controller (I think), and the user can delete an inline
autocompletion, resulting in re-running an earlier text query but with
autocomplete disabled (which will result in a different match set).

Maybe you want to union the URLs from all instances with the same text?  I
dunno.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:324:
row.number_of_misses = is_hit ? 0 : 1;
Nit: Or "= 1 - row.number_of_hits", dunno which is clearer to you

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.h (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.h:83: return
user_text.compare(other_user_text) == 0;
Nit: Just return user_text == other_user_text.  Just as fast and more readable.

[cbentzel, 2011-11-18 21:24:06]

I just looked at the prerender portions. Now that we're starting to get so many
heuristics I'm wondering if we should consider consolidating the code into
prerender_omnibox.[h|cc] and rewrite a few of the if chains.

I'll also be on vacation all next week so you may want to find another review.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_field_trial.cc:226:
base::FieldTrialList::FindValue("PrerenderFromOmniboxHeuristic");
This and "PrerenderFromOmnibox" should probably move to constants since
mentioned twice - existing issue.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:46: if (experiment_id !=
kNoExperiment)
Feels like the experiment_id != kNoExperiment should go before the switch since
it's repeated so much now.

[dominich, 2011-11-18 22:12:08]

On 2011/11/18 21:24:06, cbentzel wrote:
> I just looked at the prerender portions. Now that we're starting to get so
many
> heuristics I'm wondering if we should consider consolidating the code into
> prerender_omnibox.[h|cc] and rewrite a few of the if chains.

In the next few weeks these will be consolidated into a single one. As such, I'm
not too worried about the code being the way it is. Once there's one, it'll be
much cleaner.


> 
> I'll also be on vacation all next week so you may want to find another review.
> 
>
http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
> File chrome/browser/prerender/prerender_field_trial.cc (right):
> 
>
http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
> chrome/browser/prerender/prerender_field_trial.cc:226:
> base::FieldTrialList::FindValue("PrerenderFromOmniboxHeuristic");
> This and "PrerenderFromOmnibox" should probably move to constants since
> mentioned twice - existing issue.
> 
>
http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
> File chrome/browser/prerender/prerender_histograms.cc (right):
> 
>
http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
> chrome/browser/prerender/prerender_histograms.cc:46: if (experiment_id !=
> kNoExperiment)
> Feels like the experiment_id != kNoExperiment should go before the switch
since
> it's repeated so much now.

[cbentzel, 2011-11-18 22:13:27]

OK

LGTM for prerender

On Fri, Nov 18, 2011 at 5:12 PM, <dominich@chromium.org> wrote:

> On 2011/11/18 21:24:06, cbentzel wrote:
>
>> I just looked at the prerender portions. Now that we're starting to get so
>>
> many
>
>> heuristics I'm wondering if we should consider consolidating the code into
>> prerender_omnibox.[h|cc] and rewrite a few of the if chains.
>>
>
> In the next few weeks these will be consolidated into a single one. As
> such, I'm
> not too worried about the code being the way it is. Once there's one,
> it'll be
> much cleaner.
>
>
>
>
>  I'll also be on vacation all next week so you may want to find another
>> review.
>>
>
>
> http://codereview.chromium.**org/8600004/diff/1/chrome/**
>
browser/prerender/prerender_**field_trial.cc<http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/prerender_field_trial.cc>
>
>> File chrome/browser/prerender/**prerender_field_trial.cc (right):
>>
>
>
> http://codereview.chromium.**org/8600004/diff/1/chrome/**
>
browser/prerender/prerender_**field_trial.cc#newcode226<http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/prerender_field_trial.cc#newcode226>
>
>> chrome/browser/prerender/**prerender_field_trial.cc:226:
>> base::FieldTrialList::**FindValue("**PrerenderFromOmniboxHeuristic"**);
>> This and "PrerenderFromOmnibox" should probably move to constants since
>> mentioned twice - existing issue.
>>
>
>
> http://codereview.chromium.**org/8600004/diff/1/chrome/**
>
browser/prerender/prerender_**histograms.cc<http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/prerender_histograms.cc>
>
>> File chrome/browser/prerender/**prerender_histograms.cc (right):
>>
>
>
> http://codereview.chromium.**org/8600004/diff/1/chrome/**
>
browser/prerender/prerender_**histograms.cc#newcode46<http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/prerender_histograms.cc#newcode46>
>
>> chrome/browser/prerender/**prerender_histograms.cc:46: if (experiment_id
>> !=
>> kNoExperiment)
>> Feels like the experiment_id != kNoExperiment should go before the switch
>>
> since
>
>> it's repeated so much now.
>>
>
>
>
>
http://codereview.chromium.**org/8600004/<http://codereview.chromium.org/8600...
>

[dominich, 2011-11-18 23:05:51]

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:144: // Don't register
if we already saw this |user_text|.
On 2011/11/18 21:17:13, Peter Kasting wrote:
> Are you sure this is what you want?  It's possible for the same user_text to
> have many different sets of matches in the same single navigation.  Two ways
> this can happen: OnChanged() can get called repeatedly as more providers
return
> async results to the controller (I think), and the user can delete an inline
> autocompletion, resulting in re-running an earlier text query but with
> autocomplete disabled (which will result in a different match set).
> 
> Maybe you want to union the URLs from all instances with the same text?  I
> dunno.

I tested this and although I saw OnChanged being called three times for every
character typed (!) I saw the same matches every time. If there's a chance that
they might be different, then yes, I want the union.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.cc:324:
row.number_of_misses = is_hit ? 0 : 1;
On 2011/11/18 21:17:13, Peter Kasting wrote:
> Nit: Or "= 1 - row.number_of_hits", dunno which is clearer to you

I like the symmetry of the existing code - it's clear to me that the values are
orthogonal.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
File chrome/browser/autocomplete/network_action_predictor.h (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/autocomplete/net...
chrome/browser/autocomplete/network_action_predictor.h:83: return
user_text.compare(other_user_text) == 0;
On 2011/11/18 21:17:13, Peter Kasting wrote:
> Nit: Just return user_text == other_user_text.  Just as fast and more
readable.

Done.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_field_trial.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_field_trial.cc:226:
base::FieldTrialList::FindValue("PrerenderFromOmniboxHeuristic");
On 2011/11/18 21:24:06, cbentzel wrote:
> This and "PrerenderFromOmnibox" should probably move to constants since
> mentioned twice - existing issue.

Done.

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/8600004/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_histograms.cc:46: if (experiment_id !=
kNoExperiment)
On 2011/11/18 21:24:06, cbentzel wrote:
> Feels like the experiment_id != kNoExperiment should go before the switch
since
> it's repeated so much now. 

Done.

[Peter Kasting, 2011-11-19 02:16:43]

Dominic assures me that everything works correctly in every case he's tried with
backspacing and such, so I guess LGTM.

Seems like in theory we might want some tests here but I'm not sure precisely
how they'd work.

[commit-bot: I haz the power, 2011-11-21 20:38:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/8600004/14001

[commit-bot: I haz the power, 2011-11-21 21:53:32]

Change committed as 111005