Builder class for SkLayerRasterizer.

Builder class for SkLayerRasterizer.

Provide builder class to make SkLayerRasterizer immutable. We have to keep the
addLayer() methods for now because they are used in Chrome. They will be removed
once this changed has been rolled into Chrome.
An added benefit of this is that this class can only be allocated on the heap.

BUG=skia:2187
Committed: http://code.google.com/p/skia/source/detail?r=13590

[Dominik Grewe, 2014-02-24 11:45:19]

PTAL, thanks.

[reed1, 2014-02-24 13:35:40]

https://codereview.chromium.org/176873004/diff/30001/include/effects/SkLayerR...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/30001/include/effects/SkLayerR...
include/effects/SkLayerRasterizer.h:51: void addLayer(const SkPaint& paint) {
Consider adding a build-guard here, to remind us to eliminate callers to the
non-builder calls like this one.

#ifndef SK_SUPPORT_LEGACY_LAYERRASTERIZER_API
    void addLayer(...);
#endif

Then we can *not* define this for skia builds (since we can fix all the callers
w/ this CL), but can #define this for Chrome if needed.

[Dominik Grewe, 2014-02-24 13:54:34]

https://codereview.chromium.org/176873004/diff/30001/include/effects/SkLayerR...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/30001/include/effects/SkLayerR...
include/effects/SkLayerRasterizer.h:51: void addLayer(const SkPaint& paint) {
On 2014/02/24 13:35:40, reed1 wrote:
> Consider adding a build-guard here, to remind us to eliminate callers to the
> non-builder calls like this one.
> 
> #ifndef SK_SUPPORT_LEGACY_LAYERRASTERIZER_API
>     void addLayer(...);
> #endif
> 
> Then we can *not* define this for skia builds (since we can fix all the
callers
> w/ this CL), but can #define this for Chrome if needed.

Done.

[scroggo, 2014-02-24 13:57:51]

From the description:

> A side-effect of this is that this class can only be
> allocated on the heap.

Isn't that the goal of this CL, rather than a side-effect?

https://codereview.chromium.org/176873004/diff/30001/src/effects/SkLayerRaste...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/176873004/diff/30001/src/effects/SkLayerRaste...
src/effects/SkLayerRasterizer.cpp:28: SkLayerRasterizer::SkLayerRasterizer() :
fLayers(SkNEW_ARGS(SkDeque, (sizeof(SkLayerRasterizer_Rec))))
nit: line too long.

Typically we would write this as

SkLayerRasterizer::SkLayerRasterizer()
    : fLayers(SkNEW_ARGS(SkDeque, sizeof(SkLayerRasterizer_Rec))
{
}

That will help with the line length.

[reed1, 2014-02-24 13:59:48]

I like the guard.
Some style nits.

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
include/effects/SkLayerRasterizer.h:81: SkDeque* fLayers;
question: can this be 'const' when we switch over to only using the builder?

https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:28: SkLayerRasterizer::SkLayerRasterizer() :
fLayers(SkNEW_ARGS(SkDeque, (sizeof(SkLayerRasterizer_Rec))))
nit: this is an 80-col file, and this line is over 100

Preferred overflow style for constructors...

SkLayerRasterizer::SkLayerRasterizer()
    : fLayers(...)
    , other(...)
{
...
}

[scroggo, 2014-02-24 14:00:21]

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
include/effects/SkLayerRasterizer.h:81: SkDeque* fLayers;
In the interest of immutability, should these be const * const?

[Dominik Grewe, 2014-02-24 14:44:41]

I suppose side-effect sounds a bit negative :) I've updated the description.

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
include/effects/SkLayerRasterizer.h:81: SkDeque* fLayers;
On 2014/02/24 14:00:21, scroggo wrote:
> In the interest of immutability, should these be const * const?

Yes, I've made them const behind the guard. That also required removing the
constructor taking a buffer.

https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:28: SkLayerRasterizer::SkLayerRasterizer() :
fLayers(SkNEW_ARGS(SkDeque, (sizeof(SkLayerRasterizer_Rec))))
On 2014/02/24 13:59:48, reed1 wrote:
> nit: this is an 80-col file, and this line is over 100
> 
> Preferred overflow style for constructors...
> 
> SkLayerRasterizer::SkLayerRasterizer()
>     : fLayers(...)
>     , other(...)
> {
> ...
> }

Done.

[scroggo, 2014-02-24 14:48:09]

On 2014/02/24 14:44:41, Dominik Grewe wrote:
> I suppose side-effect sounds a bit negative :) I've updated the description.
> 
>
https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
> File include/effects/SkLayerRasterizer.h (right):
> 
>
https://codereview.chromium.org/176873004/diff/100001/include/effects/SkLayer...
> include/effects/SkLayerRasterizer.h:81: SkDeque* fLayers;
> On 2014/02/24 14:00:21, scroggo wrote:
> > In the interest of immutability, should these be const * const?
> 
> Yes, I've made them const behind the guard. That also required removing the
> constructor taking a buffer.
> 
>
https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
> File src/effects/SkLayerRasterizer.cpp (right):
> 
>
https://codereview.chromium.org/176873004/diff/100001/src/effects/SkLayerRast...
> src/effects/SkLayerRasterizer.cpp:28: SkLayerRasterizer::SkLayerRasterizer() :
> fLayers(SkNEW_ARGS(SkDeque, (sizeof(SkLayerRasterizer_Rec))))
> On 2014/02/24 13:59:48, reed1 wrote:
> > nit: this is an 80-col file, and this line is over 100
> > 
> > Preferred overflow style for constructors...
> > 
> > SkLayerRasterizer::SkLayerRasterizer()
> >     : fLayers(...)
> >     , other(...)
> > {
> > ...
> > }
> 
> Done.

LGTM

[reed1, 2014-02-24 14:54:57]

Not sure you have to remove the constructor(ReadBuffer&) -- without this we
can't subclass this guy in the future.

I think you can keep the constructor, and just invoke your new CreateProc dude
to init the Layers.

[Dominik Grewe, 2014-02-24 14:57:31]

On 2014/02/24 14:54:57, reed1 wrote:
> Not sure you have to remove the constructor(ReadBuffer&) -- without this we
> can't subclass this guy in the future.
> 
> I think you can keep the constructor, and just invoke your new CreateProc dude
> to init the Layers.

But then I can't make fLayers const, can I? I have to set it in the initializer
list of the constructor.

[reed1, 2014-02-24 14:58:57]

On 2014/02/24 14:57:31, Dominik Grewe wrote:
> On 2014/02/24 14:54:57, reed1 wrote:
> > Not sure you have to remove the constructor(ReadBuffer&) -- without this we
> > can't subclass this guy in the future.
> > 
> > I think you can keep the constructor, and just invoke your new CreateProc
dude
> > to init the Layers.
> 
> But then I can't make fLayers const, can I? I have to set it in the
initializer
> list of the constructor.

Layers* make_layers(Buffer& buffer) {
    loop
        layer->add();
    return layer;
}

Class::Class(Buffer& buffer) : fLayers(make_layers(buffer)) ...

[Dominik Grewe, 2014-02-24 15:11:16]

On 2014/02/24 14:58:57, reed1 wrote:
> On 2014/02/24 14:57:31, Dominik Grewe wrote:
> > On 2014/02/24 14:54:57, reed1 wrote:
> > > Not sure you have to remove the constructor(ReadBuffer&) -- without this
we
> > > can't subclass this guy in the future.
> > > 
> > > I think you can keep the constructor, and just invoke your new CreateProc
> dude
> > > to init the Layers.
> > 
> > But then I can't make fLayers const, can I? I have to set it in the
> initializer
> > list of the constructor.
> 
> Layers* make_layers(Buffer& buffer) {
>     loop
>         layer->add();
>     return layer;
> }
> 
> Class::Class(Buffer& buffer) : fLayers(make_layers(buffer)) ...

Done.
I should probably do the same for SkLayerDrawLooper where I also removed the
constructor (see https://codereview.chromium.org/133813005/). I'll file a
separate CL.

[reed1, 2014-02-24 15:22:21]

good catch on drawlooper

lgtm w/ request to make ReadLayers private (if possible)

https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
include/effects/SkLayerRasterizer.h:75: static SkDeque* ReadLayers(SkReadBuffer&
buffer);
Lets make this guy private if possible. -- less is more

[Dominik Grewe, 2014-02-24 15:48:22]

https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
File include/effects/SkLayerRasterizer.h (right):

https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
include/effects/SkLayerRasterizer.h:75: static SkDeque* ReadLayers(SkReadBuffer&
buffer);
On 2014/02/24 15:22:22, reed1 wrote:
> Lets make this guy private if possible. -- less is more

Done.

[scroggo, 2014-02-24 15:54:09]

On 2014/02/24 15:48:22, Dominik Grewe wrote:
>
https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
> File include/effects/SkLayerRasterizer.h (right):
> 
>
https://codereview.chromium.org/176873004/diff/330001/include/effects/SkLayer...
> include/effects/SkLayerRasterizer.h:75: static SkDeque*
ReadLayers(SkReadBuffer&
> buffer);
> On 2014/02/24 15:22:22, reed1 wrote:
> > Lets make this guy private if possible. -- less is more
> 
> Done.

lgtm

[Dominik Grewe, 2014-02-25 11:05:55]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-25 11:06:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/176873004/370001

[Dominik Grewe, 2014-02-25 11:07:01]

The CQ bit was unchecked by dominikg@chromium.org

[Dominik Grewe, 2014-02-26 13:27:06]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-26 13:27:18]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/dominikg@chromium.org/176873004/370001

[commit-bot: I haz the power, 2014-02-26 13:27:41]

Change committed as 13590