Adding quickContains API method to SkClipStack

Adding quickContains API method to SkClipStack

BUG=http://code.google.com/p/chromium/issues/detail?id=164580
TEST=unit test ClipStack/quickContains
Committed: https://code.google.com/p/skia/source/detail?r=6760

[junov1, 2012-12-10 20:09:16]

PTAL.

Brian, Robert, I think this might intersect with stuff you are doing on the
ganesh side?

I need this in skia core to fix a problem with deferred canvas.

[bsalomon, 2012-12-11 17:19:12]

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

https://codereview.appspot.com/6919044/diff/1/src/core/SkClipStack.cpp#newcod...
src/core/SkClipStack.cpp:502: SkDeque::F2BIter iter(fDeque);
Might be clearer to use the SkClipStack::Iter. It makes us less dependent on the
deque implementation and is less confusing about whether you're walking from the
top or bottom of the stack.

If you hit a replace you can treat it as an intersect but then stop and return
true if it is contained.

[robertphillips, 2012-12-11 20:48:56]

LGTM + comment suggestions & a question

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

https://codereview.appspot.com/6919044/diff/1/src/core/SkClipStack.cpp#newcod...
src/core/SkClipStack.cpp:508: if (element->isInverseFilled()) {
// Part of 'rect' could be trimmed off by the inverse-filled clip element

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp
File tests/ClipStackTest.cpp (right):

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode506
tests/ClipStackTest.cpp:506: 
Something in me wants to make arrays of SkRects and SkPaths and convert the
following tests into loops. Feel free to ignore.

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode522
tests/ClipStackTest.cpp:522: 
Intersect

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode547
tests/ClipStackTest.cpp:547: 
Intersect

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode572
tests/ClipStackTest.cpp:572: 
Intersect

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode609
tests/ClipStackTest.cpp:609: 
Intersect

https://codereview.appspot.com/6919044/diff/1/tests/ClipStackTest.cpp#newcode922
tests/ClipStackTest.cpp:922: #endif
shouldn't we be calling test_quickContains here?

[junov1, 2012-12-11 21:10:57]

Patch.

[bsalomon, 2012-12-11 22:47:59]

https://codereview.appspot.com/6919044/diff/9001/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

https://codereview.appspot.com/6919044/diff/9001/src/core/SkClipStack.cpp#new...
src/core/SkClipStack.cpp:509: if (SkRect::Intersects(element->getBounds(),
rect))
style nit: {}'s here a few more places below.

https://codereview.appspot.com/6919044/diff/9001/src/core/SkClipStack.cpp#new...
src/core/SkClipStack.cpp:517: element = iter.next();
If I'm not mistaken this should be iter.prev(). Regardless of how it is
initialized next moves towards the top of the stack.

[junov1, 2012-12-12 15:54:30]

Patch.

I beefed-up unit testing to catch problems related to clip element traversal.

[bsalomon, 2012-12-12 16:17:18]

https://codereview.appspot.com/6919044/diff/12001/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

https://codereview.appspot.com/6919044/diff/12001/src/core/SkClipStack.cpp#ne...
src/core/SkClipStack.cpp:503: const Element* element = iter.prev();
Hm... I think this is supposed to be "iter.get()" or you'll miss the top-most
element. Sorry for not catching this the first time!

[junov1, 2012-12-12 16:44:09]

On 2012/12/12 16:17:18, bsalomon wrote:
> https://codereview.appspot.com/6919044/diff/12001/src/core/SkClipStack.cpp
> File src/core/SkClipStack.cpp (right):
> 
>
https://codereview.appspot.com/6919044/diff/12001/src/core/SkClipStack.cpp#ne...
> src/core/SkClipStack.cpp:503: const Element* element = iter.prev();
> Hm... I think this is supposed to be "iter.get()" or you'll miss the top-most
> element. Sorry for not catching this the first time!

Nope. If you do that, you will process the top-most element twice.  This
iterator returns the element that was current before updating its position.

[junov1, 2012-12-12 16:44:44]

FYI:

void* SkDeque::Iter::prev() {
    char* pos = fPos;

    if (pos) {   // if we were valid, try to move to the prior setting
        char* prev = pos - fElemSize;
        SkASSERT(prev >= fCurBlock->fBegin - fElemSize);
        if (prev < fCurBlock->fBegin) { // exhausted this chunk, move to prior
            do {
                fCurBlock = fCurBlock->fPrev;
            } while (fCurBlock != NULL && fCurBlock->fEnd == NULL);
            prev = fCurBlock ? fCurBlock->fEnd - fElemSize : NULL;
        }
        fPos = prev;
    }
    return pos;
}

[bsalomon, 2012-12-12 16:49:17]

On 2012/12/12 16:44:44, junov1 wrote:
> FYI:
> 
> void* SkDeque::Iter::prev() {
>     char* pos = fPos;
> 
>     if (pos) {   // if we were valid, try to move to the prior setting
>         char* prev = pos - fElemSize;
>         SkASSERT(prev >= fCurBlock->fBegin - fElemSize);
>         if (prev < fCurBlock->fBegin) { // exhausted this chunk, move to prior
>             do {
>                 fCurBlock = fCurBlock->fPrev;
>             } while (fCurBlock != NULL && fCurBlock->fEnd == NULL);
>             prev = fCurBlock ? fCurBlock->fEnd - fElemSize : NULL;
>         }
>         fPos = prev;
>     }
>     return pos;
> }

Sorry you're right... I was thinking about the SkTLList iterator.

LTM

[bsalomon, 2012-12-12 16:49:34]

On 2012/12/12 16:49:17, bsalomon wrote:
> On 2012/12/12 16:44:44, junov1 wrote:
> > FYI:
> > 
> > void* SkDeque::Iter::prev() {
> >     char* pos = fPos;
> > 
> >     if (pos) {   // if we were valid, try to move to the prior setting
> >         char* prev = pos - fElemSize;
> >         SkASSERT(prev >= fCurBlock->fBegin - fElemSize);
> >         if (prev < fCurBlock->fBegin) { // exhausted this chunk, move to
prior
> >             do {
> >                 fCurBlock = fCurBlock->fPrev;
> >             } while (fCurBlock != NULL && fCurBlock->fEnd == NULL);
> >             prev = fCurBlock ? fCurBlock->fEnd - fElemSize : NULL;
> >         }
> >         fPos = prev;
> >     }
> >     return pos;
> > }
> 
> Sorry you're right... I was thinking about the SkTLList iterator.
> 
> LTM

LGTM, I mean.