Add pending preloads indexed by routing id. Start preloading once we navigate.

Add pending preloads indexed by routing id. Start preloading once we navigate.

BUG=72519
TEST=Create a page A that has a <link rel=prefetch> hint for another page B. In page B, add a <link rel=prefetch> hint for page A. Visit A and observe the prefetch for A only starts on navigation to B.

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

[cbentzel, 2011-03-14 14:05:55]

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:471: bool
is_from_prerender = ((load_flags & net::LOAD_PRERENDER) != 0);
It seems like this CL would be a lot simpler if you simply skipped MaybeCreate
in this case, or failed internally in MaybeCreate. 

Why would we try to create a Prerender within a Prerender? Is it something we
should re-evaluate on the WebKit side when the page becomes activated?

[cbentzel, 2011-03-14 14:32:25]

I'm generally of the mindset that I'd prefer to implement the
delayed-prerender-instantiation when it becomes necessary - it adds a fair
amount of complexity.

Perhaps add a histogram to see how frequently the prefetch-during-prerender
events happen?

If you decide to keep it, please add unit tests to prerender_manager_unittest.

http://codereview.chromium.org/6625066/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6625066/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:114:
DCHECK(pending_prerender_map_.find(route_id) == pending_prerender_map_.end());
What if there are two <link rel="prefetch"> elements on the page that is
prerendered? This will trigger then.

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:471: bool
is_from_prerender = ((load_flags & net::LOAD_PRERENDER) != 0);
On 2011/03/14 14:05:55, cbentzel wrote:
> It seems like this CL would be a lot simpler if you simply skipped MaybeCreate
> in this case, or failed internally in MaybeCreate. 
> 
> Why would we try to create a Prerender within a Prerender? Is it something we
> should re-evaluate on the WebKit side when the page becomes activated?

Ah, I see you are caching the values in the PrerenderManager to start the nested
prerenders when the page becomes activated. Let me look some more.

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:477: route_id);
You need both child_id and route_id here. 

Not a huge deal now since we only allow 1 prerender at a time, but if this
loosens up it will break.

[dominich, 2011-03-14 17:47:15]

Right now I expect this to never happen as the likelihood of one link rel
prefetch links pointing to another page with a link rel prefetch is vanishingly
small. However, if we do get this situation (and it is easy to build a site that
would do that if all the menu options have link rel prefetch links) I'd rather
have a solution in place now than see bandwidth usage and server load get hit
down the road and have to respond to it.

The other CL http://codereview.chromium.org/6677005/ that I have also works
around this issue but it is not as elegant a solution and is meant as a
catch-all to stop any flooding.

http://codereview.chromium.org/6625066/diff/8001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/6625066/diff/8001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:114:
DCHECK(pending_prerender_map_.find(route_id) == pending_prerender_map_.end());
On 2011/03/14 14:32:26, cbentzel wrote:
> What if there are two <link rel="prefetch"> elements on the page that is
> prerendered? This will trigger then.

Good catch. I can remove the DCHECK, but perhaps it is better to also change the
container from a map to something like a vector of pairs. Then on navigation I
can trigger all of the prerenders for the page in the right order and let the
existing logic take care of which start. What do you think?

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:471: bool
is_from_prerender = ((load_flags & net::LOAD_PRERENDER) != 0);
On 2011/03/14 14:05:55, cbentzel wrote:
> It seems like this CL would be a lot simpler if you simply skipped MaybeCreate
> in this case, or failed internally in MaybeCreate. 
> 
> Why would we try to create a Prerender within a Prerender? Is it something we
> should re-evaluate on the WebKit side when the page becomes activated?

This happens when we have a prerender link to a page with a prerender link.
WebKit could take this into account instead of doing it at the browser level,
but maybe in the future we'd want to do multiple prerenders and we might rate a
prerender from a prerendered page as higher priority than a second prerender on
the current page.

http://codereview.chromium.org/6625066/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.cc:477: route_id);
On 2011/03/14 14:32:26, cbentzel wrote:
> You need both child_id and route_id here. 
> 
> Not a huge deal now since we only allow 1 prerender at a time, but if this
> loosens up it will break.

Done.

[cbentzel, 2011-03-14 18:06:12]

If you are going to add logic for multiple-prefetch-within-prerendered page
case, I'd like to see tests for it.

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

http://codereview.chromium.org/6625066/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:23: #include
"content/browser/renderer_host/resource_dispatcher_host.h"
Is this needed?

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

