Change multi-prerender API to include per launcher slots.

Change multi-prerender API to include per launcher slots.

After https://codereview.chromium.org/11459003/ lands, this API is the last bit required for multiple prerenders to be well supported by applications.

This change moves concurrency enforcement out of PrerenderManager and into PrerenderLinkManager; this has the effect of in the default case of one prerender allowing
two at a time including one for the omnibox.

R=mmenke@chromium.org
BUG=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174751

[gavinp, 2012-12-12 18:50:45]

Matt,

This is still baking, and not really ready for review, but it is the CL that
creates the slots for prerendering, and I wanted to share it early if you want
to look at it.

I'm letting this CL get its first run at the try jobs right now, and I'll go
remediate our earlier reviews.

[gavinp, 2012-12-15 17:29:09]

Matt,

Please take a look at this, the new slot interface. It could go a lot farther,
and probably should: we still have some leaks in some of the tables still, and I
think this new prerenders_ list in PrerenderLinkManager could handle all of the
pending prerenders, removing that complexity from the PrerenderContents and the
PrerenderManager.

[mmenke, 2012-12-17 18:34:57]

One quick high level comment.

Still need to dig into the code, but haven't seen anything else significant.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: }
Hmm...Multiple prerenders with the same PrerenderContents count towards the max,
as do pending prerenders.

Suppose a page want to keep a prerender of another page going...and it's not
smart enough to use the API.  Could see a ton of queued prerenders resulting.

Also, I wonder about prerendering very stale prerender requests.  If a request
was issued 20 minutes ago, but we've had prerenders from other pages running
since then....should we really prerender the page?

[mmenke, 2012-12-17 20:02:23]

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:606:
render_view_host_observer_->set_prerender_contents(NULL);
Why does this need to be moved?

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_histograms.cc:177:
kMaxRecordableConcurrency),
May want to rename the histogram, so can look at histograms from multiple OS /
chrome versions without getting junk data.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:113: RemovePrerender(&(*it));
Erm...  This seems to violate the comment at the end of OnAbandonPrerender. 
Should we only be removing the prerenders that haven't been started, and relying
on the cancel events of the others?

As-is, it's also not clear when we'll be calling StartPrerender(), if we have
other prerenders pending and end up removing an active one from the list here,
unexpectedly.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: }
On 2012/12/17 18:34:57, Matt Menke wrote:
> Hmm...Multiple prerenders with the same PrerenderContents count towards the
max,
> as do pending prerenders.
> 
> Suppose a page want to keep a prerender of another page going...and it's not
> smart enough to use the API.  Could see a ton of queued prerenders resulting.
> 
> Also, I wonder about prerendering very stale prerender requests.  If a request
> was issued 20 minutes ago, but we've had prerenders from other pages running
> since then....should we really prerender the page?

Also, seems like if we have one active prerender, which tries to prerender
another page, that prerender will occupy two active prerender slots, until its
cancelled.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:243: i->handle = NULL;
Don't think this does anything useful.

[gavinp, 2012-12-18 00:44:13]

thanks for that first look.

I'm thinking about the problems with pending prerenders and duplicate contenses
some more. My hope was to delay that for later CLs, but perhaps they're serious
enough that we'll want them from the start.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:606:
render_view_host_observer_->set_prerender_contents(NULL);
On 2012/12/17 20:02:23, Matt Menke wrote:
> Why does this need to be moved?

It doesn't. I just moved it while I was debugging a reentrancy issue, but that's
now guarded in the prerenderlinkmanager. So no point having this file in the CL
at all.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_histograms.cc:177:
kMaxRecordableConcurrency),
On 2012/12/17 20:02:23, Matt Menke wrote:
> May want to rename the histogram, so can look at histograms from multiple OS /
> chrome versions without getting junk data.

Well, it sort of is renamed, right? It's now "CountOf20" until we need to
increase that? Or am I missing something?

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:113: RemovePrerender(&(*it));
On 2012/12/17 20:02:23, Matt Menke wrote:
> Erm...  This seems to violate the comment at the end of OnAbandonPrerender. 
> Should we only be removing the prerenders that haven't been started, and
relying
> on the cancel events of the others?
> 
> As-is, it's also not clear when we'll be calling StartPrerender(), if we have
> other prerenders pending and end up removing an active one from the list here,
> unexpectedly.

Guh, good point. I moved the remove up into OnAbandonPrerender, and so the
DCHECK can't be here any more. I added a DCHECK here to make sure we don't need
to start any prerenders.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: }
On 2012/12/17 20:02:23, Matt Menke wrote:
> On 2012/12/17 18:34:57, Matt Menke wrote:
> > Hmm...Multiple prerenders with the same PrerenderContents count towards the
> max,
> > as do pending prerenders.
> > 
> > Suppose a page want to keep a prerender of another page going...and it's not
> > smart enough to use the API.  Could see a ton of queued prerenders
resulting.
> > 
> > Also, I wonder about prerendering very stale prerender requests.  If a
request
> > was issued 20 minutes ago, but we've had prerenders from other pages running
> > since then....should we really prerender the page?
> 
> Also, seems like if we have one active prerender, which tries to prerender
> another page, that prerender will occupy two active prerender slots, until its
> cancelled.

Hrm. I was hoping to delay adding handling for duplicates and pending prerenders
until a followup CL, but these issues make me think they might need to be dealt
with sooner.

In the standard case (concurrency = 1) it's not a problem except for the
duplicates, which hopefully isn't a common case (the UMAs say it's mostly
omnibox doing this).

