Fix memory leak in PrerenderDispatcher.

Fix memory leak in PrerenderDispatcher.

Make the PrerenderingInterface into a scoped pointer.

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

[gavinp, 2012-05-15 08:34:03]

I wonder if we should rename the PrerenderDispatcher, or add a second observer?

[dominich, 2012-05-15 16:06:25]

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:17:
WebKit::WebPrerenderingSupport::initialize(prerendering_support_.get());
this seems like an arbitrary place to put this. For 1, the Dispatcher isn't
dispatching anything. And 2, it has nothing to do with
WebKit::WebPrerenderingSupport. I wonder if this should be initialized/destroyed
directly in the chrome_content_render_client alongside the PrerenderDispatcher.

I wonder if we should somehow make the parallels between the Dispatcher and
Tracker more obvious. PrerenderBrowserTracker and PrerenderRenderTracker maybe?
or PrerenderRenderViewTracker and PrerenderUrlTracker?

[gavinp, 2012-05-18 05:46:25]

+jam, to review changes to chrome_content_renderer_client.

Dominic,

You're right; the PrerenderDispatcher was always an arbitrary place to put this.
 This patch moves the code into the ChromeContentRendererClient.

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:17:
WebKit::WebPrerenderingSupport::initialize(prerendering_support_.get());
On 2012/05/15 16:06:25, dominich wrote:
> this seems like an arbitrary place to put this. For 1, the Dispatcher isn't
> dispatching anything. And 2, it has nothing to do with
> WebKit::WebPrerenderingSupport. I wonder if this should be
initialized/destroyed
> directly in the chrome_content_render_client alongside the
PrerenderDispatcher.

OK, the new patch does it this way.

> I wonder if we should somehow make the parallels between the Dispatcher and
> Tracker more obvious. PrerenderBrowserTracker and PrerenderRenderTracker
maybe?
> or PrerenderRenderViewTracker and PrerenderUrlTracker?

Sounds good.  Do you want to create a bug for this, or do you think I should
pull that into this CL?

[dominich, 2012-05-18 14:48:09]

I'll write up a bug about Dispatcher vs Tracker with some suggestions.

This CL is really close.

http://codereview.chromium.org/10391134/diff/4001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/10391134/diff/4001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:180:
prerendering_support_.reset(new prerender::PrerenderingSupport());
Could you move these two lines up to 166? It would keep all the .reset calls
together.

http://codereview.chromium.org/10391134/diff/4001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_dispatcher.h (right):

http://codereview.chromium.org/10391134/diff/4001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_dispatcher.h:22: // such as the
PrerenderingSupport.
comment is no longer accurate.

[jam, 2012-05-18 16:38:15]

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
chrome/renderer/prerender/prerender_dispatcher.cc:17:
WebKit::WebPrerenderingSupport::initialize(prerendering_support_.get());
On 2012/05/18 05:46:25, gavinp wrote:
> On 2012/05/15 16:06:25, dominich wrote:
> > this seems like an arbitrary place to put this. For 1, the Dispatcher isn't
> > dispatching anything. And 2, it has nothing to do with
> > WebKit::WebPrerenderingSupport. I wonder if this should be
> initialized/destroyed
> > directly in the chrome_content_render_client alongside the
> PrerenderDispatcher.

it seems that CCRC has less to do with prerendering than this file..
> 
> OK, the new patch does it this way.
> 
> > I wonder if we should somehow make the parallels between the Dispatcher and
> > Tracker more obvious. PrerenderBrowserTracker and PrerenderRenderTracker
> maybe?
> > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> 
> Sounds good.  Do you want to create a bug for this, or do you think I should
> pull that into this CL?
>

[dominich, 2012-05-18 17:01:09]

On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
> File chrome/renderer/prerender/prerender_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
> chrome/renderer/prerender/prerender_dispatcher.cc:17:
> WebKit::WebPrerenderingSupport::initialize(prerendering_support_.get());
> On 2012/05/18 05:46:25, gavinp wrote:
> > On 2012/05/15 16:06:25, dominich wrote:
> > > this seems like an arbitrary place to put this. For 1, the Dispatcher
isn't
> > > dispatching anything. And 2, it has nothing to do with
> > > WebKit::WebPrerenderingSupport. I wonder if this should be
> > initialized/destroyed
> > > directly in the chrome_content_render_client alongside the
> > PrerenderDispatcher.
> 
> it seems that CCRC has less to do with prerendering than this file..

My understanding is that CCRC is the point at which any chrome-specific
per-render process objects should be created. These classes represent
independent aspects of the Prerender system and both need to be created per
render process.

The alternative is to create a class that, when instantiated, instantiates a
PrerenderDispatcher and a PrerenderSupport. That extra level of indirection
seems redundant.

