Make GrDrawState a real class with getters and setters

[bsalomon, 2011-12-06 19:41:49]

http://codereview.appspot.com/5448119/diff/3001/src/gpu/GrGpuGLShaders.cpp
File src/gpu/GrGpuGLShaders.cpp (right):

http://codereview.appspot.com/5448119/diff/3001/src/gpu/GrGpuGLShaders.cpp#ne...
src/gpu/GrGpuGLShaders.cpp:413: const GrMatrix* hwViewMatrix;
This function used to unconditionally set the view matrix. The logic to check
for the same value was in the caller. I had to rewrite that code anyway and
other similar GrGpuGLShaders::flush* functions do the checks.

[TomH, 2011-12-07 22:30:24]

LGTM

http://codereview.appspot.com/5448119/diff/5001/src/gpu/GrDrawTarget.h
File src/gpu/GrDrawTarget.h (right):

http://codereview.appspot.com/5448119/diff/5001/src/gpu/GrDrawTarget.h#newcod...
src/gpu/GrDrawTarget.h:165: // IN A COMING REVISION.
Yes it will.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode813
src/gpu/GrContext.cpp:813: record->fScale * GR_Scalar1);
transM isn't being used any more - remove?

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1333: #if GR_STATIC_RECT_VB
I know it's not part of this CL, but should we _ever_ indent preprocessor
conditionals?

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1940: drawState->setColor(paint.fColor);
Smells bad...

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode63
src/gpu/GrDrawState.h:63: // default stencil setting should be disabled
Unnecessary comment?

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode69
src/gpu/GrDrawState.h:69: // Color
It's a little uglier, but if you make this
 ///@name Color
and then terminate the block with
 ///@}
Doxygen will pick up on it. This isn't public API, though, so it isn't as
important.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode82
src/gpu/GrDrawState.h:82: */
NIT: This is one of those places where I'm going to appear inconsistent - I like
Doxygen, but I don't think I like this comment. It doesn't add anything beyond
the name of the function. Those for the other documented Color functions are
important - but you did realize that getColorFilterMode() didn't need anything.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode468
src/gpu/GrDrawState.h:468: *                    equations in Ax + By + C form
3 * 6 float values doesn't tell me enough as a user to understand the data
format... and those look like magic numbers? Reading the code it looks like it
ought to be 3 * numEdges?

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrGpuGL.cpp
File src/gpu/GrGpuGL.cpp (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrGpuGL.cpp#newcode2186
src/gpu/GrGpuGL.cpp:2186: fHWDrawState.copyStateFlags(this->getDrawState());
The comment above doesn't adequately explain this line for me.

[bsalomon, 2011-12-08 14:29:22]

Diff of patchsets 5 and 7 will show changes related to comments.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode813
src/gpu/GrContext.cpp:813: record->fScale * GR_Scalar1);
On 2011/12/07 22:30:24, TomH wrote:
> transM isn't being used any more - remove?

Done.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1333: #if GR_STATIC_RECT_VB
On 2011/12/07 22:30:24, TomH wrote:
> I know it's not part of this CL, but should we _ever_ indent preprocessor
> conditionals?
This probably shouldn't be indented. I think our rule is that we indent nested
preproc conditionals separately from code indentation.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrContext.cpp#newcode...
src/gpu/GrContext.cpp:1940: drawState->setColor(paint.fColor);
On 2011/12/07 22:30:24, TomH wrote:
> Smells bad...

Meant to remove the target->setColor.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h
File src/gpu/GrDrawState.h (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode63
src/gpu/GrDrawState.h:63: // default stencil setting should be disabled
On 2011/12/07 22:30:24, TomH wrote:
> Unnecessary comment?

Done.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode69
src/gpu/GrDrawState.h:69: // Color
On 2011/12/07 22:30:24, TomH wrote:
> It's a little uglier, but if you make this
>  ///@name Color
> and then terminate the block with
>  ///@}
> Doxygen will pick up on it. This isn't public API, though, so it isn't as
> important.

Done.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode82
src/gpu/GrDrawState.h:82: */
On 2011/12/07 22:30:24, TomH wrote:
> NIT: This is one of those places where I'm going to appear inconsistent - I
like
> Doxygen, but I don't think I like this comment. It doesn't add anything beyond
> the name of the function. Those for the other documented Color functions are
> important - but you did realize that getColorFilterMode() didn't need
anything.

Done.

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode468
src/gpu/GrDrawState.h:468: *                    equations in Ax + By + C form
On 2011/12/07 22:30:24, TomH wrote:
> 3 * 6 float values doesn't tell me enough as a user to understand the data
> format... and those look like magic numbers? Reading the code it looks like it
> ought to be 3 * numEdges?

Most of this header is copy->paste from GrDrawTarget.h. "6 * 3" seems odd and
wrong. I changed it to 3 * numEdges

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrGpuGL.cpp
File src/gpu/GrGpuGL.cpp (right):

http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrGpuGL.cpp#newcode2186
src/gpu/GrGpuGL.cpp:2186: fHWDrawState.copyStateFlags(this->getDrawState());
On 2011/12/07 22:30:24, TomH wrote:
> The comment above doesn't adequately explain this line for me.

Done.

[TomH, 2011-12-08 15:46:32]

LGTM

On 2011/12/08 14:29:22, bsalomon wrote:
http://codereview.appspot.com/5448119/diff/7001/src/gpu/GrDrawState.h#newcode69
> src/gpu/GrDrawState.h:69: // Color
> On 2011/12/07 22:30:24, TomH wrote:
> > It's a little uglier, but if you make this
> >  ///@name Color
> > and then terminate the block with
> >  ///@}
> > Doxygen will pick up on it. This isn't public API, though, so it isn't as
> > important.
> 
> Done.

NIT: doxygen *only* processes lines that start with /// or /**. So //@{ or
//@name won't trigger.

Tom

[bsalomon, 2011-12-09 16:17:14]

Landed, reverted, relanded at r2841/2