Fix prerender histograms for multiple prerender case.

Fix prerender histograms for multiple prerender case.

Unfortunately, the prerender histograms mechanism for determining origin was basically a global variable; when multiple prerenders can be in flight at the same time, this doesn't work so well.

To fix this, track the origin of prerenders in the PrerenderData and in the structure keeping track of prerendered web_contents, and pass explicit origins in for histogram recording.

The "wash" determination still uses the window, which is the same as the TTL, and so it's passed in at initialization rather than being another constant in prerender_histograms.cc.


BUG=None


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

[gavinp, 2012-10-05 15:20:43]

This CL is being done up as a prerequisite of
http://codereview.chromium.org/10933065/ , to make reviewing that separate issue
easier.

Timo, WDYT?

[mmenke, 2012-10-05 17:01:15]

This code makes my brain hurt.  :(

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:523: const Origin*
prerender_origin = NULL;
Using a pointer for this seems a little weird and overly complicated.  Why not
just use an Origin, initialize it to ORIGIN_NONE, and have
IsWebContentsPrerendered guaranteed to leave it alone when it returns false (Or
set it to ORIGIN_NONE).

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
So only real prerenders get an origin?  Non-prerenders don't get one, and
neither do "WouldBePrerendered" prerenders?  We could get the origin for the
latter set now, right?

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:539:
fraction_plt_elapsed_at_swap_in);
If was_prerender is false, should we even bother calling this function?  While
it's not exactly obvious what's going on, since the code is more than a little
confusing, it looks like |actual_load_start_| is only set for real prerenders,
so it's NULL exactly when was_prerender is false...Right?

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:505: // Set of WebContents which
are currently displaying a prerendered page.
While you're here, could you clarify this comment?  "displaying" means
WebContents that have been swapped into a tab.

WebContents currently being prerendered are not in the set.

[gavinp, 2012-10-05 20:43:34]

Thanks for the review, Matt.

Is this better?

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:523: const Origin*
prerender_origin = NULL;
On 2012/10/05 17:01:15, Matt Menke wrote:
> Using a pointer for this seems a little weird and overly complicated.  Why not
> just use an Origin, initialize it to ORIGIN_NONE, and have
> IsWebContentsPrerendered guaranteed to leave it alone when it returns false
(Or
> set it to ORIGIN_NONE).

Done.

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:523: const Origin*
prerender_origin = NULL;
On 2012/10/05 17:01:15, Matt Menke wrote:
> Using a pointer for this seems a little weird and overly complicated.  Why not
> just use an Origin, initialize it to ORIGIN_NONE, and have
> IsWebContentsPrerendered guaranteed to leave it alone when it returns false
(Or
> set it to ORIGIN_NONE).

Done.

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
On 2012/10/05 17:01:15, Matt Menke wrote:
> So only real prerenders get an origin?  Non-prerenders don't get one, and
> neither do "WouldBePrerendered" prerenders?  We could get the origin for the
> latter set now, right?

Non prerenders would be tricky, yeah. And it was always weird that in histograms
they were all considered link prerenders.

We could track it for "WouldBePrerendered", yes. But right now we don't in that
case, so in control group, all of these perf histograms will be ORIGIN_NONE,
which is fine by me.

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:539:
fraction_plt_elapsed_at_swap_in);
On 2012/10/05 17:01:15, Matt Menke wrote:
> If was_prerender is false, should we even bother calling this function?  While
> it's not exactly obvious what's going on, since the code is more than a little
> confusing, it looks like |actual_load_start_| is only set for real prerenders,
> so it's NULL exactly when was_prerender is false...Right?

Ah, but in PrerenderTabHelper::DidStopLoading(), the test that controls is on
pplt_load_start_, which is usually not null. Now, in this case,
actual_load_start_ will be null, so the fraction_plt_elapsed_at_swap_in is set
to -1.0, which is a proxy here for NaN, and the first test in that function
bails out.

It's easier to read with the test here, so I'm adding it. Thanks.

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:505: // Set of WebContents which
are currently displaying a prerendered page.
On 2012/10/05 17:01:15, Matt Menke wrote:
> While you're here, could you clarify this comment?  "displaying" means
> WebContents that have been swapped into a tab.
> 
> WebContents currently being prerendered are not in the set.