I'm all for minimizing the surface area of an API, but I also want to  ensure
that each class is responsible for a single aspect of the prerendering system.

> > 
> > OK, the new patch does it this way.
> > 
> > > I wonder if we should somehow make the parallels between the Dispatcher
and
> > > Tracker more obvious. PrerenderBrowserTracker and PrerenderRenderTracker
> > maybe?
> > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > 
> > Sounds good.  Do you want to create a bug for this, or do you think I should
> > pull that into this CL?
> >

[jam, 2012-05-18 23:12:04]

On 2012/05/18 17:01:09, dominich wrote:
> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
> > File chrome/renderer/prerender/prerender_dispatcher.cc (right):
> > 
> >
>
http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prer...
> > chrome/renderer/prerender/prerender_dispatcher.cc:17:
> > WebKit::WebPrerenderingSupport::initialize(prerendering_support_.get());
> > On 2012/05/18 05:46:25, gavinp wrote:
> > > On 2012/05/15 16:06:25, dominich wrote:
> > > > this seems like an arbitrary place to put this. For 1, the Dispatcher
> isn't
> > > > dispatching anything. And 2, it has nothing to do with
> > > > WebKit::WebPrerenderingSupport. I wonder if this should be
> > > initialized/destroyed
> > > > directly in the chrome_content_render_client alongside the
> > > PrerenderDispatcher.
> > 
> > it seems that CCRC has less to do with prerendering than this file..
> 
> My understanding is that CCRC is the point at which any chrome-specific
> per-render process objects should be created. These classes represent
> independent aspects of the Prerender system and both need to be created per
> render process.
> 
> The alternative is to create a class that, when instantiated, instantiates a
> PrerenderDispatcher and a PrerenderSupport. That extra level of indirection
> seems redundant.

Agreed that adding another class is needless.

> 
> I'm all for minimizing the surface area of an API, but I also want to  ensure
> that each class is responsible for a single aspect of the prerendering system.

I don't really understand why you say that every class has to be responsible for
a single aspect. It seems that patchset 1 here is not convoluted and easy to
follow. This is a common pattern that we do with other features in chrome.

so from my pov, ps1 lgtm
> 
> > > 
> > > OK, the new patch does it this way.
> > > 
> > > > I wonder if we should somehow make the parallels between the Dispatcher
> and
> > > > Tracker more obvious. PrerenderBrowserTracker and PrerenderRenderTracker
> > > maybe?
> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > > 
> > > Sounds good.  Do you want to create a bug for this, or do you think I
should
> > > pull that into this CL?
> > >

[jam, 2012-05-18 23:12:25]

just to be clear, my lgtm is for patchset 1 and not patchset 2

[dominich, 2012-05-18 23:31:12]

On Fri, May 18, 2012 at 4:12 PM, <jam@chromium.org> wrote:

