Remove a chrome dependency by removing Prerender from ResourceDispatcherHost.

Remove a chrome dependency by removing Prerender from ResourceDispatcherHost.

BUG=82590, 77090
TEST=Prerender*

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

[dominich, 2011-05-23 20:54:29]

John, can you look over the content/ and chrome/browser changes?
Matt, can you look over the PrerenderTracker changes?
Chris, can you look over everything?

Thanks!

[mmenke, 2011-05-23 21:58:41]

http://codereview.chromium.org/6966017/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.h:38: namespace prerender {
nit: space above this line.

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:191: child_id_, route_id_,
prerender_manager_);
nit:  Should use the same line breaking style in both OnPrerenderingStarted
calls.

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_tracker.cc:9: #include
"chrome/browser/ui/browser.h"
Is this needed?

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_tracker.h:59: bool
IsPrerenderingOnIOThread(int child_id, int route_id) const;
Please make this private and remove the TODO.

http://codereview.chromium.org/6966017/diff/1/chrome/test/testing_browser_pro...
File chrome/test/testing_browser_process.h (left):

http://codereview.chromium.org/6966017/diff/1/chrome/test/testing_browser_pro...
chrome/test/testing_browser_process.h:26: 
nit:  Leave these blank lines in.

[dominich, 2011-05-23 22:22:55]

http://codereview.chromium.org/6966017/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.h:38: namespace prerender {
On 2011/05/23 21:58:41, Matt Menke wrote:
> nit: space above this line.

Done.

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:191: child_id_, route_id_,
prerender_manager_);
On 2011/05/23 21:58:41, Matt Menke wrote:
> nit:  Should use the same line breaking style in both OnPrerenderingStarted
> calls.

Done.

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_tracker.cc:9: #include
"chrome/browser/ui/browser.h"
On 2011/05/23 21:58:41, Matt Menke wrote:
> Is this needed?

Done.

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_tracker.h:59: bool
IsPrerenderingOnIOThread(int child_id, int route_id) const;
On 2011/05/23 21:58:41, Matt Menke wrote:
> Please make this private and remove the TODO.

Done.

http://codereview.chromium.org/6966017/diff/1/chrome/test/testing_browser_pro...
File chrome/test/testing_browser_process.h (left):

http://codereview.chromium.org/6966017/diff/1/chrome/test/testing_browser_pro...
chrome/test/testing_browser_process.h:26: 
On 2011/05/23 21:58:41, Matt Menke wrote:
> nit:  Leave these blank lines in.

Done.

[mmenke, 2011-05-23 22:51:02]

Prerender changes LGTM.

I landed a PrerenderTracker cleanup earlier today, in response to Chris's
comment, which you'll need to sync to, at some point.

http://codereview.chromium.org/6966017/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:39: namespace prerender {
nit:  Not needed, since browser_process.h does this, and as it's the return
value of the virtual function, the forward declaration here is not independent
of the one in browser_process.h.

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:26: virtual ~PrerenderTracker();
Since nothing inherits from PrerenderTracker, so we need a virtual destructor? 
I don't have a strong opinion on this, and it may make sense to restructure the
unit test to inherit from it, anyways.

[jam, 2011-05-24 05:36:23]

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
prerender_tracker() = 0;
you only need to add a getter here if people need it from Browser*.  I don't see
that being used

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:739: prerender_tracker_.reset(new
prerender::PrerenderTracker);
we're going to use the one ResourceDispatcher::Observer to remove a lot of
depenendencies, not just the prerender ones.  so there should be a generically
named ResourceDispatcherHostObserver in the Chrome directory.