In the multi prerender case though; these gaps are more serious.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:243: i->handle = NULL;
On 2012/12/17 20:02:23, Matt Menke wrote:
> Don't think this does anything useful.

There's a DCHECK() in PrerenderListManager::~Prerender which asserts that handle
is NULL. By zeroing it out before erasing, we make a bit more sure that there
isn't some destruction of a PrerenderData that is leaking one of these.

[mmenke, 2012-12-18 17:15:14]

I still need to take a look at the unit tests.  This is going to be another
review-heavy day for me.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_histograms.cc:177:
kMaxRecordableConcurrency),
On 2012/12/18 00:44:14, gavinp wrote:
> On 2012/12/17 20:02:23, Matt Menke wrote:
> > May want to rename the histogram, so can look at histograms from multiple OS
/
> > chrome versions without getting junk data.
> 
> Well, it sort of is renamed, right? It's now "CountOf20" until we need to
> increase that? Or am I missing something?
> 

You're right...I just saw histogram statement, with the changed limit, and
assumed you were changing the max of the histogram here, not the histogram name.

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: }
On 2012/12/18 00:44:14, gavinp wrote:
> On 2012/12/17 20:02:23, Matt Menke wrote:
> > On 2012/12/17 18:34:57, Matt Menke wrote:
> > > Hmm...Multiple prerenders with the same PrerenderContents count towards
the
> > max,
> > > as do pending prerenders.
> > > 
> > > Suppose a page want to keep a prerender of another page going...and it's
not
> > > smart enough to use the API.  Could see a ton of queued prerenders
> resulting.
> > > 
> > > Also, I wonder about prerendering very stale prerender requests.  If a
> request
> > > was issued 20 minutes ago, but we've had prerenders from other pages
running
> > > since then....should we really prerender the page?
> > 
> > Also, seems like if we have one active prerender, which tries to prerender
> > another page, that prerender will occupy two active prerender slots, until
its
> > cancelled.
> 
> Hrm. I was hoping to delay adding handling for duplicates and pending
prerenders
> until a followup CL, but these issues make me think they might need to be
dealt
> with sooner.
> 
> In the standard case (concurrency = 1) it's not a problem except for the
> duplicates, which hopefully isn't a common case (the UMAs say it's mostly
> omnibox doing this).
> 
> In the multi prerender case though; these gaps are more serious.

Speaking of which, you did switch the omnibox to cancelling rather than relying
that behavior in an earlier CL, didn't you?

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:243: i->handle = NULL;
On 2012/12/18 00:44:14, gavinp wrote:
> On 2012/12/17 20:02:23, Matt Menke wrote:
> > Don't think this does anything useful.
> 
> There's a DCHECK() in PrerenderListManager::~Prerender which asserts that
handle
> is NULL. By zeroing it out before erasing, we make a bit more sure that there
> isn't some destruction of a PrerenderData that is leaking one of these.

Sounds reasonable.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_config.h (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_config.h:22: // Number of simultaneous
prerendered pages allowed.
This is no longer correct.  Suggest something like "Number of simultaneous
prerender pages from link elements allowed.  Enforced by PrerenderLinkManager."

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_config.h:23: size_t max_concurrency;
|max_link_concurrency|?

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_histograms.cc:171: size_t max_concurrency)
const {
Since there is no longer a real "max_concurrency", I think it's a little odd to
take it as an argument.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:82:
RemovePrerender(prerender);
Is the goal of all this not to send stop events for prerenders cancelled by the
other process, or to make sure we delete prerenders that haven't yet been
started?

Think it's worth a comment.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:85:
own_prerender_handle->OnCancel();
Just a general comment:  Think it's rather unfortunate that in a couple of these
functions we have to deal with 3 or 4 cases separately:

1)  It doesn't exist due to an expected race (Or because of a deliberate design
decision).
2)  We haven't started prerendering.
3)  We have started prerendering, but it's a pending prerender.
4)  It has started, and has a WebContents.

Suppose we can get rid of 3) in a future CL by handling all the pending
prerender logic here, though it may be a little polling heavy (Check if the
requester is prerendering before starting a prerender.  If it is, don't start. 
We'll also need a callback from the PrerenderManager when a prerender is
displayed).

We need to make sure we testing all the cases for all these functions.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:88: }
nit:  Remove indent.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:93: if (!prerender ||
!prerender->handle)
If !prerender->handle, we should still be removing the prerender, right?  Do we
have a unit test that covers this?

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:100: if (!prerender->handle
|| prerender->handle->IsPrerendering())
Why check prernder->handle here?  If OnNavigateAway clears it, then it will have
presumably deleted the prerender as well, and we'll have more serious issues...

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:190: ++next;
Maybe a DCHECK_EQ(CountRunningPrerenders(), running_count);?

