Make GrGLShaderBuilder check whether GrEffect advertised that it would require the dst color or frag

Make GrGLShaderBuilder check whether GrEffect advertised that it would require the dst color or fragment position

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

[bsalomon, 2013-05-08 21:14:51]

Next step towards making shaders that read fragment position be keyed on RT
origin. Now effects have to state up front that they will read the frag
position.

Built on top of https://codereview.chromium.org/14925010/

[Stephen White, 2013-05-09 01:33:46]

LGTM

https://codereview.chromium.org/14998007/diff/2001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/14998007/diff/2001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:236: GrPrintf("GrGLEffect asked for dst color
but its generating GrEffect "
Maybe this should assert instead of printing?  It seems like a coding error that
should be caught.

https://codereview.chromium.org/14998007/diff/2001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:471: GrPrintf("GrGLEffect asked for frag
position but its generating GrEffect "
Same here.

[robertphillips, 2013-05-09 13:16:11]

lgtm

https://codereview.chromium.org/14998007/diff/2001/src/gpu/gl/GrGLShaderBuild...
File src/gpu/gl/GrGLShaderBuilder.cpp (right):

https://codereview.chromium.org/14998007/diff/2001/src/gpu/gl/GrGLShaderBuild...
src/gpu/gl/GrGLShaderBuilder.cpp:237: "did not request access.");
Is there something better we could do in this case then return NULL?

[bsalomon, 2013-05-09 13:32:36]

The new version asserts in the debug build, but always prints, and returns ""
rather than NULL.

[Stephen White, 2013-05-09 13:55:06]

On 2013/05/09 13:32:36, bsalomon wrote:
> The new version asserts in the debug build, but always prints, and returns ""
> rather than NULL.

It might be better to simply GrCrash() in all builds. The Chrome philosophy is
that it's better to crash early and deterministically (which will get found and
fixed) than to limp along and fail in unpredictable ways later.

[bsalomon, 2013-05-09 13:57:34]

On 2013/05/09 13:55:06, Stephen White wrote:
> On 2013/05/09 13:32:36, bsalomon wrote:
> > The new version asserts in the debug build, but always prints, and returns
""
> > rather than NULL.
> 
> It might be better to simply GrCrash() in all builds. The Chrome philosophy is
> that it's better to crash early and deterministically (which will get found
and
> fixed) than to limp along and fail in unpredictable ways later.

Hmmm... we would just skip the draw when the shader fails to compile.

[Stephen White, 2013-05-09 14:15:35]

On 2013/05/09 13:57:34, bsalomon wrote:
> On 2013/05/09 13:55:06, Stephen White wrote:
> > On 2013/05/09 13:32:36, bsalomon wrote:
> > > The new version asserts in the debug build, but always prints, and returns
> ""
> > > rather than NULL.
> > 
> > It might be better to simply GrCrash() in all builds. The Chrome philosophy
is
> > that it's better to crash early and deterministically (which will get found
> and
> > fixed) than to limp along and fail in unpredictable ways later.
> 
> Hmmm... we would just skip the draw when the shader fails to compile.

That's a lot more subtle to track down than a crash with a stack, though. :)

Anyway it's totally up to you, I'm fine with this patch either way.

[commit-bot: I haz the power, 2013-05-09 14:42:01]

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

[commit-bot: I haz the power, 2013-05-09 14:55:49]

Change committed as 9074