Done.

[mmenke, 2012-10-05 20:47:45]

On 2012/10/05 20:43:34, gavinp wrote:
> Thanks for the review, Matt.
> 
> Is this better?

I'm sure it will be, once you upload the CL.  :)

I have a couple more comments, but want read carefully through the entire CL
before making them, to cut down on round trips.

[gavinp, 2012-10-05 21:25:13]

On 2012/10/05 20:47:45, Matt Menke wrote:
> On 2012/10/05 20:43:34, gavinp wrote:
> > Thanks for the review, Matt.
> > 
> > Is this better?
> 
> I'm sure it will be, once you upload the CL.  :)

DONED.

> 
> I have a couple more comments, but want read carefully through the entire CL
> before making them, to cut down on round trips.

[tburkard, 2012-10-05 21:54:04]

Very subtle change.  I will need to review this more, but the items mentioned
below are definitely a problem.

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

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
WouldWebContentsBePrerendered must pass back the origin (see my comment about
that in the .h file). If IsWebContentsPrerendered returned false, then
WouldWebContentsBePrerendered must be called, and the appropriate origin must be
retrieved (rather than the ORIGIN_NONE that the false caused to be stored in
prerender_origin).

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:531: prerender_origin,
perceived_page_load_time, url);
prerender_origin is not correct here: if it is currently prerendering,
IsWebContentsPrerendered should return false, but IsWebContentsPrerendering
would return true.  Could you verify that's the case.  I think the way to fix
this is to augment IsWebContentsPrerendering to pass back the origin, just like
IsWebContenstPrerendered did.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:521: WouldBePrerenderedMap
would_be_prerendered_map_;
In order for this to work symmetrically for control group and prerender group,
you must duplicate the map for would_be_prerendered.  However, this already is a
map.  So you'd have to use a struct instead of the bool, and put the bool & the
origin in the struct.

[mmenke, 2012-10-08 18:20:07]

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
On 2012/10/05 20:43:35, gavinp wrote:
> On 2012/10/05 17:01:15, Matt Menke wrote:
> > So only real prerenders get an origin?  Non-prerenders don't get one, and
> > neither do "WouldBePrerendered" prerenders?  We could get the origin for the
> > latter set now, right?
> 
> Non prerenders would be tricky, yeah. And it was always weird that in
histograms
> they were all considered link prerenders.
> 
> We could track it for "WouldBePrerendered", yes. But right now we don't in
that
> case, so in control group, all of these perf histograms will be ORIGIN_NONE,
> which is fine by me.

Sorry, this is fine.  I was getting PrerenderedWebContentsData and
WouldBePrerenderedMap confused, so was thinking we were already tracking it. 
Since we're not tracking it, I strongly agree that it's not worth adding.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.cc:420: Origin origin,
The function name kinda implies the origin.  Think renaming it to
"RecordSimulatedLocalBrowsingTimeUntilUsed" would make the most sense, and could
put it next to the corresponding PLT functions as well.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:203: // Returns true if
|web_contents| is being prerendered. The optional parameter
Could you fix this comment as well?  It's actually if |web_contents| *was*
prerendered.

