Add PrerenderResourceHandler and hook it into the ResourceDispatcherHost....

Add PrerenderResourceHandler and hook it into the ResourceDispatcherHost.

The PrerenderResourceHandler will initiate prerendering of a web page under the following conditions:
  - The initial request is a GET for a ResourceType::PREFETCH resource. 
  - The mime-type (sniffed or explicitly specified) of the resource is text/html.
  - The response status code is a 200.
  - The top-level page will not need to be revalidated until after it's guaranteed to be removed from potential use.

The handler passes along all data to the backing resource handler.

BUG=61745
TEST=unit_tests --gtest_filter="PrerenderResourceHandlerTest*"

Manual Test: Start Chrome with --enable-page-prerender. Go to a page with a <link rel=prefetch> element. Navigate to that element, and make sure that it uses the prerendered view.

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

[cbentzel, 2010-12-16 21:59:52]

darin: I added you as reviewer since this touches ResourceDispatcherHost and
ChromeURLRequestContext. Please redirect if necessary.

gavinp+tburkard: please review it all.

I need to add a few more test cases to the unit test, but wanted to send this
out.

[cbentzel, 2010-12-16 22:49:00]

[+eroman, who could also review the RenderDispatcherHost/ChromeURLRequestContext
changes]

[gavinp, 2010-12-17 16:15:33]

Just one question about design...

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_resource_handler.cc:56: //   
DCHECK(url_.is_valid());
lose the comment.

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_resource_handler.cc:117: bool
PrerenderResourceHandler::ShouldPrerender(
anon namespace?

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:454:
context->prerender_manager());
An idea, not sure if it's the right thing: how about PreRenderResourceHandler
has a private constructor, and instead there's a static method taking handler
and returning an handler, which hides CouldPrerender's call?  Two wins: this
call site is more succinct, and the unit test can check that CouldPrerender is
good?

[cbentzel, 2010-12-17 16:23:21]

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_resource_handler.cc (right):

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_resource_handler.cc:56: //   
DCHECK(url_.is_valid());
On 2010/12/17 16:15:33, gavinp wrote:
> lose the comment.

Done.

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_resource_handler.cc:117: bool
PrerenderResourceHandler::ShouldPrerender(
On 2010/12/17 16:15:33, gavinp wrote:
> anon namespace?

Huh? These are static methods on PrerenderResourceHandler.

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/5912001/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:454:
context->prerender_manager());
On 2010/12/17 16:15:33, gavinp wrote:
> An idea, not sure if it's the right thing: how about PreRenderResourceHandler
> has a private constructor, and instead there's a static method taking handler
> and returning an handler, which hides CouldPrerender's call?  Two wins: this
> call site is more succinct, and the unit test can check that CouldPrerender is
> good?

I'm not sure I understand what you are asking for - or more importantly, the
value of it.

It seems like this would simply to change to

if (context && context->prerender_manager()) {
  handler = PrerenderResourceHandler::MaybeCreate(handler, request,
context->prerender_manager());
}

Where MaybeCreate would also need to return the passed in handler if
CouldPrerender is false. It seems like that makes the behavior less explicit at
the call site, and it doesn't make it that much more succinct.

I could add a unit test explicitly for CouldPrerender if desired.

[cbentzel, 2010-12-17 19:24:03]

[-eroman,+willchan]

Eric is busy today, so adding Will Chan for the
ResourceDispatcherHost/ChromeURLRequestContext changes.

[darin (slow to review), 2010-12-17 19:58:21]

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:101:
prerender_callback_->Run(url);
there seems to be the assumption here that GET requests are idempotent.  that's
not really true in all cases.

ideally, you would inspect the prefetched response to determine if it is
cachable, and then only kick off a prerender if you know that the document is
going to be fetched from the cache.  or maybe you would set some flag to coerce
it to be fetched from the cache (and fail if it cannot be for some reason, as we
do for form resubmissions).

[cbentzel, 2010-12-17 20:03:50]

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:101:
prerender_callback_->Run(url);
On 2010/12/17 19:58:21, darin wrote:
> there seems to be the assumption here that GET requests are idempotent. 
that's
> not really true in all cases.
> 
> ideally, you would inspect the prefetched response to determine if it is
> cachable, and then only kick off a prerender if you know that the document is
> going to be fetched from the cache.  or maybe you would set some flag to
coerce
> it to be fetched from the cache (and fail if it cannot be for some reason, as
we
> do for form resubmissions).

I can add that to ShouldPrerender - this is called before we decide to
prerender, but after the response headers have been handled [and after
content-sniffing has been done if necessary].

