Make PrerenderHandle an observer of PrerenderContents.

Make PrerenderHandle an observer of PrerenderContents.

The big implication of this is that PrerenderLinkManager is an observer of PrerenderHandle, and so messaging to/from the browser process about prerenders ends up mostly in the same place.

Interestingly, we basically can toss out the pending_prerenders_ list in the PrerenderManager; the only thing it bought us was tracking cancelation, which is now done with a bool in PrerenderHandle, just as easily.

The earlier work on the lifetime of PrerenderContents and PrerenderHandle was building up to this. This CL depends on https://codereview.chromium.org/11348357/ , and can't land until after it.

R=mmenke@chromium.org
BUG=None

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

[gavinp, 2012-12-05 20:00:59]

Matt, WDYT?

Notice I made eleven uploads before sending out the mail... That may be a record
for me!

[mmenke, 2012-12-06 22:22:15]

I haven't yet fully reviewed this CL - sorry about that.  Know you prefer to
deal with all comments at once, but I was wondering what you thought about the
fact we can now cancel and abandon the same PrerenderHandle - it doesn't looks
like this leads to any problems, but seems strange.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.h:86: protected:
nit:  Line break above protected

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:49: return prerender_data_ &&
prerender_data_->contents();
prerender_data_->contents() cannot be null.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:79: bool IsValid() const;
I suggest getting rid of this, and just relying on IsPrerendering, which has a
clearer name (And now has the same behavior).  Or just use the name
"StartedPrerender" or "HasStarted" or some such.  Could also inline, but I'm
fine with it as-is.

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

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:38:
DCHECK(!prerender_handle->IsValid());
Think this is worth a comment, along the lines "All prerenders handles that were
started should have been destroyed when the PrerenderManager was destroyed."

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:61: url, referrer, size)) {
I don't think we should be putting variable declarations in if statements.

I've thought about this a bit, and the fact is that while it does in fact obey
the scoping rule, it's not the prevailing style, and it's used rarely enough
that I think it decreases readability for the average Chromian (Chromus? 
Chromeite?  Chromizan?  Chromizone?).

That goes for the rest of this file as well.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:65:
DCHECK(insert_result.second);
I think this would be much cleaner if we just did:

DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
ids_to_handle_map_[child_and_prerender_id] = prerender_handle;

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:80: // in OnAddPrerender
above.
This TODO will no longer work - if a prerender is deleted because it's does
something bad, and then the prerendering page cancels the prerender before it
learns of this, we'll have a count of 0.

Also, "ids_to_url_map_" no longer exists.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:104:
prerender_handle->OnNavigateAway();
Hmm...we can now cancel and then navigate aware the same prerender handle... 
This seems kinda strange.  Since all handles for a prerender have the same
SiteInstance, I think this is fine, but it's kinda unexpected.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:147: NOTREACHED();
While this is true for all the places that this function is called, I don't
think this behavior is implied by the function name, and could easily see that
resulting in bugs.

I'd suggest removing the NOTREACHED, and adding a:
DCHECK_NE(ids_to_handle_map_.end(), FindPrerenderHandle(prerender_handle));

Just before each place it's called.  I'm open to other ideas.  I could live with
a comment in the function definition, but I'd prefer a code change to a
documentation change.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:161: }
Think it would be a little cleaner to have a function in a private namespace: 
Send(child_id, IPC::Message* msg); and just use it in these 3 methods.  Doesn't
savea huge amount of code, but increases readability.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:877: if (contents_)
contents_ can no longer be null.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:890: PrerenderHandle* handle) {
Why do we need the handle here?  Just for consistency?

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:896: if (contents_) {
contents_ can't be null.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:281: void
OnNewHandle(PrerenderHandle* prerender_handle);
I think "OnHandleCreated" seems to fit in a bit more with common Chrome naming
conventions, and the others as well.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:292: void
OnCancelByHandle(PrerenderHandle* prerender_handle);
I'd suggest OnHandleNavigatedAway and OnHandleCanceled, but I don't feel as
strongly about it as "OnHandleCreated".  The old names are perhaps a bit more
accurate, but I prefer the parallel names.

If you prefer to keep your names, suggest changing them to past tense.

[mmenke, 2012-12-07 15:44:49]

On 2012/12/06 22:22:15, Matt Menke wrote:
> I haven't yet fully reviewed this CL - sorry about that.  Know you prefer to
> deal with all comments at once, but I was wondering what you thought about the
> fact we can now cancel and abandon the same PrerenderHandle - it doesn't looks
> like this leads to any problems, but seems strange.

Sorry, that was unusually articulate, even for me.

[mmenke, 2012-12-07 16:46:08]

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:103:
base::WeakPtr<PrerenderManager::PrerenderData> prerender_data_;
This can *almost* be made a normal pointer - the only reason that doesn't
currently work is pending prerenders.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:934: session_storage_namespace));
optional (Or feel free to think about in a later CL):  May make life simpler to
have AddPrerender just return a PrerenderContents*, and have the calling
function wrap it into the PrerenderHandle.

