Make GrPaint have a variable sized array of color and coverage stages rather than a fixed size.

Make GrPaint have a variable sized array of color and coverage stages rather than a fixed size.

Committed: http://code.google.com/p/skia/source/detail?r=10062

[robertphillips, 2013-07-12 12:54:33]

lgtm + some suggestions, questions & nits

https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
src/effects/SkXfermodeImageFilter.cpp:129: } else {
foregroundPaint, backgroundPaint?
fgPaint, bgPaint?

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
src/gpu/GrContext.cpp:1796: AutoRestoreEffects are;
can we just pass in NULL here?

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
src/gpu/GrContext.cpp:1815: AutoRestoreEffects are;
Here too?

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
File src/gpu/GrPaint.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
src/gpu/GrPaint.cpp:23: void GrPaint::addColorTextureEffect(GrTexture* texture,
missing space in next two lines?

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
src/gpu/SkGpuDevice.cpp:1344: // Construct a GrPaint by setting the bitmap
texture as the first effect and then configuring
reset -> rest?

[bsalomon, 2013-07-12 13:38:59]

https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
src/effects/SkXfermodeImageFilter.cpp:129: } else {
On 2013/07/12 12:54:33, robertphillips wrote:
> foregroundPaint, backgroundPaint?
> fgPaint, bgPaint?

yeah, that's better.

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
src/gpu/GrContext.cpp:1796: AutoRestoreEffects are;
On 2013/07/12 12:54:33, robertphillips wrote:
> can we just pass in NULL here?

No... it's removing the effects from the draw state on the draw target (those
copied from the paint). It probably would be safe to pass NULL for all but the
last draw in this func, but we have some assertions to protect against ever
passing in a non-NULL paint and NULL for the a.r.e. and I think that protection
is worth preserving. Anyway, this function is slated to be removed and rewritten
against GrContext.

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
File src/gpu/GrPaint.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
src/gpu/GrPaint.cpp:23: void GrPaint::addColorTextureEffect(GrTexture* texture,
On 2013/07/12 12:54:33, robertphillips wrote:
> missing space in next two lines?

Done.

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
src/gpu/SkGpuDevice.cpp:1344: // Construct a GrPaint by setting the bitmap
texture as the first effect and then configuring
On 2013/07/12 12:54:33, robertphillips wrote:
> reset -> rest?

Done.

[commit-bot: I haz the power, 2013-07-12 14:01:04]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/18686007/13002

[jvanverth1, 2013-07-12 14:13:29]

The only comment I have is that it feels like an inconsistent interface to have
the TextureEffect methods (in GrPaint) automatically unref 
while the other Effect methods don't. That's not going to be immediately obvious
to a client. Otherwise this is a lot cleaner. 

On 2013/07/12 13:38:59, bsalomon wrote:
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
> File src/effects/SkXfermodeImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/effects/Sk...
> src/effects/SkXfermodeImageFilter.cpp:129: } else {
> On 2013/07/12 12:54:33, robertphillips wrote:
> > foregroundPaint, backgroundPaint?
> > fgPaint, bgPaint?
> 
> yeah, that's better.
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrCont...
> src/gpu/GrContext.cpp:1796: AutoRestoreEffects are;
> On 2013/07/12 12:54:33, robertphillips wrote:
> > can we just pass in NULL here?
> 
> No... it's removing the effects from the draw state on the draw target (those
> copied from the paint). It probably would be safe to pass NULL for all but the
> last draw in this func, but we have some assertions to protect against ever
> passing in a non-NULL paint and NULL for the a.r.e. and I think that
protection
> is worth preserving. Anyway, this function is slated to be removed and
rewritten
> against GrContext.
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
> File src/gpu/GrPaint.cpp (right):
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/GrPain...
> src/gpu/GrPaint.cpp:23: void GrPaint::addColorTextureEffect(GrTexture*
texture,
> On 2013/07/12 12:54:33, robertphillips wrote:
> > missing space in next two lines?
> 
> Done.
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.chromium.org/18686007/diff/5066549580791808/src/gpu/SkGpuD...
> src/gpu/SkGpuDevice.cpp:1344: // Construct a GrPaint by setting the bitmap
> texture as the first effect and then configuring
> On 2013/07/12 12:54:33, robertphillips wrote:
> > reset -> rest?
> 
> Done.

[bsalomon, 2013-07-12 18:09:46]

On 2013/07/12 14:13:29, JimVV wrote:
> The only comment I have is that it feels like an inconsistent interface to
have
> the TextureEffect methods (in GrPaint) automatically unref 
> while the other Effect methods don't. That's not going to be immediately
obvious
> to a client. Otherwise this is a lot cleaner. 
> 

The generic method takes an effect object from the caller. It can't remove the
caller's ref (though often the caller will immediately unref it because it isn't
reused). The TextureEffect methods create the effect object inside the method
and must unref it or there would be a leak.

[commit-bot: I haz the power, 2013-07-13 17:24:28]

Change committed as 10062