Adding protected API in SkCanvas to disable use of SkRasterClip

Adding protected API in SkCanvas to disable use of SkRasterClip

This CL adds SkCanvas::disableRasterClip which can be invoked by
derived classes that wish to turn off complex clip region 
construction. When in this mode, the canvas only tracks the 
bounds of the clip and getTotalClip always returns a rectangular
region corresponding to conservative bounds.

Motivation: replace the current strategy used by the
kUsePathBoundsForClip_RecordingFlag optimization in SkPicture,
which has caveats.

BUG=https://code.google.com/p/chromium/issues/detail?id=174367
TEST=covered by gm+render_pictures

[junov1, 2013-02-06 19:41:39]

PTAL

This CL passes all GMs
I will add unit tests to cover use cases that were broken with the old code.

[junov1, 2013-02-06 22:15:11]

New Patch.
- Added unit test that demonstrates bug fixed by this patch
- Fixed bug in previous patchwith ho wide-open clips were handled
- Fixed pre-existing bug with how SkClipStack::intersectRectWithClip handles
inside-out path bounds.

[reed1, 2013-02-08 20:12:42]

I will have more inline questions no doubt, but here are some.

Meta: this looks *very* invasive (even if its correct). I feel like we will need
a lot of time to test (and write new tests) this.

Is there perhaps a smaller CL that would allow us to make
picture-record-with-bounds more correct, but was easier to review/test?

https://codereview.appspot.com/7301054/diff/13002/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.appspot.com/7301054/diff/13002/include/core/SkCanvas.h#new...
include/core/SkCanvas.h:1083: RasterClipMode fRasterClipMode;
What is this new method?
How does it differ from getClipDeviceBounds()'s return value?
Does this CL affect getClipDeviceBounds()?

https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:174: SkRect devRect =
SkRect::MakeFromIRect(devClipBounds);
Should we check for intersectRectWithClip() to return false?

https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:608: bool SkCanvas::isClipKnownToBeEmpty() const {
fRasterClipMode is an enum, but we are treating it like a bool

https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp#newco...
src/core/SkCanvas.cpp:689: 
treating enum as bool :(

// Why can we skip all of this code based on fRasterClipMode?

[junov1, 2013-02-08 20:33:46]

On 2013/02/08 20:12:42, reed1 wrote:
> I will have more inline questions no doubt, but here are some.
> 
> Meta: this looks *very* invasive (even if its correct). I feel like we will
need
> a lot of time to test (and write new tests) this.

I agree I did some investigation on the WebKit side to try to produce an HTML
file that would trigger this bug.  BAsically I was looking for place where use
clip op other than intersect and places where we use inverse filled paths with
shapes that are no rectangles
It turns out that we use an inverse filled clip under some circumstances to draw
rounded rects.  Because of how the sequence of draw commands is structured, that
code does not trigger the bug.

I also looked at svg. Because svg only supports evenodd and nonzero clip rules,
we never get inverse filled clip paths. Also svg can only do intersect an union
clip ops (indirectly), which work out OK for now.  Unless we can find a
real-world case that trigger the bug, I think we should postpone this to after
the M26 branchpoint, so it gets some simmer time.

[junov1, 2013-02-08 21:26:01]

https://codereview.appspot.com/7301054/diff/13002/include/core/SkCanvas.h#new...
> include/core/SkCanvas.h:1083: RasterClipMode fRasterClipMode;
> What is this new method?
> How does it differ from getClipDeviceBounds()'s return value?
You're right. It is completely equivalent. I will remove it.
> Does this CL affect getClipDeviceBounds()?
If the raster clip is not disabled, getClipDeviceBounds will behave exactly the
same as before. If the raster clip is disabled, this method will use a
conservative estimate base on the clip stack. In the kDisabled_RasterClipMode
case it is possible for getClipDeviceBounds to return false positive, but not
the opposite (conservative). It is also possible that it will report larger
bounds that are guaranteed to contain the actual clip bounds (conservative).  I
will update the doxygen comments with this info.
> 
> https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp#newco...
> src/core/SkCanvas.cpp:174: SkRect devRect =
> SkRect::MakeFromIRect(devClipBounds);
> Should we check for intersectRectWithClip() to return false?
Good point. I just looked at the implementation the the returned rect may not be
set to empty when false is returned (which I was assuming).

https://codereview.appspot.com/7301054/diff/13002/src/core/SkCanvas.cpp#newco...
> src/core/SkCanvas.cpp:689: 
> treating enum as bool :(
> 
> Why can we skip all of this code based on fRasterClipMode?

All this code does is update fRasterClip

[junov1, 2013-02-11 14:47:30]

Patch.