"Is being prerenered" really sounds like what "IsWebContentsPrerendering"
returns.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_origin.h (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.h:19: ORIGIN_LOCAL_PREDICTOR = 6,
This origin is currently not used in the same histogram as any other origin. 
It's fed directly to 3 histogram functions not used with anything else, and has
no line in PREFIXED_HISTOGRAM_INTERNAL.

It looks like it works fine, this just seems a little odd, and fragile.  If the
predictor is ever enabled, I could see it being switched to this code without
adding it to the PREFIXED_HISTOGRAM_INTERNAL.

I suggest either removing this origin or adding it to the histogram to protect
against such a regression, and maybe get rid of
RecordLocalPredictorTimeUntilUsed and use RecordTimeUntilUsed instead (Though
suppose the histogram is a bit different from the others, so may be best to just
keep them separate).

[gavinp, 2012-10-10 18:14:55]

mmenke and timo, thanks for your reviews.

This code is sublte. I'm going to go over it to make sure I got the
WouldBePrerendered case right.

What do you think?

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/10003/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
On 2012/10/08 18:20:07, Matt Menke wrote:
> On 2012/10/05 20:43:35, gavinp wrote:
> > On 2012/10/05 17:01:15, Matt Menke wrote:
> > > So only real prerenders get an origin?  Non-prerenders don't get one, and
> > > neither do "WouldBePrerendered" prerenders?  We could get the origin for
the
> > > latter set now, right?
> > 
> > Non prerenders would be tricky, yeah. And it was always weird that in
> histograms
> > they were all considered link prerenders.
> > 
> > We could track it for "WouldBePrerendered", yes. But right now we don't in
> that
> > case, so in control group, all of these perf histograms will be ORIGIN_NONE,
> > which is fine by me.
> 
> Sorry, this is fine.  I was getting PrerenderedWebContentsData and
> WouldBePrerenderedMap confused, so was thinking we were already tracking it. 
> Since we're not tracking it, I strongly agree that it's not worth adding.

Timo asked for it in his review, so I've added it. Previously this information
was sort-of-tracked via the global variable in PrerenderHistograms, so adding it
was fair. I would like to talk about how much reporting we do for prerendering
though: please wait for my upcoming mailing list post to be followed by a
contentious team meeting.

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

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:528:
prerender_manager->WouldWebContentsBePrerendered(web_contents);
On 2012/10/05 21:54:04, tburkard wrote:
> WouldWebContentsBePrerendered must pass back the origin (see my comment about
> that in the .h file). If IsWebContentsPrerendered returned false, then
> WouldWebContentsBePrerendered must be called, and the appropriate origin must
be
> retrieved (rather than the ORIGIN_NONE that the false caused to be stored in
> prerender_origin).

Done.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:531: prerender_origin,
perceived_page_load_time, url);
On 2012/10/05 21:54:04, tburkard wrote:
> prerender_origin is not correct here: if it is currently prerendering,
> IsWebContentsPrerendered should return false, but IsWebContentsPrerendering
> would return true.  Could you verify that's the case.  I think the way to fix
> this is to augment IsWebContentsPrerendering to pass back the origin, just
like
> IsWebContenstPrerendered did.

Done.

Good catch. This would have broken stats on all currently running prerenders if
not fixed.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:203: // Returns true if
|web_contents| is being prerendered. The optional parameter
On 2012/10/08 18:20:07, Matt Menke wrote:
> Could you fix this comment as well?  It's actually if |web_contents| *was*
> prerendered.
> 
> "Is being prerenered" really sounds like what "IsWebContentsPrerendering"
> returns.

Done.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:521: WouldBePrerenderedMap
would_be_prerendered_map_;
On 2012/10/05 21:54:04, tburkard wrote:
> In order for this to work symmetrically for control group and prerender group,
> you must duplicate the map for would_be_prerendered.  However, this already is
a
> map.  So you'd have to use a struct instead of the bool, and put the bool &
the
> origin in the struct.

Done, but I used an enum, since bools always confuse me.

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_origin.h (right):

http://codereview.chromium.org/11028037/diff/6004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_origin.h:19: ORIGIN_LOCAL_PREDICTOR = 6,
On 2012/10/08 18:20:07, Matt Menke wrote:
> This origin is currently not used in the same histogram as any other origin. 
> It's fed directly to 3 histogram functions not used with anything else, and
has
> no line in PREFIXED_HISTOGRAM_INTERNAL.
> 
> It looks like it works fine, this just seems a little odd, and fragile.  If
the
> predictor is ever enabled, I could see it being switched to this code without
> adding it to the PREFIXED_HISTOGRAM_INTERNAL.
> 
> I suggest either removing this origin or adding it to the histogram to protect
> against such a regression, and maybe get rid of
> RecordLocalPredictorTimeUntilUsed and use RecordTimeUntilUsed instead (Though
> suppose the histogram is a bit different from the others, so may be best to
just
> keep them separate).

Very good catch. This kind of subtlety is what's made writing this CL and
revising it hard. :/ And I'm sad, but not surprised to hear I missed a neat
special case.

I don't like renaming these to not predictor names, since then someone might
think they should be called for different origins.