Think creating a handle only to destroy it makes things overly complicated here,
and creating the handle in a couple places isn't too terribly ugly.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:935: if
(new_prerender_handle.get()) {
nit:  Can remove the .get() while you're here.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:942: }
Random comment for the future - if a prerender is cancelled without really being
started...Should there be some notification of that?

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

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:839: // PrerenderLinkManager has
already removed this prerender.
Doesn't this mess up book keeping?  If the prerender is cancelled, we want to
delete the dummy...

[mmenke, 2012-12-07 17:59:29]

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:79: if (prerender_data_ &&
prerender_data_->contents()) {
Since we had a DCHECK, and prerender_data_->contents can't be NULL, think this
is no longer needed.

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:83: if
(prerender_data_->contents()->prerendering_has_started())
Shouldn't this just use IsPrerendering(), just like PrerenderLinkManager?

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

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:152: PrerenderHandle*
prerender_handle) {
Think it's worth mentioning that this is called from
PrerenderLinkManager::OnAddPrerender, except from pending prerenders, where the
handle calls OnAddPrerender in AdoptPrerenderDataFrom.

[gavinp, 2012-12-10 17:55:10]

Matt,

This should be closer.

The WebKit side has landed, BTW.  :D

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_contents.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.h:86: protected:
On 2012/12/06 22:22:15, Matt Menke wrote:
> nit:  Line break above protected

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:49: return prerender_data_ &&
prerender_data_->contents();
On 2012/12/06 22:22:15, Matt Menke wrote:
> prerender_data_->contents() cannot be null.

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:79: bool IsValid() const;
On 2012/12/06 22:22:15, Matt Menke wrote:
> I suggest getting rid of this, and just relying on IsPrerendering, which has a
> clearer name (And now has the same behavior).  Or just use the name
> "StartedPrerender" or "HasStarted" or some such.  Could also inline, but I'm
> fine with it as-is.

Done. I was halfway there already (remember seeing me treating this function
with less than respect randomly throughout these CLs; that was this decision
kind of half done and not split out).

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

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:38:
DCHECK(!prerender_handle->IsValid());
On 2012/12/06 22:22:15, Matt Menke wrote:
> Think this is worth a comment, along the lines "All prerenders handles that
were
> started should have been destroyed when the PrerenderManager was destroyed."

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:65:
DCHECK(insert_result.second);
On 2012/12/06 22:22:15, Matt Menke wrote:
> I think this would be much cleaner if we just did:
> 
> DCHECK_EQ(0u, ids_to_handle_map_.count(child_and_prerender_id));
> ids_to_handle_map_[child_and_prerender_id] = prerender_handle;

Done, and I moved around some other code to avoid double DCHECKs.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:80: // in OnAddPrerender
above.
On 2012/12/06 22:22:15, Matt Menke wrote:
> This TODO will no longer work - if a prerender is deleted because it's does
> something bad, and then the prerendering page cancels the prerender before it
> learns of this, we'll have a count of 0.
> 
> Also, "ids_to_url_map_" no longer exists.

Gone. I still am thinking about how to keep all these different sets in sync to
avoid leaks. See a FIXME I put in
WebKit/Source/WebKit/chromium/tests/PrerenderingTest.cpp around the redundant
messaging from WebKit, which is part of the problem.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:104:
prerender_handle->OnNavigateAway();
On 2012/12/06 22:22:15, Matt Menke wrote:
> Hmm...we can now cancel and then navigate aware the same prerender handle... 
> This seems kinda strange.  Since all handles for a prerender have the same
> SiteInstance, I think this is fine, but it's kinda unexpected.

The reason I thought to keep them in was so that events could keep travelling
down to Prerenders after these notices came up. But the underlying cause of
Cancel is that there's no element in the document (which I _guess_ could catch
events, but come on...), and Abandon means there isn't even a document anymore.

So, sure, let's remove here. Addresses the concerns you raised below, too.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:147: NOTREACHED();
On 2012/12/06 22:22:15, Matt Menke wrote:
> While this is true for all the places that this function is called, I don't
> think this behavior is implied by the function name, and could easily see that
> resulting in bugs.
> 
> I'd suggest removing the NOTREACHED, and adding a:
> DCHECK_NE(ids_to_handle_map_.end(), FindPrerenderHandle(prerender_handle));
> 
> Just before each place it's called.  I'm open to other ideas.  I could live
with
> a comment in the function definition, but I'd prefer a code change to a
> documentation change.

Done. I like simple names, and I like names that correspond to functionality.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:161: }
On 2012/12/06 22:22:15, Matt Menke wrote:
> Think it would be a little cleaner to have a function in a private namespace: 
> Send(child_id, IPC::Message* msg); and just use it in these 3 methods. 
Doesn't
> savea huge amount of code, but increases readability.

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:877: if (contents_)
On 2012/12/06 22:22:15, Matt Menke wrote:
> contents_ can no longer be null.

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:890: PrerenderHandle* handle) {
On 2012/12/06 22:22:15, Matt Menke wrote:
> Why do we need the handle here?  Just for consistency?

Just for consistency.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:896: if (contents_) {
On 2012/12/06 22:22:15, Matt Menke wrote:
> contents_ can't be null.

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:934: session_storage_namespace));
On 2012/12/07 16:46:08, Matt Menke wrote:
> optional (Or feel free to think about in a later CL):  May make life simpler
to
> have AddPrerender just return a PrerenderContents*, and have the calling
> function wrap it into the PrerenderHandle.
> 
> Think creating a handle only to destroy it makes things overly complicated
here,
> and creating the handle in a couple places isn't too terribly ugly.