[cbentzel, 2010-12-17 20:06:07]

On 2010/12/17 20:03:50, cbentzel wrote:
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_handler.cc (right):
> 
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.cc:101:
> prerender_callback_->Run(url);
> On 2010/12/17 19:58:21, darin wrote:
> > there seems to be the assumption here that GET requests are idempotent. 
> that's
> > not really true in all cases.
> > 
> > ideally, you would inspect the prefetched response to determine if it is
> > cachable, and then only kick off a prerender if you know that the document
is
> > going to be fetched from the cache.  or maybe you would set some flag to
> coerce
> > it to be fetched from the cache (and fail if it cannot be for some reason,
as
> we
> > do for form resubmissions).
> 
> I can add that to ShouldPrerender - this is called before we decide to
> prerender, but after the response headers have been handled [and after
> content-sniffing has been done if necessary].

To be more clear: we can inspect the headers to see if the content is cacheable,
and only do the prerendering in that case. However, this is only done on the
top-level page rather than subresources.

[willchan no longer on Chromium, 2010-12-17 21:45:17]

Preliminary look seems fine to me, although I didn't read the
PrerenderResourceHandler stuff in detail.  I'm hoping Darin can handle that, so
I can be lazy and get to some other work :)  Darin, let me know if I should take
a look.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager : public
base::RefCounted<PrerenderManager>,
Add a TODO(willchan): to remove the refcounting.  I of course see why you've
done it.  But as noted in the multi-profile discussion emails, I plan to solve
the reason you've had to make this refcounted.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:24: private NonThreadSafe {
Please avoid private inheritance:
http://www.parashift.com/c++-faq-lite/private-inheritance.html.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:28: virtual ~PrerenderManager();
This should be private, since it's refcounted.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:54: protected:
Why are these virtuals protected?  They don't seem to be called by the test
subtype.  They should probably be private virtuals.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:31: }
indentation

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:116: bool
PrerenderResourceHandler::ShouldPrerender(
I'm not sure why this is a private static function rather than put in an
anonymous namespace here.  It doesn't seem to require any access to
PrerenderResourceHandler.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:18: 
I think we should start enforcing namespace chrome {
}  // namespace chrome

for new classes.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:33: static bool
CouldPrerender(const net::URLRequest* request);
My preference is to use const reference rather than pointer here, because it
implies that this will never be NULL.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler_unittest.cc:158: }
  // namespace

[cbentzel, 2010-12-19 00:40:56]

Thanks for the reviews so far.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager : public
base::RefCounted<PrerenderManager>,
On 2010/12/17 21:45:17, willchan wrote:
> Add a TODO(willchan): to remove the refcounting.  I of course see why you've
> done it.  But as noted in the multi-profile discussion emails, I plan to solve
> the reason you've had to make this refcounted.

Yes. I didn't want to make it refcounted.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager : public
base::RefCounted<PrerenderManager>,
On 2010/12/17 21:45:17, willchan wrote:
> Add a TODO(willchan): to remove the refcounting.  I of course see why you've
> done it.  But as noted in the multi-profile discussion emails, I plan to solve
> the reason you've had to make this refcounted.

I put the TODO in ChromeURLRequestContext, which seemed like a better place
[most of those objects should not be refcounted].

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:24: private NonThreadSafe {
On 2010/12/17 21:45:17, willchan wrote:
> Please avoid private inheritance:
> http://www.parashift.com/c++-faq-lite/private-inheritance.html.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

I can, but this seems like a perfect example of where private inheritance
actually makes sense. NonThreadSafe is implementation only, not interface.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:28: virtual ~PrerenderManager();
On 2010/12/17 21:45:17, willchan wrote:
> This should be private, since it's refcounted.

Made protected [TestPrerenderManager's virtual destructor also needs to call it,
not just override, so private is not possible].

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:54: protected:
On 2010/12/17 21:45:17, willchan wrote:
> Why are these virtuals protected?  They don't seem to be called by the test
> subtype.  They should probably be private virtuals.

Done.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:31: }
On 2010/12/17 21:45:17, willchan wrote:
> indentation