Besides adding this origin text, the other thing the macros in
prerender_histograms.cc do is to mix in the experiment, which of course doesn't
make sense for the predictor either (and when present, it's only confusing).

So I'm removing this origin, and moving these histograms out of
prerender_histograms.cc.

http://codereview.chromium.org/11028037/diff/25/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

http://codereview.chromium.org/11028037/diff/25/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_tab_helper.cc:148: if
(prerender_manager->IsWebContentsPrerendering(web_contents(), NULL))
I was really torn about adding this param, or creating
IsWebContentsPrerenderingInternal() which took both args...

[tburkard, 2012-10-11 22:12:11]

http://codereview.chromium.org/11028037/diff/24005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/24005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:213: histograms_.reset(new
PrerenderHistograms(config_.time_to_live));
This value is used to determine WithinWindow in prerender_histograms.  I thought
we had agreed to use a value of 30s here, reflecting that most pageloads
complete within that time.  We definitely don't want to use 5min or 15min here,
otherwise we'll have all washs.

[gavinp, 2012-10-12 13:05:43]

http://codereview.chromium.org/11028037/diff/24005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/24005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:213: histograms_.reset(new
PrerenderHistograms(config_.time_to_live));
On 2012/10/11 22:12:12, tburkard wrote:
> This value is used to determine WithinWindow in prerender_histograms.  I
thought
> we had agreed to use a value of 30s here, reflecting that most pageloads
> complete within that time.  We definitely don't want to use 5min or 15min
here,
> otherwise we'll have all washs.

Yup, you're right. I have reverted this in my latest upload.

[tburkard, 2012-10-12 16:37:39]

lgtm

I guess once this is committed, you can rebase the other CL?  can you send
an update on that other change once the rebase is done?  thanks.

[mmenke, 2012-10-12 17:59:03]

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:410: event,
PrerenderLocalPredictor::EVENT_MAX_VALUE);
This used to call RecordLocalPredictorEvent, not
RecordLocalPredictorTimeUntilUsed

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:535: &prerender_origin);
Couldn't these two bools be moved to the else case?

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:539: prerender_origin,
perceived_page_load_time, url);
optional:  Could return early here, and get rid of the else (And add a line
break before the rest of the code).

My general opinion is if code is packed too tightly, and over 90% of the first
characters in each word are "p[er]{2}", reducing indent and spreading things out
a bit helps increase readability.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:361: SEEN_PROVISIONAL_LOAD,
Think both this struct and this enum are worth comments.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:364: static const State
INITIAL_STATE = WAITING_FOR_PROVISIONAL_LOAD;
What's the reason for this?

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:369: explicit
WouldBePrerenderedWebContentsData(Origin origin);
Constructor (And all methods) should go be all data members except static
consts.

[gavinp, 2012-10-13 13:48:09]

Thanks Matt for the review.

I've updated the patch. Is this closer?

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:410: event,
PrerenderLocalPredictor::EVENT_MAX_VALUE);
On 2012/10/12 17:59:03, Matt Menke wrote:
> This used to call RecordLocalPredictorEvent, not
> RecordLocalPredictorTimeUntilUsed

Indeed. Of the four histograms I moved to this file, I named three of them
incorrectly.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:535: &prerender_origin);
On 2012/10/12 17:59:03, Matt Menke wrote:
> Couldn't these two bools be moved to the else case?

Done.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:539: prerender_origin,
perceived_page_load_time, url);
On 2012/10/12 17:59:03, Matt Menke wrote:
> optional:  Could return early here, and get rid of the else (And add a line
> break before the rest of the code).
> 
> My general opinion is if code is packed too tightly, and over 90% of the first
> characters in each word are "p[er]{2}", reducing indent and spreading things
out
> a bit helps increase readability.

Done.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:361: SEEN_PROVISIONAL_LOAD,
On 2012/10/12 17:59:03, Matt Menke wrote:
> Think both this struct and this enum are worth comments.

Done. I mostly copied from the comment later in the file with the old map.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:364: static const State
INITIAL_STATE = WAITING_FOR_PROVISIONAL_LOAD;
On 2012/10/12 17:59:03, Matt Menke wrote:
> What's the reason for this?