Definitely a good idea, and I'm definitely punting to a later CL.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:935: if
(new_prerender_handle.get()) {
On 2012/12/07 16:46:08, Matt Menke wrote:
> nit:  Can remove the .get() while you're here.

enne is Awesome! Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:281: void
OnNewHandle(PrerenderHandle* prerender_handle);
On 2012/12/06 22:22:15, Matt Menke wrote:
> I think "OnHandleCreated" seems to fit in a bit more with common Chrome naming
> conventions, and the others as well.

Done.

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:292: void
OnCancelByHandle(PrerenderHandle* prerender_handle);
On 2012/12/06 22:22:15, Matt Menke wrote:
> I'd suggest OnHandleNavigatedAway and OnHandleCanceled, but I don't feel as
> strongly about it as "OnHandleCreated".  The old names are perhaps a bit more
> accurate, but I prefer the parallel names.
> 
> If you prefer to keep your names, suggest changing them to past tense.

Done. Good work using the American spelling of "Canceled."

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

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:839: // PrerenderLinkManager has
already removed this prerender.
On 2012/12/07 16:46:08, Matt Menke wrote:
> Doesn't this mess up book keeping?  If the prerender is cancelled, we want to
> delete the dummy...

But the PrerenderStop event was sent, and that removed it from the map. The
bookkeeping is already quite messed up once we start sending events down...

The fix is architecturally pretty nasty:
PrerenderManager::MoveEntryToPendingDelete needs to reach into the handle to
reinsert an entry? Or perhaps squash the event through an output parameter?

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:79: if (prerender_data_ &&
prerender_data_->contents()) {
On 2012/12/07 17:59:29, Matt Menke wrote:
> Since we had a DCHECK, and prerender_data_->contents can't be NULL, think this
> is no longer needed.

What if we're adopting from a prerender handle that doesn't have a prerender
data?

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:83: if
(prerender_data_->contents()->prerendering_has_started())
On 2012/12/07 17:59:29, Matt Menke wrote:
> Shouldn't this just use IsPrerendering(), just like PrerenderLinkManager?

Done, and it was redundant anyway.

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

https://codereview.chromium.org/11316311/diff/36001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:152: PrerenderHandle*
prerender_handle) {
On 2012/12/07 17:59:29, Matt Menke wrote:
> Think it's worth mentioning that this is called from
> PrerenderLinkManager::OnAddPrerender, except from pending prerenders, where
the
> handle calls OnAddPrerender in AdoptPrerenderDataFrom.

Done.

I'm not super happy about this either; perhaps the PrerenderManager::Add() and
PrerenderContents::Start... should take a PrerenderContents/PrerenderHandle
observer argument?

[mmenke, 2012-12-10 18:04:02]

I'll try to get to this by the end of the day, but it may have to wait until
tomorrow morning.

[gavinp, 2012-12-10 18:34:24]

On 2012/12/10 18:04:02, Matt Menke wrote:
> I'll try to get to this by the end of the day, but it may have to wait until
> tomorrow morning.

No problem. I'm still deciding what to do about the event handling in
matchcomplete case, so I don't think that last upload is the final word anyway.

[gavinp, 2012-12-11 15:50:45]

Matt,

This latest upload makes sure MatchComplete dummies are properly connected to
PrerenderHandle for their lifetime.

[mmenke, 2012-12-11 19:53:45]

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:292: void
OnCancelByHandle(PrerenderHandle* prerender_handle);
On 2012/12/10 17:55:10, gavinp wrote:
> On 2012/12/06 22:22:15, Matt Menke wrote:
> > I'd suggest OnHandleNavigatedAway and OnHandleCanceled, but I don't feel as
> > strongly about it as "OnHandleCreated".  The old names are perhaps a bit
more
> > accurate, but I prefer the parallel names.
> > 
> > If you prefer to keep your names, suggest changing them to past tense.
> 
> Done. Good work using the American spelling of "Canceled."

Thanks.  It's one of the few cases where I hate the American spelling.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:122:
replacement->AddObserver(this);
The replacement won't send the stop message, since it was never started.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:28: // which will make calls on the
handle into no-ops.
They're also no-ops if the prerender has not yet started, in which case we use
the destruction of the handle itself as a signal not to start prerendering.

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

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:123: // OnCancel may end up
calling back into OnPrerenderStop, and so
nit:  OnNavigateAway.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:123: // OnCancel may end up
calling back into OnPrerenderStop, and so
Actually...Is there any way that OnNavigateAway would end up calling into
OnPrerenderStop?

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:180: if (it ==
ids_to_handle_map_.end())
Does this ever happen?  Same goes for the next two functions.

We add a handle to the map at the same point we start listening to it.

RemovePrerender both removes the handle from the map and deletes it.

[mmenke, 2012-12-12 16:54:12]

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:107: // notified of the prerender
ending.
This comment isn't correct.  OnPrerenderStop is called synchronously in
Destroy(FinalStatus), or in PrepareForUse.  Both happen before the funky
deletion stuff.  However, PrepareForUse is currently called after we destroy the
data, so you're right about a DCHECK not working, without more information.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:97: ObserverList<Observer>
observer_list_;
Do we really need an observer list?  Seems like in general, we're going to have
at most one Observer - the owner.

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

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:75: manager_->
AddPrerenderFromLinkRelPrerender(
nit:  Remove space

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:879:
DCHECK_NE(static_cast<PrerenderContents*>(NULL), contents_);
nit:  DCHECKs are generally put at the start of functions.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:879:
DCHECK_NE(static_cast<PrerenderContents*>(NULL), contents_);
May want to put one of these in the constructor as well.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:925: if
(existing_prerender_handle->has_been_canceled())
Since we delete handles when cancelled, and don't start prerenders on cancel, is
this check even needed?

I think we can get rid of has_been_canceled() and the underlying flag entirely.

Fine to keep this as-is if you disagree, or feel it may be needed in the future.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // situation.
Is this comment accurate?  We only delete prerenders from the list in response
to this message.

Not suggesting removing the check (yet, at least), just fixing the comment.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:99: extra_data.size(),
extra_data.render_view_route_id()));
For future consideration:  Another leak case:  If a prerender is never started,
we just leak it into the prerenders_ list.  These aren't huge leaks, but still
think that fixing them should be given a fairly high priority, after landing
these CLs.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:47:
CHECK(insert_result.second);
I think that the following is much clearer:

DCHECK_EQ(0u, prerender_dispatcher_.prerenders_.count(g_next_prerender_id));
prerender_dispatcher_.prerenders_[g_next_prerender_id] = url;

I don't think I've ever seen the insertion result of adding something to a map
used in Chrome.  Probably because the data type is rather complicated.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:122: 
May want to add a second url as well.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:128:
TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveWithoutAdd) {
Does this ever happen?

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:137:
TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveTooMany) {
Does this ever happen?

[gavinp, 2012-12-13 13:38:03]

How's this looking, Matt?

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:942: }
On 2012/12/07 16:46:08, Matt Menke wrote:
> Random comment for the future - if a prerender is cancelled without really
being
> started...Should there be some notification of that?