http://codereview.chromium.org/6625066/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:170: GURL referrer_;
It looks like default copy-constructor will work correctly here, since needed.

http://codereview.chromium.org/6625066/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:174: PendingPrerenderList
pending_prerender_list_;
Why not map of pair<child_id, route_id> to vector<PendingContents> where
PendingContents has url_, alias_urls_, referrer? That would preserve ordering.

[dominich, 2011-03-14 19:42:34]

Added a test too.

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

http://codereview.chromium.org/6625066/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:23: #include
"content/browser/renderer_host/resource_dispatcher_host.h"
On 2011/03/14 18:06:13, cbentzel wrote:
> Is this needed?

Merge issue that keeps coming back.

Done.

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

http://codereview.chromium.org/6625066/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:174: PendingPrerenderList
pending_prerender_list_;
On 2011/03/14 18:06:13, cbentzel wrote:
> Why not map of pair<child_id, route_id> to vector<PendingContents> where
> PendingContents has url_, alias_urls_, referrer? That would preserve ordering.

Done.

[cbentzel, 2011-03-15 18:42:24]

+mmenke for the browsertest changes.

I'll LGTM after these suggestions, although I'm still going to advocate that the
deferral seems very complicated.

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

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:94:
expected_final_status_map_[url] = expected_final_status;
Should this DCHECK that it doesn't already exist?

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:103: if (it !=
expected_final_status_map_.end())
Does this need to be erased if found, or can it stick around?

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:230: bool
UrlIsInPrerenderManager(const std::string& url) {
const

also, the name is misleading - the parameter is not actually a full URL.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:239: for
(PrerenderManager::PendingPrerenderList::iterator map_it =
Add a FindPendingEntry() to PrerenderManager? Would make more sense to have the
logic there.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:254: GURL UrlForHtmlFile(const
std::string& html_file) {
const, I think GetURL is const.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:271:
WaitForLoadPrerenderContentsFactory* prc_factory =
I would just set a pointer to prc_factory in PrerenderTestURL, or even create it
in the constructor - this is a bit gross.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:277: void
SetExpectedFinalStatusForUrl(const GURL& url,
I would change this to take a const std::string& like
UrlIsPendingInPrerenderManager does, call UrlForHtmlFile internally, and make
UrlForHtmlFile private. I think prerender_manager() could also be private.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:413: const char* url_a =
"prerender_infinite_a.html";
Nit: const char* const kPathA

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:465: }
Nit: insert an extra newline here.

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

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:110: void
PrerenderManager::AddPendingPreload(int child_id, int route_id,
Why isn't this just a const-ref-pair? You are unpacking the pair in
PrerenderResourceHandler and repacking here.

http://codereview.chromium.org/6625066/diff/16005/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/16005/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:469: bool
is_from_prerender = ((load_flags & net::LOAD_PRERENDER) != 0);
load_flags & net::LOAD_PRERENDER is sufficient.

you could also just do this inline with the arguments, removing the need for
is_from_prerender.

[cbentzel, 2011-03-15 18:46:13]

On Tue, Mar 15, 2011 at 2:42 PM, <cbentzel@chromium.org> wrote:

> +mmenke for the browsertest changes.
>
> I'll LGTM after these suggestions, although I'm still going to advocate
> that the
> deferral seems very complicated.
>

To be more clear, I think the implementation of deferral is not
overly-complicated - just that the need for a deferral mechanism is
speculative and adds extra complexity. But I've mentioned that a few points,
and since it's implemented we may as well use it.

[dominich.google, 2011-03-15 19:12:51]

On Tue, Mar 15, 2011 at 11:46 AM, Chris Bentzel <cbentzel@chromium.org>wrote:

>
>
> On Tue, Mar 15, 2011 at 2:42 PM, <cbentzel@chromium.org> wrote:
>
>> +mmenke for the browsertest changes.
>>
>> I'll LGTM after these suggestions, although I'm still going to advocate
>> that the
>> deferral seems very complicated.
>>
>
> To be more clear, I think the implementation of deferral is not
> overly-complicated - just that the need for a deferral mechanism is
> speculative and adds extra complexity. But I've mentioned that a few points,
> and since it's implemented we may as well use it.
>
>
>
I agree that it is speculative, and it's entirely possible to fix the
infinite loop of prerendering without this, however I think it is important
that this feature be as foolproof as we can make it for people. If pages on
their site sometimes prerender and sometimes don't, which is how the
cancellation solution to the infinite loop issue would appear, then they're
less likely to use the feature.

[mmenke, 2011-03-15 19:20:14]

Unless I'm missing something, it seems like there's a potential leak here.  If a
prerendered page is navigated to or destroyed (Or both), and a resource request
initiated by that page makes it to PrerenderResourceHandler::OnResponseStarted
afterwards, the url remains in pending_prerender_list_ until the
PrerenderManager is deleted (Or another prerender occurs for the same rvh, I
suppose).  In the case that the RVH was displayed, in addition to the leak, you
end up skipping prerendering in a case where it should have happened.

[dominich, 2011-03-15 19:32:22]

On 2011/03/15 19:20:14, Matt Menke wrote:
> Unless I'm missing something, it seems like there's a potential leak here.  If
a
> prerendered page is navigated to or destroyed (Or both), and a resource
request
> initiated by that page makes it to PrerenderResourceHandler::OnResponseStarted
> afterwards, the url remains in pending_prerender_list_ until the
> PrerenderManager is deleted (Or another prerender occurs for the same rvh, I
> suppose).  In the case that the RVH was displayed, in addition to the leak,
you
> end up skipping prerendering in a case where it should have happened.

When RemoveEntry is called on the PrerenderManager, it also removes any pending
prerenders that were set up for the RVH. If we navigate, we'll already have
started them. If not, they'll be erased here.

[mmenke, 2011-03-15 19:35:08]

On 2011/03/15 19:32:22, dominic wrote:
> On 2011/03/15 19:20:14, Matt Menke wrote:
> > Unless I'm missing something, it seems like there's a potential leak here. 
If
> a
> > prerendered page is navigated to or destroyed (Or both), and a resource
> request
> > initiated by that page makes it to
PrerenderResourceHandler::OnResponseStarted
> > afterwards, the url remains in pending_prerender_list_ until the
> > PrerenderManager is deleted (Or another prerender occurs for the same rvh, I
> > suppose).  In the case that the RVH was displayed, in addition to the leak,
> you
> > end up skipping prerendering in a case where it should have happened.
> 
> When RemoveEntry is called on the PrerenderManager, it also removes any
pending
> prerenders that were set up for the RVH. If we navigate, we'll already have
> started them. If not, they'll be erased here.

A pending prerender may not be in that list...It may still be on the IO thread,
prior to being passed to the UI thread to be added to the list, or it may be in
a task queued by the PrerenderResourceManager for the UI thread to handle.

[dominich, 2011-03-15 20:13:00]

On 2011/03/15 19:35:08, Matt Menke wrote:
> On 2011/03/15 19:32:22, dominic wrote:
> > On 2011/03/15 19:20:14, Matt Menke wrote:
> > > Unless I'm missing something, it seems like there's a potential leak here.

> If
> > a
> > > prerendered page is navigated to or destroyed (Or both), and a resource
> > request
> > > initiated by that page makes it to
> PrerenderResourceHandler::OnResponseStarted
> > > afterwards, the url remains in pending_prerender_list_ until the
> > > PrerenderManager is deleted (Or another prerender occurs for the same rvh,
I
> > > suppose).  In the case that the RVH was displayed, in addition to the
leak,
> > you
> > > end up skipping prerendering in a case where it should have happened.
> > 
> > When RemoveEntry is called on the PrerenderManager, it also removes any
> pending
> > prerenders that were set up for the RVH. If we navigate, we'll already have
> > started them. If not, they'll be erased here.
> 
> A pending prerender may not be in that list...It may still be on the IO
thread,
> prior to being passed to the UI thread to be added to the list, or it may be
in
> a task queued by the PrerenderResourceManager for the UI thread to handle.

Matt convinced me over IM and I'm working on a way to solve this before I
commit. My first thought is to add the pending prerender list to the periodic
cleanup, but that feels like a quick fix rather than the right fix.

[dominich, 2011-03-15 20:41:13]

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

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:94:
expected_final_status_map_[url] = expected_final_status;
On 2011/03/15 18:42:24, cbentzel wrote:
> Should this DCHECK that it doesn't already exist? 

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:103: if (it !=
expected_final_status_map_.end())
On 2011/03/15 18:42:24, cbentzel wrote:
> Does this need to be erased if found, or can it stick around? 

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:239: for
(PrerenderManager::PendingPrerenderList::iterator map_it =
On 2011/03/15 18:42:24, cbentzel wrote:
> Add a FindPendingEntry() to PrerenderManager? Would make more sense to have
the
> logic there. 

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:254: GURL UrlForHtmlFile(const
std::string& html_file) {
On 2011/03/15 18:42:24, cbentzel wrote:
> const, I think GetURL is const.

Sadly not.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:271:
WaitForLoadPrerenderContentsFactory* prc_factory =
On 2011/03/15 18:42:24, cbentzel wrote:
> I would just set a pointer to prc_factory in PrerenderTestURL, or even create
it
> in the constructor - this is a bit gross. 

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:277: void
SetExpectedFinalStatusForUrl(const GURL& url,
On 2011/03/15 18:42:24, cbentzel wrote:
> I would change this to take a const std::string& like
> UrlIsPendingInPrerenderManager does, call UrlForHtmlFile internally, and make
> UrlForHtmlFile private. I think prerender_manager() could also be private.

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:413: const char* url_a =
"prerender_infinite_a.html";
On 2011/03/15 18:42:24, cbentzel wrote:
> Nit: const char* const kPathA

Done.

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:465: }
On 2011/03/15 18:42:24, cbentzel wrote:
> Nit: insert an extra newline here.

Done.

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

http://codereview.chromium.org/6625066/diff/16005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:110: void
PrerenderManager::AddPendingPreload(int child_id, int route_id,
On 2011/03/15 18:42:24, cbentzel wrote:
> Why isn't this just a const-ref-pair? You are unpacking the pair in
> PrerenderResourceHandler and repacking here.

Done.

http://codereview.chromium.org/6625066/diff/16005/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/16005/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:469: bool
is_from_prerender = ((load_flags & net::LOAD_PRERENDER) != 0);
On 2011/03/15 18:42:24, cbentzel wrote:
> load_flags & net::LOAD_PRERENDER is sufficient.
> 
> you could also just do this inline with the arguments, removing the need for
> is_from_prerender.

Done.

[cbentzel, 2011-03-16 01:23:28]

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

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:238: bool
UrlIsInPrerenderManager(const std::string& html_file) {
const

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:244: bool
UrlIsPendingInPrerenderManager(const std::string& html_file){
Nit: space between ) and { at end of line.

also, const

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

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:126:
PendingPrerenderList::iterator it =
Should this do FindPendingEntry(url) and early return, like AddPreload does?

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:289:
PrerenderManager::FindPendingEntry(const GURL& url) {
This should be const [I noticed FindEntry wasn't, however].

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:297: if (content_it->url_ == url)
This will probably want to match against alias urls as well.

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6625066/diff/23014/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:205: if (make_pending)
Nit: use {} here since statement spans multiple lines - ditto for else.

http://codereview.chromium.org/6625066/diff/23014/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_infinite_a.html (right):

http://codereview.chromium.org/6625066/diff/23014/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_infinite_a.html:3: header("Expires: Mon, 26
Jul 1997 05:00:00 GMT");
Why do these matter?

[dominich, 2011-03-18 17:29:57]

Fixed the leak issue and fixed the nits.

[cbentzel, 2011-03-18 18:35:43]

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

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:244: bool
UrlIsPendingInPrerenderManager(const std::string& html_file){
Nit: space between ) and {

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:275: GURL UrlForHtmlFile(const
std::string& html_file) {
const

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

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:129: it != prerender_list_.end();
++it) {
This is a pretty subtle case. Could you add a test for it? It may be easier to
do with a prerender_manager_unittest rather than a browser test.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:133: rvh->process() &&
You don't check for process() in RemovePendingPreload below, is it needed?

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:146: // from prerendered pages
will be missed if the user navigates quickly.
Should you add a FINAL_STATUS entry here? This is equivalent to a missed chance
for prerendering.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:147: if (!is_valid_prerender)
Thanks for the comment here.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:355: void
PrerenderManager::RemovePendingPreload(PrerenderContents* entry) {
can |entry| be const?

http://codereview.chromium.org/6625066/diff/17002/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_infinite_a_multiple.html (right):

http://codereview.chromium.org/6625066/diff/17002/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_infinite_a_multiple.html:3:
header("Expires: Mon, 26 Jul 1997 05:00:00 GMT");
Why are these cache headers needed?

http://codereview.chromium.org/6625066/diff/17002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/17002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:471: *request,
context, handler, ((load_flags & net::LOAD_PRERENDER) != 0),
Nit: can just be load_flags & net::LOAD_PRERENDER, don't need to compare against
0.

[dominich, 2011-03-21 16:36:11]

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

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:244: bool
UrlIsPendingInPrerenderManager(const std::string& html_file){
On 2011/03/18 18:35:43, cbentzel wrote:
> Nit: space between ) and {

Done.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:275: GURL UrlForHtmlFile(const
std::string& html_file) {
On 2011/03/18 18:35:43, cbentzel wrote:
> const

test_server()->GetURL is not const. I'm considering building a CL to try to
const up test_server() to enable more correctness.

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

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:129: it != prerender_list_.end();
++it) {
On 2011/03/18 18:35:43, cbentzel wrote:
> This is a pretty subtle case. Could you add a test for it? It may be easier to
> do with a prerender_manager_unittest rather than a browser test. 

Done.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:133: rvh->process() &&
On 2011/03/18 18:35:43, cbentzel wrote:
> You don't check for process() in RemovePendingPreload below, is it needed?

Actually no - I checked that when I wrote RemovePendingPreload but didn't check
when I wrote this one, and apparently my memory is terrible.

Done.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:146: // from prerendered pages
will be missed if the user navigates quickly.
On 2011/03/18 18:35:43, cbentzel wrote:
> Should you add a FINAL_STATUS entry here? This is equivalent to a missed
chance
> for prerendering. 

I thought about it, but we're not creating the prerender and then throwing it
away so I didn't think there were wasted resources. However you're right that
it's good to track any time we fail to prerender when we can as if it's a high
rate we might want to follow the advice in the comment. Thanks for pointing that
out.

http://codereview.chromium.org/6625066/diff/17002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:355: void
PrerenderManager::RemovePendingPreload(PrerenderContents* entry) {
On 2011/03/18 18:35:43, cbentzel wrote:
> can |entry| be const? 

no - render_view_host() is not const.

Is there precedence for having a const version of accessors? Based on the style
guide having a const overload would be a bad thing, but how about
render_view_host_const()?

http://codereview.chromium.org/6625066/diff/17002/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_infinite_a_multiple.html (right):

http://codereview.chromium.org/6625066/diff/17002/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_infinite_a_multiple.html:3:
header("Expires: Mon, 26 Jul 1997 05:00:00 GMT");
On 2011/03/18 18:35:43, cbentzel wrote:
> Why are these cache headers needed?

They were needed when I was testing something locally. I can remove them.

http://codereview.chromium.org/6625066/diff/17002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/17002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:471: *request,
context, handler, ((load_flags & net::LOAD_PRERENDER) != 0),
On 2011/03/18 18:35:43, cbentzel wrote:
> Nit: can just be load_flags & net::LOAD_PRERENDER, don't need to compare
against
> 0. 

Windows build fails with an error about forcing an int to a boolean.

[cbentzel, 2011-03-21 19:46:04]

You are going to want to get rid of the changes I made to
PrerenderResourceHandler in this CL - otherwise prefetches in prerender will be
ignored.

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:87: virtual bool child_id(int*
child_id) const;
Since these are not simple getters, should probably change to GetChildId and
indicate what the boolean return value indicates

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:30: FINAL_STATUS_NESTED,
You will probably want to get rid of NESTED when you introduce this CL.

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

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:145: bool has_child_id =
pc->child_id(&child_id);
Probably not important, but you could do

if (pc->child_id(&child_id) && pc->route_id(&route_id) && child_id == ...)

to short-circuit the route_id call when child_id fails.

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:227:
CHECK(pc->child_id(&child_id));
CHECK is probably OK here, although I'd hate to crash in a release if we are
wrong. 

Maybe DCHECK and have child_id, route_id assign an invalid_number in the false
case?

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:407: RenderViewHost* rvh =
entry->render_view_host();
You could remove the rvh here, remove the CHECK's from child_id and route_id,
and test if those work.

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

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:116: struct PendingContentsData;
the struct declaration should go at top of protected: section

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:209: if (make_pending)
Nit: add braces

http://codereview.chromium.org/6625066/diff/33002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/33002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:464:
prerender::PrerenderResourceHandler::MaybeCreate(
One argument per line in this case [many others are that way in
resource_dispatcher_host]

[dominich, 2011-03-21 20:25:51]

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:87: virtual bool child_id(int*
child_id) const;
On 2011/03/21 19:46:04, cbentzel wrote:
> Since these are not simple getters, should probably change to GetChildId and
> indicate what the boolean return value indicates

Done.

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_final_status.h (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_final_status.h:30: FINAL_STATUS_NESTED,
On 2011/03/21 19:46:04, cbentzel wrote:
> You will probably want to get rid of NESTED when you introduce this CL. 

Done.

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

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:145: bool has_child_id =
pc->child_id(&child_id);
On 2011/03/21 19:46:04, cbentzel wrote:
> Probably not important, but you could do
> 
> if (pc->child_id(&child_id) && pc->route_id(&route_id) && child_id == ...)
> 
> to short-circuit the route_id call when child_id fails.

Short-circuited, but slightly differently to retain what I think is the more
readable conditional statement.

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:227:
CHECK(pc->child_id(&child_id));
On 2011/03/21 19:46:04, cbentzel wrote:
> CHECK is probably OK here, although I'd hate to crash in a release if we are
> wrong. 
> 
> Maybe DCHECK and have child_id, route_id assign an invalid_number in the false
> case?

I considered that, but I wasn't sure what an invalid child or route id would be.
-2 is reserved for 'we don't have one yet' and kint32max is reserved for general
messages. There isn't an INVALID routing ID that I can find.

I could assume -1, but I'm not comfortable with that. I could also introduce -1
as invalid.

Thanks to the prerendering_has_started() check above we know we have a valid
RenderViewHost at this point. If you prefer, I can replace the CHECK with a
DCHECK on the returned booleans?

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:407: RenderViewHost* rvh =
entry->render_view_host();
On 2011/03/21 19:46:04, cbentzel wrote:
> You could remove the rvh here, remove the CHECK's from child_id and route_id,
> and test if those work. 

Done.

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

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:116: struct PendingContentsData;
On 2011/03/21 19:46:04, cbentzel wrote:
> the struct declaration should go at top of protected: section
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:209: if (make_pending)
On 2011/03/21 19:46:04, cbentzel wrote:
> Nit: add braces

Done.

http://codereview.chromium.org/6625066/diff/33002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/6625066/diff/33002/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:464:
prerender::PrerenderResourceHandler::MaybeCreate(
On 2011/03/21 19:46:04, cbentzel wrote:
> One argument per line in this case [many others are that way in
> resource_dispatcher_host]

Done.

[cbentzel, 2011-03-21 21:07:38]

LGTM

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:209:
prerender_manager_->AddPreload(url, alias_urls, referrer);
Nit: braces go here as well [same for else than if]

[mmenke, 2011-03-21 21:18:57]

LGTM

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

http://codereview.chromium.org/6625066/diff/33002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:227:
CHECK(pc->child_id(&child_id));
On 2011/03/21 20:25:51, dominic wrote:
> On 2011/03/21 19:46:04, cbentzel wrote:
> > CHECK is probably OK here, although I'd hate to crash in a release if we are
> > wrong. 
> > 
> > Maybe DCHECK and have child_id, route_id assign an invalid_number in the
false
> > case?
> 
> I considered that, but I wasn't sure what an invalid child or route id would
be.
> -2 is reserved for 'we don't have one yet' and kint32max is reserved for
general
> messages. There isn't an INVALID routing ID that I can find.
> 
> I could assume -1, but I'm not comfortable with that. I could also introduce
-1
> as invalid.
> 
> Thanks to the prerendering_has_started() check above we know we have a valid
> RenderViewHost at this point. If you prefer, I can replace the CHECK with a
> DCHECK on the returned booleans?

I've seen -1 used to check for invalid elsewhere, though I can't recall the
context.  I believe it was in either RenderViewHost or a different
ResourceHandler.

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.h (right):

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:73: typedef
Callback5<std::pair<int, int>,
You can make all 3 of these "const std::pair<int, int>&".  Not sure if it has
any real performance impact, but as you do it elsewhere, may as well do it here,
too.  And yes, it works fine with callbacks.

[dominich, 2011-03-22 15:54:45]

Everything passed try and is merged with trunk. I figured you might want one
more look before I commit.

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:209:
prerender_manager_->AddPreload(url, alias_urls, referrer);
On 2011/03/21 21:07:38, cbentzel wrote:
> Nit: braces go here as well [same for else than if]

Done.

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler.h (right):

http://codereview.chromium.org/6625066/diff/37002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:73: typedef
Callback5<std::pair<int, int>,
On 2011/03/21 21:18:58, Matt Menke wrote:
> You can make all 3 of these "const std::pair<int, int>&".  Not sure if it has
> any real performance impact, but as you do it elsewhere, may as well do it
here,
> too.  And yes, it works fine with callbacks.

Nice catch :)

[cbentzel, 2011-03-22 18:43:40]

LGTM