cc: Separate RasterWorkerPool interface from implementation details.

cc: Separate RasterWorkerPool interface from implementation details.

RasterWorkerPool class has a lot of implementation specific details
that the tile manager and the delegate should not be affected by.
Moving implementation specific parts into a separate class and file
makes it clear what is part of the interface used by the tile
manager and what are details of the implementation. ie. changes to
implementation specific files alone should not affect or require
changes to the tile manager.

BUG=269841
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263120

[reveman, 2014-04-08 16:04:23]

Follow up to making RasterWorkerPool an abstract class.

[vmpstr, 2014-04-08 22:20:46]

Overall looks good, I just have some questions:

I'm assuming the impl here means what it means outside of compositor, and really
has nothing to do with the impl thread vs main thread? If so, it kind of sucks
to be mixing names like that. One could mistake raster_worker_pool and
raster_worker_pool_impl as same thing on different threads (whatever that may
mean).

Why not make raster_worker_pool_impl functionality be protected in
raster_worker_pool instead? I mean it's really nice that raster_worker_pool
class is so simple now and it's clear what you can do with it, but it adds a bit
of complexity to achieve that, IMHO.

What's preventing a worker pool implementation not to derive from
RasterWorkerPoolImpl? Or is that optional now?

[reveman, 2014-04-09 19:24:56]

On 2014/04/08 22:20:46, vmpstr wrote:
> Overall looks good, I just have some questions:
> 
> I'm assuming the impl here means what it means outside of compositor, and
really
> has nothing to do with the impl thread vs main thread? If so, it kind of sucks
> to be mixing names like that. One could mistake raster_worker_pool and
> raster_worker_pool_impl as same thing on different threads (whatever that may
> mean).

It's unfortunate that cc is abusing the _impl suffix a bit. I'm not sure that
should prevent proper usage of this suffix though.. 

> 
> Why not make raster_worker_pool_impl functionality be protected in
> raster_worker_pool instead? I mean it's really nice that raster_worker_pool
> class is so simple now and it's clear what you can do with it, but it adds a
bit
> of complexity to achieve that, IMHO.

I'd rather keep the interface clean from implementation details. Even if static,
protected members in an interface is a form of implementation inheritance which
I think we should strictly disallow. So if we want to maintain a clean
RasterWorkerPool interface, I think this is necessary.

That said, I like RasterWorkerPool for the name of the interface and
RasterWorkerPoolImpl as the base class for our implementations but it could be
different. ie. RasterWorkerPool can be the name of the base class and the
interface could have a completely different name. I considered naming the
interface just "WorkerPool" but it doesn't seem right as it's for raster
specifically.. Fyi, renaming the impls vs renaming the interface will result in
a similar amount of lines of code changing..

> 
> What's preventing a worker pool implementation not to derive from
> RasterWorkerPoolImpl? Or is that optional now?

Anything can implement the RasterWorkerPool interface as far as the tile manager
cares. LTHI could just implement it if that happens to be what we want, or it
could potentially be implemented completely outside cc/. Deriving from
RasterWorkerPoolImpl is optional. It's the base class that allows us to share
code between different implementations of this interface.

This refactor is in line with what we'd do if RasterWorkerPool would be exposed
as a public interface. That might be overkill but I've seen people (including
myself) being confused about what is the interface required by the tile manager
and what are details of the implementation and I'm trying to eliminate that and
think this does that effectively. I also think this will make it easier to work
and review changes to this code as it becomes clear what parts of the code a
change should and shouldn't affect.

[vmpstr, 2014-04-09 19:53:29]

On 2014/04/09 19:24:56, reveman wrote:
> On 2014/04/08 22:20:46, vmpstr wrote:
> > Overall looks good, I just have some questions:
> > 
> > I'm assuming the impl here means what it means outside of compositor, and
> really
> > has nothing to do with the impl thread vs main thread? If so, it kind of
sucks
> > to be mixing names like that. One could mistake raster_worker_pool and
> > raster_worker_pool_impl as same thing on different threads (whatever that
may
> > mean).
> 
> It's unfortunate that cc is abusing the _impl suffix a bit. I'm not sure that
> should prevent proper usage of this suffix though.. 

Maybe, maybe not :P What about RasterWorkerPoolBase or something like that? We
have a few FooBase that act as a common base of several particular
implementation (PicturePileBase, TilingData::BaseIterator (not quite
IteratorBase, but still), base::subtle::RefCountedBase, and a bunch of unittests
have FooBase).

As an aside, I agree that your use of impl is appropriate in general code.
However, in cc, once we started using impl to mean impl-thread, I don't think
it's appropriate. I don't feel too strongly about this, so feel free to push
back. Ideally we'd take the time to rename our current impl to something else,
but that's probably not going to happen any time soon.