Done.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:101:
prerender_callback_->Run(url);
On 2010/12/17 20:03:51, cbentzel wrote:
> On 2010/12/17 19:58:21, darin wrote:
> > there seems to be the assumption here that GET requests are idempotent. 
> that's
> > not really true in all cases.
> > 
> > ideally, you would inspect the prefetched response to determine if it is
> > cachable, and then only kick off a prerender if you know that the document
is
> > going to be fetched from the cache.  or maybe you would set some flag to
> coerce
> > it to be fetched from the cache (and fail if it cannot be for some reason,
as
> we
> > do for form resubmissions).
> 
> I can add that to ShouldPrerender - this is called before we decide to
> prerender, but after the response headers have been handled [and after
> content-sniffing has been done if necessary].

Now, prerendering only happens if the top-level resource will not need to go
through cache revalidation while the prerendered page could still be used. We
currently will only use prerendered pages 20 seconds after they have been
instantiated.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.cc:116: bool
PrerenderResourceHandler::ShouldPrerender(
On 2010/12/17 21:45:17, willchan wrote:
> I'm not sure why this is a private static function rather than put in an
> anonymous namespace here.  It doesn't seem to require any access to
> PrerenderResourceHandler.

Gavin had the same concern. I changed it.

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

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:18: 
On 2010/12/17 21:45:17, willchan wrote:
> I think we should start enforcing namespace chrome {
> }  // namespace chrome
> 
> for new classes.

The chromium-dev email led to me believe this would actually be

namespace chrome {
namespace browser {

instead. 

I would prefer to do it in a separate CL for everything in
chrome/browser/prerender - this CL is already fairly big.

http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_resource_handler.h:33: static bool
CouldPrerender(const net::URLRequest* request);
On 2010/12/17 21:45:17, willchan wrote:
> My preference is to use const reference rather than pointer here, because it
> implies that this will never be NULL.

Done.

[cbentzel, 2010-12-21 15:54:47]

Ping. Any chance for re-review?

On Sat, Dec 18, 2010 at 7:40 PM, <cbentzel@chromium.org> wrote:

> Thanks for the reviews so far.
>
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_manager.h (right):
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager
> : public base::RefCounted<PrerenderManager>,
> On 2010/12/17 21:45:17, willchan wrote:
>
>> Add a TODO(willchan): to remove the refcounting.  I of course see why
>>
> you've
>
>> done it.  But as noted in the multi-profile discussion emails, I plan
>>
> to solve
>
>> the reason you've had to make this refcounted.
>>
>
> Yes. I didn't want to make it refcounted.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager
> : public base::RefCounted<PrerenderManager>,
> On 2010/12/17 21:45:17, willchan wrote:
>
>> Add a TODO(willchan): to remove the refcounting.  I of course see why
>>
> you've
>
>> done it.  But as noted in the multi-profile discussion emails, I plan
>>
> to solve
>
>> the reason you've had to make this refcounted.
>>
>
> I put the TODO in ChromeURLRequestContext, which seemed like a better
> place [most of those objects should not be refcounted].
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:24: private NonThreadSafe {
> On 2010/12/17 21:45:17, willchan wrote:
>
>> Please avoid private inheritance:
>> http://www.parashift.com/c++-faq-lite/private-inheritance.html.
>>
>
>
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
>
> I can, but this seems like a perfect example of where private
> inheritance actually makes sense. NonThreadSafe is implementation only,
> not interface.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:28: virtual
> ~PrerenderManager();
> On 2010/12/17 21:45:17, willchan wrote:
>
>> This should be private, since it's refcounted.
>>
>
> Made protected [TestPrerenderManager's virtual destructor also needs to
> call it, not just override, so private is not possible].
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_manager.h:54: protected:
> On 2010/12/17 21:45:17, willchan wrote:
>
>> Why are these virtuals protected?  They don't seem to be called by the
>>
> test
>
>> subtype.  They should probably be private virtuals.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_handler.cc (right):
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.cc:31: }
> On 2010/12/17 21:45:17, willchan wrote:
>
>> indentation
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.cc:101:
> prerender_callback_->Run(url);
> On 2010/12/17 20:03:51, cbentzel wrote:
>
>> On 2010/12/17 19:58:21, darin wrote:
>> > there seems to be the assumption here that GET requests are
>>
> idempotent.
>
>> that's
>> > not really true in all cases.
>> >
>> > ideally, you would inspect the prefetched response to determine if
>>
> it is
>
>> > cachable, and then only kick off a prerender if you know that the
>>
> document is
>
>> > going to be fetched from the cache.  or maybe you would set some
>>
> flag to
>
>> coerce
>> > it to be fetched from the cache (and fail if it cannot be for some
>>
> reason, as
>
>> we
>> > do for form resubmissions).
>>
>
>  I can add that to ShouldPrerender - this is called before we decide to
>> prerender, but after the response headers have been handled [and after
>> content-sniffing has been done if necessary].
>>
>
> Now, prerendering only happens if the top-level resource will not need
> to go through cache revalidation while the prerendered page could still
> be used. We currently will only use prerendered pages 20 seconds after
> they have been instantiated.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.cc:116: bool
> PrerenderResourceHandler::ShouldPrerender(
> On 2010/12/17 21:45:17, willchan wrote:
>
>> I'm not sure why this is a private static function rather than put in
>>
> an
>
>> anonymous namespace here.  It doesn't seem to require any access to
>> PrerenderResourceHandler.
>>
>
> Gavin had the same concern. I changed it.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_resource_handler.h (right):
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.h:18:
> On 2010/12/17 21:45:17, willchan wrote:
>
>> I think we should start enforcing namespace chrome {
>> }  // namespace chrome
>>
>
>  for new classes.
>>
>
> The chromium-dev email led to me believe this would actually be
>
> namespace chrome {
> namespace browser {
>
> instead.
>
> I would prefer to do it in a separate CL for everything in
> chrome/browser/prerender - this CL is already fairly big.
>
>
>
>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_resource_handler.h:33: static bool
> CouldPrerender(const net::URLRequest* request);
> On 2010/12/17 21:45:17, willchan wrote:
>
>> My preference is to use const reference rather than pointer here,
>>
> because it
>
>> implies that this will never be NULL.
>>
>
> Done.
>
>
> http://codereview.chromium.org/5912001/
>

[tburkard, 2010-12-22 20:28:48]

LGTM

On Tue, Dec 21, 2010 at 7:52 AM, Chris Bentzel <cbentzel@chromium.org>wrote:

> Ping. Any chance for re-review?
>
>
> On Sat, Dec 18, 2010 at 7:40 PM, <cbentzel@chromium.org> wrote:
>
>> Thanks for the reviews so far.
>>
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> File chrome/browser/prerender/prerender_manager.h (right):
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager
>> : public base::RefCounted<PrerenderManager>,
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> Add a TODO(willchan): to remove the refcounting.  I of course see why
>>>
>> you've
>>
>>> done it.  But as noted in the multi-profile discussion emails, I plan
>>>
>> to solve
>>
>>> the reason you've had to make this refcounted.
>>>
>>
>> Yes. I didn't want to make it refcounted.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_manager.h:23: class PrerenderManager
>> : public base::RefCounted<PrerenderManager>,
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> Add a TODO(willchan): to remove the refcounting.  I of course see why
>>>
>> you've
>>
>>> done it.  But as noted in the multi-profile discussion emails, I plan
>>>
>> to solve
>>
>>> the reason you've had to make this refcounted.
>>>
>>
>> I put the TODO in ChromeURLRequestContext, which seemed like a better
>> place [most of those objects should not be refcounted].
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_manager.h:24: private NonThreadSafe {
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> Please avoid private inheritance:
>>> http://www.parashift.com/c++-faq-lite/private-inheritance.html.
>>>
>>
>>
>>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...
>>
>> I can, but this seems like a perfect example of where private
>> inheritance actually makes sense. NonThreadSafe is implementation only,
>> not interface.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_manager.h:28: virtual
>> ~PrerenderManager();
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> This should be private, since it's refcounted.
>>>
>>
>> Made protected [TestPrerenderManager's virtual destructor also needs to
>> call it, not just override, so private is not possible].
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_manager.h:54: protected:
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> Why are these virtuals protected?  They don't seem to be called by the
>>>
>> test
>>
>>> subtype.  They should probably be private virtuals.
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> File chrome/browser/prerender/prerender_resource_handler.cc (right):
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_resource_handler.cc:31: }
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> indentation
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_resource_handler.cc:101:
>> prerender_callback_->Run(url);
>> On 2010/12/17 20:03:51, cbentzel wrote:
>>
>>> On 2010/12/17 19:58:21, darin wrote:
>>> > there seems to be the assumption here that GET requests are
>>>
>> idempotent.
>>
>>> that's
>>> > not really true in all cases.
>>> >
>>> > ideally, you would inspect the prefetched response to determine if
>>>
>> it is
>>
>>> > cachable, and then only kick off a prerender if you know that the
>>>
>> document is
>>
>>> > going to be fetched from the cache.  or maybe you would set some
>>>
>> flag to
>>
>>> coerce
>>> > it to be fetched from the cache (and fail if it cannot be for some
>>>
>> reason, as
>>
>>> we
>>> > do for form resubmissions).
>>>
>>
>>  I can add that to ShouldPrerender - this is called before we decide to
>>> prerender, but after the response headers have been handled [and after
>>> content-sniffing has been done if necessary].
>>>
>>
>> Now, prerendering only happens if the top-level resource will not need
>> to go through cache revalidation while the prerendered page could still
>> be used. We currently will only use prerendered pages 20 seconds after
>> they have been instantiated.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_resource_handler.cc:116: bool
>> PrerenderResourceHandler::ShouldPrerender(
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> I'm not sure why this is a private static function rather than put in
>>>
>> an
>>
>>> anonymous namespace here.  It doesn't seem to require any access to
>>> PrerenderResourceHandler.
>>>
>>
>> Gavin had the same concern. I changed it.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> File chrome/browser/prerender/prerender_resource_handler.h (right):
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_resource_handler.h:18:
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> I think we should start enforcing namespace chrome {
>>> }  // namespace chrome
>>>
>>
>>  for new classes.
>>>
>>
>> The chromium-dev email led to me believe this would actually be
>>
>> namespace chrome {
>> namespace browser {
>>
>> instead.
>>
>> I would prefer to do it in a separate CL for everything in
>> chrome/browser/prerender - this CL is already fairly big.
>>
>>
>>
>>
http://codereview.chromium.org/5912001/diff/62001/chrome/browser/prerender/pr...
>> chrome/browser/prerender/prerender_resource_handler.h:33: static bool
>> CouldPrerender(const net::URLRequest* request);
>> On 2010/12/17 21:45:17, willchan wrote:
>>
>>> My preference is to use const reference rather than pointer here,
>>>
>> because it
>>
>>> implies that this will never be NULL.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/5912001/
>>
>
>

[cbentzel, 2011-01-04 17:22:54]

darin/willchan: Any other concerns?

[cbentzel, 2011-01-04 18:58:52]

On 2011/01/04 17:22:54, cbentzel wrote:
> darin/willchan: Any other concerns?

Actually, hold off. I did a bad merge with the most recent patch.

[cbentzel, 2011-01-04 21:21:26]

On 2011/01/04 18:58:52, cbentzel wrote:
> On 2011/01/04 17:22:54, cbentzel wrote:
> > darin/willchan: Any other concerns?
> 
> Actually, hold off. I did a bad merge with the most recent patch.

Please review. Thanks.

[darin (slow to review), 2011-01-06 06:18:19]

http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):

http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_resource_handler_unittest.cc:71: void
AdvanceTime(base::TimeDelta dt) {
I don't see any callers of AdvanceTime or set_time.

Have you considered just making PrerenderResourceHandler::Timer be a function
pointer?  It seems like you don't really need it to be an interface because you
don't really need to anything more than global "fake timer" state.

[cbentzel, 2011-01-06 15:29:49]

http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):

http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_resource_handler_unittest.cc:71: void
AdvanceTime(base::TimeDelta dt) {
On 2011/01/06 06:18:20, darin wrote:
> I don't see any callers of AdvanceTime or set_time.
> 
> Have you considered just making PrerenderResourceHandler::Timer be a function
> pointer?  It seems like you don't really need it to be an interface because
you
> don't really need to anything more than global "fake timer" state.

Moved to a function pointer. I initially had a test which used AdvanceTime [but
not set_time], but I realized that this was testing an unrealistic scenario and
was unnecessary.

[cbentzel, 2011-01-07 23:04:24]

Are there any other concerns, or is this ok to land? 

On 2011/01/06 15:29:49, cbentzel wrote:
>
http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
> File chrome/browser/prerender/prerender_resource_handler_unittest.cc (right):
> 
>
http://codereview.chromium.org/5912001/diff/110001/chrome/browser/prerender/p...
> chrome/browser/prerender/prerender_resource_handler_unittest.cc:71: void
> AdvanceTime(base::TimeDelta dt) {
> On 2011/01/06 06:18:20, darin wrote:
> > I don't see any callers of AdvanceTime or set_time.
> > 
> > Have you considered just making PrerenderResourceHandler::Timer be a
function
> > pointer?  It seems like you don't really need it to be an interface because
> you
> > don't really need to anything more than global "fake timer" state.
> 
> Moved to a function pointer. I initially had a test which used AdvanceTime
[but
> not set_time], but I realized that this was testing an unrealistic scenario
and
> was unnecessary.

[cbentzel, 2011-01-11 21:17:17]

I talked to Darin over chat, and the resulting lack of LGTM was neutral.

I'll land this - the Mac trybot failures were due to the fork/testserver issue.