Adding unit test for verifying copy on write in SkSurface + bug fix in SkCanvas::clear

Adding unit test for verifying copy on write in SkSurface + bug fix in SkCanvas::clear
Committed: https://code.google.com/p/skia/source/detail?r=8429

[Justin Novosad, 2013-03-27 19:31:27]

PTAL

[reed1, 2013-03-27 19:46:59]

great idea for unittest! can it be easily refactored to cover raster and
pictures too?

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp#newc...
tests/SurfaceTest.cpp:43: // Verify that the right canvas commands trigger a
copy on write
Can this be refactored to take different factories for making surfaces, so we
can test the other backends as well (raster, picture)?

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp#newc...
tests/SurfaceTest.cpp:82: SkPoint testPoints2[] = {
is this array used?

[Justin Novosad, 2013-03-27 20:32:32]

On 2013/03/27 19:46:59, reed1 wrote:
> great idea for unittest! can it be easily refactored to cover raster and
> pictures too?

Yes, and it already covers raster. The createSurface factory function is the
key.  Right now, it creates a raster surface if the GrContext is NULL. No need
to cover the picture backend with this test since SkSurface_Picture does not
copy on write.

[Justin Novosad, 2013-03-27 20:34:37]

testPoints2 is used by the drawPosText test. It needs the same number of points
as there are characters in the string.

[reed1, 2013-03-27 20:35:18]

On 2013/03/27 20:32:32, junov wrote:
> On 2013/03/27 19:46:59, reed1 wrote:
> > great idea for unittest! can it be easily refactored to cover raster and
> > pictures too?
> 
> Yes, and it already covers raster. The createSurface factory function is the
> key.  Right now, it creates a raster surface if the GrContext is NULL.

Ah, right. Should have read closer.

> No need
> to cover the picture backend with this test since SkSurface_Picture does not
> copy on write.

It doesn't? Does your test fail for that case? Yikes, that sounds terrible.
Please add a big TODO somewhere so we can add that test (and fix it).

lgtm

[reed1, 2013-03-27 20:35:54]

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp#newc...
tests/SurfaceTest.cpp:82: SkPoint testPoints2[] = {
On 2013/03/27 19:46:59, reed1 wrote:
> is this array used?

doh, my "Find" somehow missed that. sorry.

[reed1, 2013-03-27 20:36:19]

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/13116018/diff/6001/tests/SurfaceTest.cpp#newc...
tests/SurfaceTest.cpp:82: SkPoint testPoints2[] = {
const?

[Justin Novosad, 2013-03-27 21:24:15]

Patch. Added SkSurface_Picture pass.

[reed1, 2013-03-28 13:37:13]

lgtm