> 
> > 
> > Why not make raster_worker_pool_impl functionality be protected in
> > raster_worker_pool instead? I mean it's really nice that raster_worker_pool
> > class is so simple now and it's clear what you can do with it, but it adds a
> bit
> > of complexity to achieve that, IMHO.
> 
> I'd rather keep the interface clean from implementation details. Even if
static,
> protected members in an interface is a form of implementation inheritance
which
> I think we should strictly disallow. So if we want to maintain a clean
> RasterWorkerPool interface, I think this is necessary.

Ok, I can buy this.

> 
> That said, I like RasterWorkerPool for the name of the interface and
> RasterWorkerPoolImpl as the base class for our implementations but it could be
> different. ie. RasterWorkerPool can be the name of the base class and the
> interface could have a completely different name. I considered naming the
> interface just "WorkerPool" but it doesn't seem right as it's for raster
> specifically.. Fyi, renaming the impls vs renaming the interface will result
in
> a similar amount of lines of code changing..
> 
> > 
> > What's preventing a worker pool implementation not to derive from
> > RasterWorkerPoolImpl? Or is that optional now?
> 
> Anything can implement the RasterWorkerPool interface as far as the tile
manager
> cares. LTHI could just implement it if that happens to be what we want, or it
> could potentially be implemented completely outside cc/. Deriving from
> RasterWorkerPoolImpl is optional. It's the base class that allows us to share
> code between different implementations of this interface.
> 
> This refactor is in line with what we'd do if RasterWorkerPool would be
exposed
> as a public interface. That might be overkill but I've seen people (including
> myself) being confused about what is the interface required by the tile
manager
> and what are details of the implementation and I'm trying to eliminate that
and
> think this does that effectively. I also think this will make it easier to
work
> and review changes to this code as it becomes clear what parts of the code a
> change should and shouldn't affect.

Ok, thanks for clearing this up. Your reasoning makes sense, so as long as
RasterWorkerPool is implemented, the actual details don't matter that much for
tile manager.

[reveman, 2014-04-09 20:46:37]

On 2014/04/09 19:53:29, vmpstr wrote:
> On 2014/04/09 19:24:56, reveman wrote:
> > On 2014/04/08 22:20:46, vmpstr wrote:
> > > Overall looks good, I just have some questions:
> > > 
> > > I'm assuming the impl here means what it means outside of compositor, and
> > really
> > > has nothing to do with the impl thread vs main thread? If so, it kind of
> sucks
> > > to be mixing names like that. One could mistake raster_worker_pool and
> > > raster_worker_pool_impl as same thing on different threads (whatever that
> may
> > > mean).
> > 
> > It's unfortunate that cc is abusing the _impl suffix a bit. I'm not sure
that
> > should prevent proper usage of this suffix though.. 
> 
> Maybe, maybe not :P What about RasterWorkerPoolBase or something like that? We
> have a few FooBase that act as a common base of several particular
> implementation (PicturePileBase, TilingData::BaseIterator (not quite
> IteratorBase, but still), base::subtle::RefCountedBase, and a bunch of
unittests
> have FooBase).

The difference is that these are not implementations of some abstract class. ie.
there's no abstract PicturePile class that classes deriving from PicturePileBase
implements. I'm afraid that it might be a bit confusing to have an interface
called RasterWorkerPool and a concrete RasterWorkerPoolBase class that
implementations of this interface derive from. We wouldn't need the "impl"
suffix if the implementation had a different name from the interface though..

How about something like this:

RasterWorkerPool -> AsyncRasterizier (or simply Rasterizer)

and that's the interface used by the tile manager. For the implementation we
have:

// Base class for worker pool implementations of Rasterizer interface in cc.
class RasterWorkerPoolBase {}

// Base class for worker pool implementations of Rasterizer interface in cc.
class ImageRasterWorkerPool : public RasterWorkerPoolBase,
                              public Rasterizer {}

wdyt?

[vmpstr, 2014-04-09 21:06:55]

