SkShader::asNewEffect Refactoring

SkShader::asNewEffect Refactoring

The new signature is:

	bool asNewEffect(GrContext* context, const SkPaint& paint, GrColor* grColor, GrEffectRef** grEffect, const SkMatrix* localMatrixOrNull) const;

It will fix the hack for skcolorshader by modifying the GrColor parameter in SkGr::SkPaint2GrPaintShader.

BUG=skia:2646
Committed: https://skia.googlesource.com/skia/+/9de5b514d38c5b36066bcdc14fba2f7e5196d372

[jvanverth1, 2014-06-05 17:03:07]

Initial comments -- I still need to go through all the SkShaders.

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:383: virtual bool asNewEffect(GrContext* context, const
SkPaint& paint, GrColor* grColor,
I'd probably put grColor and grEffect at the end of the parameter list.
Otherwise you're mixing inputs and outputs. Also, you should change the comment
to reflect the new parameter list. E.g. a return value of 'false' means that it
did not create a ref or set a color.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkRadialGradient.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkRadialGradient.h:37: const SK_OVERRIDE;
Indent.

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode498
src/gpu/SkGr.cpp:498: shader->asNewEffect(context, skPaint, &grColor, &grEffect,
NULL);
The bool return value isn't checked here. Also, in looking at it written out,
getting the ref * and then adding it to a SkAutoTUnref seems a little clunky.
I'm not sure if there's a better way to do this -- Brian?

[bsalomon, 2014-06-05 17:32:45]

I just realized that maybe the idea is to address some of the things I've been
commenting on in a follow-up CL, so I'm going to stop now :)

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:377: *  If the shader subclass has a GrEffect
implementation, this resturns the effect to install.
These comments need updating.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:386: GrEffectRef** grEffect, const SkMatrix*
localMatrix) const {
Doesn't this function need to set grColor?

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:456: *grEffect = NULL;
I don't think we should require this. The passed in ptr should already point to
NULL.

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode433
src/gpu/SkGr.cpp:433: if (justAlpha) {
Can we kill this?

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode498
src/gpu/SkGr.cpp:498: shader->asNewEffect(context, skPaint, &grColor, &grEffect,
NULL);
On 2014/06/05 17:03:07, jvanverth1 wrote:
> The bool return value isn't checked here. Also, in looking at it written out,
> getting the ref * and then adding it to a SkAutoTUnref seems a little clunky.
> I'm not sure if there's a better way to do this -- Brian?

I can't think of a better way other than sending down a SkAutoTUnref* but I'm
not sure I like that.

We should probably fall back to solid color painting if false is returned...
e.g. copy SkPaint's color to GrPaint.

[jvanverth1, 2014-06-05 17:36:31]

More comments:

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:386: GrEffectRef** grEffect, const SkMatrix*
localMatrix) const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.h:33: #if SK_SUPPORT_GPU
Given that the base class asNewEffect() definition isn't protected by this #if,
you can probably remove it, and similar ones.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkPictureShader.h
File src/core/SkPictureShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkPictureShader...
src/core/SkPictureShader.h:34: const SK_OVERRIDE;
indent

https://codereview.chromium.org/318923005/diff/20001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:1285: const {
indent 'const {', and grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkLinearGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkLinearGradient.cpp:554: GrEffectRef** grEffect, const
SkMatrix* localMatrix)  const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkRadialGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkRadialGradient.cpp:566: GrEffectRef** grEffect, const
SkMatrix* localMatrix) const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkSweepGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkSweepGradient.cpp:286: GrEffectRef** grEffect, const
SkMatrix* localMatrix)  const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointConicalGradient.cpp:385: const SkMatrix*
localMatrix)  const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointRadialGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointRadialGradient.cpp:678: const SkMatrix*
localMatrix)  const {
grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode498
src/gpu/SkGr.cpp:498: shader->asNewEffect(context, skPaint, &grColor, &grEffect,
NULL);
If it does succeed, it should also use the returned value of grColor in some
way.

On 2014/06/05 17:32:44, bsalomon wrote:
> On 2014/06/05 17:03:07, jvanverth1 wrote:
> > The bool return value isn't checked here. Also, in looking at it written
out,
> > getting the ref * and then adding it to a SkAutoTUnref seems a little
clunky.
> > I'm not sure if there's a better way to do this -- Brian?
> 
> I can't think of a better way other than sending down a SkAutoTUnref* but I'm
> not sure I like that.
> 
> We should probably fall back to solid color painting if false is returned...
> e.g. copy SkPaint's color to GrPaint.

[dandov, 2014-06-06 21:50:44]

I ended up implementing the asNewEffect in SkColorShader to address some of the
comments.

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:377: *  If the shader subclass has a GrEffect
implementation, this resturns the effect to install.
On 2014/06/05 17:32:44, bsalomon wrote:
> These comments need updating.

Done.

https://codereview.chromium.org/318923005/diff/20001/include/core/SkShader.h#...
include/core/SkShader.h:383: virtual bool asNewEffect(GrContext* context, const
SkPaint& paint, GrColor* grColor,
On 2014/06/05 17:03:07, jvanverth1 wrote:
> I'd probably put grColor and grEffect at the end of the parameter list.
> Otherwise you're mixing inputs and outputs. Also, you should change the
comment
> to reflect the new parameter list. E.g. a return value of 'false' means that
it
> did not create a ref or set a color.

Done.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:386: GrEffectRef** grEffect, const SkMatrix*
localMatrix) const {
On 2014/06/05 17:32:44, bsalomon wrote:
> Doesn't this function need to set grColor?

Done.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:386: GrEffectRef** grEffect, const SkMatrix*
localMatrix) const {
On 2014/06/05 17:36:30, jvanverth1 wrote:
> grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:386: GrEffectRef** grEffect, const SkMatrix*
localMatrix) const {
The grColor is set the alpha of the SkPaint's alpha

On 2014/06/05 17:36:30, jvanverth1 wrote:
> grColor never gets set by this method

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:456: *grEffect = NULL;
On 2014/06/05 17:32:44, bsalomon wrote:
> I don't think we should require this. The passed in ptr should already point
to
> NULL.

Done.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.h:33: #if SK_SUPPORT_GPU
I didn't change this because I wanted to first test if everything worked fine
after fixing the hack. 

On 2014/06/05 17:36:31, jvanverth1 wrote:
> Given that the base class asNewEffect() definition isn't protected by this
#if,
> you can probably remove it, and similar ones.

https://codereview.chromium.org/318923005/diff/20001/src/core/SkPictureShader.h
File src/core/SkPictureShader.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/core/SkPictureShader...
src/core/SkPictureShader.h:34: const SK_OVERRIDE;
On 2014/06/05 17:36:31, jvanverth1 wrote:
> indent

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:1285: const {
On 2014/06/05 17:36:31, jvanverth1 wrote:
> indent 'const {', and grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkLinearGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkLinearGradient.cpp:554: GrEffectRef** grEffect, const
SkMatrix* localMatrix)  const {
On 2014/06/05 17:36:31, jvanverth1 wrote:
> grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkRadialGradient.h (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkRadialGradient.h:37: const SK_OVERRIDE;
On 2014/06/05 17:03:07, jvanverth1 wrote:
> Indent.

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkSweepGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkSweepGradient.cpp:286: GrEffectRef** grEffect, const
SkMatrix* localMatrix)  const {
On 2014/06/05 17:36:31, jvanverth1 wrote:
> grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointConicalGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointConicalGradient.cpp:385: const SkMatrix*
localMatrix)  const {
On 2014/06/05 17:36:31, jvanverth1 wrote:
> grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
File src/effects/gradients/SkTwoPointRadialGradient.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/effects/gradients/Sk...
src/effects/gradients/SkTwoPointRadialGradient.cpp:678: const SkMatrix*
localMatrix)  const {
On 2014/06/05 17:36:31, jvanverth1 wrote:
> grColor never gets set by this method

Done.

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode433
src/gpu/SkGr.cpp:433: if (justAlpha) {
I removed this by modifying the arguments that this method receives, instead of
a bool justAlpha now it gets a GrColor that is the one that is set for the
GrPaint.
The caller is in charge of creating and supplying the color.

On 2014/06/05 17:32:44, bsalomon wrote:
> Can we kill this?

https://codereview.chromium.org/318923005/diff/20001/src/gpu/SkGr.cpp#newcode498
src/gpu/SkGr.cpp:498: shader->asNewEffect(context, skPaint, &grColor, &grEffect,
NULL);
When the call to asNewEffect returns true (if nothing wrong happens and if
grEffect is not NULL) it adds the effect to the GrPaint. The call to asNewEffect
also modifies the value so now we only need to setup the rest of the paint. 
Each shader determines the GrColor it needs and if asNewEffect is not
implemented it is just set to the SkPaint's color. 

On 2014/06/05 17:36:31, jvanverth1 wrote:
> If it does succeed, it should also use the returned value of grColor in some
> way.
> 
> On 2014/06/05 17:32:44, bsalomon wrote:
> > On 2014/06/05 17:03:07, jvanverth1 wrote:
> > > The bool return value isn't checked here. Also, in looking at it written
> out,
> > > getting the ref * and then adding it to a SkAutoTUnref seems a little
> clunky.
> > > I'm not sure if there's a better way to do this -- Brian?
> > 
> > I can't think of a better way other than sending down a SkAutoTUnref* but
I'm
> > not sure I like that.
> > 
> > We should probably fall back to solid color painting if false is returned...
> > e.g. copy SkPaint's color to GrPaint.
>

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h#...
include/core/SkShader.h:471: GrColor getColorAsAlpha(SkPaint paint) const;
function to get the SkPaint's alpha in the 4 channels of a GrColor

https://codereview.chromium.org/318923005/diff/30001/include/effects/SkPerlin...
File include/effects/SkPerlinNoiseShader.h (right):

https://codereview.chromium.org/318923005/diff/30001/include/effects/SkPerlin...
include/effects/SkPerlinNoiseShader.h:99: GrEffectRef**) const SK_OVERRIDE;
Change method's signature to have the output parameters at the end.

https://codereview.chromium.org/318923005/diff/30001/include/gpu/SkGr.h
File include/gpu/SkGr.h (right):

https://codereview.chromium.org/318923005/diff/30001/include/gpu/SkGr.h#newco...
include/gpu/SkGr.h:73: }
function to get the alpha value of a SkColor in all the channels of a GrColor

https://codereview.chromium.org/318923005/diff/30001/include/gpu/SkGr.h#newco...
include/gpu/SkGr.h:91: void SkPaint2GrPaintNoShader(GrContext* context, const
SkPaint& skPaint, GrColor grColor,
Change method's signature, now it receives the GrColor that is going to be set
in the GrPaint and the caller must create and provide this value.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:466: return NULL != *grEffect;
check if it will return NULL or a valid GrEffectRef*

https://codereview.chromium.org/318923005/diff/30001/src/core/SkLocalMatrixSh...
File src/core/SkLocalMatrixShader.h (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkLocalMatrixSh...
src/core/SkLocalMatrixShader.h:41: return fProxyShader->asNewEffect(context,
paint, &tmp, grColor, grEffect);
return the value of the proxy shader call to asNewEffect

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:216: *grColor = SkColor2GrColor(paint.getColor());
default grColor value is the paint's color

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:355: GrEffectRef** grEffect) const {
Use the SkColorShader's color as the grColor, modulated by the SkPaint's alpha.

https://codereview.chromium.org/318923005/diff/30001/src/effects/SkPerlinNois...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/effects/SkPerlinNois...
src/effects/SkPerlinNoiseShader.cpp:1350: return false;
I don't if here I should call SkShader::asNewEffect or set the default values of
grColor and grEffect to comply with the function behavior.

[bsalomon, 2014-06-09 14:11:31]

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h#...
include/core/SkShader.h:379: *  It always sets the value of grColor, the default
is the paint color.
It really always sets the color? Seems like the caller could set the skpaint
color on gr paint and then only override it if this returns true. Failing and
changing an out param feels weird to me.

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h#...
include/core/SkShader.h:471: GrColor getColorAsAlpha(SkPaint paint) const;
On 2014/06/06 21:50:44, dandov wrote:
> function to get the SkPaint's alpha in the 4 channels of a GrColor

I don't think this should be a member of SkShader. It could be an inline in a Gr
header (e.g. GrColor.h or SkGr.h)

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:388: *grColor = this->getColorAsAlpha(paint);
It can be a follow on CL but I think you'd fix an existing bug here if you made
this copy all the channels when the bitmap is alpha-only.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:215: *grEffect = NULL;
I'd feel better if this did nothing other than return false. It seems like a
cleaner contract for the caller to be able to assume that nothing was
initialized if the function fails.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:216: *grColor = SkColor2GrColor(paint.getColor());
On 2014/06/06 21:50:44, dandov wrote:
> default grColor value is the paint's color

I think this default should be implemented by the calling function.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:220: GrColor SkShader::getColorAsAlpha(SkPaint paint)
const {
Having this as a function doesn't seem to add much (and could add cost to a
single operation).

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:360: return false;
This should return true since it succeeded. Otherwise why have a return value?

[dandov, 2014-06-09 19:10:55]

Finished changing the contract of SkShader::asNewEffect and fixed bug in
SkBitmapProcShader::asNewEffect

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h#...
include/core/SkShader.h:379: *  It always sets the value of grColor, the default
is the paint color.
Change contract of function, returns false if it fails at some point or if there
is no implementation of this method in the shader subclass. The caller is in
charge of defining what to do with the effect and grcolor if it fails.

On 2014/06/09 14:11:30, bsalomon wrote:
> It really always sets the color? Seems like the caller could set the skpaint
> color on gr paint and then only override it if this returns true. Failing and
> changing an out param feels weird to me.

https://codereview.chromium.org/318923005/diff/30001/include/core/SkShader.h#...
include/core/SkShader.h:471: GrColor getColorAsAlpha(SkPaint paint) const;
On 2014/06/09 14:11:30, bsalomon wrote:
> On 2014/06/06 21:50:44, dandov wrote:
> > function to get the SkPaint's alpha in the 4 channels of a GrColor
> 
> I don't think this should be a member of SkShader. It could be an inline in a
Gr
> header (e.g. GrColor.h or SkGr.h)

Done.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:388: *grColor = this->getColorAsAlpha(paint);
I fixed the bug, do I need to do something else to relate this issue with that
bug too?

On 2014/06/09 14:11:30, bsalomon wrote:
> It can be a follow on CL but I think you'd fix an existing bug here if you
made
> this copy all the channels when the bitmap is alpha-only.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:215: *grEffect = NULL;
On 2014/06/09 14:11:30, bsalomon wrote:
> I'd feel better if this did nothing other than return false. It seems like a
> cleaner contract for the caller to be able to assume that nothing was
> initialized if the function fails.

Done.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:220: GrColor SkShader::getColorAsAlpha(SkPaint paint)
const {
On 2014/06/09 14:11:31, bsalomon wrote:
> Having this as a function doesn't seem to add much (and could add cost to a
> single operation).

Done.

https://codereview.chromium.org/318923005/diff/30001/src/core/SkShader.cpp#ne...
src/core/SkShader.cpp:360: return false;
On 2014/06/09 14:11:31, bsalomon wrote:
> This should return true since it succeeded. Otherwise why have a return value?

Done.

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
File expectations/gm/ignored-tests.txt (right):

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
expectations/gm/ignored-tests.txt:54: # dandov: Fix for bitmap shader by taking
into account if the bitmap is alpha only
The new image is correct.

https://codereview.chromium.org/318923005/diff/50001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/318923005/diff/50001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:457: *grColor = (kAlpha_8_SkColorType ==
fRawBitmap.colorType()) ? SkColor2GrColor(paint.getColor())
Fix for the bitmap shader bug

https://codereview.chromium.org/318923005/diff/50001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:472: bool
SkBitmapProcShader::asNewEffect(GrContext* context, const SkPaint& paint,
Added to have the same implementation as in Gradient shaders and to follow the
new function's contract.

https://codereview.chromium.org/318923005/diff/50001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.h (right):

https://codereview.chromium.org/318923005/diff/50001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.h:33: 
Remove this because of a previous comment from Jim, I added this macro to the
.cpp files where it was needed to have the same behavior as in Gradient shaders
where skdebugfail is called and returns false when the function is called in a
gpu-less environment.

[bsalomon, 2014-06-09 19:23:15]

lgtm

[dandov, 2014-06-10 13:02:10]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 13:02:53]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/50001

[commit-bot: I haz the power, 2014-06-10 13:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 13:03:00]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:51
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
1a6ce8d22fd1fb5c3ca9b58eb15e27d0dc095dfb..f229c342b7e9c0fafe73c292210de41f03b6b162
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -51,3 +51,6 @@ tilemode_bitmap
 tilemodes_npot
 tilemodes
 
+# dandov: Fix for bitmap shader by taking into account if the bitmap is alpha
only
+# https://codereview.chromium.org/318923005/
+bitmapshaders_gpu

[bsalomon, 2014-06-10 13:24:10]

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
File expectations/gm/ignored-tests.txt (right):

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
expectations/gm/ignored-tests.txt:56: bitmapshaders_gpu
Late comment: Is the test named bitmapshaders or bitmapshaders_gpu? The files
that GM writes out get the config name appended to them (e.g. "_gpu" or "_8888")
but this file expects just the test name.

[dandov, 2014-06-10 13:30:05]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 13:30:13]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/70001

[commit-bot: I haz the power, 2014-06-10 13:30:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 13:30:18]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:51
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
1a6ce8d22fd1fb5c3ca9b58eb15e27d0dc095dfb..fa6892ab9c1f8bbbafa8fcc76f6d1faf253ac25c
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -51,3 +51,6 @@ tilemode_bitmap
 tilemodes_npot
 tilemodes
 
+# dandov: Fix for bitmap shader by taking into account if the bitmap is alpha
only
+# https://codereview.chromium.org/318923005/
+bitmapshaders

[dandov, 2014-06-10 13:49:33]

change name of ignored test

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
File expectations/gm/ignored-tests.txt (right):

https://codereview.chromium.org/318923005/diff/50001/expectations/gm/ignored-...
expectations/gm/ignored-tests.txt:56: bitmapshaders_gpu
On 2014/06/10 13:24:10, bsalomon wrote:
> Late comment: Is the test named bitmapshaders or bitmapshaders_gpu? The files
> that GM writes out get the config name appended to them (e.g. "_gpu" or
"_8888")
> but this file expects just the test name.

Done.

[dandov, 2014-06-10 13:51:13]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 13:52:23]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/70001

[commit-bot: I haz the power, 2014-06-10 13:52:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 13:52:31]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:51
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
1a6ce8d22fd1fb5c3ca9b58eb15e27d0dc095dfb..fa6892ab9c1f8bbbafa8fcc76f6d1faf253ac25c
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -51,3 +51,6 @@ tilemode_bitmap
 tilemodes_npot
 tilemodes
 
+# dandov: Fix for bitmap shader by taking into account if the bitmap is alpha
only
+# https://codereview.chromium.org/318923005/
+bitmapshaders

[dandov, 2014-06-10 13:59:11]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 13:59:23]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/90001

[dandov, 2014-06-10 14:36:43]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 14:37:31]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/110001

[commit-bot: I haz the power, 2014-06-10 16:02:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.chromium
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[commit-bot: I haz the power, 2014-06-10 16:50:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 16:50:15]

Try jobs failed on following builders:
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.chromium
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[dandov, 2014-06-10 19:05:58]

The CQ bit was checked by dandov@google.com

[commit-bot: I haz the power, 2014-06-10 19:06:35]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dandov@google.com/318923005/130001

[commit-bot: I haz the power, 2014-06-10 21:38:32]

Change committed as 9de5b514d38c5b36066bcdc14fba2f7e5196d372