Could also (Or alternatively) put one just before this loop.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:241:
PrerenderLinkManager::FindByPrerenderHandle(PrerenderHandle* prerender_handle) {
DCHECK(prerender_handle);

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:115: bool is_shutdown_;
|has_shutdown_| may be clearer, since is_shutdown has two interpretations.  No
ambiguity if one thinks about it, but that requires thought.  Maybe my internal
lexical parser just needs an upgrade.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:121: // at the front.
Youngest prerender is at the back.

[gavinp, 2012-12-18 20:15:12]

+shishir: Can you take a look at the autocomplete action predictor changes?

Matt: WDYT?

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/34005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: }
On 2012/12/18 17:15:15, Matt Menke wrote:
> On 2012/12/18 00:44:14, gavinp wrote:
> > On 2012/12/17 20:02:23, Matt Menke wrote:
> > > On 2012/12/17 18:34:57, Matt Menke wrote:
> > > > Hmm...Multiple prerenders with the same PrerenderContents count towards
> the
> > > max,
> > > > as do pending prerenders.
> > > > 
> > > > Suppose a page want to keep a prerender of another page going...and it's
> not
> > > > smart enough to use the API.  Could see a ton of queued prerenders
> > resulting.
> > > > 
> > > > Also, I wonder about prerendering very stale prerender requests.  If a
> > request
> > > > was issued 20 minutes ago, but we've had prerenders from other pages
> running
> > > > since then....should we really prerender the page?
> > > 
> > > Also, seems like if we have one active prerender, which tries to prerender
> > > another page, that prerender will occupy two active prerender slots, until
> its
> > > cancelled.
> > 
> > Hrm. I was hoping to delay adding handling for duplicates and pending
> prerenders
> > until a followup CL, but these issues make me think they might need to be
> dealt
> > with sooner.
> > 
> > In the standard case (concurrency = 1) it's not a problem except for the
> > duplicates, which hopefully isn't a common case (the UMAs say it's mostly
> > omnibox doing this).
> > 
> > In the multi prerender case though; these gaps are more serious.
> 
> Speaking of which, you did switch the omnibox to cancelling rather than
relying
> that behavior in an earlier CL, didn't you?

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_config.h (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_config.h:22: // Number of simultaneous
prerendered pages allowed.
On 2012/12/18 17:15:15, Matt Menke wrote:
> This is no longer correct.  Suggest something like "Number of simultaneous
> prerender pages from link elements allowed.  Enforced by
PrerenderLinkManager."

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_config.h:23: size_t max_concurrency;
On 2012/12/18 17:15:15, Matt Menke wrote:
> |max_link_concurrency|?

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_histograms.cc:171: size_t max_concurrency)
const {
On 2012/12/18 17:15:15, Matt Menke wrote:
> Since there is no longer a real "max_concurrency", I think it's a little odd
to
> take it as an argument.

But max_link_concurrency  is also used in here as a proxy for being on the multi
prerender experiment. So I've moved a test on that experiment into prerender
manager.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:82:
RemovePrerender(prerender);
On 2012/12/18 17:15:15, Matt Menke wrote:
> Is the goal of all this not to send stop events for prerenders cancelled by
the
> other process, or to make sure we delete prerenders that haven't yet been
> started?
> 
> Think it's worth a comment.

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:85:
own_prerender_handle->OnCancel();
On 2012/12/18 17:15:15, Matt Menke wrote:
> Just a general comment:  Think it's rather unfortunate that in a couple of
these
> functions we have to deal with 3 or 4 cases separately:
> 
> 1)  It doesn't exist due to an expected race (Or because of a deliberate
design
> decision).
> 2)  We haven't started prerendering.
> 3)  We have started prerendering, but it's a pending prerender.
> 4)  It has started, and has a WebContents.
> 
> Suppose we can get rid of 3) in a future CL by handling all the pending
> prerender logic here, though it may be a little polling heavy (Check if the
> requester is prerendering before starting a prerender.  If it is, don't start.

> We'll also need a callback from the PrerenderManager when a prerender is
> displayed).
> 
> We need to make sure we testing all the cases for all these functions.

Yes, it is unfortunate. Pulling things into the LinkManager will simplify a lot
of these; the special cases though are already mirrored through PrerenderManager
and other places. I wish Prerendering could be made a lot less special case
heavy.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:88: }
On 2012/12/18 17:15:15, Matt Menke wrote:
> nit:  Remove indent.

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:93: if (!prerender ||
!prerender->handle)
On 2012/12/18 17:15:15, Matt Menke wrote:
> If !prerender->handle, we should still be removing the prerender, right?  Do
we
> have a unit test that covers this?

Added  a few more tests walking us through these cases, and confirmed these
lines are key to them passing.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:100: if (!prerender->handle
|| prerender->handle->IsPrerendering())
On 2012/12/18 17:15:15, Matt Menke wrote:
> Why check prernder->handle here?  If OnNavigateAway clears it, then it will
have
> presumably deleted the prerender as well, and we'll have more serious
issues...

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:190: ++next;
On 2012/12/18 17:15:15, Matt Menke wrote:
> Maybe a DCHECK_EQ(CountRunningPrerenders(), running_count);?
> 
> Could also (Or alternatively) put one just before this loop.

