Added SkClipStack portion of new clip mask caching system

The generation ID computed and stored by the SkClipStack will be used to identify the cached clip masks in the texture cache.

[bsalomon, 2012-08-03 12:38:45]

http://codereview.appspot.com/6449089/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/6449089/diff/1/include/core/SkCanvas.h#newcode900
include/core/SkCanvas.h:900: SkClipStack* getClipStack() {
Mutable pointer to the clip stack in the public API?

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode430
src/core/SkClipStack.cpp:430: , fGenID(1) {       // gen ID of 0 is reserved for
invalid
Shouldn't they get the next global gen ID?

Would it make any sense to have predefined IDs for both wideOpen and empty?

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode481
src/core/SkClipStack.cpp:481: fGenID != b.fGenID ||
Is this right? Can't to CSs be equal but have different genIDs (constructed the
same way independently?)

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode555
src/core/SkClipStack.cpp:555: ++fGenID;
shouldn't this be based on a global using atomic inc?

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode800
src/core/SkClipStack.cpp:800: if (0 <= index) {
index >= 0?

[reed1, 2012-08-03 12:42:22]

can you summarize what is really needed? is it enough to call a device-virtual
on my save/restore calls?

http://codereview.appspot.com/6449089/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/6449089/diff/1/include/core/SkCanvas.h#newcode899
include/core/SkCanvas.h:899: 
Can we avoid making the non-const version public? (can we make all of them
private?). If you need a writable ptr, and are following the secret-handshake
rules, perhaps we can use friend to permit this?

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode66
src/core/SkClipStack.cpp:66: 
This is odd. Why is this needed (other than for the private impl of assign?)

[robertphillips, 2012-08-03 14:33:56]

Of the possible universe of bad solutions to removing the public SkClipStack
getter, here is one that abuses mutability.

The basic goal here is to give the devices a chance to purge whatever they may
have cached for a given clip state. For the SkGpuDevice this will be the alpha
clip masks. It would be possible to do this as a virtual function on device but
this would require routing the calls through the canvas to the device. The
problem is that although a restore will always invalid 1 clip state the other
invalidations occur in clipRect and clipPath but not for all calls (e.g., a
clip* call after a save doesn't invalidate the cached data for the prior clip
state). We could do it as a 'clipStateChanged' virtual that is fired on
save/restore/clip* and leave it up to the SkDevice's to figure out what to
invalidate.

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode66
src/core/SkClipStack.cpp:66: 
Done.

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode430
src/core/SkClipStack.cpp:430: , fGenID(1) {       // gen ID of 0 is reserved for
invalid
Done - although right now only the empty generation ID is set correctly.

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode481
src/core/SkClipStack.cpp:481: fGenID != b.fGenID ||
On 2012/08/03 12:38:45, bsalomon wrote:
> Is this right? Can't to CSs be equal but have different genIDs (constructed
the
> same way independently?)

Done.

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode555
src/core/SkClipStack.cpp:555: ++fGenID;
On 2012/08/03 12:38:45, bsalomon wrote:
> shouldn't this be based on a global using atomic inc?

Done.

http://codereview.appspot.com/6449089/diff/1/src/core/SkClipStack.cpp#newcode800
src/core/SkClipStack.cpp:800: if (0 <= index) {
On 2012/08/03 12:38:45, bsalomon wrote:
> index >= 0?

Done.

[bsalomon, 2012-08-03 19:07:15]

I'll let you and Mike fight out the notification system.

http://codereview.appspot.com/6449089/diff/12/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6449089/diff/12/src/core/SkClipStack.cpp#newcod...
src/core/SkClipStack.cpp:559: int32_t genID = sk_atomic_inc(&gGenID);
Since we get the next gen id in several places, it might be good to have a
static method GetNextGenID().

[robertphillips, 2012-08-06 11:19:49]

http://codereview.appspot.com/6449089/diff/12/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

http://codereview.appspot.com/6449089/diff/12/src/core/SkClipStack.cpp#newcod...
src/core/SkClipStack.cpp:559: int32_t genID = sk_atomic_inc(&gGenID);
On 2012/08/03 19:07:15, bsalomon wrote:
> Since we get the next gen id in several places, it might be good to have a
> static method GetNextGenID().

Done.

[robertphillips, 2012-08-07 14:22:38]

Brian - it appears this is now yours to approve/disapprove.

[bsalomon, 2012-08-07 14:28:28]

On 2012/08/07 14:22:38, robertphillips wrote:
> Brian - it appears this is now yours to approve/disapprove.

LGTM.

[robertphillips, 2012-08-07 15:38:31]

committed as r4978