I'm going to go with no. The launcher has no promises about which ones will be
started...

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/11316311/diff/16001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.h:292: void
OnCancelByHandle(PrerenderHandle* prerender_handle);
On 2012/12/11 19:53:45, Matt Menke wrote:
> On 2012/12/10 17:55:10, gavinp wrote:
> > On 2012/12/06 22:22:15, Matt Menke wrote:
> > > I'd suggest OnHandleNavigatedAway and OnHandleCanceled, but I don't feel
as
> > > strongly about it as "OnHandleCreated".  The old names are perhaps a bit
> more
> > > accurate, but I prefer the parallel names.
> > > 
> > > If you prefer to keep your names, suggest changing them to past tense.
> > 
> > Done. Good work using the American spelling of "Canceled."
> 
> Thanks.  It's one of the few cases where I hate the American spelling.

Wait: the American spelling is "Canceled." Would you like these methods renamed
to use the Canadian spelling of "Cancelled" ?

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:107: // notified of the prerender
ending.
On 2012/12/12 16:54:12, Matt Menke wrote:
> This comment isn't correct.  OnPrerenderStop is called synchronously in
> Destroy(FinalStatus), or in PrepareForUse.  Both happen before the funky
> deletion stuff.  However, PrepareForUse is currently called after we destroy
the
> data, so you're right about a DCHECK not working, without more information.

