Added asNewCustomStage to SkShader and implemented it for all the gradient shaders.

Added asNewCustomStage to SkShader and implemented it for all the gradient shaders.

Not actually hooked up yet, but it should be ready to replace asABitmap for a future CL.
Committed: https://code.google.com/p/skia/source/detail?r=4702

[reed1, 2012-07-20 17:28:26]

Just a thought. Given the huge size of SkGradientShader.cpp already, should we
move the Gpu functions into a separate file?

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode311
include/core/SkShader.h:311: */
Skia likes to pass const Foo& for const parameters, and Foo* for non-const...

[TomH, 2012-07-20 17:35:29]

Mike said:
> Just a thought. Given the huge size of SkGradientShader.cpp already, should we
move the Gpu functions into a separate file?

That's done above, unless I'm missing something?

LGTM overall.

https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
File src/gpu/effects/GrGradientEffects.cpp (right):

https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
src/gpu/effects/GrGradientEffects.cpp:62: // retrieve the gradient bitmap
Nit: this comment isn't useful?

[bsalomon, 2012-07-20 17:36:38]

Mostly LGTM.

Should we move the gradient gpu files to src/effects to be next to their CPU
cousins?

The GrCustomStage subclasses are now calling the "asA" functions that we'd
eventually like to eliminate. That said, this is a big step in the right
direction. In the future we could pass something like GradientInfo (as a
non-public struct) down to the custom effect directly. We'd still need a
mechanism to extract code-share the SkBitmap creation were asABitmap to be
removed.

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode313
include/core/SkShader.h:313: GrSamplerState& sampler) const;
Skia convention is to pass T* rather than T& for non-const params.

[reed1, 2012-07-20 17:47:56]

On 2012/07/20 17:35:29, TomH wrote:
> Mike said:
> > Just a thought. Given the huge size of SkGradientShader.cpp already, should
we
> move the Gpu functions into a separate file?
> 
> That's done above, unless I'm missing something?

I see Gr code in the main file. Can that be moved out?


> 
> LGTM overall.
> 
>
https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
> File src/gpu/effects/GrGradientEffects.cpp (right):
> 
>
https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
> src/gpu/effects/GrGradientEffects.cpp:62: // retrieve the gradient bitmap
> Nit: this comment isn't useful?

[rileya, 2012-07-20 19:34:59]

> I see Gr code in the main file. Can that be moved out?

The Gr code in SkGradientShader.cpp is limited to setting up the sampler state
parameters (matrix, tilemode, and filtering) in asNewCustomStage, it sounds like
all of that is going to eventually be moved into GrCustomStage, so I think that
can cleanly get pulled out once that is done, until then I don't see a way of
pulling that out that doesn't involve making asNewCustomStage return
tilemode/matrix/etc and making the caller apply those to the sampler (which is
what currently happens with asABitmap in SkGpuDevice).

>Should we move the gradient gpu files to src/effects to be next to their CPU
>cousins?

Sounds good, how would this work naming-wise? I think you mentioned something
like: GrGradientEffects.cpp/h -> SkGradientEffectsGpu.cpp/h? Would the contents
of those files keep their Gr/GrGL prefixes?

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h
File include/core/SkShader.h (right):

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode311
include/core/SkShader.h:311: */
On 2012/07/20 17:28:26, reed1 wrote:
> Skia likes to pass const Foo& for const parameters, and Foo* for non-const...

Fixed.

https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode313
include/core/SkShader.h:313: GrSamplerState& sampler) const;
On 2012/07/20 17:36:38, bsalomon wrote:
> Skia convention is to pass T* rather than T& for non-const params.

Fixed.

https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
File src/gpu/effects/GrGradientEffects.cpp (right):

https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
src/gpu/effects/GrGradientEffects.cpp:62: // retrieve the gradient bitmap
On 2012/07/20 17:35:29, TomH wrote:
> Nit: this comment isn't useful?

Removed.

[bsalomon, 2012-07-20 19:50:07]

On 2012/07/20 19:34:59, rileya wrote:
> > I see Gr code in the main file. Can that be moved out?
> 
> The Gr code in SkGradientShader.cpp is limited to setting up the sampler state
> parameters (matrix, tilemode, and filtering) in asNewCustomStage, it sounds
like
> all of that is going to eventually be moved into GrCustomStage, so I think
that
> can cleanly get pulled out once that is done, until then I don't see a way of
> pulling that out that doesn't involve making asNewCustomStage return
> tilemode/matrix/etc and making the caller apply those to the sampler (which is
> what currently happens with asABitmap in SkGpuDevice).

Once all that junk is setup by custom effect it seems like all the
asANewCustomStage funcs will be one-liners. Given that they represent a pretty
small fraction of the overall file length already, it seems overkill to pull
those defs into another file. If we did pull the custom effect definitions into
a cpp in src/effects/ then these virtuals could be defined in those cpps.
However, ...


> 
> >Should we move the gradient gpu files to src/effects to be next to their CPU
> >cousins?
> 
> Sounds good, how would this work naming-wise? I think you mentioned something
> like: GrGradientEffects.cpp/h -> SkGradientEffectsGpu.cpp/h? Would the
contents
> of those files keep their Gr/GrGL prefixes?

... I'm not sure. Let's have a quick discussion about this on Monday when
everyone is back in the office. We'll need a consistent policy on this going
forward.

> 
> https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h
> File include/core/SkShader.h (right):
> 
>
https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode311
> include/core/SkShader.h:311: */
> On 2012/07/20 17:28:26, reed1 wrote:
> > Skia likes to pass const Foo& for const parameters, and Foo* for
non-const...
> 
> Fixed.
> 
>
https://codereview.appspot.com/6351113/diff/1/include/core/SkShader.h#newcode313
> include/core/SkShader.h:313: GrSamplerState& sampler) const;
> On 2012/07/20 17:36:38, bsalomon wrote:
> > Skia convention is to pass T* rather than T& for non-const params.
> 
> Fixed.
> 
>
https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
> File src/gpu/effects/GrGradientEffects.cpp (right):
> 
>
https://codereview.appspot.com/6351113/diff/1/src/gpu/effects/GrGradientEffec...
> src/gpu/effects/GrGradientEffects.cpp:62: // retrieve the gradient bitmap
> On 2012/07/20 17:35:29, TomH wrote:
> > Nit: this comment isn't useful?
> 
> Removed.