I'm just trying to make the state machine more explicit by naming the initial
state. Since it's an integral type, putting it in the header seemed the way to
go as a const. Should I just put WAITING_FOR_PROVISIONAL_LOAD in the ctor
argument?

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:369: explicit
WouldBePrerenderedWebContentsData(Origin origin);
On 2012/10/12 17:59:03, Matt Menke wrote:
> Constructor (And all methods) should go be all data members except static
> consts.

Done.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:369: explicit
WouldBePrerenderedWebContentsData(Origin origin);
On 2012/10/12 17:59:03, Matt Menke wrote:
> Constructor (And all methods) should go be all data members except static
> consts.

Done.

[mmenke, 2012-10-15 15:28:27]

Not signing off on this because of the "IMPORTANT" issue.  Once that's
addressed, I'm good.

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:410: event,
PrerenderLocalPredictor::EVENT_MAX_VALUE);
On 2012/10/13 13:48:09, gavinp wrote:
> On 2012/10/12 17:59:03, Matt Menke wrote:
> > This used to call RecordLocalPredictorEvent, not
> > RecordLocalPredictorTimeUntilUsed
> 
> Indeed. Of the four histograms I moved to this file, I named three of them
> incorrectly.

Hmm...Looks like two, to me (Which are both now fixed).

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/11028037/diff/33001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.h:364: static const State
INITIAL_STATE = WAITING_FOR_PROVISIONAL_LOAD;
On 2012/10/13 13:48:09, gavinp wrote:
> On 2012/10/12 17:59:03, Matt Menke wrote:
> > What's the reason for this?
> 
> I'm just trying to make the state machine more explicit by naming the initial
> state. Since it's an integral type, putting it in the header seemed the way to
> go as a const. Should I just put WAITING_FOR_PROVISIONAL_LOAD in the ctor
> argument?

I'd prefer that.  As-is, just stands out as being a little weird /
non-Chrome-style-like.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:38: bool is_wash, const
std::string& name) {
Random comment for future refactoring:  This seems to completely mirror the
logic in PREFIXED_HISTOGRAM_INTERNAL.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:213:
UMA_HISTOGRAM_CUSTOM_TIMES( \
While you're here, believe this should be indented two spaces more.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:381: 100);
RecordSimulatedLocalBrowsingBaselinePLT and RecordSimulatedLocalBrowsingPLT did
nothing if !IsWebURL(url).  It looks to me like this was redundant, since we
don't prerender other URLs, anyways.  This change was deliberate, right?

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:174: }
While you're here, please put the constructor first.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:561:
IsWebContentsPrerendered(web_contents, &origin);
Maybe add a DCHECK that IsWebContentsPrerendered returns true?  Would need to
cache the result, or call the function twice, of course.

bool is_prerendered = IsWebContentsPrerendered(web_contents, &origin);
DCHECK(is_prerendered);

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:655: return true;
IMPORTANT:  Add brackets!

[mmenke, 2012-10-15 15:31:30]

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:111: HISTOGRAM; \
Wait...you need a NONE case in here, too, I believe.

[mmenke, 2012-10-15 15:34:04]

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:111: HISTOGRAM; \
On 2012/10/15 15:31:30, Matt Menke wrote:
> Wait...you need a NONE case in here, too, I believe.

Actually, we should never be returning such a histogram, right?  So do we even
need it in GetHistogramName()?  Either way, we should either be using it in both
locations or neither.

[mmenke, 2012-10-15 15:37:15]

Last comment from me, for now...Erm...I hope.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:655: return true;
On 2012/10/15 15:28:28, Matt Menke wrote:
> IMPORTANT:  Add brackets!

One other comment:  We really should have unit tests for these functions.

[gavinp, 2012-10-15 18:32:39]

+ben for OWNERS review

Thank you for the reviews, Matt.

Ben: Can you look at chrome_content_browser_client.cc and
task_manager_resource_providers.cc ?

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_histograms.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:111: HISTOGRAM; \
On 2012/10/15 15:34:04, Matt Menke wrote:
> On 2012/10/15 15:31:30, Matt Menke wrote:
> > Wait...you need a NONE case in here, too, I believe.
> 
> Actually, we should never be returning such a histogram, right?  So do we even
> need it in GetHistogramName()?  Either way, we should either be using it in
both
> locations or neither.

Done.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_histograms.cc:213:
UMA_HISTOGRAM_CUSTOM_TIMES( \
On 2012/10/15 15:28:28, Matt Menke wrote:
> While you're here, believe this should be indented two spaces more.

Done.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:381: 100);
On 2012/10/15 15:28:28, Matt Menke wrote:
> RecordSimulatedLocalBrowsingBaselinePLT and RecordSimulatedLocalBrowsingPLT
did
> nothing if !IsWebURL(url).  It looks to me like this was redundant, since we
> don't prerender other URLs, anyways.  This change was deliberate, right?

Yes. I was hoping to cut the confusion a bit by that test. I'm happy to add it
back if you think being double-sure makes for better readability (which
sometimes it does in these long code paths...).

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:174: }
On 2012/10/15 15:28:28, Matt Menke wrote:
> While you're here, please put the constructor first.