I think it'd DCHECK_LE, right? Some prerenders have handles but not all are
running. I'll change names to make this less confusing.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:241:
PrerenderLinkManager::FindByPrerenderHandle(PrerenderHandle* prerender_handle) {
On 2012/12/18 17:15:15, Matt Menke wrote:
> DCHECK(prerender_handle);

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:115: bool is_shutdown_;
On 2012/12/18 17:15:15, Matt Menke wrote:
> |has_shutdown_| may be clearer, since is_shutdown has two interpretations.  No
> ambiguity if one thinks about it, but that requires thought.  Maybe my
internal
> lexical parser just needs an upgrade.

Done.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:121: // at the front.
On 2012/12/18 17:15:15, Matt Menke wrote:
> Youngest prerender is at the back.

Ya, the other front.

Done.

[gavinp, 2012-12-18 21:09:41]

Matt,

Here's the max wait timeout that we talked about. Rather than an epsilon, I just
put it in the config.

[gavinp, 2012-12-19 17:15:38]

On 2012/12/18 21:09:41, gavinp wrote:
> Matt,
> 
> Here's the max wait timeout that we talked about. Rather than an epsilon, I
just
> put it in the config.

Guh, it looks like this patch is flaking the InfiniteLoop browser tests.

[gavinp, 2012-12-19 18:26:56]

On 2012/12/19 17:15:38, gavinp wrote:
> On 2012/12/18 21:09:41, gavinp wrote:
> > Matt,
> > 
> > Here's the max wait timeout that we talked about. Rather than an epsilon, I
> just
> > put it in the config.
> 
> Guh, it looks like this patch is flaking the InfiniteLoop browser tests.

Don't review this until I upload a deflake, or at least admit defeat.

[gavinp, 2012-12-20 17:09:35]

-Shishir (he's on vacation)

+Dominic

Dominic: Can you take a look at predictors?

Matt: I've admitted defeat, for now, against the infinite loop flake. We have an
open bug and are prioritizing this in the new year.

[mmenke, 2012-12-20 17:14:42]

On 2012/12/20 17:09:35, gavinp wrote:
> -Shishir (he's on vacation)
> 
> +Dominic
> 
> Dominic: Can you take a look at predictors?
> 
> Matt: I've admitted defeat, for now, against the infinite loop flake. We have
an
> open bug and are prioritizing this in the new year.

I'll take a look at it tomorrow (I'm technically taking tomorrow off, but I
figure I'll head out early today, and spend a couple hours tomorrow doing
reviews)

[mmenke, 2012-12-28 18:25:33]

Think it looks good - a bunch of comments, but mostly nits / test suggestions /
considerations for future CLs.

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/47005/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:190: ++next;
On 2012/12/18 20:15:12, gavinp wrote:
> On 2012/12/18 17:15:15, Matt Menke wrote:
> > Maybe a DCHECK_EQ(CountRunningPrerenders(), running_count);?
> > 
> > Could also (Or alternatively) put one just before this loop.
> 
> I think it'd DCHECK_LE, right? Some prerenders have handles but not all are
> running. I'll change names to make this less confusing.

Correct - I made an incorrect assumption about what CountRunningPrerenders()
did, based on the naming.  Changing the names seems like a good idea.  :)

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/predictors...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/predictors...
chrome/browser/predictors/autocomplete_action_predictor.cc:144:
prerender_handle_->OnCancel();
Hmm...  Calling "OnCancel" to cancel something is a little strange.  Suppose the
naming makes the most sense, though, as long as OnNavigateAway is also a
PrerenderHandle function.

Suggestion for a later CL:  Since OnNavigateAway is a <channel
user="jam">prerender link concept, not a general prerender concept</channel>, we
may want to structure things to get rid of the function, and just have
PrerenderLinkManager doing something like calling
"PrerenderManager::UpdateTimeoutForPrerender" or some such, and renaming this
function to Cancel().

We should put some thought into the division of labor between the two *Manager
classes.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:1710:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, DISABLED_PrerenderInfiniteLoop) {
Just out of curiousity, (i.e. I'm fine with disabling these tests in this CL)
what type of failure were you seeing here?  Timing out, assertion failure,
crashes, zombie apocalypses?

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:149:
DCHECK_EQ(static_cast<PrerenderHandle*>(NULL), handle);
Think this is non-intuitive enough to be worth a comment - just something along
the lines of "Make sure the PrerenderHandle was destroyed before the Prerender."

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:166: void
PrerenderLinkManager::StartPrerenders() {
Some thoughts on this change (No need to address these in this CL):

We no longer see stats involved never started prerenders.  This will affect
existing histograms.  We may want to add new histograms (Blargh).  Think adding
fake FINAL_STATUS results directly to the histograms here would be a bit of a
layering violation.

net-internal's prerender tab won't display anything about these.  If we care
enough to have the tab display "active" prerenders in the first place, I assume
we care enough to add this information.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: std::set<std::pair<int,
int> > launcher_and_prerender_id_set;
This is only used for sanity checking - are compilers smart enough to optimize
it out in release builds?

I'm not sure we actually need it - we have DCHECKs renderer-side for double
starts already.  Not a big fan of debugging-only code making it into production
binaries, unless we need it.

If we keep this, I think we should have a comment that it's only for DCHECKing,
and rename it to running_launcher_and_prerender_id_set, so these all have
consistent names.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:172: size_t
added_prerender_count = 0;
nit:  Think "running" or "started" would be clearer than "added".  "running" is
used in the variable below, so would be the more consistent choice.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:174:
running_launcher_and_render_view_route_set;
optional:  You may want to use a map here instead of a multiset.  multiset does
exactly what you want, uses less C++ code than map of pairs, and may compile
into marginally smaller code.  However, there are fewer than 30 files that use
them in the entire Chrome code base (Only 3 in chrome/, 5 in content/), so may
be easier for people to follow using a map here.  I leave the choice to you.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:174:
running_launcher_and_render_view_route_set;
optional:  May want to just pluralize this and remove the "set".  Same goes for
the other "set".  Given the mix of a set and a multiset, don't think the "set"
helps much, and I've noticed a tendency for STL types to switch over time (Not
that I think it's likely here, just not sure the suffixes are worth it, in
general).

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:230: if (prerender_age >=
manager_->config().max_wait_to_launch) {
This should go before checking the launcher concurrency limit, since once this
has happened, we'll never launch the prerender, anyways.

Not exactly a huge/relevant optimization, but seems the logical thing to be
doing.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:69: struct Prerender {
Think this name may be be a little too vague - I like something that can easily
be searched for, even if it's only used in one file.  LinkRelPrerender? 
LinkPrerender?

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:105: int prerender_id);
Think it's worth mentioning:

1)  This is only used by tests (So therefore can be a little funky in the
multiple prerender case).
2)  It only returns the first started active prerender with the given launcher.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:327: bool PrerenderHasHandle(int
child_id, int prerender_id) {
Think this name is confusing.  LauncherHasRunningPrerender?

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:329: prerender_link_manager()->
FindByLauncherChildIdAndPrerenderId(
nit:  Remove space

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:491: TEST_F(PrerenderTest,
MaxConcurrencyTest) {
Three test suggestions, with per_launcher = 1, max_link_concurrency = 2:

Two prerenders from:

1)  Same RenderView.
2)  Same process, different RenderViews.
3)  Different processes, same RenderView ID (Since IDs are only unique within a
process).

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:499:
prerender_manager()->config(). max_link_concurrency_per_launcher},
nit:  Remove space.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:549:
EXPECT_TRUE(prerender_contents_to_delay->prerendering_has_started());
Suggest checking this at the end of the above loop (Should start when the first
one is used, not just after the last one).

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:596: TEST_F(PrerenderTest,
RateLimitInWindowTest) {
Test comment and name no longer make any sense.  Suggest deleting it, since it
doesn't seem to be testing anything MaxConcurrencyTest doesn't test for.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:620: TEST_F(PrerenderTest,
RateLimitOutsideWindowTest) {
This is now identical to the above test, so should be deleted.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:627:
FINAL_STATUS_MANAGER_SHUTDOWN);
If we wanted to test the same thing as before, this should be TIMED_OUT, but
that doesn't really get us anything...  Closesy analog would be to modify
ExpireTest (Or make a modified copy of it) to have a pending prerender when the
first prerender times out.

Not sure if we absolutely need such a test, but I think it would be a reasonable
test to have, and seems to fit with the 3 abandon/timeout tests at the bottom
(None of which currently start the pending prerender).

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1139: TEST_F(PrerenderTest,
LinkManagerAddTwiceAbandonTwice) {
nit:  May want to rename to LinkManagerAddTwiceAbandonTwiceUseTwice while you're
here.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1276: TEST_F(PrerenderTest,
LinkManagerClearOnAbandon) {
I think "LinkManagerAbandonPendingPrerender" (Slowly redefining "pending" - see
note below) would be clearer.  If you prefer not to use "pending", then maybe
"LinkManagerAbandonInactivePrerender"?

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1276: TEST_F(PrerenderTest,
LinkManagerClearOnAbandon) {
You're depending on max_link_concurrency being 1 in this test.  Suggest
explicitly setting it in tests that depend on it.  Suspect at some point we'll
want to be able to have multiple prerenders active from a single site.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1311: // are cleared from the
PrerenderLinkManager.
Description should mention the second prerender is launched by the first.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1312: TEST_F(PrerenderTest,
LinkManagerClearOnPendingAbandon) {
Suggest renaming the test.  Now that PrerenderLinkManager has prerenders that
haven't yet started, but may be started in the future, we have another type of
prerender that is "pending".

I think using "pending" solely to describe prerenders that haven't yet started
due to being prerenders that a prerendered page tried to start is confusing, and
is terminology we should move away from.

Suggested name:  LinkManagerAbandonPrerenderWithChildPrerender (Or
"WithLinkRelPrerender")

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1356: TEST_F(PrerenderTest,
LinkManagerWaitToLaunch) {
Suggest calling this LinkManagerWaitToLaunchNotLaunched or somesuch -
"WaitToLaunch" kinda has the implication of actually being launched after
waiting to do so.

[mmenke, 2012-12-28 18:32:39]

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:174:
running_launcher_and_render_view_route_set;
On 2012/12/28 18:25:33, Matt Menke wrote:
> optional:  May want to just pluralize this and remove the "set".  Same goes
for
> the other "set".  Given the mix of a set and a multiset, don't think the "set"
> helps much, and I've noticed a tendency for STL types to switch over time (Not
> that I think it's likely here, just not sure the suffixes are worth it, in
> general).

This is one of those time-of-day comments, btw - if it were between 10 am and 2
pm when reviewing, I may say one thing, between 2 pm and 6 pm, I may say
another.  After 6 pm, I may not comment either way.  Completely up to you.

[mmenke, 2012-12-28 18:35:18]

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:105: int prerender_id);
On 2012/12/28 18:25:33, Matt Menke wrote:
> Think it's worth mentioning:
> 
> 1)  This is only used by tests (So therefore can be a little funky in the
> multiple prerender case).
> 2)  It only returns the first started active prerender with the given
launcher.

Blegh - I was thinking this was child_id and renderer_id.  Ignore the comment.

[gavinp, 2012-12-28 19:53:46]

Most nits fixed, and I agree some future CLs would clean this up a lot further.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/predictors...
File chrome/browser/predictors/autocomplete_action_predictor.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/predictors...
chrome/browser/predictors/autocomplete_action_predictor.cc:144:
prerender_handle_->OnCancel();
On 2012/12/28 18:25:33, Matt Menke wrote:
> Hmm...  Calling "OnCancel" to cancel something is a little strange.  Suppose
the
> naming makes the most sense, though, as long as OnNavigateAway is also a
> PrerenderHandle function.
> 
> Suggestion for a later CL:  Since OnNavigateAway is a <channel
> user="jam">prerender link concept, not a general prerender concept</channel>,
we
> may want to structure things to get rid of the function, and just have
> PrerenderLinkManager doing something like calling
> "PrerenderManager::UpdateTimeoutForPrerender" or some such, and renaming this
> function to Cancel().
> 
> We should put some thought into the division of labor between the two *Manager
> classes.

Yes. I think this series of CLs has cleaned up the existing behaviour some on
the whole, but introduced enough new behaviour that things are probably as
confusing as before (if not more). Separating the link manager out more could be
a good way to go.

I've also considered splitting the prerender manager into the prerender
interface (the prerender link manager + some stuff) and prerender-runner. Those
two phases are related, but that split would certainly make life easier for
consumers, and we could then re-unify the interface for omnibox/local prerenders
and links.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:1710:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, DISABLED_PrerenderInfiniteLoop) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Just out of curiousity, (i.e. I'm fine with disabling these tests in this CL)
> what type of failure were you seeing here?  Timing out, assertion failure,
> crashes, zombie apocalypses?

Timeouts.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:149:
DCHECK_EQ(static_cast<PrerenderHandle*>(NULL), handle);
On 2012/12/28 18:25:33, Matt Menke wrote:
> Think this is non-intuitive enough to be worth a comment - just something
along
> the lines of "Make sure the PrerenderHandle was destroyed before the
Prerender."

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:166: void
PrerenderLinkManager::StartPrerenders() {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Some thoughts on this change (No need to address these in this CL):
> 
> We no longer see stats involved never started prerenders.  This will affect
> existing histograms.  We may want to add new histograms (Blargh).  Think
adding
> fake FINAL_STATUS results directly to the histograms here would be a bit of a
> layering violation.
> 
> net-internal's prerender tab won't display anything about these.  If we care
> enough to have the tab display "active" prerenders in the first place, I
assume
> we care enough to add this information.

Noted.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: std::set<std::pair<int,
int> > launcher_and_prerender_id_set;
On 2012/12/28 18:25:33, Matt Menke wrote:
> This is only used for sanity checking - are compilers smart enough to optimize
> it out in release builds?
> 
> I'm not sure we actually need it - we have DCHECKs renderer-side for double
> starts already.  Not a big fan of debugging-only code making it into
production
> binaries, unless we need it.
> 
> If we keep this, I think we should have a comment that it's only for
DCHECKing,
> and rename it to running_launcher_and_prerender_id_set, so these all have
> consistent names.

I doubt compilers are that smart. I'm removing it.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:172: size_t
added_prerender_count = 0;
On 2012/12/28 18:25:33, Matt Menke wrote:
> nit:  Think "running" or "started" would be clearer than "added".  "running"
is
> used in the variable below, so would be the more consistent choice.

total_started_prerender_count avoided confusion I had around running and added.
Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:174:
running_launcher_and_render_view_route_set;
On 2012/12/28 18:32:39, Matt Menke wrote:
> On 2012/12/28 18:25:33, Matt Menke wrote:
> > optional:  May want to just pluralize this and remove the "set".  Same goes
> for
> > the other "set".  Given the mix of a set and a multiset, don't think the
"set"
> > helps much, and I've noticed a tendency for STL types to switch over time
(Not
> > that I think it's likely here, just not sure the suffixes are worth it, in
> > general).
> 
> This is one of those time-of-day comments, btw - if it were between 10 am and
2
> pm when reviewing, I may say one thing, between 2 pm and 6 pm, I may say
> another.  After 6 pm, I may not comment either way.  Completely up to you.

I liked the name change, but I can't bear to move to the map of pairs. I know
the pattern we're talking about, and it's a confusing anti-pattern if you ask
me...

You said you'll tolerate me leaving it in, so my next upload does that. If you
start to feel strongly about it, tell me, and I'll switch to the map of pairs.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:230: if (prerender_age >=
manager_->config().max_wait_to_launch) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> This should go before checking the launcher concurrency limit, since once this
> has happened, we'll never launch the prerender, anyways.
> 
> Not exactly a huge/relevant optimization, but seems the logical thing to be
> doing.

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:69: struct Prerender {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Think this name may be be a little too vague - I like something that can
easily
> be searched for, even if it's only used in one file.  LinkRelPrerender? 
> LinkPrerender?

LinkPrerender. Trying to do the rename on my part proved what you were saying
about the searching.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:105: int prerender_id);
On 2012/12/28 18:35:18, Matt Menke wrote:
> On 2012/12/28 18:25:33, Matt Menke wrote:
> > Think it's worth mentioning:
> > 
> > 1)  This is only used by tests (So therefore can be a little funky in the
> > multiple prerender case).
> > 2)  It only returns the first started active prerender with the given
> launcher.
> 
> Blegh - I was thinking this was child_id and renderer_id.  Ignore the comment.