Done.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:122:
replacement->AddObserver(this);
On 2012/12/11 19:53:45, Matt Menke wrote:
> The replacement won't send the stop message, since it was never started.

Fixed, now sent. Overall, your comment elsewhere about leaks is well taken and
related... although thankfully we track OnChannelClosed so they're only life of
renderer at worst.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:28: // which will make calls on the
handle into no-ops.
On 2012/12/11 19:53:45, Matt Menke wrote:
> They're also no-ops if the prerender has not yet started, in which case we use
> the destruction of the handle itself as a signal not to start prerendering.

Done.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:97: ObserverList<Observer>
observer_list_;
On 2012/12/12 16:54:12, Matt Menke wrote:
> Do we really need an observer list?  Seems like in general, we're going to
have
> at most one Observer - the owner.

We can do away with the list. Done.

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

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:75: manager_->
AddPrerenderFromLinkRelPrerender(
On 2012/12/12 16:54:12, Matt Menke wrote:
> nit:  Remove space

Done.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:123: // OnCancel may end up
calling back into OnPrerenderStop, and so
On 2012/12/11 19:53:45, Matt Menke wrote:
> Actually...Is there any way that OnNavigateAway would end up calling into
> OnPrerenderStop?

No. But we also need to leave the prerender in the map, since if the prerender
continues discovering new aliases, we want those to make it down to the
prerender dispatcher. So I've just tossed this whole code block out.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:180: if (it ==
ids_to_handle_map_.end())
On 2012/12/11 19:53:45, Matt Menke wrote:
> Does this ever happen?  Same goes for the next two functions.
> 
> We add a handle to the map at the same point we start listening to it.
> 
> RemovePrerender both removes the handle from the map and deletes it.

Good point. Now a DCHECK.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:879:
DCHECK_NE(static_cast<PrerenderContents*>(NULL), contents_);
On 2012/12/12 16:54:12, Matt Menke wrote:
> May want to put one of these in the constructor as well.

Done & Done.

https://codereview.chromium.org/11316311/diff/65002/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:925: if
(existing_prerender_handle->has_been_canceled())
On 2012/12/12 16:54:12, Matt Menke wrote:
> Since we delete handles when cancelled, and don't start prerenders on cancel,
is
> this check even needed?
> 
> I think we can get rid of has_been_canceled() and the underlying flag
entirely.
> 
> Fine to keep this as-is if you disagree, or feel it may be needed in the
future.

I think that's a good idea. It simplifies the handle, I've moved cancelation
logic into the destructor.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // situation.
On 2012/12/12 16:54:12, Matt Menke wrote:
> Is this comment accurate?  We only delete prerenders from the list in response
> to this message.
> 
> Not suggesting removing the check (yet, at least), just fixing the comment.

Done.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:99: extra_data.size(),
extra_data.render_view_route_id()));
On 2012/12/12 16:54:12, Matt Menke wrote:
> For future consideration:  Another leak case:  If a prerender is never
started,
> we just leak it into the prerenders_ list.  These aren't huge leaks, but still
> think that fixing them should be given a fairly high priority, after landing
> these CLs.

Good point. I think it'd be good if we tightened up WebKit (which sends a lot of
redundant events), and got these different maps in strong longstep with each
other, and leak free.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher_unittest.cc (right):

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:47:
CHECK(insert_result.second);
On 2012/12/12 16:54:12, Matt Menke wrote:
> I think that the following is much clearer:
> 
> DCHECK_EQ(0u, prerender_dispatcher_.prerenders_.count(g_next_prerender_id));
> prerender_dispatcher_.prerenders_[g_next_prerender_id] = url;
> 
> I don't think I've ever seen the insertion result of adding something to a map
> used in Chrome.  Probably because the data type is rather complicated.

