Store SkRRects in SkClipStack

Store SkRRects in SkClipStack

BUG=skia:2181
Committed: http://code.google.com/p/skia/source/detail?r=13465

[robertphillips, 2014-02-14 20:36:00]

lgtm + some questions

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:130: */
Do you think a "contains(const SkRRect&)" would be worthwhile?

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:157: SkPath          fPath;
What do you think about storing the rect in the RR's rect space?

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:240: fGenID = kEmptyGenID;
SkASSERT(this->checkEmpty())? Shouldn't matter though.

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:49:
fPath.setFillType(SkPath::kInverseEvenOdd_FillType);
Do we really care about setting fRect to be empty?

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:56:
fPath.setFillType(SkPath::kInverseEvenOdd_FillType);
Same for rrect?

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:62: case kEmpty_Type:
Probably

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:673: element->setEmpty();
Seems like we don't really need this switch any more.

https://codereview.chromium.org/163683002/diff/100001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/163683002/diff/100001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:708: } else {
Do we need path?

[bsalomon, 2014-02-14 21:20:57]

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStack.h
File include/core/SkClipStack.h (right):

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:130: */
On 2014/02/14 20:36:01, robertphillips wrote:
> Do you think a "contains(const SkRRect&)" would be worthwhile?

Maybe for tightly nested rrects, but I think I'd rather wait until we have a
caller.

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:157: SkPath          fPath;
On 2014/02/14 20:36:01, robertphillips wrote:
> What do you think about storing the rect in the RR's rect space?

That's a good idea. I thought about making a kOval_Type that used fRRect. I'd
like to defer it to a follow up CL, though.

https://codereview.chromium.org/163683002/diff/100001/include/core/SkClipStac...
include/core/SkClipStack.h:240: fGenID = kEmptyGenID;
On 2014/02/14 20:36:01, robertphillips wrote:
> SkASSERT(this->checkEmpty())? Shouldn't matter though.

Done (actually SkDEBUGCODE since checkEmpty() doesn't return anything).

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cpp
File src/core/SkClipStack.cpp (right):

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:49:
fPath.setFillType(SkPath::kInverseEvenOdd_FillType);
On 2014/02/14 20:36:01, robertphillips wrote:
> Do we really care about setting fRect to be empty?

I guess not

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:56:
fPath.setFillType(SkPath::kInverseEvenOdd_FillType);
On 2014/02/14 20:36:01, robertphillips wrote:
> Same for rrect?

Done.

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:62: case kEmpty_Type:
On 2014/02/14 20:36:01, robertphillips wrote:
> Probably

Agreed, but don't want to conflate any regressions/changes with the those from
the main thrust of this CL.

https://codereview.chromium.org/163683002/diff/100001/src/core/SkClipStack.cp...
src/core/SkClipStack.cpp:673: element->setEmpty();
On 2014/02/14 20:36:01, robertphillips wrote:
> Seems like we don't really need this switch any more.

Done.

https://codereview.chromium.org/163683002/diff/100001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/163683002/diff/100001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:708: } else {
On 2014/02/14 20:36:01, robertphillips wrote:
> Do we need path?

Done.

[bsalomon, 2014-02-16 12:29:58]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-02-16 12:30:10]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/163683002/190001

[commit-bot: I haz the power, 2014-02-16 12:36:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-16 12:36:41]

Retried try job too often on Build-Mac10.7-Clang-x86-Release-Trybot for step(s)
BuildTests
http://108.170.219.164:10117/buildstatus?builder=Build-Mac10.7-Clang-x86-Rele...

[bsalomon, 2014-02-16 13:12:50]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-02-16 13:13:00]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bsalomon@google.com/163683002/300003

[commit-bot: I haz the power, 2014-02-16 13:25:30]

Change committed as 13465