Done.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:561:
IsWebContentsPrerendered(web_contents, &origin);
On 2012/10/15 15:28:28, Matt Menke wrote:
> Maybe add a DCHECK that IsWebContentsPrerendered returns true?  Would need to
> cache the result, or call the function twice, of course.
> 
> bool is_prerendered = IsWebContentsPrerendered(web_contents, &origin);
> DCHECK(is_prerendered);

Done.

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:655: return true;
On 2012/10/15 15:37:15, Matt Menke wrote:
> On 2012/10/15 15:28:28, Matt Menke wrote:
> > IMPORTANT:  Add brackets!
> 

OMG. Fixed.

> One other comment:  We really should have unit tests for these functions.

Yes; although that amounts to unit tests for histograms, which is a sort of
scary precedent.  I've added some calls through existing unit tests which should
test these.

[gavinp, 2012-10-15 18:43:00]

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:655: return true;
On 2012/10/15 18:32:39, gavinp wrote:
> On 2012/10/15 15:37:15, Matt Menke wrote:
> > On 2012/10/15 15:28:28, Matt Menke wrote:
> > > IMPORTANT:  Add brackets!
> > 
> 
> OMG. Fixed.
> 
> > One other comment:  We really should have unit tests for these functions.
> 
> Yes; although that amounts to unit tests for histograms, which is a sort of
> scary precedent.  I've added some calls through existing unit tests which
should
> test these.

Actually, I added a whole new unit test file: prerender_manager_unittest.cc

[mmenke, 2012-10-15 18:46:21]

LGTM

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/45001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:655: return true;
On 2012/10/15 18:32:39, gavinp wrote:
> On 2012/10/15 15:37:15, Matt Menke wrote:
> > On 2012/10/15 15:28:28, Matt Menke wrote:
> > > IMPORTANT:  Add brackets!
> > 
> 
> OMG. Fixed.
> 
> > One other comment:  We really should have unit tests for these functions.
> 
> Yes; although that amounts to unit tests for histograms, which is a sort of
> scary precedent.  I've added some calls through existing unit tests which
should
> test these.

IsWebContentsPrerendering is actually used for prerender cancellation on some
errors, when to use the TabContents icon, and maybe other things as well.  The
control group equivalent is not, of course.

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:183: : origin(origin),
nit:  4 space indent.

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager_unittest.cc:47: EXPECT_EQ(origin,
test_origin);
Suggest another false case, after adding one WebContents, making sure another
one results false.  That still wouldn't have caught the bug, but it would have
caught an equivalent bug in the other loop.

[gavinp, 2012-10-16 13:52:00]

Ben: any chance of a rubber stamp today?

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager.cc:183: : origin(origin),
On 2012/10/15 18:46:22, Matt Menke wrote:
> nit:  4 space indent.

Done.

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_manager_unittest.cc (right):

http://codereview.chromium.org/11028037/diff/48005/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_manager_unittest.cc:47: EXPECT_EQ(origin,
test_origin);
On 2012/10/15 18:46:22, Matt Menke wrote:
> Suggest another false case, after adding one WebContents, making sure another
> one results false.  That still wouldn't have caught the bug, but it would have
> caught an equivalent bug in the other loop.

Done.

[Ben Goodger (Google), 2012-10-16 18:17:06]

lgtm

[commit-bot: I haz the power, 2012-10-17 14:01:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11028037/43005

[commit-bot: I haz the power, 2012-10-17 16:01:03]

Change committed as 162418