Ignore what comment?

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:327: bool PrerenderHasHandle(int
child_id, int prerender_id) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Think this name is confusing.  LauncherHasRunningPrerender?

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:329: prerender_link_manager()->
FindByLauncherChildIdAndPrerenderId(
On 2012/12/28 18:25:33, Matt Menke wrote:
> nit:  Remove space

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:491: TEST_F(PrerenderTest,
MaxConcurrencyTest) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Three test suggestions, with per_launcher = 1, max_link_concurrency = 2:
> 
> Two prerenders from:
> 
> 1)  Same RenderView.
> 2)  Same process, different RenderViews.
> 3)  Different processes, same RenderView ID (Since IDs are only unique within
a
> process).

(1) is already tested in this test, right?

I've added (2). Good catch.

(3) scares me. child_id == -1 is special cased everywhere...

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:499:
prerender_manager()->config(). max_link_concurrency_per_launcher},
On 2012/12/28 18:25:33, Matt Menke wrote:
> nit:  Remove space.

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:549:
EXPECT_TRUE(prerender_contents_to_delay->prerendering_has_started());
On 2012/12/28 18:25:33, Matt Menke wrote:
> Suggest checking this at the end of the above loop (Should start when the
first
> one is used, not just after the last one).

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:596: TEST_F(PrerenderTest,
RateLimitInWindowTest) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Test comment and name no longer make any sense.  Suggest deleting it, since it
> doesn't seem to be testing anything MaxConcurrencyTest doesn't test for.

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:627:
FINAL_STATUS_MANAGER_SHUTDOWN);
On 2012/12/28 18:25:33, Matt Menke wrote:
> If we wanted to test the same thing as before, this should be TIMED_OUT, but
> that doesn't really get us anything...  Closesy analog would be to modify
> ExpireTest (Or make a modified copy of it) to have a pending prerender when
the
> first prerender times out.
> 
> Not sure if we absolutely need such a test, but I think it would be a
reasonable
> test to have, and seems to fit with the 3 abandon/timeout tests at the bottom
> (None of which currently start the pending prerender).