On 2014/04/09 20:46:37, reveman wrote:
> On 2014/04/09 19:53:29, vmpstr wrote:
> > On 2014/04/09 19:24:56, reveman wrote:
> > > On 2014/04/08 22:20:46, vmpstr wrote:
> > > > Overall looks good, I just have some questions:
> > > > 
> > > > I'm assuming the impl here means what it means outside of compositor,
and
> > > really
> > > > has nothing to do with the impl thread vs main thread? If so, it kind of
> > sucks
> > > > to be mixing names like that. One could mistake raster_worker_pool and
> > > > raster_worker_pool_impl as same thing on different threads (whatever
that
> > may
> > > > mean).
> > > 
> > > It's unfortunate that cc is abusing the _impl suffix a bit. I'm not sure
> that
> > > should prevent proper usage of this suffix though.. 
> > 
> > Maybe, maybe not :P What about RasterWorkerPoolBase or something like that?
We
> > have a few FooBase that act as a common base of several particular
> > implementation (PicturePileBase, TilingData::BaseIterator (not quite
> > IteratorBase, but still), base::subtle::RefCountedBase, and a bunch of
> unittests
> > have FooBase).
> 
> The difference is that these are not implementations of some abstract class.
ie.
> there's no abstract PicturePile class that classes deriving from
PicturePileBase
> implements. I'm afraid that it might be a bit confusing to have an interface
> called RasterWorkerPool and a concrete RasterWorkerPoolBase class that
> implementations of this interface derive from. We wouldn't need the "impl"
> suffix if the implementation had a different name from the interface though..
> 
> How about something like this:
> 
> RasterWorkerPool -> AsyncRasterizier (or simply Rasterizer)
> 
> and that's the interface used by the tile manager. For the implementation we
> have:
> 
> // Base class for worker pool implementations of Rasterizer interface in cc.
> class RasterWorkerPoolBase {}
> 
> // Base class for worker pool implementations of Rasterizer interface in cc.
> class ImageRasterWorkerPool : public RasterWorkerPoolBase,
>                               public Rasterizer {}
> 
> wdyt?

That makes sense to me, that way we avoid the fact that RasterWorkerPool doesn't
derive from RasterWorkerPoolBase, because it's called Rasterizer, and we avoid
the confusion of the impl thing

[reveman, 2014-04-10 13:00:02]

PTAL

[vmpstr, 2014-04-10 18:21:18]

I like the name, I think this is a good separation of what is implementation and
what is interface.

https://codereview.chromium.org/228173002/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:389:
std::find_if(raster_task_states_.begin(),
This whole find can now be a part of a dcheck

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
File cc/resources/rasterizer_delegate.cc (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
cc/resources/rasterizer_delegate.cc:63: DCHECK_LT(0u,
did_finish_running_tasks_pending_count_);
While here, could you TRACE_EVENT this one and the for activation one?

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
File cc/resources/rasterizer_delegate.h (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
cc/resources/rasterizer_delegate.h:14: class RasterizerDelegate : public
RasterizerClient {
Does this implement all the functions for it to qualify to be a Rasterizer? If
so, can we make it a rasterizer, so the tile manager only deals with a
rasterizer (after creating a delegate), instead of dealing with a rasterizer
delegate?

[reveman, 2014-04-10 19:38:15]

PTAL

https://codereview.chromium.org/228173002/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:389:
std::find_if(raster_task_states_.begin(),
On 2014/04/10 18:21:18, vmpstr wrote:
> This whole find can now be a part of a dcheck

Removed this and unnecessary find_if above. We still have a DCHECK in the top
loop.

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
File cc/resources/rasterizer_delegate.cc (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
cc/resources/rasterizer_delegate.cc:63: DCHECK_LT(0u,
did_finish_running_tasks_pending_count_);
On 2014/04/10 18:21:18, vmpstr wrote:
> While here, could you TRACE_EVENT this one and the for activation one?

Done.

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
File cc/resources/rasterizer_delegate.h (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
cc/resources/rasterizer_delegate.h:14: class RasterizerDelegate : public
RasterizerClient {
On 2014/04/10 18:21:18, vmpstr wrote:
> Does this implement all the functions for it to qualify to be a Rasterizer? If
> so, can we make it a rasterizer, so the tile manager only deals with a
> rasterizer (after creating a delegate), instead of dealing with a rasterizer
> delegate?

Not yet. It sort of does after https://codereview.chromium.org/228183002/ lands
but not really. ScheduleTasks accepts an array of "RasterTaskQueue*" in the case
of the delegate but that's of course not the case for the general Rasterizer
interface.

[vmpstr, 2014-04-10 19:40:52]

lgtm

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
File cc/resources/rasterizer_delegate.h (right):

https://codereview.chromium.org/228173002/diff/60001/cc/resources/rasterizer_...
cc/resources/rasterizer_delegate.h:14: class RasterizerDelegate : public
RasterizerClient {
On 2014/04/10 19:38:15, reveman wrote:
> On 2014/04/10 18:21:18, vmpstr wrote:
> > Does this implement all the functions for it to qualify to be a Rasterizer?
If
> > so, can we make it a rasterizer, so the tile manager only deals with a
> > rasterizer (after creating a delegate), instead of dealing with a rasterizer
> > delegate?
> 
> Not yet. It sort of does after https://codereview.chromium.org/228183002/
lands
> but not really. ScheduleTasks accepts an array of "RasterTaskQueue*" in the
case
> of the delegate but that's of course not the case for the general Rasterizer
> interface.

Ah good point.

[reveman, 2014-04-10 19:42:13]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-10 19:43:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/228173002/80001

[commit-bot: I haz the power, 2014-04-10 23:09:58]

Change committed as 263120