Done. You are slowly winning me over to your crazy "use
std::map<>::operator[]()" arguments.

With respect to std::map<>::insert(), the return data type is complicated, but
it's not very high cost to allocate; if used wisely, the virtue is that you save
a traversal of the structure: you can use insert as a test for existence.

However, every comment you've made on this has been spot on and correct: I was
using the insert result only for a DCHECK, and that's bad form. In debug, an
extra search is fine if the benefit is huge simplification of code. So I'm sold.

If the insert result was being checked in the non-debug code path? Well, that's
a topic for a different review...

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:122: 
On 2012/12/12 16:54:12, Matt Menke wrote:
> May want to add a second url as well.

Throwing out the test, as we seem to be able to DCHECK on non-duplicate Stop
events.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:128:
TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveWithoutAdd) {
On 2012/12/12 16:54:12, Matt Menke wrote:
> Does this ever happen?

I don't think so.

https://codereview.chromium.org/11316311/diff/65002/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher_unittest.cc:137:
TEST_F(PrerenderDispatcherTest, PrerenderDispatcherRemoveTooMany) {
On 2012/12/12 16:54:12, Matt Menke wrote:
> Does this ever happen?

I don't think so.

[gavinp, 2012-12-13 17:53:16]

Shishir: Can you look at my predictor changes?

[Shishir, 2012-12-13 18:12:02]

lgtm for predictors.

[mmenke, 2012-12-13 18:27:16]

Think we can get this in today - most of these comments should be pretty fast to
respond to, and won't require a whole lot of review (Just holding off signoff
for one or two of the comments).

The other one is a much smaller CL, and don't see that taking too long, other
than perhaps the extra tests I asked for.  I think they're important enough that
we should include more tests of the API before landing, but still think we can
manage to get it in by the branch, if we push things.

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

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/predictors...
chrome/browser/predictors/autocomplete_action_predictor.cc:103:
prerender_handle_.reset();
Since it automatically is cancelled on destruction, and we aren't observing the
handle, can we just delete this line?

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:29: // prerender.
Think it's worth mentioning that destroying "will stop the prerender without
making any calls to the Observer".

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:35: virtual ~Observer();
Could make this protected as well - mostly as a promise we aren't going to go
around destroying anybody.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:57: DCHECK(!observer_);
We need to include "base/logging.h" for DCHECK.

While someone in some header up there apparently disagrees with me,
base/logging.h is a sufficiently busy header that I prefer to keep it out of
header files unless we really need it.  I'd suggest switching to SetObserver,
and de-inlining this, though I think I've also seen similar cases where
set_observer naming style was used, but the body with the DCHECK was in the cc
file.

In terms of compile time, it's also unclear what DCHECK expands out into, though
think that makes this no longer a "simple setter" in debug builds.

Also doesn't look to me like a whole lot of header files currently use DCHECKs
like this.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:889: DCHECK_LE(1, handle_count_);
nit:  This uses "DCHECK_LE", and the above function uses "DCHECK_LT", but
they're checking the exact same thing.  Should use the same code.

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

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:718:
EXPECT_FALSE(pending_prerender_handle->IsPrerendering());
nit:  No need to check this twice (Though suppose it is the season for that kind
of thing...)

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:724: SUCCEED();
I'm not following this.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:970: // TODO(gavinp): Re-enabmed
this test after abandon has an effect on Prerenders,
nit:  while you're here, "re-enbalmed"...  Or maybe "re-enable".  Sure it's one
of those.

Actually, the test is enabled...Remove comment?  Or does it mean something else?

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:989:
ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
I don't think we have a test with a cancel after an abandon, or a use after an
abandon.  Suggest duplicating this test and checking both cases.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:37: void
PrerenderDispatcher::OnPrerenderStart(int prerender_id) {
Since we have it on stop, too, may want a:

DCHECK_NE(0u, prerenders_.count(prerender_id));

Think DCHECKing closer to the problem area is generally a good idea.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:43: std::multimap<GURL,
int>::value_type(url, prerender_id));
	
(Draft) 2012/12/13 17:03:52
optional:  Think it's cleaner to use operator[] instead of insert here. 
[multi]map::insert just isn't used much in Chrome, probably because it's less
intuitive, ugly, and uses twice the code.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:50: std::multimap<GURL,
int>::value_type(url, prerender_id));
optional:  Think it's cleaner to use operator[] instead of insert here. 
[multi]map::insert just isn't used much in Chrome, probably because it's less
intuitive, ugly, and uses twice the code.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:66: to_delete.push(it);
Think it would be a little simpler to have only one while loop here.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:91: prerender.url()));
optional:  Think it's cleaner to use operator[] instead of insert here.