I just tossed it.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1139: TEST_F(PrerenderTest,
LinkManagerAddTwiceAbandonTwice) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> nit:  May want to rename to LinkManagerAddTwiceAbandonTwiceUseTwice while
you're
> here.

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1276: TEST_F(PrerenderTest,
LinkManagerClearOnAbandon) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> You're depending on max_link_concurrency being 1 in this test.  Suggest
> explicitly setting it in tests that depend on it.  Suspect at some point we'll
> want to be able to have multiple prerenders active from a single site.

I set the concurrency in the test now. I went with "Inactive", for reasons why
see my comment on line 1312 below.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1311: // are cleared from the
PrerenderLinkManager.
On 2012/12/28 18:25:33, Matt Menke wrote:
> Description should mention the second prerender is launched by the first.

Done.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1312: TEST_F(PrerenderTest,
LinkManagerClearOnPendingAbandon) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Suggest renaming the test.  Now that PrerenderLinkManager has prerenders that
> haven't yet started, but may be started in the future, we have another type of
> prerender that is "pending".
> 
> I think using "pending" solely to describe prerenders that haven't yet started
> due to being prerenders that a prerendered page tried to start is confusing,
and
> is terminology we should move away from.
> 
> Suggested name:  LinkManagerAbandonPrerenderWithChildPrerender (Or
> "WithLinkRelPrerender")

