Builder class for SkLayerDrawLooper.

Builder class for SkLayerDrawLooper.

SkLayerDrawLooper provides methods like addLayer() to build up a linked list
of layers. Working towards making this class immutable, this patch introduces
the SkLayerDrawLooperBuilder class which is used to accumulate all the layers
first. Once all layers are in place, it creates a new SkLayerDrawLooper object
and hands over the list of layers to that object.

For now we keep the addLayer methods in SkLayerDrawLooper so we don't break
Chrome and Blink when this is landed. Once we've updated all users, we can
remove the methods.

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

[Dominik Grewe, 2014-02-07 18:10:55]

This is how we could eliminate the addLayer methods. Together with the patch on
introducing a DrawContext, this would make SkLayerDrawLooper immutable.

[reed1, 2014-02-07 18:32:22]

I'm fine with the idea.

Seems like eventually we'd want to move all of the apis into the builder,
including the definition of the layerinfo as well?

If so, perhaps to help stage that, we can dup the LayerInfo definition into the
builder now, so that we could update the call-sites in one pass to soley use the
builder for everything... marking the existing methods as deprecated.

https://codereview.chromium.org/133813005/diff/1/include/effects/SkLayerDrawL...
File include/effects/SkLayerDrawLooper.h (right):

https://codereview.chromium.org/133813005/diff/1/include/effects/SkLayerDrawL...
include/effects/SkLayerDrawLooper.h:177: SkDrawLooper* createLooper();
possibly call this detachLooper(), as in other places Skia uses the term
"detach" to signify that one object is transferring ownership to the caller.

[Dominik Grewe, 2014-02-07 18:56:55]

On 2014/02/07 18:32:22, reed1 wrote:
> I'm fine with the idea.
> 
> Seems like eventually we'd want to move all of the apis into the builder,
> including the definition of the layerinfo as well?
> 
> If so, perhaps to help stage that, we can dup the LayerInfo definition into
the
> builder now, so that we could update the call-sites in one pass to soley use
the
> builder for everything... marking the existing methods as deprecated.

But then we'd have to cast between the types in different classes while we have
two version, right?

Is it really necessary to move theses things into the builder? I feel like the
LayerInfo is more a part of the looper than of the builder.

[Dominik Grewe, 2014-02-07 18:57:02]

https://codereview.chromium.org/133813005/diff/1/include/effects/SkLayerDrawL...
File include/effects/SkLayerDrawLooper.h (right):

https://codereview.chromium.org/133813005/diff/1/include/effects/SkLayerDrawL...
include/effects/SkLayerDrawLooper.h:177: SkDrawLooper* createLooper();
On 2014/02/07 18:32:22, reed1 wrote:
> possibly call this detachLooper(), as in other places Skia uses the term
> "detach" to signify that one object is transferring ownership to the caller.

Done.

[scroggo, 2014-02-10 18:13:53]

lgtm

[reed1, 2014-02-10 21:17:42]

On 2014/02/07 18:56:55, Dominik Grewe wrote:
> On 2014/02/07 18:32:22, reed1 wrote:
> > I'm fine with the idea.
> > 
> > Seems like eventually we'd want to move all of the apis into the builder,
> > including the definition of the layerinfo as well?
> > 
> > If so, perhaps to help stage that, we can dup the LayerInfo definition into
> the
> > builder now, so that we could update the call-sites in one pass to soley use
> the
> > builder for everything... marking the existing methods as deprecated.
> 
> But then we'd have to cast between the types in different classes while we
have
> two version, right?
> 
> Is it really necessary to move theses things into the builder? I feel like the
> LayerInfo is more a part of the looper than of the builder.

Hmmm, now that you ask, perahps the entire builder should just be an inner class
of the looper.

[reed1, 2014-02-10 21:18:07]

lets land some tests or a gm that exercises the new builder.

[Dominik Grewe, 2014-02-11 10:51:40]

I've moved the builder class inside the SkLayerDrawLooper class. Turns out we
also need to be able to initialize an existing looper object rather than always
creating a new one with detachLooper(). But it's only inside one of the
SkLayerDrawLooper constructors, so I made that method private to discourage
people from allocating loopers on the stack.

I've also changed the tests to use the builder class. I left other uses, e.g. in
gm, unchanged so we still exercise both paths on the whole.

[scroggo, 2014-02-11 14:48:36]

https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
File src/effects/SkLayerDrawLooper.cpp (right):

https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
src/effects/SkLayerDrawLooper.cpp:242: builder.initLooperAndReset(this);
I think the main place this will be called is readFlattenable(). What if we made
the caller call the builder instead of using this constructor? Other
flattenables we create builders for can work the same way.

[Dominik Grewe, 2014-02-12 10:04:57]

On 2014/02/11 14:48:36, scroggo wrote:
>
https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
> File src/effects/SkLayerDrawLooper.cpp (right):
> 
>
https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
> src/effects/SkLayerDrawLooper.cpp:242: builder.initLooperAndReset(this);
> I think the main place this will be called is readFlattenable(). What if we
made
> the caller call the builder instead of using this constructor? Other
> flattenables we create builders for can work the same way.

The constructor was only called by CreateProc (via
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS). If we implement our own
CreateProc we can just use the builder and we don't need the constructor any
more. PTAL.

[scroggo, 2014-02-12 19:03:02]

On 2014/02/12 10:04:57, Dominik Grewe wrote:
> On 2014/02/11 14:48:36, scroggo wrote:
> >
>
https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
> > File src/effects/SkLayerDrawLooper.cpp (right):
> > 
> >
>
https://codereview.chromium.org/133813005/diff/160001/src/effects/SkLayerDraw...
> > src/effects/SkLayerDrawLooper.cpp:242: builder.initLooperAndReset(this);
> > I think the main place this will be called is readFlattenable(). What if we
> made
> > the caller call the builder instead of using this constructor? Other
> > flattenables we create builders for can work the same way.
> 
> The constructor was only called by CreateProc (via
> SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS). If we implement our own
> CreateProc we can just use the builder and we don't need the constructor any
> more. PTAL.

I like this much better with the new CreateProc. lgtm

[reed2, 2014-02-13 15:13:52]

very clean api -- lgtm

[Dominik Grewe, 2014-02-14 09:52:54]

Thanks!

[Dominik Grewe, 2014-02-14 09:53:00]

The CQ bit was checked by dominikg@chromium.org

[commit-bot: I haz the power, 2014-02-14 09:53:10]

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

[commit-bot: I haz the power, 2014-02-14 10:06:47]

Change committed as 13448