[gavinp, 2012-12-13 20:45:33]

Matt,

Please take a look.

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

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/predictors...
chrome/browser/predictors/autocomplete_action_predictor.cc:103:
prerender_handle_.reset();
On 2012/12/13 18:27:16, Matt Menke wrote:
> Since it automatically is cancelled on destruction, and we aren't observing
the
> handle, can we just delete this line?

I had to remove that auto delete on destruction behaviour so that we'd be less
leaky in the link manager on channel close... Whoops. That will be in the next
upload of the events CL.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.h (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:29: // prerender.
On 2012/12/13 18:27:16, Matt Menke wrote:
> Think it's worth mentioning that destroying "will stop the prerender without
> making any calls to the Observer".

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:35: virtual ~Observer();
On 2012/12/13 18:27:16, Matt Menke wrote:
> Could make this protected as well - mostly as a promise we aren't going to go
> around destroying anybody.

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.h:57: DCHECK(!observer_);
On 2012/12/13 18:27:16, Matt Menke wrote:
> We need to include "base/logging.h" for DCHECK.
> 
> While someone in some header up there apparently disagrees with me,
> base/logging.h is a sufficiently busy header that I prefer to keep it out of
> header files unless we really need it.  I'd suggest switching to SetObserver,
> and de-inlining this, though I think I've also seen similar cases where
> set_observer naming style was used, but the body with the DCHECK was in the cc
> file.
> 
> In terms of compile time, it's also unclear what DCHECK expands out into,
though
> think that makes this no longer a "simple setter" in debug builds.
> 
> Also doesn't look to me like a whole lot of header files currently use DCHECKs
> like this.

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:889: DCHECK_LE(1, handle_count_);
On 2012/12/13 18:27:16, Matt Menke wrote:
> nit:  This uses "DCHECK_LE", and the above function uses "DCHECK_LT", but
> they're checking the exact same thing.  Should use the same code.

Done.

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

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:718:
EXPECT_FALSE(pending_prerender_handle->IsPrerendering());
On 2012/12/13 18:27:16, Matt Menke wrote:
> nit:  No need to check this twice (Though suppose it is the season for that
kind
> of thing...)

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:724: SUCCEED();
On 2012/12/13 18:27:16, Matt Menke wrote:
> I'm not following this.

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:970: // TODO(gavinp): Re-enabmed
this test after abandon has an effect on Prerenders,
On 2012/12/13 18:27:16, Matt Menke wrote:
> nit:  while you're here, "re-enbalmed"...  Or maybe "re-enable".  Sure it's
one
> of those.
> 
> Actually, the test is enabled...Remove comment?  Or does it mean something
else?

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:989:
ASSERT_EQ(prerender_contents, prerender_manager()->FindAndUseEntry(url));
On 2012/12/13 18:27:16, Matt Menke wrote:
> I don't think we have a test with a cancel after an abandon, or a use after an
> abandon.  Suggest duplicating this test and checking both cases.

The abandon tests do try and use the prerender.

I added a test with cancel after abandon.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:37: void
PrerenderDispatcher::OnPrerenderStart(int prerender_id) {
On 2012/12/13 18:27:16, Matt Menke wrote:
> Since we have it on stop, too, may want a:
> 
> DCHECK_NE(0u, prerenders_.count(prerender_id));
> 
> Think DCHECKing closer to the problem area is generally a good idea.

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:43: std::multimap<GURL,
int>::value_type(url, prerender_id));
On 2012/12/13 18:27:16, Matt Menke wrote:
> 	
> (Draft) 2012/12/13 17:03:52
> optional:  Think it's cleaner to use operator[] instead of insert here. 
> [multi]map::insert just isn't used much in Chrome, probably because it's less
> intuitive, ugly, and uses twice the code.

Multimap has no subscript operator.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:50: std::multimap<GURL,
int>::value_type(url, prerender_id));
On 2012/12/13 18:27:16, Matt Menke wrote:
> optional:  Think it's cleaner to use operator[] instead of insert here. 
> [multi]map::insert just isn't used much in Chrome, probably because it's less
> intuitive, ugly, and uses twice the code.