I went with Inactive on the rename request on the earlier test, and kept Pending
here with the overly careful distinction that you are complaining about. I
concur about the name distinction being confusing, but I think the better path
forward is to move pending prerenders out of the prerender manager into the
prerender link manager. The only reason that wasn't in this CL is that it's not
small, and seemed separate enough not to include here.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1356: TEST_F(PrerenderTest,
LinkManagerWaitToLaunch) {
On 2012/12/28 18:25:33, Matt Menke wrote:
> Suggest calling this LinkManagerWaitToLaunchNotLaunched or somesuch -
> "WaitToLaunch" kinda has the implication of actually being launched after
> waiting to do so.

Done.

[mmenke, 2012-12-28 20:15:13]

LGTM.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: std::set<std::pair<int,
int> > launcher_and_prerender_id_set;
On 2012/12/28 19:53:46, gavinp wrote:
> On 2012/12/28 18:25:33, Matt Menke wrote:
> > This is only used for sanity checking - are compilers smart enough to
optimize
> > it out in release builds?
> > 
> > I'm not sure we actually need it - we have DCHECKs renderer-side for double
> > starts already.  Not a big fan of debugging-only code making it into
> production
> > binaries, unless we need it.
> > 
> > If we keep this, I think we should have a comment that it's only for
> DCHECKing,
> > and rename it to running_launcher_and_prerender_id_set, so these all have
> > consistent names.
> 
> I doubt compilers are that smart. I'm removing it.

optional:  Actually...you could do something like:

DCHECK_EQ(FindByLauncherChildIdAndPrerenderId(i->child_id, i->prerender_id),
&(*i)); where the current DCHECK_EQ was, if you want.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:174:
running_launcher_and_render_view_route_set;
On 2012/12/28 19:53:46, gavinp wrote:
> On 2012/12/28 18:32:39, Matt Menke wrote:
> > On 2012/12/28 18:25:33, Matt Menke wrote:
> > > optional:  May want to just pluralize this and remove the "set".  Same
goes
> > for
> > > the other "set".  Given the mix of a set and a multiset, don't think the
> "set"
> > > helps much, and I've noticed a tendency for STL types to switch over time
> (Not
> > > that I think it's likely here, just not sure the suffixes are worth it, in
> > > general).
> > 
> > This is one of those time-of-day comments, btw - if it were between 10 am
and
> 2
> > pm when reviewing, I may say one thing, between 2 pm and 6 pm, I may say
> > another.  After 6 pm, I may not comment either way.  Completely up to you.
> 
> I liked the name change, but I can't bear to move to the map of pairs. I know
> the pattern we're talking about, and it's a confusing anti-pattern if you ask
> me...
> 
> You said you'll tolerate me leaving it in, so my next upload does that. If you
> start to feel strongly about it, tell me, and I'll switch to the map of pairs.

I'm content with keeping it as-is.  If I had particularly strong opinions about
it, I wouldn't have included the "optional" (Though I welcome pushback when I
don't use it, as well).

Think there are definite advantages to the multiset, and there are reasonable
reasons to stick with it.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.h:105: int prerender_id);
On 2012/12/28 19:53:46, gavinp wrote:
> On 2012/12/28 18:35:18, Matt Menke wrote:
> > On 2012/12/28 18:25:33, Matt Menke wrote:
> > > Think it's worth mentioning:
> > > 
> > > 1)  This is only used by tests (So therefore can be a little funky in the
> > > multiple prerender case).
> > > 2)  It only returns the first started active prerender with the given
> > launcher.
> > 
> > Blegh - I was thinking this was child_id and renderer_id.  Ignore the
comment.
> 
> Ignore what comment?

