Make GrClipMaskManager configure the stencil and scissor on GrGpu

BUG= http://code.google.com/p/skia/issues/detail?id=631

[bsalomon, 2012-06-20 18:31:49]

The main change is to move the responsibility of fiddling with the stencil
settings to GrClipMaskManager from GrGpuGL. The new model is GrGpu draw
functions call GrClipMaskManager::setupClipping() before calling its own
flushGraphicsState(). setupClipping() calls setters on the gpu to inform it of
the scissor and stencil settings to use for the draw. Subsequently,
flushGraphicsState(), which is implemented by the GrGpu subclass, realizes those
settings in the 3D API.

In order to simplify GrClipMaskManager::adjustStencilSettings() I made
GrStencilSettings use a Face enum to set front/back stencil settings. I left all
the old front & back accessors / setters on GrStencilSettings, but it may make
sense to find any users in the code and change them to use the versions that
take a Face.

[robertphillips, 2012-06-20 18:58:27]

Ouch - LGTM modulo nits/questions. I do like that the stencil settings are being
better handled.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:868: // but the clip is in the stencil buffer
What's up with the void parameter?

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:909: if
(GrClipMaskManager::kRespectClip_StencilClipMode == clipMode) {
Why not have an "initToBasic" member function on GrStencilSettings?

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:948: if (kModifyClip_StencilClipMode == mode) {
tothe -> to the

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h#newcode329
src/gpu/gl/GrGpuGL.h:329: }
Is there a better name for this besides "fHWBounds"?

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

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:456: this->flushColorMatrix();
It seems a bit odd here that you check the state of fScissorState and then pass
the results back to flushScissorRect

[bsalomon, 2012-06-20 19:07:02]

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:868: // but the clip is in the stencil buffer
On 2012/06/20 18:58:27, robertphillips wrote:
> What's up with the void parameter?

weird... I copied and pasted this from GrGpuGL and renamed it. I guess it was
already there for some reason. Will remove.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:909: if
(GrClipMaskManager::kRespectClip_StencilClipMode == clipMode) {
On 2012/06/20 18:58:27, robertphillips wrote:
> Why not have an "initToBasic" member function on GrStencilSettings?

Hm... not a bad idea.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h#newcode329
src/gpu/gl/GrGpuGL.h:329: }
On 2012/06/20 18:58:27, robertphillips wrote:
> Is there a better name for this besides "fHWBounds"?

I'll pull out viewport and just call this fHWScissorSettings or similar.

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

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:456: this->flushColorMatrix();
On 2012/06/20 18:58:27, robertphillips wrote:
> It seems a bit odd here that you check the state of fScissorState and then
pass
> the results back to flushScissorRect

True.. The reason I did it this way is that there are places we need to flush
the scissor to something other than fScissorState. I could make those call sites
save/overwrite/restore fScissorState and then flushScissor() wouldn't need a
param. Will try and see if it looks better.

[bsalomon, 2012-06-20 20:41:20]

Updated patch

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:868: // but the clip is in the stencil buffer
On 2012/06/20 19:07:03, bsalomon wrote:
> On 2012/06/20 18:58:27, robertphillips wrote:
> > What's up with the void parameter?
> 
> weird... I copied and pasted this from GrGpuGL and renamed it. I guess it was
> already there for some reason. Will remove.

Done.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:909: if
(GrClipMaskManager::kRespectClip_StencilClipMode == clipMode) {
On 2012/06/20 19:07:03, bsalomon wrote:
> On 2012/06/20 18:58:27, robertphillips wrote:
> > Why not have an "initToBasic" member function on GrStencilSettings?
> 
> Hm... not a bad idea.

I tried it and it felt weird. The notion of stencil settings that respect the
clip seems particular to this class.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/GrClipMaskManager.cpp...
src/gpu/GrClipMaskManager.cpp:948: if (kModifyClip_StencilClipMode == mode) {
On 2012/06/20 18:58:27, robertphillips wrote:
> tothe -> to the

Done.

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h
File src/gpu/gl/GrGpuGL.h (right):

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL.h#newcode329
src/gpu/gl/GrGpuGL.h:329: }
On 2012/06/20 19:07:03, bsalomon wrote:
> On 2012/06/20 18:58:27, robertphillips wrote:
> > Is there a better name for this besides "fHWBounds"?
> 
> I'll pull out viewport and just call this fHWScissorSettings or similar.
> 

Done.

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

http://codereview.appspot.com/6308096/diff/4012/src/gpu/gl/GrGpuGL_program.cp...
src/gpu/gl/GrGpuGL_program.cpp:456: this->flushColorMatrix();
On 2012/06/20 19:07:03, bsalomon wrote:
> On 2012/06/20 18:58:27, robertphillips wrote:
> > It seems a bit odd here that you check the state of fScissorState and then
> pass
> > the results back to flushScissorRect
> 
> True.. The reason I did it this way is that there are places we need to flush
> the scissor to something other than fScissorState. I could make those call
sites
> save/overwrite/restore fScissorState and then flushScissor() wouldn't need a
> param. Will try and see if it looks better.
> 

Done.

http://codereview.appspot.com/6308096/diff/10001/gyp/gpu.gyp
File gyp/gpu.gyp (right):

http://codereview.appspot.com/6308096/diff/10001/gyp/gpu.gyp#newcode198
gyp/gpu.gyp:198: '../include/gpu/GrTemplates.h',
This guy was missing! Prolly want to move it to src/gpu (in another CL).

http://codereview.appspot.com/6308096/diff/10001/include/gpu/GrClip.h
File include/gpu/GrClip.h (left):

http://codereview.appspot.com/6308096/diff/10001/include/gpu/GrClip.h#oldcode16
include/gpu/GrClip.h:16: #include "GrTemplates.h"
Not used here, other files were relying on this, though, so I had to #include it
elsewhere.

http://codereview.appspot.com/6308096/diff/10001/include/gpu/GrTemplates.h
File include/gpu/GrTemplates.h (right):

http://codereview.appspot.com/6308096/diff/10001/include/gpu/GrTemplates.h#ne...
include/gpu/GrTemplates.h:40: template <typename T> class GrAutoTRestore :
public GrNoncopyable {
This wasn't being used anywhere currently. I liked the functionality but not the
name. I'm using it to temporarily change the GrGpu's fScissorState before a
flushScissor() call.

[robertphillips, 2012-06-20 20:55:14]

LGTM

[bsalomon, 2012-06-21 20:14:09]

Closed with r4288