Added prerendering to omnibox.

Added prerendering to omnibox. Enable through --prerender-from-omnibox command-line-flag or through the about:flag with the same name. Only effective if prerender is enabled and instant is disabled or restricted to search.

BUG=87124
TEST=none


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

[dominich, 2011-06-20 17:22:01]

I should have someone else review this for the chrome/app/*,
chrome/browser/about_flags.cc and chrome/common/* changes but there are no
OWNERS files in those folders. Any suggestions for reviewers welcome.

[cbentzel, 2011-06-20 18:06:01]

pkasting may be a good person to look at this as well.

I'm worried that the misprediction rate is going to be too high, but the omnibox
may be smart enough about the suggestions it generates to make this a non-issue.

It would be great if we could annotate some of the prerender histograms with the
source of the prerender trigger, so we can track how well this actually works
with a small experiment, without it being obscured by the <link rel=prerender>
approach.

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:243:
CurrentMatch().destination_url);
What is the likelihood that users will choose CurrentMatch()? Do we retain
per-browser-stats, or at least have aggregate stats?

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:95: bool
AddPrerenderWithNoTag(const GURL& url);
Can you be consistent about the AddPreload/AddPrerender name? I'm fine with
moving to AddPrerender.

[dominich, 2011-06-20 18:13:59]

On 2011/06/20 18:06:01, cbentzel wrote:
> pkasting may be a good person to look at this as well.
> 
> I'm worried that the misprediction rate is going to be too high, but the
omnibox
> may be smart enough about the suggestions it generates to make this a
non-issue.

The suggestions are taken from places the user has been before, and this is
already an issue of sorts with Instant. Ie, Instant can mispredict as you type
if the suggestions are not what you really want.

I'm less worried about this as it is user-instigated so we have a fairly good
idea that the user wants to go somewhere.

> 
> It would be great if we could annotate some of the prerender histograms with
the
> source of the prerender trigger, so we can track how well this actually works
> with a small experiment, without it being obscured by the <link rel=prerender>
> approach.

I agree. Perhaps just adding a prerender source histogram would be enough.

> 
>
http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/autocomplete_edit.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/autocomplete_edit.cc:243:
> CurrentMatch().destination_url);
> What is the likelihood that users will choose CurrentMatch()? Do we retain
> per-browser-stats, or at least have aggregate stats?
> 
>
http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_manager.h (right):
> 
>
http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_manager.h:95: bool
> AddPrerenderWithNoTag(const GURL& url);
> Can you be consistent about the AddPreload/AddPrerender name? I'm fine with
> moving to AddPrerender.

[sky, 2011-06-20 18:15:54]

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:242:
prerender_manager->AddPrerenderWithNoTag(
Is this cumulative? Meaning might I end up preloading a page for each unique url
generated as I'm typing? Is there any throttling?

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:174: CommandLine* cl =
CommandLine::ForCurrentProcess();
Seems like you only want to do this if you hit the last if in
ShouldShouldPreviewFor.

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:765: const char kPrerenderFromOmnibox[]        
 = "prerender-from-omnibox";
Need sorted.

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
chrome/common/chrome_switches.h:218: extern const char kPrerenderFromOmnibox[];
Keep sorted.

[cbentzel, 2011-06-20 18:28:59]

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:242:
prerender_manager->AddPrerenderWithNoTag(
On 2011/06/20 18:15:54, sky wrote:
> Is this cumulative? Meaning might I end up preloading a page for each unique
url
> generated as I'm typing? Is there any throttling?

There is throttling in the PrerenderManager code - a 500ms gap between
prerenders is enforced

[dominich, 2011-06-20 18:39:59]

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:242:
prerender_manager->AddPrerenderWithNoTag(
On 2011/06/20 18:15:54, sky wrote:
> Is this cumulative? Meaning might I end up preloading a page for each unique
url
> generated as I'm typing? Is there any throttling?

There is a rate limiter that will kick in that only allows for one prerender
every X ms. I've also added the same check as you do above for
user_input_in_progress() and popup_->IsOpen().

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:243:
CurrentMatch().destination_url);
On 2011/06/20 18:06:01, cbentzel wrote:
> What is the likelihood that users will choose CurrentMatch()? Do we retain
> per-browser-stats, or at least have aggregate stats?

It doesn't look like we have any statistics on how often we predict correctly as
it hasn't really been a concern for the suggestion box.

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:174: CommandLine* cl =
CommandLine::ForCurrentProcess();
On 2011/06/20 18:15:54, sky wrote:
> Seems like you only want to do this if you hit the last if in
> ShouldShouldPreviewFor.

You're right. I didn't think it would matter that much and this feels like the
right place, but prerendering when the host is blacklisted or it's a javascript
scheme probably isn't a good idea.

Moved.

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.h:95: bool
AddPrerenderWithNoTag(const GURL& url);
On 2011/06/20 18:06:01, cbentzel wrote:
> Can you be consistent about the AddPreload/AddPrerender name? I'm fine with
> moving to AddPrerender.

Done.

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:765: const char kPrerenderFromOmnibox[]        
 = "prerender-from-omnibox";
On 2011/06/20 18:15:54, sky wrote:
> Need sorted.

Done.

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/7210020/diff/1011/chrome/common/chrome_switche...
chrome/common/chrome_switches.h:218: extern const char kPrerenderFromOmnibox[];
On 2011/06/20 18:15:54, sky wrote:
> Keep sorted.

Oops. Alphabet fail.

Done.

[sky, 2011-06-20 19:29:23]

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:235: } else if (tab &&
tab->tab_contents()) {
Combine your ifs. Also, I believe you only want to do this if update_instant_ is
true. We should rename update_instant_ to something like in_revert_.

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:698: CommandLine* cl =
CommandLine::ForCurrentProcess();
I don't like that this method has a side effect. Can you instead make it return
an enum indicating why it shouldn't show the preview and key off that.

[dominich, 2011-06-20 20:48:40]

I'm still going to look into how we can add info to histograms and potentially
filter by some confidence level but I'd appreciate any other comments on the
general approach.

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:235: } else if (tab &&
tab->tab_contents()) {
On 2011/06/20 19:29:24, sky wrote:
> Combine your ifs. Also, I believe you only want to do this if update_instant_
is
> true. We should rename update_instant_ to something like in_revert_.

Done.

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/instant/insta...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/4002/chrome/browser/instant/insta...
chrome/browser/instant/instant_controller.cc:698: CommandLine* cl =
CommandLine::ForCurrentProcess();
On 2011/06/20 19:29:24, sky wrote:
> I don't like that this method has a side effect. Can you instead make it
return
> an enum indicating why it shouldn't show the preview and key off that.

Done.

[sky, 2011-06-20 20:53:49]

http://codereview.chromium.org/7210020/diff/37/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/37/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_edit.cc:85: in_revert_(true),
Sorry I wasn't clear. in_revert_ and update_instant_ are the inverse of each
other. So, you need to invert the testing/setting every where.

http://codereview.chromium.org/7210020/diff/37/chrome/browser/instant/instant...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/37/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:175: } else {
combine else and if, eg:
else if (previw_condition ==

[dominich, 2011-06-20 21:02:36]

http://codereview.chromium.org/7210020/diff/37/chrome/browser/autocomplete/au...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/37/chrome/browser/autocomplete/au...
chrome/browser/autocomplete/autocomplete_edit.cc:85: in_revert_(true),
On 2011/06/20 20:53:49, sky wrote:
> Sorry I wasn't clear. in_revert_ and update_instant_ are the inverse of each
> other. So, you need to invert the testing/setting every where.

Done.

http://codereview.chromium.org/7210020/diff/37/chrome/browser/instant/instant...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/7210020/diff/37/chrome/browser/instant/instant...
chrome/browser/instant/instant_controller.cc:175: } else {
On 2011/06/20 20:53:49, sky wrote:
> combine else and if, eg:
> else if (previw_condition == 

Done.

[sky, 2011-06-20 21:11:44]

LGTM

http://codereview.chromium.org/7210020/diff/3004/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/3004/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:236: } else if
(tab->tab_contents() && user_input_in_progress() &&
Remove the tab_contents() check, it's always non-null.

[dominich, 2011-06-20 21:55:37]

I've added a histogram to track how many prerenders we get from the omnibox vs
<link> tags. Based on that, I may do further work to limit which URLs we
actually end up starting prerenders for.

http://codereview.chromium.org/7210020/diff/3004/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/3004/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:236: } else if
(tab->tab_contents() && user_input_in_progress() &&
On 2011/06/20 21:11:44, sky wrote:
> Remove the tab_contents() check, it's always non-null.

Done.

[cbentzel, 2011-06-22 17:07:13]

+tburkard - he's been looking at other ways to break down the metrics
per-source.

[cbentzel, 2011-06-22 17:07:41]

Also - could you add a bug to help track this and reference it in the changelist
description.

[cbentzel, 2011-06-22 17:27:32]

It would be really nice to come up with a way to track all prerender statics,
such as perceived page load time, time until used, etc. by origin [omnibox,
<link rel=prerender>]

I'm not going to require it in this CL, but it's worth thinking through. The UMA
macros make this a little difficult, and we would probably need to track origin
in more locations.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:239: if
(cl->HasSwitch(switches::kPrerenderFromOmnibox)) {
Move the command-line check be moved into
PrerenderManager::AddPrerenderWithNoTag - it's done in more than one caller
location, and hides more prerender logic from autocomplete/omnibox.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
chrome/browser/instant/instant_controller.h:243: PreviewCondition
ShouldShowPreviewFor(const AutocompleteMatch& match,
Should this be named GetPreviewCondition now that it no longer returns a bool?
The "Should" prefix is usually used when a bool is returned.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:169: if (source_render_view_host)
{
Move the DCHECK back in for source_render_view_host->view()

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:68:
UMA_HISTOGRAM_ENUMERATION("Prerender.FinalStatusLinkRelPrerender",
Should these be done as 

Prerender.FinalStatus_LinkRelPrerender and Prerender.FinalStatus_Omnibox? That's
more consistent with typical naming schemes.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:78: LOG(WARNING) << "Not
logging final status for origin " <<
This should be a NOTREACHED();

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:372: bool
PrerenderManager::AddPrerenderWithNoTag(Origin origin, const GURL& url) {
I don't really like the NoTag name, but can't think of an alternative. Maybe
AddTopLevelPrerender - which indicates that it doesn't come from a referrer?

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:572:
AddPrerender(ORIGIN_PENDING_LINK_REL_PRERENDER,
Should the pending ones maintain a cached origin, instead of hardcoding it here?

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_origin.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:25: void RecordOrigin(Origin
origin) {
Why do we need to record this? We have all the data we need from the FinalStatus
histograms.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:37: const char*
NameFromOrigin(Origin origin) {
Is this being used?

[cbentzel, 2011-06-22 17:30:55]

One other "CL description" issue - could you add a description of the new
about:flags and command-line flag value to turn this on? It could perhaps be in
the TEST= field.

[dominich, 2011-06-22 18:32:01]

On 2011/06/22 17:27:32, cbentzel wrote:
> It would be really nice to come up with a way to track all prerender statics,
> such as perceived page load time, time until used, etc. by origin [omnibox,
> <link rel=prerender>]
> 
> I'm not going to require it in this CL, but it's worth thinking through. The
UMA
> macros make this a little difficult, and we would probably need to track
origin
> in more locations.

Yes, I've been struggling to come up with a way of doing this. The final status
being split into 3 (all, and per-origin) is a start but not particularly
generic. It might be worth making a wrapper for our UMA stuff that automatically
outputs a histogram per origin, but that will massively increase the histograms
we have.

> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit.cc:239: if
> (cl->HasSwitch(switches::kPrerenderFromOmnibox)) {
> Move the command-line check be moved into
> PrerenderManager::AddPrerenderWithNoTag - it's done in more than one caller
> location, and hides more prerender logic from autocomplete/omnibox.
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
> File chrome/browser/instant/instant_controller.h (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
> chrome/browser/instant/instant_controller.h:243: PreviewCondition
> ShouldShowPreviewFor(const AutocompleteMatch& match,
> Should this be named GetPreviewCondition now that it no longer returns a bool?
> The "Should" prefix is usually used when a bool is returned.
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_contents.cc:169: if
(source_render_view_host)
> {
> Move the DCHECK back in for source_render_view_host->view()
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_final_status.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_final_status.cc:68:
> UMA_HISTOGRAM_ENUMERATION("Prerender.FinalStatusLinkRelPrerender",
> Should these be done as 
> 
> Prerender.FinalStatus_LinkRelPrerender and Prerender.FinalStatus_Omnibox?
That's
> more consistent with typical naming schemes.
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_final_status.cc:78: LOG(WARNING) << "Not
> logging final status for origin " <<
> This should be a NOTREACHED();
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.cc:372: bool
> PrerenderManager::AddPrerenderWithNoTag(Origin origin, const GURL& url) {
> I don't really like the NoTag name, but can't think of an alternative. Maybe
> AddTopLevelPrerender - which indicates that it doesn't come from a referrer?
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.cc:572:
> AddPrerender(ORIGIN_PENDING_LINK_REL_PRERENDER,
> Should the pending ones maintain a cached origin, instead of hardcoding it
here?
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_origin.cc (right):
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_origin.cc:25: void RecordOrigin(Origin
> origin) {
> Why do we need to record this? We have all the data we need from the
FinalStatus
> histograms.
> 
>
http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_origin.cc:37: const char*
> NameFromOrigin(Origin origin) {
> Is this being used?

[cbentzel, 2011-06-22 18:39:16]

On Wed, Jun 22, 2011 at 2:32 PM,  <dominich@chromium.org> wrote:
> On 2011/06/22 17:27:32, cbentzel wrote:
>>
>> It would be really nice to come up with a way to track all prerender
>> statics,
>> such as perceived page load time, time until used, etc. by origin
>> [omnibox,
>> <link rel=prerender>]
>
>> I'm not going to require it in this CL, but it's worth thinking through.
>> The
>
> UMA
>>
>> macros make this a little difficult, and we would probably need to track
>
> origin
>>
>> in more locations.
>
> Yes, I've been struggling to come up with a way of doing this. The final
> status
> being split into 3 (all, and per-origin) is a start but not particularly
> generic. It might be worth making a wrapper for our UMA stuff that
> automatically
> outputs a histogram per origin, but that will massively increase the
> histograms
> we have.

I'm actually OK with exploding the number of histograms. My concern is
more how to do this cleanly in the code - it's pretty tricky with the
statics and macros that are typically used for the UMA system.

[dominich.google, 2011-06-22 18:46:36]

On Wed, Jun 22, 2011 at 11:39 AM, Chris Bentzel <cbentzel@chromium.org>wrote:

> On Wed, Jun 22, 2011 at 2:32 PM,  <dominich@chromium.org> wrote:
> > On 2011/06/22 17:27:32, cbentzel wrote:
> >>
> >> It would be really nice to come up with a way to track all prerender
> >> statics,
> >> such as perceived page load time, time until used, etc. by origin
> >> [omnibox,
> >> <link rel=prerender>]
> >
> >> I'm not going to require it in this CL, but it's worth thinking through.
> >> The
> >
> > UMA
> >>
> >> macros make this a little difficult, and we would probably need to track
> >
> > origin
> >>
> >> in more locations.
> >
> > Yes, I've been struggling to come up with a way of doing this. The final
> > status
> > being split into 3 (all, and per-origin) is a start but not particularly
> > generic. It might be worth making a wrapper for our UMA stuff that
> > automatically
> > outputs a histogram per origin, but that will massively increase the
> > histograms
> > we have.
>
> I'm actually OK with exploding the number of histograms. My concern is
> more how to do this cleanly in the code - it's pretty tricky with the
> statics and macros that are typically used for the UMA system.
>

I believe tburkard had a plan for this, but if not I have a few ideas.

[dominich, 2011-06-22 19:00:51]

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit.cc:239: if
(cl->HasSwitch(switches::kPrerenderFromOmnibox)) {
On 2011/06/22 17:27:32, cbentzel wrote:
> Move the command-line check be moved into
> PrerenderManager::AddPrerenderWithNoTag - it's done in more than one caller
> location, and hides more prerender logic from autocomplete/omnibox. 

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/instant/inst...
chrome/browser/instant/instant_controller.h:243: PreviewCondition
ShouldShowPreviewFor(const AutocompleteMatch& match,
On 2011/06/22 17:27:32, cbentzel wrote:
> Should this be named GetPreviewCondition now that it no longer returns a bool?
> The "Should" prefix is usually used when a bool is returned.

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:169: if (source_render_view_host)
{
On 2011/06/22 17:27:32, cbentzel wrote:
> Move the DCHECK back in for source_render_view_host->view()

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:68:
UMA_HISTOGRAM_ENUMERATION("Prerender.FinalStatusLinkRelPrerender",
On 2011/06/22 17:27:32, cbentzel wrote:
> Should these be done as 
> 
> Prerender.FinalStatus_LinkRelPrerender and Prerender.FinalStatus_Omnibox?
That's
> more consistent with typical naming schemes.

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:78: LOG(WARNING) << "Not
logging final status for origin " <<
On 2011/06/22 17:27:32, cbentzel wrote:
> This should be a NOTREACHED(); 

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:372: bool
PrerenderManager::AddPrerenderWithNoTag(Origin origin, const GURL& url) {
On 2011/06/22 17:27:32, cbentzel wrote:
> I don't really like the NoTag name, but can't think of an alternative. Maybe
> AddTopLevelPrerender - which indicates that it doesn't come from a referrer?

That to me suggests that it's a prerender of the top-level resource. It's a
shame overloading is forbidden.

AddPrerender for this and AddPrerenderFromPage for the older method?

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:572:
AddPrerender(ORIGIN_PENDING_LINK_REL_PRERENDER,
On 2011/06/22 17:27:32, cbentzel wrote:
> Should the pending ones maintain a cached origin, instead of hardcoding it
here?

Done.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_origin.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:25: void RecordOrigin(Origin
origin) {
On 2011/06/22 17:27:32, cbentzel wrote:
> Why do we need to record this? We have all the data we need from the
FinalStatus
> histograms. 

It is calculable from FinalStatus but this is a quicker source for the data.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:37: const char*
NameFromOrigin(Origin origin) {
On 2011/06/22 17:27:32, cbentzel wrote:
> Is this being used?

Not currently, but I was going to add the info to the net-internals prerender
tab.

[cbentzel, 2011-06-22 19:08:40]

Pretty much looks good. I want to remove the origin histogram, though.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:372: bool
PrerenderManager::AddPrerenderWithNoTag(Origin origin, const GURL& url) {
On 2011/06/22 19:00:51, dominich wrote:
> On 2011/06/22 17:27:32, cbentzel wrote:
> > I don't really like the NoTag name, but can't think of an alternative. Maybe
> > AddTopLevelPrerender - which indicates that it doesn't come from a referrer?
> 
> That to me suggests that it's a prerender of the top-level resource. It's a
> shame overloading is forbidden.
> 
> AddPrerender for this and AddPrerenderFromPage for the older method?

That sounds good.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_origin.cc (right):

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:25: void RecordOrigin(Origin
origin) {
On 2011/06/22 19:00:51, dominich wrote:
> On 2011/06/22 17:27:32, cbentzel wrote:
> > Why do we need to record this? We have all the data we need from the
> FinalStatus
> > histograms. 
> 
> It is calculable from FinalStatus but this is a quicker source for the data.

It's pretty easy to calculate from FinalStauts. I'd prefer to just have one
histograms to prevent chance of skew in the numbers due to forgetting to update
this location.

http://codereview.chromium.org/7210020/diff/12001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.cc:37: const char*
NameFromOrigin(Origin origin) {
On 2011/06/22 19:00:51, dominich wrote:
> On 2011/06/22 17:27:32, cbentzel wrote:
> > Is this being used?
> 
> Not currently, but I was going to add the info to the net-internals prerender
> tab.

OK, I'm fine with it if there is a plan for it's use.

[dominich, 2011-06-22 19:43:56]

Renamed methods and remove (now redundant) PENDING origin.

I feel strongly about keeping the Origin histogram as separate from the final
status, at least for the first version. I want to get a sense if the feature is
even used before putting more effort into being smarter about the feature and
having a measure of that directly will make my life easier.

I understand your concerns about the possibility of divergence between Origin
and FinalStatus but I don't think it is that much of a problem if it does
happen, and I don't see that it will at least in the short term. Once I'm
confident the feature is staying in, I can remove the origin histogram.

[cbentzel, 2011-06-22 20:02:17]

LGTM

Is there any way to write a browser test for this? I guess it's speculative
enough now, but I don't know if there's a way to force the omnibox to provide a
particular suggestion which will reliably trigger a prerender.

[tburkard, 2011-06-23 16:24:07]

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.cc (right):

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:72:
UMA_HISTOGRAM_ENUMERATION("Prerender.FinalStatus_Omnibox",
Why not have a different top level name, such as
Prerender_Omnibox?

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:671: #define RECORD_PLT(tag,
perceived_page_load_time) { \
How does recording PPLT's work?  Will they all be munged together?  I think it'd
be better to have the omnibox ones separate, e.g. using a different toplevel
name.

[dominich, 2011-06-23 16:32:47]

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.cc (right):

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.cc:72:
UMA_HISTOGRAM_ENUMERATION("Prerender.FinalStatus_Omnibox",
On 2011/06/23 16:24:07, tburkard wrote:
> Why not have a different top level name, such as
> Prerender_Omnibox?

After I get this change in, I'm going to look at how to effectively get all of
our histograms split into per-origin histograms (and keep the total numbers
too). This will at least give me some starting statistics on how good the
omnibox prediction is.

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:671: #define RECORD_PLT(tag,
perceived_page_load_time) { \
On 2011/06/23 16:24:07, tburkard wrote:
> How does recording PPLT's work?  Will they all be munged together?  I think
it'd
> be better to have the omnibox ones separate, e.g. using a different toplevel
> name.

All histograms will currently be munged together except final status. I'm
thinking about creating a similar macro to this for all prerender histograms
that will prepend/append the origin.

[tburkard, 2011-06-23 16:35:26]

LGTM

Yes, that cleanup sounds like a good idea.  I had a change already having
such a facility for conducting experiments -- if you implement such a
central facility, be sure that it would cover this case as well.  So please
talk to me about how you are planning to do it before you start working on
it.

Thanks.


On Thu, Jun 23, 2011 at 9:32 AM, <dominich@chromium.org> wrote:

>
> http://codereview.chromium.**org/7210020/diff/20003/chrome/**
>
browser/prerender/prerender_**final_status.cc<http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/prerender_final_status.cc>
> File chrome/browser/prerender/**prerender_final_status.cc (right):
>
> http://codereview.chromium.**org/7210020/diff/20003/chrome/**
>
browser/prerender/prerender_**final_status.cc#newcode72<http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/prerender_final_status.cc#newcode72>
> chrome/browser/prerender/**prerender_final_status.cc:72:
> UMA_HISTOGRAM_ENUMERATION("**Prerender.FinalStatus_Omnibox"**,
> On 2011/06/23 16:24:07, tburkard wrote:
>
>> Why not have a different top level name, such as
>> Prerender_Omnibox?
>>
>
> After I get this change in, I'm going to look at how to effectively get
> all of our histograms split into per-origin histograms (and keep the
> total numbers too). This will at least give me some starting statistics
> on how good the omnibox prediction is.
>
>
> http://codereview.chromium.**org/7210020/diff/20003/chrome/**
>
browser/prerender/prerender_**manager.cc<http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/prerender_manager.cc>
> File chrome/browser/prerender/**prerender_manager.cc (right):
>
> http://codereview.chromium.**org/7210020/diff/20003/chrome/**
>
browser/prerender/prerender_**manager.cc#newcode671<http://codereview.chromium.org/7210020/diff/20003/chrome/browser/prerender/prerender_manager.cc#newcode671>
> chrome/browser/prerender/**prerender_manager.cc:671: #define
> RECORD_PLT(tag, perceived_page_load_time) { \
> On 2011/06/23 16:24:07, tburkard wrote:
>
>> How does recording PPLT's work?  Will they all be munged together?  I
>>
> think it'd
>
>> be better to have the omnibox ones separate, e.g. using a different
>>
> toplevel
>
>> name.
>>
>
> All histograms will currently be munged together except final status.
> I'm thinking about creating a similar macro to this for all prerender
> histograms that will prepend/append the origin.
>
>
>
http://codereview.chromium.**org/7210020/<http://codereview.chromium.org/7210...
>

[Peter Kasting, 2011-06-23 19:10:38]

sky is a better reviewer than me for this; removing myself.

[commit-bot: I haz the power, 2011-06-24 17:55:41]

Change committed as 90384