Not rightly sure.  Maybe you should ignore that comment, too, then?  :)

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:491: TEST_F(PrerenderTest,
MaxConcurrencyTest) {
On 2012/12/28 19:53:46, gavinp wrote:
> On 2012/12/28 18:25:33, Matt Menke wrote:
> > Three test suggestions, with per_launcher = 1, max_link_concurrency = 2:
> > 
> > Two prerenders from:
> > 
> > 1)  Same RenderView.
> > 2)  Same process, different RenderViews.
> > 3)  Different processes, same RenderView ID (Since IDs are only unique
within
> a
> > process).
> 
> (1) is already tested in this test, right?
Yup.  Wrote this comment early, before I looked at the others.

> 
> I've added (2). Good catch.
> 
> (3) scares me. child_id == -1 is special cased everywhere...
SGTM.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1312: TEST_F(PrerenderTest,
LinkManagerClearOnPendingAbandon) {
On 2012/12/28 19:53:46, gavinp wrote:
> On 2012/12/28 18:25:33, Matt Menke wrote:
> > Suggest renaming the test.  Now that PrerenderLinkManager has prerenders
that
> > haven't yet started, but may be started in the future, we have another type
of
> > prerender that is "pending".
> > 
> > I think using "pending" solely to describe prerenders that haven't yet
started
> > due to being prerenders that a prerendered page tried to start is confusing,
> and
> > is terminology we should move away from.
> > 
> > Suggested name:  LinkManagerAbandonPrerenderWithChildPrerender (Or
> > "WithLinkRelPrerender")
> 
> I went with Inactive on the rename request on the earlier test, and kept
Pending
> here with the overly careful distinction that you are complaining about. I
> concur about the name distinction being confusing, but I think the better path
> forward is to move pending prerenders out of the prerender manager into the
> prerender link manager. The only reason that wasn't in this CL is that it's
not
> small, and seemed separate enough not to include here.

SGTM.  Still think this test needs renaming, though - Tests can be named after
the activities they simulate, or what the results of those activities are that
they test for.  For the most part, tests in this file are named for what they
simulate, so think the "Clear" in the test name is confusing.

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:69: int render_view_route_id)
{
optional:  Maybe a DCHECK(!FindByLauncherChildIdAndPrerenderId(child_id,
prerender_id));?

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:193: i->prerender_id);
This isn't doing anything.

[gavinp, 2012-12-28 20:54:16]

Thanks for all the time on this Matt.

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/81001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:170: std::set<std::pair<int,
int> > launcher_and_prerender_id_set;
On 2012/12/28 20:15:13, Matt Menke wrote:
> On 2012/12/28 19:53:46, gavinp wrote:
> > On 2012/12/28 18:25:33, Matt Menke wrote:
> > > This is only used for sanity checking - are compilers smart enough to
> optimize
> > > it out in release builds?
> > > 
> > > I'm not sure we actually need it - we have DCHECKs renderer-side for
double
> > > starts already.  Not a big fan of debugging-only code making it into
> > production
> > > binaries, unless we need it.
> > > 
> > > If we keep this, I think we should have a comment that it's only for
> > DCHECKing,
> > > and rename it to running_launcher_and_prerender_id_set, so these all have
> > > consistent names.
> > 
> > I doubt compilers are that smart. I'm removing it.
> 
> optional:  Actually...you could do something like:
> 
> DCHECK_EQ(FindByLauncherChildIdAndPrerenderId(i->child_id, i->prerender_id),
> &(*i)); where the current DCHECK_EQ was, if you want.

Done. Also found I'd left a std::pair sitting around, which is probably not
great practice.

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:69: int render_view_route_id)
{
On 2012/12/28 20:15:14, Matt Menke wrote:
> optional:  Maybe a DCHECK(!FindByLauncherChildIdAndPrerenderId(child_id,
> prerender_id));?

Done. I used DCHECK_EQ, just because I debug by printf, so this will potentially
help me in the future.

https://codereview.chromium.org/11551003/diff/89002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:193: i->prerender_id);
On 2012/12/28 20:15:14, Matt Menke wrote:
> This isn't doing anything.

Yes. Amazing that it compiled, isn't it? I'm using clang. Goes to show you that
the optimizer can't see very deeply into the STL at all.

[commit-bot: I haz the power, 2012-12-28 20:55:10]

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

[commit-bot: I haz the power, 2012-12-28 20:55:17]

Presubmit check for 11551003-94003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/predictors

Presubmit checks took 2.9s to calculate.

[Shishir, 2012-12-28 21:42:10]

predictors LGTM

[commit-bot: I haz the power, 2012-12-28 22:19:55]

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

[commit-bot: I haz the power, 2012-12-29 00:16:46]

Change committed as 174751

[gavinp, 2013-01-03 19:24:43]

On 2012/12/29 00:16:46, I haz the power (commit-bot) wrote:
> Change committed as 174751

Note: this commit had a bug fixed in https://codereview.chromium.org/11727006/ .