[cbentzel, 2011-05-24 11:37:19]

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:190:
g_browser_process->prerender_tracker()->OnPrerenderingStarted(
Would it make more sense to pass the PrerenderTracker* into the PrerenderManager
[and in turn the PrerenderContents object]

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:62: class Observer {
Should this be called Delegate instead of Observer? It seems like we use the
name Delegate when there is only one other thing which can control behavior, and
observer when there are N.

[cbentzel, 2011-05-24 14:54:08]

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:62: class Observer {
On 2011/05/24 11:37:19, cbentzel wrote:
> Should this be called Delegate instead of Observer? It seems like we use the
> name Delegate when there is only one other thing which can control behavior,
and
> observer when there are N. 

Ah, I saw the off-code-review discussion. I'm fine any way.

[dominich, 2011-05-24 15:10:28]

http://codereview.chromium.org/6966017/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:39: namespace prerender {
On 2011/05/23 22:51:02, Matt Menke wrote:
> nit:  Not needed, since browser_process.h does this, and as it's the return
> value of the virtual function, the forward declaration here is not independent
> of the one in browser_process.h.

Done.

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
prerender_tracker() = 0;
On 2011/05/24 05:36:23, John Abd-El-Malek wrote:
> you only need to add a getter here if people need it from Browser*.  I don't
see
> that being used

PrerenderContents accesses through BrowserProcess* and thus needs this to be
declared.

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:739: prerender_tracker_.reset(new
prerender::PrerenderTracker);
On 2011/05/24 05:36:23, John Abd-El-Malek wrote:
> we're going to use the one ResourceDispatcher::Observer to remove a lot of
> depenendencies, not just the prerender ones.  so there should be a generically
> named ResourceDispatcherHostObserver in the Chrome directory.

Done.

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:190:
g_browser_process->prerender_tracker()->OnPrerenderingStarted(
On 2011/05/24 11:37:19, cbentzel wrote:
> Would it make more sense to pass the PrerenderTracker* into the
PrerenderManager
> [and in turn the PrerenderContents object]

I prefer to be explicit about where this is coming from rather than muddy the
waters of ownership. The lookup here is very low cost compared to the other
processing we're doing and it makes it obvious that we're calling out to
something global.

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:26: virtual ~PrerenderTracker();
On 2011/05/23 22:51:03, Matt Menke wrote:
> Since nothing inherits from PrerenderTracker, so we need a virtual destructor?

> I don't have a strong opinion on this, and it may make sense to restructure
the
> unit test to inherit from it, anyways.

Done.

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:62: class Observer {
On 2011/05/24 11:37:19, cbentzel wrote:
> Should this be called Delegate instead of Observer? It seems like we use the
> name Delegate when there is only one other thing which can control behavior,
and
> observer when there are N. 

Named after discussion with jam and brettw and following the model set by
resource_dispatcher. If others agree then I'm happy to change it to delegate.

[cbentzel, 2011-05-24 15:13:25]

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:190:
g_browser_process->prerender_tracker()->OnPrerenderingStarted(
On 2011/05/24 15:10:28, dominic wrote:
> On 2011/05/24 11:37:19, cbentzel wrote:
> > Would it make more sense to pass the PrerenderTracker* into the
> PrerenderManager
> > [and in turn the PrerenderContents object]
> 
> I prefer to be explicit about where this is coming from rather than muddy the
> waters of ownership. The lookup here is very low cost compared to the other
> processing we're doing and it makes it obvious that we're calling out to
> something global.

I wasn't worried about performance, just dislike reaching out to the global
instead of passing in the dependency.

[jam, 2011-05-24 16:06:24]

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_contents.cc:191: child_id_, route_id_,
prerender_manager_);
On 2011/05/23 22:22:55, dominic wrote:
> On 2011/05/23 21:58:41, Matt Menke wrote:
> > nit:  Should use the same line breaking style in both OnPrerenderingStarted
> > calls.
> 
> Done.

nit: this is totally personal style, but for the record I prefer that when a
function has arguments that can all fit on one line, we do so.  the 80 line
limit makes the code very vertically heavy, and so if we can reduce some lines,
it makes it easier to read code without as much scrolling

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:190:
g_browser_process->prerender_tracker()->OnPrerenderingStarted(
On 2011/05/24 15:13:25, cbentzel wrote:
> On 2011/05/24 15:10:28, dominic wrote:
> > On 2011/05/24 11:37:19, cbentzel wrote:
> > > Would it make more sense to pass the PrerenderTracker* into the
> > PrerenderManager
> > > [and in turn the PrerenderContents object]
> > 
> > I prefer to be explicit about where this is coming from rather than muddy
the
> > waters of ownership. The lookup here is very low cost compared to the other
> > processing we're doing and it makes it obvious that we're calling out to
> > something global.
> 
> I wasn't worried about performance, just dislike reaching out to the global
> instead of passing in the dependency.

I don't think this makes ownership murky.  It's a very common pattern that we
give objects on creation the other objects that they might need.  Otherwise
BrowserProcess will explode with a list of getters.

http://codereview.chromium.org/6966017/diff/5002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host_observer.h (right):

http://codereview.chromium.org/6966017/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host_observer.h:13: class
ResourceDispatcherHostObserver : public ResourceDispatcherHost::Observer {
sorry I realize I suggested ResourceDispatcherHostObserver, but now looking at
it it's a little confusing having both ResourceDispatcherHostObserver and
ResourceDispatcherHost::Observer.  How about
ChromeResourceDispatcherHostObserver?  We have been using the Chrome prefix for
classes that the embedder provides to the content layer.

[dominich, 2011-05-24 16:54:21]

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/6001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:190:
g_browser_process->prerender_tracker()->OnPrerenderingStarted(
On 2011/05/24 11:37:19, cbentzel wrote:
> Would it make more sense to pass the PrerenderTracker* into the
PrerenderManager
> [and in turn the PrerenderContents object]

Done.

http://codereview.chromium.org/6966017/diff/5002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host_observer.h (right):

http://codereview.chromium.org/6966017/diff/5002/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host_observer.h:13: class
ResourceDispatcherHostObserver : public ResourceDispatcherHost::Observer {
On 2011/05/24 16:06:24, John Abd-El-Malek wrote:
> sorry I realize I suggested ResourceDispatcherHostObserver, but now looking at
> it it's a little confusing having both ResourceDispatcherHostObserver and
> ResourceDispatcherHost::Observer.  How about
> ChromeResourceDispatcherHostObserver?  We have been using the Chrome prefix
for
> classes that the embedder provides to the content layer.

Done.

[jam, 2011-05-24 17:09:21]

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
prerender_tracker() = 0;
why is this still needed?

[cbentzel, 2011-05-24 17:13:54]

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
prerender_tracker() = 0;
Do you need this anymore?

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:282:
scoped_ptr<prerender::PrerenderTracker> prerender_tracker_;
You may want to comment that prerender_tracker_ needs to be ordered before
resource_dispatcher_host_observer_ due to destruction ordering.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.h:66: virtual PrerenderContents*
CreatePrerenderContents(
The lack-of-ownership and lifetime-expectations of prerender_manager, profile,
and prerender_tracker should probably be documented here.

Also, very minor nit, but perhaps prerender_tracker should be right after
prerender_manager?

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:77: explicit
RenderViewInfo(PrerenderManager* prerender_manager);
Why did this need to move out of the .cc file?

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:9:
#include
"chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h"
I think this is supposed to come before the other includes.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:33: if
(request_data.resource_type == ResourceType::PREFETCH) {
Depending on when Gavin's prerender change goes in, this will want to pick up
those changes.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:60:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
This should be using prerender_tracker->TryCancelOnIOThread()

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h
(right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:15:
class ChromeResourceDispatcherHostObserver :
Maybe a short description of this class in a comment?

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:18:
ChromeResourceDispatcherHostObserver(
explicit

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:19:
prerender::PrerenderTracker* prerender_tracker);
Document lifetime of prerender_tracker relative to this class.

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:64: virtual bool
ShouldBeginRequest(int child_id, int route_id,
Please document these methods.

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:261: void
set_observer(Observer* observer) { observer_ = observer; }
Please document lifetime expectations.

Also, would it make sense to pass into the constructor and always require it to
be non-NULL?

[dominich, 2011-05-24 17:18:06]

On 2011/05/24 17:09:21, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
> File chrome/browser/browser_process.h (right):
> 
>
http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
> chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
> prerender_tracker() = 0;
> why is this still needed?

The PrerenderTracker calls into the BrowserProcess to get the PrerenderTracker
instance in the static functions used as tasks for adding/removing prerenders.

[dominich, 2011-05-24 17:18:43]

On 2011/05/24 17:18:06, dominic wrote:
> On 2011/05/24 17:09:21, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
> > File chrome/browser/browser_process.h (right):
> > 
> >
>
http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
> > chrome/browser/browser_process.h:228: virtual prerender::PrerenderTracker*
> > prerender_tracker() = 0;
> > why is this still needed?
> 
> The PrerenderTracker calls into the BrowserProcess to get the PrerenderTracker
> instance in the static functions used as tasks for adding/removing prerenders.

Sorry, it's also used in the Profile when instantiating the PrerenderManager.

[mmenke, 2011-05-24 17:56:22]

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:37: class
ChromeResourceDispatcherHostObserver;
nit:  This should be in alphabetical order.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:394:
prerender_tracker_->OnPrerenderingFinished(
nit:  This now fits on a single line.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:6:
#include "chrome/browser/browser_process.h"
Is this needed?

[dominich, 2011-05-24 18:01:47]

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:37: class
ChromeResourceDispatcherHostObserver;
On 2011/05/24 17:56:22, Matt Menke wrote:
> nit:  This should be in alphabetical order.

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.h:282:
scoped_ptr<prerender::PrerenderTracker> prerender_tracker_;
On 2011/05/24 17:13:54, cbentzel wrote:
> You may want to comment that prerender_tracker_ needs to be ordered before
> resource_dispatcher_host_observer_ due to destruction ordering.

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:394:
prerender_tracker_->OnPrerenderingFinished(
On 2011/05/24 17:56:22, Matt Menke wrote:
> nit:  This now fits on a single line.

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.h:66: virtual PrerenderContents*
CreatePrerenderContents(
On 2011/05/24 17:13:54, cbentzel wrote:
> The lack-of-ownership and lifetime-expectations of prerender_manager, profile,
> and prerender_tracker should probably be documented here.
> 
> Also, very minor nit, but perhaps prerender_tracker should be right after
> prerender_manager?

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:77: explicit
RenderViewInfo(PrerenderManager* prerender_manager);
On 2011/05/24 17:13:54, cbentzel wrote:
> Why did this need to move out of the .cc file?

When PrerenderTracker was implementing Observer there was an issue with the map
instantiation. I'll put it back.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:6:
#include "chrome/browser/browser_process.h"
On 2011/05/24 17:56:22, Matt Menke wrote:
> Is this needed?

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:9:
#include
"chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h"
On 2011/05/24 17:13:54, cbentzel wrote:
> I think this is supposed to come before the other includes.

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:60:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2011/05/24 17:13:54, cbentzel wrote:
> This should be using prerender_tracker->TryCancelOnIOThread()

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h
(right):

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:15:
class ChromeResourceDispatcherHostObserver :
On 2011/05/24 17:13:54, cbentzel wrote:
> Maybe a short description of this class in a comment?

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:18:
ChromeResourceDispatcherHostObserver(
On 2011/05/24 17:13:54, cbentzel wrote:
> explicit

Done.

http://codereview.chromium.org/6966017/diff/2016/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:19:
prerender::PrerenderTracker* prerender_tracker);
On 2011/05/24 17:13:54, cbentzel wrote:
> Document lifetime of prerender_tracker relative to this class.

Done.

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:64: virtual bool
ShouldBeginRequest(int child_id, int route_id,
On 2011/05/24 17:13:54, cbentzel wrote:
> Please document these methods.

Done.

http://codereview.chromium.org/6966017/diff/2016/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:261: void
set_observer(Observer* observer) { observer_ = observer; }
On 2011/05/24 17:13:54, cbentzel wrote:
> Please document lifetime expectations.

Done.

> 
> Also, would it make sense to pass into the constructor and always require it
to
> be non-NULL? 

That would reduce some of the checks in the implementation, but this is how
other observers have been set up already and I'd rather be consistent.

[mmenke, 2011-05-24 18:38:08]

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

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:73: Profile* profile,
This argument order is kind of funky.  I'd go with manager, tracker, profile,
just like PrerenderContents and CreatePrerenderContents.

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h
(right):

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:17:
class ChromeResourceDispatcherHostObserver :
nit:  I believe the colon should go on the next line.

[jam, 2011-05-24 19:21:28]

lgtm

[jam, 2011-05-24 19:22:13]

also please remove the dependency from content\browser\DEPS and add the bug id
(77090) to this change

[dominich, 2011-05-24 19:45:41]

Rebased to gavinp's change that required significant merging.

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

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:73: Profile* profile,
On 2011/05/24 18:38:09, Matt Menke wrote:
> This argument order is kind of funky.  I'd go with manager, tracker, profile,
> just like PrerenderContents and CreatePrerenderContents.

Done.

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h
(right):

http://codereview.chromium.org/6966017/diff/5026/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.h:17:
class ChromeResourceDispatcherHostObserver :
On 2011/05/24 18:38:09, Matt Menke wrote:
> nit:  I believe the colon should go on the next line.

Done.

[mmenke, 2011-05-24 19:50:20]

http://codereview.chromium.org/6966017/diff/6020/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/6020/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:56:
return false;
You should be doing this one line down.  If prerendering isn't possible, we want
to cancel prerendering requests, too.

[dominich, 2011-05-24 20:20:34]

http://codereview.chromium.org/6966017/diff/6020/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/6020/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:56:
return false;
On 2011/05/24 19:50:20, Matt Menke wrote:
> You should be doing this one line down.  If prerendering isn't possible, we
want
> to cancel prerendering requests, too.

Good catch, thank you!

Done.

[mmenke, 2011-05-25 00:56:23]

LGTM

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.cc:8: #include
"chrome/browser/prerender/prerender_tracker.h"
nit:  This should go first.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:12: #include
"base/memory/weak_ptr.h"
nit:  No longer needed, now that RenderViewInfo is no longer defined here.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:77: //  struct RenderViewInfo {
nit:  Remove this.  Assume you would have anyways, but easy to forget about such
things.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:74: int
route_id,
nit:  Fix indentation.

http://codereview.chromium.org/6966017/diff/4020/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/4020/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:65: virtual bool
ShouldBeginRequest(int child_id, int route_id,
nit:  Move first two parameters to next line.

[cbentzel, 2011-05-25 04:24:37]

LGTM

[commit-bot: I haz the power, 2011-05-25 16:11:50]

Can't apply patch for file content/browser/DEPS.
patching file content/browser/DEPS
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej

[mmenke, 2011-05-25 16:13:58]

Err...You didn't update in response to my last set of comments.

On Wed, May 25, 2011 at 12:11 PM, <commit-bot@chromium.org> wrote:

> Can't apply patch for file content/browser/DEPS.
> patching file content/browser/DEPS
> Hunk #1 FAILED at 53.
> 1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej
>
>
>
> http://codereview.chromium.org/6966017/
>

[commit-bot: I haz the power, 2011-05-25 16:14:04]

Can't apply patch for file content/browser/DEPS.
patching file content/browser/DEPS
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej

[dominich.google, 2011-05-25 16:17:34]

I didn't see that. I'll do that as I rebase again.

On Wed, May 25, 2011 at 9:13 AM, Matt Menke <mmenke@chromium.org> wrote:

> Err...You didn't update in response to my last set of comments.
>
>
> On Wed, May 25, 2011 at 12:11 PM, <commit-bot@chromium.org> wrote:
>
>> Can't apply patch for file content/browser/DEPS.
>> patching file content/browser/DEPS
>> Hunk #1 FAILED at 53.
>> 1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej
>>
>>
>>
>> http://codereview.chromium.org/6966017/
>>
>
>

[dominich, 2011-05-25 16:42:28]

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.cc (right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.cc:8: #include
"chrome/browser/prerender/prerender_tracker.h"
On 2011/05/25 00:56:24, Matt Menke wrote:
> nit:  This should go first.

Done.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_tracker.h (right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:12: #include
"base/memory/weak_ptr.h"
On 2011/05/25 00:56:24, Matt Menke wrote:
> nit:  No longer needed, now that RenderViewInfo is no longer defined here.

Done.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_tracker.h:77: //  struct RenderViewInfo {
On 2011/05/25 00:56:24, Matt Menke wrote:
> nit:  Remove this.  Assume you would have anyways, but easy to forget about
such
> things.

Done.

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc
(right):

http://codereview.chromium.org/6966017/diff/4020/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_observer.cc:74: int
route_id,
On 2011/05/25 00:56:24, Matt Menke wrote:
> nit:  Fix indentation.

Done.

http://codereview.chromium.org/6966017/diff/4020/content/browser/renderer_hos...
File content/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/6966017/diff/4020/content/browser/renderer_hos...
content/browser/renderer_host/resource_dispatcher_host.h:65: virtual bool
ShouldBeginRequest(int child_id, int route_id,
On 2011/05/25 00:56:24, Matt Menke wrote:
> nit:  Move first two parameters to next line.

Done.

[commit-bot: I haz the power, 2011-05-25 18:36:26]

Can't apply patch for file content/browser/DEPS.
patching file content/browser/DEPS
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej

[commit-bot: I haz the power, 2011-05-25 18:44:53]

Can't apply patch for file content/browser/DEPS.
patching file content/browser/DEPS
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file content/browser/DEPS.rej