Split GrContext's AA Rect drawing methods into helper class

Simple refactoring to split these into their own file in preparation for reuse by the AA Clip mask code.

[bsalomon, 2012-06-08 13:22:16]

Could we make the name a little more specific than "Helper" (e.g.
GrAARectRenderer)?

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrAARectHelper.h
File include/gpu/GrAARectHelper.h (right):

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrAARectHelper.h#...
include/gpu/GrAARectHelper.h:45: protected:
can we delete this?

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrContext.h
File include/gpu/GrContext.h (left):

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrContext.h#oldco...
include/gpu/GrContext.h:713: const GrRect& devRect,
w00t, I like that we're deleting these functions.

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

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode84
src/gpu/GrContext.cpp:84: fAARectHelper.cleanUp();
Should we allocate this on the heap and delete it (and hide cleanUp())? Should
it be ref counted so that the clip mask manager can own a ref to it?

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode767
src/gpu/GrContext.cpp:767: strokeSize, useVertexCoverage);
Not for this CL, but it'd be nice if we could hide the useVertexCoverage
decision in the class. I believe we may be able to just use it all the time. (We
used to not have a coverage vertex attribute)

A common pattern in Skia is to say strokeWidth < 0 means fill. Perhaps the
helper class should just have a draw() or drawRect() method that takes a width
with this semantic rather than two functions.

[robertphillips, 2012-06-11 18:34:40]

This version renames the class as suggested and makes it ref countable.

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrAARectHelper.h
File include/gpu/GrAARectHelper.h (right):

http://codereview.appspot.com/6300070/diff/2001/include/gpu/GrAARectHelper.h#...
include/gpu/GrAARectHelper.h:45: protected:
On 2012/06/08 13:22:16, bsalomon wrote:
> can we delete this?

Done.

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

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode84
src/gpu/GrContext.cpp:84: fAARectHelper.cleanUp();
Done - cleanUp still needs to be visible for contextDestroyed call.

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode767
src/gpu/GrContext.cpp:767: strokeSize, useVertexCoverage);
I would like to hold off on fusing the two functions until I see how its usage
plays out. If we don't need to suck any more functionality into the class that
will make sense but if we have to move more of drawRect's logic in here the
problem may resolve itself.

I have added TODOs for both of these items in GrAARectRenderer.h so they don't
get lost/forgotten.

[bsalomon, 2012-06-11 19:12:32]

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

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode84
src/gpu/GrContext.cpp:84: fAARectHelper.cleanUp();
On 2012/06/11 18:34:40, robertphillips wrote:
> Done - cleanUp still needs to be visible for contextDestroyed call.

It's good that you caught this case. In general the lost context functionality
is little tested and recovery surely doesn't work correctly in many instances.
Chrome may call this to prevent Gr from making GL calls after the context has
gone away but Chrome does not continue to draw through the GrContext afterwards.

Can we do one of these three:

1) Rename cleanUp() to reset(), releaseAll(), freeAll(), freeGpuResources(), etc
(our naming convention for this type of freeing behavior is already very loose
and I'd rather not add cleanup into the mix)

2) Instead of calling a function, delete the object on lost context and recreate
lazily the helper on drawRect.

or

3) Make the helper check if its buffers are valid before using them and recreate
if necessary. (GrResource has a unfortunately rarely-called isValid() method).

3) Might be the best option and we are missing these checks in other places
(path renderer created resources, quad index buffer, ..). In general, it seems
preferable to keep the checks with the users rather than making everyone who has
a long lived resource make a reset call here. However, I think we should also
release the buffers in GrContext::freeGpuResources() which would argue for 1) or
2). So my take would be let's do 1) or 2). Going forward we can consider a
comprehensive strategy for handling lost context's better (as this becomes
important).

[robertphillips, 2012-06-11 19:52:32]

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

http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode84
src/gpu/GrContext.cpp:84: fAARectHelper.cleanUp();
I have renamed the function to reset. I don't think #2 will work once we share
this object b.c. the best we could do here would be an unref but that would
leave the ClipMaskManager w/ bogus ptrs.

[bsalomon, 2012-06-11 19:59:43]

Let's call reset() from freeGpuResources, then LGTM

On 2012/06/11 19:52:32, robertphillips wrote:
> http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
http://codereview.appspot.com/6300070/diff/2001/src/gpu/GrContext.cpp#newcode84
> src/gpu/GrContext.cpp:84: fAARectHelper.cleanUp();
> I have renamed the function to reset. I don't think #2 will work once we share
> this object b.c. the best we could do here would be an unref but that would
> leave the ClipMaskManager w/ bogus ptrs.

[robertphillips, 2012-06-12 00:33:09]

committed as r4233