Done.

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:91: prerender.url()));
On 2012/12/13 18:27:16, Matt Menke wrote:
> optional:  Think it's cleaner to use operator[] instead of insert here.

Done.

[gavinp, 2012-12-13 20:51:24]

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:66: to_delete.push(it);
On 2012/12/13 18:27:16, Matt Menke wrote:
> Think it would be a little simpler to have only one while loop here.

Done.

[mmenke, 2012-12-13 21:39:42]

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/87001/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:43: std::multimap<GURL,
int>::value_type(url, prerender_id));
On 2012/12/13 20:45:34, gavinp wrote:
> On 2012/12/13 18:27:16, Matt Menke wrote:
> > 	
> > (Draft) 2012/12/13 17:03:52
> > optional:  Think it's cleaner to use operator[] instead of insert here. 
> > [multi]map::insert just isn't used much in Chrome, probably because it's
less
> > intuitive, ugly, and uses twice the code.
> 
> Multimap has no subscript operator.

Erm...  Good point.  I'd thought I'd seen prettier insertions, but probably was
just because of typedefs.

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:9: #include
"chrome/browser/prerender/prerender_contents.h"
#include "base/logging.h" for DCHECK.

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

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:142:
ids_to_handle_map_.erase(it);
Isn't this leaking a handle?

https://codereview.chromium.org/11316311/diff/92004/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/92004/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:7: #include <stack>
nit:  Not needed.

[gavinp, 2012-12-13 22:01:55]

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_handle.cc (right):

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_handle.cc:9: #include
"chrome/browser/prerender/prerender_contents.h"
On 2012/12/13 21:39:43, Matt Menke wrote:
> #include "base/logging.h" for DCHECK.

Done.

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

https://codereview.chromium.org/11316311/diff/92004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_link_manager.cc:142:
ids_to_handle_map_.erase(it);
On 2012/12/13 21:39:43, Matt Menke wrote:
> Isn't this leaking a handle?

Yes. I've added a test and fixed it.

https://codereview.chromium.org/11316311/diff/92004/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11316311/diff/92004/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:7: #include <stack>
On 2012/12/13 21:39:43, Matt Menke wrote:
> nit:  Not needed.

Done.

[mmenke, 2012-12-13 22:05:46]

LGTM

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

https://codereview.chromium.org/11316311/diff/100004/chrome/browser/prerender...
chrome/browser/prerender/prerender_link_manager.cc:143:
ids_to_handle_map_.erase(it);
Replace last two with just:  RemovePrerender(it);

[gavinp, 2012-12-13 22:11:58]

Thanks for all your time on this Matt. I know it was a tricky one.

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

https://codereview.chromium.org/11316311/diff/100004/chrome/browser/prerender...
chrome/browser/prerender/prerender_link_manager.cc:8: #include <queue>
Bonus header removal.

https://codereview.chromium.org/11316311/diff/100004/chrome/browser/prerender...
chrome/browser/prerender/prerender_link_manager.cc:143:
ids_to_handle_map_.erase(it);
On 2012/12/13 22:05:46, Matt Menke wrote:
> Replace last two with just:  RemovePrerender(it);

Done.

[commit-bot: I haz the power, 2012-12-13 22:24:48]

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

[commit-bot: I haz the power, 2012-12-13 22:24:55]

Presubmit check for 11316311-97003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/prerender_messages.h

Presubmit checks took 1.9s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[gavinp, 2012-12-13 23:25:40]

Justin,

Please take a look at prerender_messages.h.

[mmenke, 2012-12-14 22:12:01]

On 2012/12/13 23:25:40, gavinp wrote:
> Justin,
> 
> Please take a look at prerender_messages.h.

Gavin:  Looking at his calendar, looks like he's on vacation.

[jschuh, 2012-12-14 22:40:21]

Rubberstamp lgtm since this is just browser-> renderer IPC changes.

[jschuh, 2012-12-14 22:41:22]

On 2012/12/14 22:12:01, Matt Menke wrote:
> On 2012/12/13 23:25:40, gavinp wrote:
> > Justin,
> > 
> > Please take a look at prerender_messages.h.
> 
> Gavin:  Looking at his calendar, looks like he's on vacation.

I was, but I came back a day early.

[commit-bot: I haz the power, 2012-12-14 22:43:50]

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

[commit-bot: I haz the power, 2012-12-15 05:00:31]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 15:13:34]

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

[commit-bot: I haz the power, 2012-12-15 21:45:20]

Change committed as 173322