Remove GrDrawState::setTexture/getTexture

[TomH, 2012-07-27 21:50:06]

LGTM.

<curmudgeon hat> Please keep widespread formatting changes separate from
functionality changes </curmudgeon hat>

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:748: GrAssert(NULL !=
this->getDrawState().getRenderTarget());
Nit: you could get replace this->getDrawState() with drawState here, too

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:774: const GrCustomStage* stage =
drawState.getSampler(s).getCustomStage();
Nit: could we GrAssert(NULL != stage) here? Or is that just too redundant after
checking isStageEnabled()?

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:859: // are opaque. Therefore, we can tell isOpaque
that the incoming color is opaque.
This comment is unclear, since what we're telling somebody is that we aren't
opaque???

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:710: if (NULL != texture) {
Nit: we end up 7 or 8 indents deep here. Is there anything we can move into
helper functions to make the convoluted logic clearer?

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:711: // we matrix to invert when orientation is
TopDown. So make sure we aren't in that
Nit: grammar: first sentence is incomprehensible. Also, comment looks like a lie
- what does 'TextureMatrixIsIdentity()' have to do with TopDown? Can we rename
the former or reword the comment? Perhaps "Make sure we're really identity - for
example, that the matrix orientation isn't inverted."

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:734: // The shader generator assumes that color
channels are bytes when rounding.
Nit: Not clear what this comment is trying to tell me.

[bsalomon, 2012-07-30 13:08:50]

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp
File src/gpu/GrDrawTarget.cpp (right):

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:748: GrAssert(NULL !=
this->getDrawState().getRenderTarget());
On 2012/07/27 21:50:07, TomH wrote:
> Nit: you could get replace this->getDrawState() with drawState here, too

Done.

http://codereview.appspot.com/6455051/diff/2001/src/gpu/GrDrawTarget.cpp#newc...
src/gpu/GrDrawTarget.cpp:859: // are opaque. Therefore, we can tell isOpaque
that the incoming color is opaque.
On 2012/07/27 21:50:07, TomH wrote:
> This comment is unclear, since what we're telling somebody is that we aren't
> opaque???

I'm justifying passing true to the function.  But actually the param doesn't
mean what I think it does. It's about the incoming texture. I think it really
should be about the incoming color. The stage now knows the texture... it
doesn't know about the incoming color. If it modulates against the incoming
color, as most do, then it needs to know whether the incoming color is solid or
not.

I'll replace this with:
FIXME: The param indicates whether the texture is opaque or not. However, the
stage already controls its textures. It really needs to know whether the
incoming color (from a uni, per-vertex colors, or previous stage) is opaque or
not.

and change the passed param to false.

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cpp
File src/gpu/gl/GrGpuGL_program.cpp (right):

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:710: if (NULL != texture) {
On 2012/07/27 21:50:07, TomH wrote:
> Nit: we end up 7 or 8 indents deep here. Is there anything we can move into
> helper functions to make the convoluted logic clearer?

Probably, but this section will be rewritten soon anyway.

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:711: // we matrix to invert when orientation is
TopDown. So make sure we aren't in that
On 2012/07/27 21:50:07, TomH wrote:
> Nit: grammar: first sentence is incomprehensible. Also, comment looks like a
lie
> - what does 'TextureMatrixIsIdentity()' have to do with TopDown? Can we rename
> the former or reword the comment? Perhaps "Make sure we're really identity -
for
> example, that the matrix orientation isn't inverted."

// We call this helper function  rather then simply checking the
client-specified
// texture matrix. This is because we may have to concat a y-inversion to
account
// for texture orientation.

http://codereview.appspot.com/6455051/diff/2001/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:734: // The shader generator assumes that color
channels are bytes when rounding.
On 2012/07/27 21:50:07, TomH wrote:
> Nit: Not clear what this comment is trying to tell me.

// Assert that if we're doing a premul conversion that the texture is 1 byte
// per color component. The rounding performed by the shader generator (in
// normalized float color space) assumes this.

[TomH, 2012-07-31 15:18:47]

Commit me?!

[bsalomon, 2012-07-31 15:42:34]

Closed with r4826