> On 2012/05/18 17:01:09, dominich wrote:
>
>> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
>> >
>>
>
> http://codereview.chromium.**org/10391134/diff/1/chrome/**
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
>
>> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/10391134/diff/1/chrome/**
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
>
>> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
>> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
>> support_.get());
>> > On 2012/05/18 05:46:25, gavinp wrote:
>> > > On 2012/05/15 16:06:25, dominich wrote:
>> > > > this seems like an arbitrary place to put this. For 1, the
>> Dispatcher
>> isn't
>> > > > dispatching anything. And 2, it has nothing to do with
>> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
>> > > initialized/destroyed
>> > > > directly in the chrome_content_render_client alongside the
>> > > PrerenderDispatcher.
>> >
>> > it seems that CCRC has less to do with prerendering than this file..
>>
>
>  My understanding is that CCRC is the point at which any chrome-specific
>> per-render process objects should be created. These classes represent
>> independent aspects of the Prerender system and both need to be created
>> per
>> render process.
>>
>
>  The alternative is to create a class that, when instantiated,
>> instantiates a
>> PrerenderDispatcher and a PrerenderSupport. That extra level of
>> indirection
>> seems redundant.
>>
>
> Agreed that adding another class is needless.
>
>
>
>  I'm all for minimizing the surface area of an API, but I also want to
>>  ensure
>> that each class is responsible for a single aspect of the prerendering
>> system.
>>
>
> I don't really understand why you say that every class has to be
> responsible for
> a single aspect. It seems that patchset 1 here is not convoluted and easy
> to
> follow. This is a common pattern that we do with other features in chrome.
>

Initializing the WebPrerenderingSupport in the PrerenderDispatcher
constructor was poor code design as that class has nothing to do with the
role the PrerenderDispatcher has. The PrerenderDispatcher should be
responsible for tracking which URLs are being prerendered, and that's all.

The classes happen to have similar lifetimes but that's not a good reason
to convolve the two. If we're doing something similar across Chrome, I'd
argue that we should look at fixing them too.


>
> so from my pov, ps1 lgtm
>
>
>  > >
>> > > OK, the new patch does it this way.
>> > >
>> > > > I wonder if we should somehow make the parallels between the
>> Dispatcher
>> and
>> > > > Tracker more obvious. PrerenderBrowserTracker and
>> PrerenderRenderTracker
>> > > maybe?
>> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
>> > >
>> > > Sounds good.  Do you want to create a bug for this, or do you think I
>>
> should
>
>> > > pull that into this CL?
>> > >
>>
>
>
>
>
http://codereview.chromium.**org/10391134/<http://codereview.chromium.org/103...
>

[jam, 2012-05-21 15:56:12]

On 2012/05/18 23:31:12, dominich wrote:
> On Fri, May 18, 2012 at 4:12 PM, <mailto:jam@chromium.org> wrote:
> 
> > On 2012/05/18 17:01:09, dominich wrote:
> >
> >> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> >> >
> >>
> >
> > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> >
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
> >
> >> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> >
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
> >
> >> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
> >> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
> >> support_.get());
> >> > On 2012/05/18 05:46:25, gavinp wrote:
> >> > > On 2012/05/15 16:06:25, dominich wrote:
> >> > > > this seems like an arbitrary place to put this. For 1, the
> >> Dispatcher
> >> isn't
> >> > > > dispatching anything. And 2, it has nothing to do with
> >> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
> >> > > initialized/destroyed
> >> > > > directly in the chrome_content_render_client alongside the
> >> > > PrerenderDispatcher.
> >> >
> >> > it seems that CCRC has less to do with prerendering than this file..
> >>
> >
> >  My understanding is that CCRC is the point at which any chrome-specific
> >> per-render process objects should be created. These classes represent
> >> independent aspects of the Prerender system and both need to be created
> >> per
> >> render process.
> >>
> >
> >  The alternative is to create a class that, when instantiated,
> >> instantiates a
> >> PrerenderDispatcher and a PrerenderSupport. That extra level of
> >> indirection
> >> seems redundant.
> >>
> >
> > Agreed that adding another class is needless.
> >
> >
> >
> >  I'm all for minimizing the surface area of an API, but I also want to
> >>  ensure
> >> that each class is responsible for a single aspect of the prerendering
> >> system.
> >>
> >
> > I don't really understand why you say that every class has to be
> > responsible for
> > a single aspect. It seems that patchset 1 here is not convoluted and easy
> > to
> > follow. This is a common pattern that we do with other features in chrome.
> >
> 
> Initializing the WebPrerenderingSupport in the PrerenderDispatcher
> constructor was poor code design as that class has nothing to do with the
> role the PrerenderDispatcher has.

> The PrerenderDispatcher should be
> responsible for tracking which URLs are being prerendered, and that's all.

I'm not sure I agree with this, see below.

> 
> The classes happen to have similar lifetimes but that's not a good reason
> to convolve the two. If we're doing something similar across Chrome, I'd
> argue that we should look at fixing them too.

During the content refactoring, we moved feature-code from
RenderThread/RenderView to self-contained classes. Some big features that used
to have render-process-wide code in RenderThread moved to FooDispatcher. I
believe that the prerendering code, when creating PrerenderDispatcher, used
ExtensionDispatcher as the inspiration. "Dispatcher" in this sense meant both
dispatching IPCs for that process, and also process-wide state. In that sense,
it seems reasonable that this class would be what sets up the prerendering
support.


> 
> 
> >
> > so from my pov, ps1 lgtm
> >
> >
> >  > >
> >> > > OK, the new patch does it this way.
> >> > >
> >> > > > I wonder if we should somehow make the parallels between the
> >> Dispatcher
> >> and
> >> > > > Tracker more obvious. PrerenderBrowserTracker and
> >> PrerenderRenderTracker
> >> > > maybe?
> >> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> >> > >
> >> > > Sounds good.  Do you want to create a bug for this, or do you think I
> >>
> > should
> >
> >> > > pull that into this CL?
> >> > >
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/10391134/%3Chttp://codereview.chromium.org/1...>
> >

[dominich, 2012-05-21 16:27:05]

On 2012/05/21 15:56:12, John Abd-El-Malek wrote:
> On 2012/05/18 23:31:12, dominich wrote:
> > On Fri, May 18, 2012 at 4:12 PM, <mailto:jam@chromium.org> wrote:
> > 
> > > On 2012/05/18 17:01:09, dominich wrote:
> > >
> > >> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
> > >
> > >> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
> > >> >
> > >> >
> > >>
> > >
> > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
> > >
> > >> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
> > >> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
> > >> support_.get());
> > >> > On 2012/05/18 05:46:25, gavinp wrote:
> > >> > > On 2012/05/15 16:06:25, dominich wrote:
> > >> > > > this seems like an arbitrary place to put this. For 1, the
> > >> Dispatcher
> > >> isn't
> > >> > > > dispatching anything. And 2, it has nothing to do with
> > >> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
> > >> > > initialized/destroyed
> > >> > > > directly in the chrome_content_render_client alongside the
> > >> > > PrerenderDispatcher.
> > >> >
> > >> > it seems that CCRC has less to do with prerendering than this file..
> > >>
> > >
> > >  My understanding is that CCRC is the point at which any chrome-specific
> > >> per-render process objects should be created. These classes represent
> > >> independent aspects of the Prerender system and both need to be created
> > >> per
> > >> render process.
> > >>
> > >
> > >  The alternative is to create a class that, when instantiated,
> > >> instantiates a
> > >> PrerenderDispatcher and a PrerenderSupport. That extra level of
> > >> indirection
> > >> seems redundant.
> > >>
> > >
> > > Agreed that adding another class is needless.
> > >
> > >
> > >
> > >  I'm all for minimizing the surface area of an API, but I also want to
> > >>  ensure
> > >> that each class is responsible for a single aspect of the prerendering
> > >> system.
> > >>
> > >
> > > I don't really understand why you say that every class has to be
> > > responsible for
> > > a single aspect. It seems that patchset 1 here is not convoluted and easy
> > > to
> > > follow. This is a common pattern that we do with other features in chrome.
> > >
> > 
> > Initializing the WebPrerenderingSupport in the PrerenderDispatcher
> > constructor was poor code design as that class has nothing to do with the
> > role the PrerenderDispatcher has.
> 
> > The PrerenderDispatcher should be
> > responsible for tracking which URLs are being prerendered, and that's all.
> 
> I'm not sure I agree with this, see below.
> 
> > 
> > The classes happen to have similar lifetimes but that's not a good reason
> > to convolve the two. If we're doing something similar across Chrome, I'd
> > argue that we should look at fixing them too.
> 
> During the content refactoring, we moved feature-code from
> RenderThread/RenderView to self-contained classes. Some big features that used
> to have render-process-wide code in RenderThread moved to FooDispatcher. I
> believe that the prerendering code, when creating PrerenderDispatcher, used
> ExtensionDispatcher as the inspiration. "Dispatcher" in this sense meant both
> dispatching IPCs for that process, and also process-wide state. In that sense,
> it seems reasonable that this class would be what sets up the prerendering
> support.

Ah, I see. So this should really be PrerenderPerRenderViewStuff or something.

It makes it clearer why we have different ideas about what this class should be
doing, but I still think that this is the wrong approach. In your eyes, this
abstracts Prerender-specific per-RenderView functionality away from CCRC. In my
eyes, it does the same but also lumps everything Prerender-specific together
into one class, which is poor design. The only way to keep all Prerender stuff
in one place outside of CCRC _and_ retain the design philosophy that each
Prerender class should do only one thing is to create a
PrerenderRenderViewSupport class that can be owned by CCRC, and that can own
individual instances of PrerenderDispatcher and PrerenderSupport. The extra
indirection is unfortunate, but it keeps both ends of the design cleaner.

As an aside, I put together a CL that renames PrerenderDispatcher to
PrerenderRenderViewTracker (http://codereview.chromium.org/10386223/) which I
think makes the class role clearer.

> 
> 
> > 
> > 
> > >
> > > so from my pov, ps1 lgtm
> > >
> > >
> > >  > >
> > >> > > OK, the new patch does it this way.
> > >> > >
> > >> > > > I wonder if we should somehow make the parallels between the
> > >> Dispatcher
> > >> and
> > >> > > > Tracker more obvious. PrerenderBrowserTracker and
> > >> PrerenderRenderTracker
> > >> > > maybe?
> > >> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > >> > >
> > >> > > Sounds good.  Do you want to create a bug for this, or do you think I
> > >>
> > > should
> > >
> > >> > > pull that into this CL?
> > >> > >
> > >>
> > >
> > >
> > >
> > >
> >
>
http://codereview.chromium.**org/10391134/%253Chttp://codereview.chromium.org...>
> > >

[jam, 2012-05-21 17:37:22]

On 2012/05/21 16:27:05, dominich wrote:
> On 2012/05/21 15:56:12, John Abd-El-Malek wrote:
> > On 2012/05/18 23:31:12, dominich wrote:
> > > On Fri, May 18, 2012 at 4:12 PM, <mailto:jam@chromium.org> wrote:
> > > 
> > > > On 2012/05/18 17:01:09, dominich wrote:
> > > >
> > > >> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> > > >> >
> > > >>
> > > >
> > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
> > > >
> > > >> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
> > > >> >
> > > >> >
> > > >>
> > > >
> > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
> > > >
> > > >> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
> > > >> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
> > > >> support_.get());
> > > >> > On 2012/05/18 05:46:25, gavinp wrote:
> > > >> > > On 2012/05/15 16:06:25, dominich wrote:
> > > >> > > > this seems like an arbitrary place to put this. For 1, the
> > > >> Dispatcher
> > > >> isn't
> > > >> > > > dispatching anything. And 2, it has nothing to do with
> > > >> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
> > > >> > > initialized/destroyed
> > > >> > > > directly in the chrome_content_render_client alongside the
> > > >> > > PrerenderDispatcher.
> > > >> >
> > > >> > it seems that CCRC has less to do with prerendering than this file..
> > > >>
> > > >
> > > >  My understanding is that CCRC is the point at which any chrome-specific
> > > >> per-render process objects should be created. These classes represent
> > > >> independent aspects of the Prerender system and both need to be created
> > > >> per
> > > >> render process.
> > > >>
> > > >
> > > >  The alternative is to create a class that, when instantiated,
> > > >> instantiates a
> > > >> PrerenderDispatcher and a PrerenderSupport. That extra level of
> > > >> indirection
> > > >> seems redundant.
> > > >>
> > > >
> > > > Agreed that adding another class is needless.
> > > >
> > > >
> > > >
> > > >  I'm all for minimizing the surface area of an API, but I also want to
> > > >>  ensure
> > > >> that each class is responsible for a single aspect of the prerendering
> > > >> system.
> > > >>
> > > >
> > > > I don't really understand why you say that every class has to be
> > > > responsible for
> > > > a single aspect. It seems that patchset 1 here is not convoluted and
easy
> > > > to
> > > > follow. This is a common pattern that we do with other features in
chrome.
> > > >
> > > 
> > > Initializing the WebPrerenderingSupport in the PrerenderDispatcher
> > > constructor was poor code design as that class has nothing to do with the
> > > role the PrerenderDispatcher has.
> > 
> > > The PrerenderDispatcher should be
> > > responsible for tracking which URLs are being prerendered, and that's all.
> > 
> > I'm not sure I agree with this, see below.
> > 
> > > 
> > > The classes happen to have similar lifetimes but that's not a good reason
> > > to convolve the two. If we're doing something similar across Chrome, I'd
> > > argue that we should look at fixing them too.
> > 
> > During the content refactoring, we moved feature-code from
> > RenderThread/RenderView to self-contained classes. Some big features that
used
> > to have render-process-wide code in RenderThread moved to FooDispatcher. I
> > believe that the prerendering code, when creating PrerenderDispatcher, used
> > ExtensionDispatcher as the inspiration. "Dispatcher" in this sense meant
both
> > dispatching IPCs for that process, and also process-wide state. In that
sense,
> > it seems reasonable that this class would be what sets up the prerendering
> > support.
> 
> Ah, I see. So this should really be PrerenderPerRenderViewStuff or something.
> 
> It makes it clearer why we have different ideas about what this class should
be
> doing, but I still think that this is the wrong approach. In your eyes, this
> abstracts Prerender-specific per-RenderView functionality away from CCRC.

per render process

> In my
> eyes, it does the same but also lumps everything Prerender-specific together
> into one class, which is poor design. The only way to keep all Prerender stuff
> in one place outside of CCRC _and_ retain the design philosophy that each
> Prerender class should do only one thing is to create a
> PrerenderRenderViewSupport class that can be owned by CCRC, and that can own
> individual instances of PrerenderDispatcher and PrerenderSupport. The extra
> indirection is unfortunate, but it keeps both ends of the design cleaner.

I don't think this is cleaner for the reasons I've tried to call out in this
message and earlier. perhaps you want to gvc about this?

> 
> As an aside, I put together a CL that renames PrerenderDispatcher to
> PrerenderRenderViewTracker (http://codereview.chromium.org/10386223/) which I
> think makes the class role clearer.

I don't think this makes things easier to understand. PrerenderDispatcher is per
render process, not per renderview. this naming, of using a Dispatcher suffix,
is a convention that we use for different features. There's already a prerender
class that's per render view, that's PrerenderHelper, and it derives from
RenderViewObserverTracker so one can get to it using a static function given a
RenderView.
> 
> > 
> > 
> > > 
> > > 
> > > >
> > > > so from my pov, ps1 lgtm
> > > >
> > > >
> > > >  > >
> > > >> > > OK, the new patch does it this way.
> > > >> > >
> > > >> > > > I wonder if we should somehow make the parallels between the
> > > >> Dispatcher
> > > >> and
> > > >> > > > Tracker more obvious. PrerenderBrowserTracker and
> > > >> PrerenderRenderTracker
> > > >> > > maybe?
> > > >> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > > >> > >
> > > >> > > Sounds good.  Do you want to create a bug for this, or do you think
I
> > > >>
> > > > should
> > > >
> > > >> > > pull that into this CL?
> > > >> > >
> > > >>
> > > >
> > > >
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/10391134/%25253Chttp://codereview.chromium.o...>
> > > >

[jam, 2012-05-21 17:49:16]

On 2012/05/21 17:37:22, John Abd-El-Malek wrote:
> On 2012/05/21 16:27:05, dominich wrote:
> > On 2012/05/21 15:56:12, John Abd-El-Malek wrote:
> > > On 2012/05/18 23:31:12, dominich wrote:
> > > > On Fri, May 18, 2012 at 4:12 PM, <mailto:jam@chromium.org> wrote:
> > > > 
> > > > > On 2012/05/18 17:01:09, dominich wrote:
> > > > >
> > > > >> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> > > > >> >
> > > > >>
> > > > >
> > > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > > >
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
> > > > >
> > > > >> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
> > > > >> >
> > > > >> >
> > > > >>
> > > > >
> > > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > > >
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
> > > > >
> > > > >> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
> > > > >> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
> > > > >> support_.get());
> > > > >> > On 2012/05/18 05:46:25, gavinp wrote:
> > > > >> > > On 2012/05/15 16:06:25, dominich wrote:
> > > > >> > > > this seems like an arbitrary place to put this. For 1, the
> > > > >> Dispatcher
> > > > >> isn't
> > > > >> > > > dispatching anything. And 2, it has nothing to do with
> > > > >> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
> > > > >> > > initialized/destroyed
> > > > >> > > > directly in the chrome_content_render_client alongside the
> > > > >> > > PrerenderDispatcher.
> > > > >> >
> > > > >> > it seems that CCRC has less to do with prerendering than this
file..
> > > > >>
> > > > >
> > > > >  My understanding is that CCRC is the point at which any
chrome-specific
> > > > >> per-render process objects should be created. These classes represent
> > > > >> independent aspects of the Prerender system and both need to be
created
> > > > >> per
> > > > >> render process.
> > > > >>
> > > > >
> > > > >  The alternative is to create a class that, when instantiated,
> > > > >> instantiates a
> > > > >> PrerenderDispatcher and a PrerenderSupport. That extra level of
> > > > >> indirection
> > > > >> seems redundant.
> > > > >>
> > > > >
> > > > > Agreed that adding another class is needless.
> > > > >
> > > > >
> > > > >
> > > > >  I'm all for minimizing the surface area of an API, but I also want to
> > > > >>  ensure
> > > > >> that each class is responsible for a single aspect of the
prerendering
> > > > >> system.
> > > > >>
> > > > >
> > > > > I don't really understand why you say that every class has to be
> > > > > responsible for
> > > > > a single aspect. It seems that patchset 1 here is not convoluted and
> easy
> > > > > to
> > > > > follow. This is a common pattern that we do with other features in
> chrome.
> > > > >
> > > > 
> > > > Initializing the WebPrerenderingSupport in the PrerenderDispatcher
> > > > constructor was poor code design as that class has nothing to do with
the
> > > > role the PrerenderDispatcher has.
> > > 
> > > > The PrerenderDispatcher should be
> > > > responsible for tracking which URLs are being prerendered, and that's
all.
> > > 
> > > I'm not sure I agree with this, see below.
> > > 
> > > > 
> > > > The classes happen to have similar lifetimes but that's not a good
reason
> > > > to convolve the two. If we're doing something similar across Chrome, I'd
> > > > argue that we should look at fixing them too.
> > > 
> > > During the content refactoring, we moved feature-code from
> > > RenderThread/RenderView to self-contained classes. Some big features that
> used
> > > to have render-process-wide code in RenderThread moved to FooDispatcher. I
> > > believe that the prerendering code, when creating PrerenderDispatcher,
used
> > > ExtensionDispatcher as the inspiration. "Dispatcher" in this sense meant
> both
> > > dispatching IPCs for that process, and also process-wide state. In that
> sense,
> > > it seems reasonable that this class would be what sets up the prerendering
> > > support.
> > 
> > Ah, I see. So this should really be PrerenderPerRenderViewStuff or
something.
> > 
> > It makes it clearer why we have different ideas about what this class should
> be
> > doing, but I still think that this is the wrong approach. In your eyes, this
> > abstracts Prerender-specific per-RenderView functionality away from CCRC.
> 
> per render process
> 
> > In my
> > eyes, it does the same but also lumps everything Prerender-specific together
> > into one class, which is poor design. The only way to keep all Prerender
stuff
> > in one place outside of CCRC _and_ retain the design philosophy that each
> > Prerender class should do only one thing is to create a
> > PrerenderRenderViewSupport class that can be owned by CCRC, and that can own
> > individual instances of PrerenderDispatcher and PrerenderSupport. The extra
> > indirection is unfortunate, but it keeps both ends of the design cleaner.
> 
> I don't think this is cleaner for the reasons I've tried to call out in this
> message and earlier. perhaps you want to gvc about this?
> 
> > 
> > As an aside, I put together a CL that renames PrerenderDispatcher to
> > PrerenderRenderViewTracker (http://codereview.chromium.org/10386223/) which
I
> > think makes the class role clearer.
> 
> I don't think this makes things easier to understand. PrerenderDispatcher is
per
> render process, not per renderview. this naming, of using a Dispatcher suffix,
> is a convention that we use for different features. There's already a
prerender
> class that's per render view, that's PrerenderHelper, and it derives from
> RenderViewObserverTracker so one can get to it using a static function given a
> RenderView.

btw see
http://code.google.com/p/chromium/source/search?q=file%3Adispatcher+file%3Are...
for all the existing usages, hence the convention of using "Dispatcher" to mean
a class that's per renderer process that dispatches the control messages from
the browser.

> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > >
> > > > > so from my pov, ps1 lgtm
> > > > >
> > > > >
> > > > >  > >
> > > > >> > > OK, the new patch does it this way.
> > > > >> > >
> > > > >> > > > I wonder if we should somehow make the parallels between the
> > > > >> Dispatcher
> > > > >> and
> > > > >> > > > Tracker more obvious. PrerenderBrowserTracker and
> > > > >> PrerenderRenderTracker
> > > > >> > > maybe?
> > > > >> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > > > >> > >
> > > > >> > > Sounds good.  Do you want to create a bug for this, or do you
think
> I
> > > > >>
> > > > > should
> > > > >
> > > > >> > > pull that into this CL?
> > > > >> > >
> > > > >>
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/10391134/%2525253Chttp://codereview.chromium...>
> > > > >

[dominich, 2012-05-21 17:53:15]

On 2012/05/21 17:37:22, John Abd-El-Malek wrote:
> On 2012/05/21 16:27:05, dominich wrote:
> > On 2012/05/21 15:56:12, John Abd-El-Malek wrote:
> > > On 2012/05/18 23:31:12, dominich wrote:
> > > > On Fri, May 18, 2012 at 4:12 PM, <mailto:jam@chromium.org> wrote:
> > > > 
> > > > > On 2012/05/18 17:01:09, dominich wrote:
> > > > >
> > > > >> On 2012/05/18 16:38:15, John Abd-El-Malek wrote:
> > > > >> >
> > > > >>
> > > > >
> > > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > > >
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc>
> > > > >
> > > > >> > File chrome/renderer/prerender/**prerender_dispatcher.cc (right):
> > > > >> >
> > > > >> >
> > > > >>
> > > > >
> > > > > http://codereview.chromium.**org/10391134/diff/1/chrome/**
> > > > >
> > > >
> > >
> >
>
renderer/prerender/prerender_**dispatcher.cc#newcode17<http://codereview.chromium.org/10391134/diff/1/chrome/renderer/prerender/prerender_dispatcher.cc#newcode17>
> > > > >
> > > > >> > chrome/renderer/prerender/**prerender_dispatcher.cc:17:
> > > > >> > WebKit::**WebPrerenderingSupport::**initialize(prerendering_**
> > > > >> support_.get());
> > > > >> > On 2012/05/18 05:46:25, gavinp wrote:
> > > > >> > > On 2012/05/15 16:06:25, dominich wrote:
> > > > >> > > > this seems like an arbitrary place to put this. For 1, the
> > > > >> Dispatcher
> > > > >> isn't
> > > > >> > > > dispatching anything. And 2, it has nothing to do with
> > > > >> > > > WebKit::**WebPrerenderingSupport. I wonder if this should be
> > > > >> > > initialized/destroyed
> > > > >> > > > directly in the chrome_content_render_client alongside the
> > > > >> > > PrerenderDispatcher.
> > > > >> >
> > > > >> > it seems that CCRC has less to do with prerendering than this
file..
> > > > >>
> > > > >
> > > > >  My understanding is that CCRC is the point at which any
chrome-specific
> > > > >> per-render process objects should be created. These classes represent
> > > > >> independent aspects of the Prerender system and both need to be
created
> > > > >> per
> > > > >> render process.
> > > > >>
> > > > >
> > > > >  The alternative is to create a class that, when instantiated,
> > > > >> instantiates a
> > > > >> PrerenderDispatcher and a PrerenderSupport. That extra level of
> > > > >> indirection
> > > > >> seems redundant.
> > > > >>
> > > > >
> > > > > Agreed that adding another class is needless.
> > > > >
> > > > >
> > > > >
> > > > >  I'm all for minimizing the surface area of an API, but I also want to
> > > > >>  ensure
> > > > >> that each class is responsible for a single aspect of the
prerendering
> > > > >> system.
> > > > >>
> > > > >
> > > > > I don't really understand why you say that every class has to be
> > > > > responsible for
> > > > > a single aspect. It seems that patchset 1 here is not convoluted and
> easy
> > > > > to
> > > > > follow. This is a common pattern that we do with other features in
> chrome.
> > > > >
> > > > 
> > > > Initializing the WebPrerenderingSupport in the PrerenderDispatcher
> > > > constructor was poor code design as that class has nothing to do with
the
> > > > role the PrerenderDispatcher has.
> > > 
> > > > The PrerenderDispatcher should be
> > > > responsible for tracking which URLs are being prerendered, and that's
all.
> > > 
> > > I'm not sure I agree with this, see below.
> > > 
> > > > 
> > > > The classes happen to have similar lifetimes but that's not a good
reason
> > > > to convolve the two. If we're doing something similar across Chrome, I'd
> > > > argue that we should look at fixing them too.
> > > 
> > > During the content refactoring, we moved feature-code from
> > > RenderThread/RenderView to self-contained classes. Some big features that
> used
> > > to have render-process-wide code in RenderThread moved to FooDispatcher. I
> > > believe that the prerendering code, when creating PrerenderDispatcher,
used
> > > ExtensionDispatcher as the inspiration. "Dispatcher" in this sense meant
> both
> > > dispatching IPCs for that process, and also process-wide state. In that
> sense,
> > > it seems reasonable that this class would be what sets up the prerendering
> > > support.
> > 
> > Ah, I see. So this should really be PrerenderPerRenderViewStuff or
something.
> > 
> > It makes it clearer why we have different ideas about what this class should
> be
> > doing, but I still think that this is the wrong approach. In your eyes, this
> > abstracts Prerender-specific per-RenderView functionality away from CCRC.
> 
> per render process
> 
> > In my
> > eyes, it does the same but also lumps everything Prerender-specific together
> > into one class, which is poor design. The only way to keep all Prerender
stuff
> > in one place outside of CCRC _and_ retain the design philosophy that each
> > Prerender class should do only one thing is to create a
> > PrerenderRenderViewSupport class that can be owned by CCRC, and that can own
> > individual instances of PrerenderDispatcher and PrerenderSupport. The extra
> > indirection is unfortunate, but it keeps both ends of the design cleaner.
> 
> I don't think this is cleaner for the reasons I've tried to call out in this
> message and earlier. perhaps you want to gvc about this?

I don't like it either, I'm just trying to find a way to compromise our
different points of view.

> 
> > 
> > As an aside, I put together a CL that renames PrerenderDispatcher to
> > PrerenderRenderViewTracker (http://codereview.chromium.org/10386223/) which
I
> > think makes the class role clearer.
> 
> I don't think this makes things easier to understand. PrerenderDispatcher is
per
> render process, not per renderview. this naming, of using a Dispatcher suffix,
> is a convention that we use for different features. There's already a
prerender
> class that's per render view, that's PrerenderHelper, and it derives from
> RenderViewObserverTracker so one can get to it using a static function given a
> RenderView.

Thanks for the reminder to look at renaming PrerenderHelper too ;)

As the naming of Dispatcher is a convention, then I understand the issue with
renaming it and will not. I'm still not convinced that it's a good idea to
overload the role of a class as you're suggesting in this CL, but it doesn't
seem like there's a good alternative. This makes me sad, but not sad enough to
block this change from going through.

> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > >
> > > > > so from my pov, ps1 lgtm
> > > > >
> > > > >
> > > > >  > >
> > > > >> > > OK, the new patch does it this way.
> > > > >> > >
> > > > >> > > > I wonder if we should somehow make the parallels between the
> > > > >> Dispatcher
> > > > >> and
> > > > >> > > > Tracker more obvious. PrerenderBrowserTracker and
> > > > >> PrerenderRenderTracker
> > > > >> > > maybe?
> > > > >> > > > or PrerenderRenderViewTracker and PrerenderUrlTracker?
> > > > >> > >
> > > > >> > > Sounds good.  Do you want to create a bug for this, or do you
think
> I
> > > > >>
> > > > > should
> > > > >
> > > > >> > > pull that into this CL?
> > > > >> > >
> > > > >>
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.**org/10391134/%2525253Chttp://codereview.chromium...>
> > > > >

[dominich, 2012-05-21 17:59:15]

Patch 1 LGTM

[commit-bot: I haz the power, 2012-05-22 16:02:26]

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

[commit-bot: I haz the power, 2012-05-22 17:36:29]

Try job failure for 10391134-1002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-05-22 18:20:47]

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

[commit-bot: I haz the power, 2012-05-22 20:01:00]

Try job failure for 10391134-1002 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-23 14:32:53]

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

[commit-bot: I haz the power, 2012-05-23 16:26:01]

Change committed as 138494