**NOTFORLANDING** New link rel=prerender API

**NOTFORLANDING**  New link rel=prerender API

This new API implements chrome-side of the new API for <link
rel=prerender...> elements.  The webkit side of this is
https://bugs.webkit.org/show_bug.cgi?id=82478

This patch includes what will end up being many commits!  Please review the architecture; after this review starts settling down into nits, I'll split it up into separate reviews for each part as I stage both this change and the webkit bug into the respective projects' trunks.

BUG=None

[gavinp, 2012-03-28 19:10:05]

WDYT?

[gavinp, 2012-03-28 19:51:31]

I'd like to note: I want to add more to the prerender_manager API in the future;
I am very intentionally in this CL keeping changes to that API absolutely
minimal.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:33: const gfx::Size&
ALLOW_UNUSED size) {
I carried this size all the way up for two reasons:

1. Because I hope to get rid of the render_view_route_id in a future patch, by
making the StorageNameSpace able to move back to the browser (the API right now
only really supports moving them the other way).

2. Because this size, from the WebView(), is actually more reliable than
GetContainerSize() on ChromeOS, and identical on all other platforms.  If we
start using it, that will make prerendering work better on ChromeOS, or at least
our brwoser tests.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:37: const GURL url =
orig_url.ReplaceComponents(replacements);
In the spirit of not changing too much as  I add this new API, I'm not turning
on fragments just yet.  This code has the same effect as the CachedResource in
WebKit to remove fragments.

[cbentzel, 2012-03-28 20:15:05]

Thanks. I'll try to do a first pass tonight, but will not worry about nits.
We've already discussed the high-level architecture and it generally sounded
good.

[cbentzel, 2012-03-29 00:13:37]

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:877:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderPageRemovingLink) {
Should probably have some tests for multiple link elements.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:41: const
ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
Why don't you need to include route_id as well? Do you assume that the
prerender_id is unique per process rather than per RenderView? Is this an
appropriate assumption to make?

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:44:
url_map_.insert(std::make_pair(url, child_and_prerender_id));
I recommend adding a CheckInvariant() function which is only called in debug in
DCHECKs() to ensure that id_map_ and url_map_ are consistent. I would put it at
the start/end of each of the mutation methods.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:59:
UrlToPrerenderIdMap::iterator url_id_iter = url_map_.find(url);
This doesn't guarantee to find the first entry in the multimap for url. Use
lower_bound instead. Or you could use equal_range, but you are mutating the
multimap as you go along.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:65: url_map_.erase(to_erase);
I always get nervous about erasing while iterating, although this appears to
handle it correctly. 

I think it would be easier to do something like to_erase = url_map_.end()
outside of the loop, have the loop bounds defined by url_map_.equal_range, and
after the loop is done erase if the iterator does not == url_map_.end(). What do
you think?

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:74: // TODO(gavinp): Track
down the correct prerender and stop it, rather than
Please fix this. Also, it doesn't look like you have feedback from the
PrerenderManager back into here in case the URL that this added was evicted by a
different prerender.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:108:
prerender_manager->link_manager()->OnNewLinkPrerenderImpl(
Not sure I like that this goes "Get the prerender managers link manager, and
have that call into the prerender manager".

In general I'd like this to exist completely outside of the PrerenderManager.
Perhaps make it a ProfileKeyedService as well?

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.h:30: class PrerenderLinkManager
{
It's probably reasonable to unit test some of the management work this does.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.h:43: static void
OnRemovedLinkPrerender(Profile* profile,
Document. What's different about OnRemoved versus OnUnloaded for example.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_message_filter.cc:113:
IPC_MESSAGE_HANDLER(PrerenderMsg_NewLinkPrerender, OnNewLinkPrerender)
Could these get moved to a separate class which is a WebContentsObserver?
Perhaps PrerenderTabHelper?

http://codereview.chromium.org/9875026/diff/2001/chrome/common/prerender_mess...
File chrome/common/prerender_messages.h (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/common/prerender_mess...
chrome/common/prerender_messages.h:28:
IPC_MESSAGE_CONTROL5(PrerenderMsg_NewLinkPrerender,
These should be named PrerenderHostMsg to differentiate that they are
render->browser. I also think they could be ROUTED messages if you move to
listening in a WebContentsObserver.

[mmenke, 2012-03-29 01:15:38]

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:65: url_map_.erase(to_erase);
I suggest that you only delete entries in one place.  It's a bit slower, but
think just calling "RemovePrerender(blah)" first from here and from
OnUnloadedLinkPrerenderImpl makes things simpler.

Also, then you won't need a loop here.  Just remove the link, and then do a
find.

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/2001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.h:53: typedef
std::multimap<GURL, ChildAndPrerenderIdPair> UrlToPrerenderIdMap;
Do we really need a multimap here?  Could just use a map with counts.  Seems a
little simpler to me, though, admittedly, removing things from it is marginally
more complicated.

[gavinp, 2012-04-20 17:54:14]

Remember this patch from three weeks ago?  Here's another view on it.  I've
uploaded it a few ways into this patch, so you can review whichever parts you
wish.  Patches 3-7 show parts close to how it will be staged, and patch 8 gives
you a view of all of my changes against trunk.

The main changes are:

1. All my prerendering code out of webkit/glue and content/. 
2. Using the updated API from webkit.  The WebKit bug should get an upload soon
that exactly corresponds to this change.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_browsertest.cc:877:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderPageRemovingLink) {
On 2012/03/29 00:13:37, cbentzel wrote:
> Should probably have some tests for multiple link elements.

Done.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:41: const
ChildAndPrerenderIdPair child_and_prerender_id(child_id, prerender_id);
On 2012/03/29 00:13:37, cbentzel wrote:
> Why don't you need to include route_id as well? Do you assume that the
> prerender_id is unique per process rather than per RenderView? Is this an
> appropriate assumption to make?

The prerender_id should be unique across the WebKit instance, and there's only
one WebKit instance per renderer process.  This is an appropriate assumption.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:44:
url_map_.insert(std::make_pair(url, child_and_prerender_id));
On 2012/03/29 00:13:37, cbentzel wrote:
> I recommend adding a CheckInvariant() function which is only called in debug
in
> DCHECKs() to ensure that id_map_ and url_map_ are consistent. I would put it
at
> the start/end of each of the mutation methods.

Done.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:59:
UrlToPrerenderIdMap::iterator url_id_iter = url_map_.find(url);
On 2012/03/29 00:13:37, cbentzel wrote:
> This doesn't guarantee to find the first entry in the multimap for url. Use
> lower_bound instead. Or you could use equal_range, but you are mutating the
> multimap as you go along. 

_The C++ Programming Language_ claims otherwise in 17.4.1.6 (pg 486 in the 1st
edition).  The C++03 standard, however, does not go so far.  In ISO 14882:2003,
the claim is only that it finds a matching element. But in <algorithm>, the
standard says in 25.1.2 [lib.alg.find] that the std::find(multimap.begin(),
multimap.end(), i) is claimed to find the first match.  I wonder why the
inconsistancy?  A review of the code to my local STL shows std::find() on a
multimap is the same as multimap<>::find, so I'm good, but I'm switching to
lower_bound here.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:65: url_map_.erase(to_erase);
On 2012/03/29 01:15:39, Matt Menke wrote:
> I suggest that you only delete entries in one place.  It's a bit slower, but
> think just calling "RemovePrerender(blah)" first from here and from
> OnUnloadedLinkPrerenderImpl makes things simpler.
> 
> Also, then you won't need a loop here.  Just remove the link, and then do a
> find.

Done.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:74: // TODO(gavinp): Track
down the correct prerender and stop it, rather than
On 2012/03/29 00:13:37, cbentzel wrote:
> Please fix this. Also, it doesn't look like you have feedback from the
> PrerenderManager back into here in case the URL that this added was evicted by
a
> different prerender.

Done, in a basic way using the existing PrerenderManager APIs.

After this CL lands, and we have the plumbing in, I intend to land a followup CL
that connects the PrerenderLinkManager and the PrerenderManager much more
closely.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.cc:108:
prerender_manager->link_manager()->OnNewLinkPrerenderImpl(
On 2012/03/29 00:13:37, cbentzel wrote:
> Not sure I like that this goes "Get the prerender managers link manager, and
> have that call into the prerender manager".
> 
> In general I'd like this to exist completely outside of the PrerenderManager.
> Perhaps make it a ProfileKeyedService as well? 

Done.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.h:30: class PrerenderLinkManager
{
On 2012/03/29 00:13:37, cbentzel wrote:
> It's probably reasonable to unit test some of the management work this does. 

Done.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.h:43: static void
OnRemovedLinkPrerender(Profile* profile,
On 2012/03/29 00:13:37, cbentzel wrote:
> Document. What's different about OnRemoved versus OnUnloaded for example.

The new names simplify this; it's Add, Cancel and Abandon.  Documented, too.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prere...
chrome/browser/prerender/prerender_link_manager.h:53: typedef
std::multimap<GURL, ChildAndPrerenderIdPair> UrlToPrerenderIdMap;
On 2012/03/29 01:15:39, Matt Menke wrote:
> Do we really need a multimap here?  Could just use a map with counts.  Seems a
> little simpler to me, though, admittedly, removing things from it is
marginally
> more complicated.

The problem with counts is that we're going to need the IDs shortly for sending
events down.  So while that fix works for this patch, that's no good even in the
near term.

So, about this map: it's with some measure of sorrow that I say it's actually
overly simple.  Treating GURL as the origin of this map ends up being broken. 
We need to key on more: the launching parameter of a prerender are the GURL, the
Referrer, and the session storage namespace.  If a launch matches those three,
it's the same.  Otherwise, it's not the same prerender.  The question is, what's
the complexity of having this logic in prerender_link_manager as well as
prerender_manager/prerender_tab_contents?  Some thought needs to go into getting
this right and not distributed across these three objects.

This upload though, I'm leaving it the same: child and GURL.  The result is that
I'll fail to launch some prerenders (same GURL, different referrer/namespace). 
To make the prerender_manager match properly on these is a lot of surgery that
feels separate from getting this messaging up here: and the brokenness is the
same as what we have already, anyway.

I'm thinking long and hard about how to capture this complexity without making
the upload implementing it awful.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/rende...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/rende...
chrome/browser/renderer_host/chrome_render_message_filter.cc:113:
IPC_MESSAGE_HANDLER(PrerenderMsg_NewLinkPrerender, OnNewLinkPrerender)
On 2012/03/29 00:13:37, cbentzel wrote:
> Could these get moved to a separate class which is a WebContentsObserver?
> Perhaps PrerenderTabHelper? 

Prerendering is confusing enough without mixing up the target and the launcher
more than we do already.  I created a PrerenderMessageFilter which catches these
messages.

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/common/preren...
File chrome/common/prerender_messages.h (right):

https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/common/preren...
chrome/common/prerender_messages.h:28:
IPC_MESSAGE_CONTROL5(PrerenderMsg_NewLinkPrerender,
On 2012/03/29 00:13:37, cbentzel wrote:
> These should be named PrerenderHostMsg to differentiate that they are
> render->browser. I also think they could be ROUTED messages if you move to
> listening in a WebContentsObserver.

Done.

[cbentzel, 2012-04-23 11:35:34]

Will try to review today.

On Fri, Apr 20, 2012 at 1:54 PM, <gavinp@chromium.org> wrote:

> Remember this patch from three weeks ago?  Here's another view on it.  I've
> uploaded it a few ways into this patch, so you can review whichever parts
> you
> wish.  Patches 3-7 show parts close to how it will be staged, and patch 8
> gives
> you a view of all of my changes against trunk.
>
> The main changes are:
>
> 1. All my prerendering code out of webkit/glue and content/.
> 2. Using the updated API from webkit.  The WebKit bug should get an upload
> soon
> that exactly corresponds to this change.
>
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_browsertest.cc<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_browsertest.cc>
> File chrome/browser/prerender/**prerender_browsertest.cc (right):
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_browsertest.cc#**newcode877<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_browsertest.cc#newcode877>
> chrome/browser/prerender/**prerender_browsertest.cc:877:
> IN_PROC_BROWSER_TEST_F(**PrerenderBrowserTest, PrerenderPageRemovingLink)
> {
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Should probably have some tests for multiple link elements.
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc>
> File chrome/browser/prerender/**prerender_link_manager.cc (right):
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode41<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode41>
> chrome/browser/prerender/**prerender_link_manager.cc:41: const
> ChildAndPrerenderIdPair child_and_prerender_id(child_**id, prerender_id);
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Why don't you need to include route_id as well? Do you assume that the
>> prerender_id is unique per process rather than per RenderView? Is this
>>
> an
>
>> appropriate assumption to make?
>>
>
> The prerender_id should be unique across the WebKit instance, and
> there's only one WebKit instance per renderer process.  This is an
> appropriate assumption.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode44<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode44>
> chrome/browser/prerender/**prerender_link_manager.cc:44:
> url_map_.insert(std::make_**pair(url, child_and_prerender_id));
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> I recommend adding a CheckInvariant() function which is only called in
>>
> debug in
>
>> DCHECKs() to ensure that id_map_ and url_map_ are consistent. I would
>>
> put it at
>
>> the start/end of each of the mutation methods.
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode59<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode59>
> chrome/browser/prerender/**prerender_link_manager.cc:59:
> UrlToPrerenderIdMap::iterator url_id_iter = url_map_.find(url);
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> This doesn't guarantee to find the first entry in the multimap for
>>
> url. Use
>
>> lower_bound instead. Or you could use equal_range, but you are
>>
> mutating the
>
>> multimap as you go along.
>>
>
> _The C++ Programming Language_ claims otherwise in 17.4.1.6 (pg 486 in
> the 1st edition).  The C++03 standard, however, does not go so far.  In
> ISO 14882:2003, the claim is only that it finds a matching element. But
> in <algorithm>, the standard says in 25.1.2 [lib.alg.find] that the
> std::find(multimap.begin(), multimap.end(), i) is claimed to find the
> first match.  I wonder why the inconsistancy?  A review of the code to
> my local STL shows std::find() on a multimap is the same as
> multimap<>::find, so I'm good, but I'm switching to lower_bound here.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode65<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode65>
> chrome/browser/prerender/**prerender_link_manager.cc:65:
> url_map_.erase(to_erase);
> On 2012/03/29 01:15:39, Matt Menke wrote:
>
>> I suggest that you only delete entries in one place.  It's a bit
>>
> slower, but
>
>> think just calling "RemovePrerender(blah)" first from here and from
>> OnUnloadedLinkPrerenderImpl makes things simpler.
>>
>
>  Also, then you won't need a loop here.  Just remove the link, and then
>>
> do a
>
>> find.
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode74<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode74>
> chrome/browser/prerender/**prerender_link_manager.cc:74: // TODO(gavinp):
> Track down the correct prerender and stop it, rather than
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Please fix this. Also, it doesn't look like you have feedback from the
>> PrerenderManager back into here in case the URL that this added was
>>
> evicted by a
>
>> different prerender.
>>
>
> Done, in a basic way using the existing PrerenderManager APIs.
>
> After this CL lands, and we have the plumbing in, I intend to land a
> followup CL that connects the PrerenderLinkManager and the
> PrerenderManager much more closely.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.cc#**newcode108<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.cc#newcode108>
> chrome/browser/prerender/**prerender_link_manager.cc:108:
> prerender_manager->link_**manager()->**OnNewLinkPrerenderImpl(
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Not sure I like that this goes "Get the prerender managers link
>>
> manager, and
>
>> have that call into the prerender manager".
>>
>
>  In general I'd like this to exist completely outside of the
>>
> PrerenderManager.
>
>> Perhaps make it a ProfileKeyedService as well?
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.h<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.h>
> File chrome/browser/prerender/**prerender_link_manager.h (right):
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.h#**newcode30<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.h#newcode30>
> chrome/browser/prerender/**prerender_link_manager.h:30: class
> PrerenderLinkManager {
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> It's probably reasonable to unit test some of the management work this
>>
> does.
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.h#**newcode43<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.h#newcode43>
> chrome/browser/prerender/**prerender_link_manager.h:43: static void
> OnRemovedLinkPrerender(**Profile* profile,
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Document. What's different about OnRemoved versus OnUnloaded for
>>
> example.
>
> The new names simplify this; it's Add, Cancel and Abandon.  Documented,
> too.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/prerender/**prerender_link_manager.h#**newcode53<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/prerender/prerender_link_manager.h#newcode53>
> chrome/browser/prerender/**prerender_link_manager.h:53: typedef
> std::multimap<GURL, ChildAndPrerenderIdPair> UrlToPrerenderIdMap;
> On 2012/03/29 01:15:39, Matt Menke wrote:
>
>> Do we really need a multimap here?  Could just use a map with counts.
>>
> Seems a
>
>> little simpler to me, though, admittedly, removing things from it is
>>
> marginally
>
>> more complicated.
>>
>
> The problem with counts is that we're going to need the IDs shortly for
> sending events down.  So while that fix works for this patch, that's no
> good even in the near term.
>
> So, about this map: it's with some measure of sorrow that I say it's
> actually overly simple.  Treating GURL as the origin of this map ends up
> being broken.  We need to key on more: the launching parameter of a
> prerender are the GURL, the Referrer, and the session storage namespace.
>  If a launch matches those three, it's the same.  Otherwise, it's not
> the same prerender.  The question is, what's the complexity of having
> this logic in prerender_link_manager as well as
> prerender_manager/prerender_**tab_contents?  Some thought needs to go into
> getting this right and not distributed across these three objects.
>
> This upload though, I'm leaving it the same: child and GURL.  The result
> is that I'll fail to launch some prerenders (same GURL, different
> referrer/namespace).  To make the prerender_manager match properly on
> these is a lot of surgery that feels separate from getting this
> messaging up here: and the brokenness is the same as what we have
> already, anyway.
>
> I'm thinking long and hard about how to capture this complexity without
> making the upload implementing it awful.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/browser/renderer_host/**chrome_render_message_filter.**cc<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/renderer_host/chrome_render_message_filter.cc>
> File chrome/browser/renderer_host/**chrome_render_message_filter.**cc
> (right):
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
> chrome/browser/renderer_host/**chrome_render_message_filter.**
>
cc#newcode113<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/browser/renderer_host/chrome_render_message_filter.cc#newcode113>
> chrome/browser/renderer_host/**chrome_render_message_filter.**cc:113:
> IPC_MESSAGE_HANDLER(**PrerenderMsg_NewLinkPrerender, OnNewLinkPrerender)
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> Could these get moved to a separate class which is a
>>
> WebContentsObserver?
>
>> Perhaps PrerenderTabHelper?
>>
>
> Prerendering is confusing enough without mixing up the target and the
> launcher more than we do already.  I created a PrerenderMessageFilter
> which catches these messages.
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/common/prerender_**messages.h<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/common/prerender_messages.h>
> File chrome/common/prerender_**messages.h (right):
>
> https://chromiumcodereview.**appspot.com/9875026/diff/2001/**
>
chrome/common/prerender_**messages.h#newcode28<https://chromiumcodereview.appspot.com/9875026/diff/2001/chrome/common/prerender_messages.h#newcode28>
> chrome/common/prerender_**messages.h:28:
> IPC_MESSAGE_CONTROL5(**PrerenderMsg_NewLinkPrerender,
> On 2012/03/29 00:13:37, cbentzel wrote:
>
>> These should be named PrerenderHostMsg to differentiate that they are
>> render->browser. I also think they could be ROUTED messages if you
>>
> move to
>
>> listening in a WebContentsObserver.
>>
>
> Done.
>
>
https://chromiumcodereview.**appspot.com/9875026/<https://chromiumcodereview....
>

[cbentzel, 2012-04-24 01:38:33]

On 2012/04/23 11:35:34, cbentzel wrote:
> Will try to review today.

Oops. Tomorrow morning, too tired now. Sorry.

[mmenke, 2012-04-24 15:26:15]

Mostly nit-ish comments, though I've tried to refrain from going overboard on
them, this pass.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:826:
loader_query_and_fragment_);
We have "src_url" but "loader_query_and_fragment_" and "loader_path".  Know it
was like this before, but I find it a little confusing.  I suggest renaming the
latter two to something like "src_url_query_and_fragment_" and "src_path"
respectively.

Alternatively, could rename src_url to loader_url.  That makes the variable
names a little clearer, at the cost of losing the src/dest symmetry.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:965: NavigateToDestURL();
I'd suggest you make sure that the link manager is empty after the tab has been
navigated.  Same goes for the 3 new tests.  There may be other tests where this
is a good idea, too (Like the crash one).

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:23: bool
CheckMapsAreInverseOfEachOther(const MapForwards& map_forwards,
nit:  Suggest you rename this "MapsAreInverseOfEachOther".  Mostly because
DCHECK(Check...) is just a little weird.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:34: e = map_backwards.end();
Is this necessary?  Given that the maps have the same size, I'd think not,
assuming that the underlying map classes work.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:92: child_id,
render_view_route_id, url, referrer);
Random comment:  When this fails, is there anything to be gained by tracking the
prerender?  We have a NOTREACHED in OnCancelPrerender, which is nice for
testing, but beyond that...

Nice thing about not tracking ids that don't have prerenders, is that we could
probably simplify things a bit here - get rid of the maps, for example, though
we'd probably want to remove ids when a prerender is cancelled by the
PrerenderManager, in that case.  Then we could simplify this class a little - no
need for two maps, for example.  The limit on prerenders will always be low
enough that we wouldn't gain anything from having inverse maps.  As it is,
they're currently only useful when a website behaves somewhat bizarrely,
spamming us with prerender requests.

Downside is that the list of requested prerenders that we ignored could one day
be useful.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:40: int child_id,
nit:  I suggest always putting child_id first.  This is both the order of the id
pair, and it makes more sense to have the "more significant" id first.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:62: typedef
std::map<ChildAndPrerenderIdPair, GURL> PrerenderIdToUrlMap;
nit:  Suggest you rename this either IdToUrlMap, IdPairToUrlMap,
PrerenderIdsToUrlMap, or some such.  Just make clear it's not just a
prerender_id.  And a corresponding change to the other typedef, of course.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:69: UrlToPrerenderIdMap
url_map_;
nit:  Suggest you rename these ids_to_url_map_, etc.  It's not uncommon to name
maps after the values instead of the keys.  In fact, a quick search indicates
that naming maps after the values may be more common.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_unittest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:250: bool
AddSimplePrerenderViaManager(const GURL& url) {
This is little weird.  Since all link rel prerenders now go through the
PrerenderLinkManager, we're basically testing things that can't happen when we
use this.

I suggest using the omnibox path instead.  Also should have a test where one url
is added by the omnibox before the same URL is added via a link tag.

Alternatively, could make prerender_link_manager()->OnAddPrerender return the
bool it gets back from AddPrerenderFromLinkRelPrerender.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:638: TEST_F(PrerenderTest,
LinkManagerCancel) {
Other test suggestions:

Cancel a prerender that's already long since been cancelled.

Add a prerender twice and make sure we don't cancel the original until it's been
cancelled twice by the link manager.

Add a prerender again after it's already been cancelled by the PrerenderManager
for some other reason.

Add a prerender, cancel it through the link manager, and then add one again.  We
don't have a test that the prerender manager cleans up its maps correctly in the
general case.

[cbentzel, 2012-04-24 15:42:59]

I just reviewed the browser process changes.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:23: bool
CheckMapsAreInverseOfEachOther(const MapForwards& map_forwards,
Nit: Perhaps AreMapsInverseOfEachOther? No actual CHECK'ing is done this.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:27: for (typename
MapForwards::const_iterator i = map_forwards.begin(),
This doesn't quite test inversion. For example, if map_forwards is {'a': 1, 'b':
2} and map_backwards is {1: 'b', 2: 'a'} this would still return true.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:54: void
PrerenderLinkManager::RemovePrerender(
Nit: ordering of class definitions should match declarations.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:73: void
PrerenderLinkManager::OnAddPrerender(const int prerender_id,
Curious - this doesn't match the signature in the header, due to the const
arguments. Does this build? Also unsure if the const is needed for pass-by-value
cases, WDYT?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:85: // TODO(gavinp): Add
tests to insure fragments work, then remove this fragment
Nit: ensure

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:95:
id_map_.insert(std::make_pair(child_and_prerender_id, url));
Should this be done prior to the AddPrerender case? Wondering if this will be
needed if AddPrerender synchronously fails and we want to signal the error back
to the link element.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:117: // TODO(gavinp): Track
down the correct prerender and stop it, rather than
Why not fix this now?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:121:
contents->Destroy(FINAL_STATUS_CANCELLED);
Yes, this seems like the appropriate FINAL_STATUS to use. Matches omnibox
cancellations.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:125: int prerender_id,
Nit: could this fit on the previous line?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:150: i =
id_map_.lower_bound(child_and_minimum_prerender_id),
Clever.

Just to be sure though - if id_map_ only consists of entries with child id's <
child_id, both i and e should be equivalent to id_map_.end()?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:158:
OnAbandonPrerender(prerender_ids_to_abandon.front(), child_id);
Why is this overloading OnAbandonPrerender, if we also want to use that for
navigating away? Seems like we could instantaneously remove prerenders in this
case.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:31: // Launch and cancel
prerenders based on the LinkPrerender element events.
I'd prefer a bit more description here.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:52: // renderer processed was
fast-closed when the last render view in it was
Nit: renderer process

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:18: if
(!PrerenderManager::IsPrerenderingPossible())
This check doesn't seem necessary - kind of covered by the
PrerenderManagerFactory::GetForProfile() which is called in
BuildServiceInstanceFor

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:30: :
ProfileKeyedServiceFactory("PrerenderLinkmanager",
It seems like this factory should depend on PrerenderManagerFactory, so the
PrerenderLinkManager is created after the PrerenderManager for a profile. You
can just use DependsOn to do this.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:49: }
Nit: // namespace prerender

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.h:35: }
Nit: // namespace prerender

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:34: {
Nit: move this up with profile_

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:52: if (message.type() ==
PrerenderHostMsg_AddPrerender::ID ||
What are the lifetime guarantees for both PrerenderMessageFilter and profile_
when these are handled on the UI thread? For example, is it possible for the
profile_ to be destroyed between when OnChannelClosing is called on IO thread
and OnChannelClosingInUIThread is invoked on UI thread?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:40: virtual
~PrerenderMessageFilter() { }
Why are you inlining this destructor?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:50: 
Nit: Remove extra newline.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:53: Profile*const profile_;
spaces please. I haven't really seen use of *const in the codebase but it does
seem appropriate.

[gavinp, 2012-04-26 23:55:39]

Thank you everyone for your reviews!

I've answered comments below, but the remediations are available in
http://codereview.chromium.org/10198040/ , and I'd like to continue hacking at
this bug there.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:826:
loader_query_and_fragment_);
On 2012/04/24 15:26:15, Matt Menke wrote:
> We have "src_url" but "loader_query_and_fragment_" and "loader_path".  Know it
> was like this before, but I find it a little confusing.  I suggest renaming
the
> latter two to something like "src_url_query_and_fragment_" and "src_path"
> respectively.
> 
> Alternatively, could rename src_url to loader_url.  That makes the variable
> names a little clearer, at the cost of losing the src/dest symmetry.

I went with the change to loader_url; what src/dest symmetry are we losing? 
What was called src was the intermediate product, made by translating the loader
file (the src src?) by the replacement text.  That's not src!  That's
intermediate state!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:965: NavigateToDestURL();
On 2012/04/24 15:26:15, Matt Menke wrote:
> I'd suggest you make sure that the link manager is empty after the tab has
been
> navigated.  Same goes for the 3 new tests.  There may be other tests where
this
> is a good idea, too (Like the crash one).

Done.  These tests can even land without being disabled until after the API is
turned on (a subsequent step due to staging), since the prerender_link_manager
will be empty at the end of a run using the phantom request launcher!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:23: bool
CheckMapsAreInverseOfEachOther(const MapForwards& map_forwards,
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename this "MapsAreInverseOfEachOther".  Mostly because
> DCHECK(Check...) is just a little weird.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:27: for (typename
MapForwards::const_iterator i = map_forwards.begin(),
On 2012/04/24 15:42:59, cbentzel wrote:
> This doesn't quite test inversion. For example, if map_forwards is {'a': 1,
'b':
> 2} and map_backwards is {1: 'b', 2: 'a'} this would still return true.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:34: e = map_backwards.end();
On 2012/04/24 15:26:15, Matt Menke wrote:
> Is this necessary?  Given that the maps have the same size, I'd think not,
> assuming that the underlying map classes work.

No it's not, especially now that I fixed the terrible brokenness that Chris
found and commented on, above.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:54: void
PrerenderLinkManager::RemovePrerender(
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: ordering of class definitions should match declarations.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:73: void
PrerenderLinkManager::OnAddPrerender(const int prerender_id,
On 2012/04/24 15:42:59, cbentzel wrote:
> Curious - this doesn't match the signature in the header, due to the const
> arguments. Does this build? Also unsure if the const is needed for
pass-by-value
> cases, WDYT?

This does compile.  You can always add "const" in the definition of a function
to impose a limitation on the definition by making the automatic portion const. 
There's no real point putting const in the declaration of a function, since the
pass-by-value semantic makes it not interesting to the caller.  I would argue as
a style issue the automatic portion should never been given as const in the
definition, but in the declaration it can make sense.

Some examples of declaration types, and the definition type you might use that
is const:

int  -->  const int
int*  -->  int* const
const int*(*f)(int) -->  const int*(*const f)(int)
const int(*(*f)(int (*)())))(const int*)  --> const int (*(*const f)(int
(*)()))(const int*)

and so on.

But, if you think they detract from readability, I'm removing all of the const
from the automatic parts here.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:85: // TODO(gavinp): Add
tests to insure fragments work, then remove this fragment
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: ensure

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:95:
id_map_.insert(std::make_pair(child_and_prerender_id, url));
On 2012/04/24 15:42:59, cbentzel wrote:
> Should this be done prior to the AddPrerender case? Wondering if this will be
> needed if AddPrerender synchronously fails and we want to signal the error
back
> to the link element.

See my reply to mmenke above...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:117: // TODO(gavinp): Track
down the correct prerender and stop it, rather than
On 2012/04/24 15:42:59, cbentzel wrote:
> Why not fix this now?

This here is advanced quite a lot from what was in the last upload; if that same
url is prerendering, it kills it.  To be really correct, we'd need to key off of
the session storage namespace, too.

But getting this perfect probably requires refactoring the interface on
PrerenderManager, and that's a whole other kettle of fish.  At present, this
change consists of thousands of lines across chrome and webkit, requiring at
least four crossings of the WebKit/Chrome boundary to commit properly....

If I can land an interface in the PrerenderLinkManager that is sufficient, if
not ideal, I'm much happier doing that than making this patch larger and keeping
it in flight longer.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:125: int prerender_id,
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: could this fit on the previous line?

Done.  This removes the last sign of the horrible *Impl names from before.  :)

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:150: i =
id_map_.lower_bound(child_and_minimum_prerender_id),
On 2012/04/24 15:42:59, cbentzel wrote:
> Clever.
> 
> Just to be sure though - if id_map_ only consists of entries with child id's <
> child_id, both i and e should be equivalent to id_map_.end()?

Yes.  But we'd never run the loop body in that case, so we'll be fine.  We might
also get two iterators in the middle of the map, or both at begin().

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.cc:158:
OnAbandonPrerender(prerender_ids_to_abandon.front(), child_id);
On 2012/04/24 15:42:59, cbentzel wrote:
> Why is this overloading OnAbandonPrerender, if we also want to use that for
> navigating away? Seems like we could instantaneously remove prerenders in this
> case. 

What about the case of the user navigating away from the last site for a
renderer, through a redirect chain to their final site?  We'll fast kill the
site we're leaving pretty much as soon as the new renderer commits, killing the
prerender.  But a redirect chain would almost certainly still be in flight...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:31: // Launch and cancel
prerenders based on the LinkPrerender element events.
On 2012/04/24 15:42:59, cbentzel wrote:
> I'd prefer a bit more description here.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:40: int child_id,
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  I suggest always putting child_id first.  This is both the order of the
id
> pair, and it makes more sense to have the "more significant" id first.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:52: // renderer processed was
fast-closed when the last render view in it was
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: renderer process

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:62: typedef
std::map<ChildAndPrerenderIdPair, GURL> PrerenderIdToUrlMap;
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename this either IdToUrlMap, IdPairToUrlMap,
> PrerenderIdsToUrlMap, or some such.  Just make clear it's not just a
> prerender_id.  And a corresponding change to the other typedef, of course.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager.h:69: UrlToPrerenderIdMap
url_map_;
On 2012/04/24 15:26:15, Matt Menke wrote:
> nit:  Suggest you rename these ids_to_url_map_, etc.  It's not uncommon to
name
> maps after the values instead of the keys.  In fact, a quick search indicates
> that naming maps after the values may be more common.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:18: if
(!PrerenderManager::IsPrerenderingPossible())
On 2012/04/24 15:42:59, cbentzel wrote:
> This check doesn't seem necessary - kind of covered by the
> PrerenderManagerFactory::GetForProfile() which is called in
> BuildServiceInstanceFor

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.cc:30: :
ProfileKeyedServiceFactory("PrerenderLinkmanager",
On 2012/04/24 15:42:59, cbentzel wrote:
> It seems like this factory should depend on PrerenderManagerFactory, so the
> PrerenderLinkManager is created after the PrerenderManager for a profile. You
> can just use DependsOn to do this.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_link_manager_factory.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_link_manager_factory.h:35: }
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: // namespace prerender 

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:34: {
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: move this up with profile_

Done.

NitFromMeToTheUniverse: WebKit and chrome have different style guides on this!

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.cc:52: if (message.type() ==
PrerenderHostMsg_AddPrerender::ID ||
On 2012/04/24 15:42:59, cbentzel wrote:
> What are the lifetime guarantees for both PrerenderMessageFilter and profile_
> when these are handled on the UI thread? For example, is it possible for the
> profile_ to be destroyed between when OnChannelClosing is called on IO thread
> and OnChannelClosingInUIThread is invoked on UI thread?

At creation, the PrerenderMessageFilter is placed on the IPC::ChannelProxy, and
that occurs on the UI thread.  The scope of that proxy should be the life of the
channel.  However, in the channel closing case, I don't know if it's legitimate,
as I'm posting my own task to another thread.  In the case of the other
messages, they should be good, since the PrerenderMessageFilter should be scoped
inside of the ChannelProxy which itself should be scoped inside of the
RendererProcessHost.

So in the OnChannelClosingInUIThread case, I believe I need to call
g_browser_process->profile_manager()->IsValidProfile(....), but I think I'm fine
in the other cases for the reason stated.  Who should we ask to double check
this logic for us?

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_message_filter.h (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:40: virtual
~PrerenderMessageFilter() { }
On 2012/04/24 15:42:59, cbentzel wrote:
> Why are you inlining this destructor?

It's against the style guide, even.  Fixed.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:50: 
On 2012/04/24 15:42:59, cbentzel wrote:
> Nit: Remove extra newline.

Done.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_message_filter.h:53: Profile*const profile_;
On 2012/04/24 15:42:59, cbentzel wrote:
> spaces please. I haven't really seen use of *const in the codebase but it does
> seem appropriate.

Done.  I'm a fan of making strong statements with const where you can.  It's
when you start seeing const_cast<> that I am willing to talk compromise...

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_unittest.cc (right):

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:250: bool
AddSimplePrerenderViaManager(const GURL& url) {
On 2012/04/24 15:26:15, Matt Menke wrote:
> This is little weird.  Since all link rel prerenders now go through the
> PrerenderLinkManager, we're basically testing things that can't happen when we
> use this.
> 
> I suggest using the omnibox path instead.  Also should have a test where one
url
> is added by the omnibox before the same URL is added via a link tag.
> 
> Alternatively, could make prerender_link_manager()->OnAddPrerender return the
> bool it gets back from AddPrerenderFromLinkRelPrerender.

Done, with the second option.

http://codereview.chromium.org/9875026/diff/15028/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_unittest.cc:638: TEST_F(PrerenderTest,
LinkManagerCancel) {
On 2012/04/24 15:26:15, Matt Menke wrote:
> Other test suggestions:
> 
> Cancel a prerender that's already long since been cancelled.

Yes, but expired instead of cancelled.


> 
> Add a prerender twice and make sure we don't cancel the original until it's
been
> cancelled twice by the link manager.

Done.


> 
> Add a prerender again after it's already been cancelled by the
PrerenderManager
> for some other reason.

Done.


> Add a prerender, cancel it through the link manager, and then add one again. 
We
> don't have a test that the prerender manager cleans up its maps correctly in
the
> general case.

Done, and PrerenderLinkManager::IsEmpty() was great here.