Adding unit tests for SkCanvas and its sub classes

[junov1, 2012-02-16 20:37:52]

This is a pretty big unit test for Canvas.  I added test cases that cover almost
every virtual method in SkCanvas.  The test cases are executed in a variety of
contexts to make sure that SkCanvas and it sub classes behave correctly and
consistently with respect to canvas stat (as opposed to rendering results, which
a verified in gm).

This test uncovered a large number of failures. I fixed the most trivial bugs
directly in this CL.  The tests that would require a considerable amount of work
to fix were commented out, and I filed bugs for them:

http://code.google.com/p/skia/issues/detail?id=498
http://code.google.com/p/skia/issues/detail?id=499
http://code.google.com/p/skia/issues/detail?id=496
http://code.google.com/p/skia/issues/detail?id=500
http://code.google.com/p/skia/issues/detail?id=501

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

http://codereview.appspot.com/5674077/diff/1/include/core/SkCanvas.h#oldcode878
include/core/SkCanvas.h:878: bool getTotalClipBounds(SkIRect* bounds) const;
This method was unreferenced and unimplemented, so I removed its declaration.

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

http://codereview.appspot.com/5674077/diff/1/include/core/SkCanvas.h#newcode932
include/core/SkCanvas.h:932: bool clipLayerBounds(const SkRect* bounds,
SaveFlags flags, SkIRect* result);
This new method contains code that was factored out of saveLayer so that it can
be called from SkPicture::saveLayer.  Doing this was necessary in order for the
getClip* methods to return correct values when called on an SkPicture object
that has layers.

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

http://codereview.appspot.com/5674077/diff/1/include/core/SkStream.h#newcode275
include/core/SkStream.h:275: size_t bytesWritten() const { return fBytesWritten;
}
Needed by new unit test

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

http://codereview.appspot.com/5674077/diff/1/src/core/SkCanvas.cpp#newcode1118
src/core/SkCanvas.cpp:1118: return;
To prevent seg fault with SkNWayCanvas, which does not shave a device.

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (left):

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp#o...
src/utils/SkDeferredCanvas.cpp:481:
SkPicture::kUsePathBoundsForClip_RecordingFlag);
Not sure what the use case for this flag is.  I removed it because it was
causing clipping to behave differently between SkCanvas and SkDeferredCanvas.

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:22: if (!paint) {
Some canvas API methods can receive NULL ptrs for their SkPaint arguments. That
was causing a seg fault in this function

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp#newcode38
tests/CanvasTest.cpp:38: *      in a single line of code.  A simple test case is
a test case is
Will correct screwed-up sentence

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp#newcode384
tests/CanvasTest.cpp:384: const SkCanvas* canvas1, const SkCanvas* canvas2,
Will fix 80col issues in next upload.

http://codereview.appspot.com/5674077/diff/1/tests/Test.h
File tests/Test.h (right):

http://codereview.appspot.com/5674077/diff/1/tests/Test.h#newcode123
tests/Test.h:123: #define REPORTER_ASSERT_MESSAGE(r, cond, message)             
                          \
Sometimes the same test code can be executed repeatedly in different contexts. 
In such cases, the source file name and line number are not enough to uniquely
identify a test failure.  I added this convenience macro so that a message can
be use to disambiguate test failures.  And I will fix 80col issue.

[twiz, 2012-02-16 21:32:17]

Cool beans, Justin.

Just some nits from a quick drive-by.

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

http://codereview.appspot.com/5674077/diff/1/include/core/SkCanvas.h#newcode929
include/core/SkCanvas.h:929: const SkMatrix&, const SkPaint& paint);
Style nit:  Need a line here.

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

http://codereview.appspot.com/5674077/diff/1/src/core/SkCanvas.cpp#newcode710
src/core/SkCanvas.cpp:710: bool SkCanvas::clipLayerBounds(const SkRect* bounds,
SaveFlags flags, SkIRect* intersection)
80 column issue & { on same line.

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp
File src/utils/SkDeferredCanvas.cpp (right):

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp#n...
src/utils/SkDeferredCanvas.cpp:24: paint = &defaultPaint;
If there is no paint, do we need to compute the rest of this function?  Or did
you intend to be defensive, in-case the default-constructed paint changes?

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp
File tests/CanvasTest.cpp (right):

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp#newcode208
tests/CanvasTest.cpp:208: #define SIMPLE_TEST_CASE(NAME, CALL)                  
           \
Maybe this is bike-shedding, but I wonder if these really are TEST_CASES.  These
macros define what could be called canvas-operations.

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp#newcode387
tests/CanvasTest.cpp:387: REPORTER_ASSERT_MESSAGE(reporter,
canvas1->getDeviceSize() == \
You don't need the \ here.

http://codereview.appspot.com/5674077/diff/1/tests/CanvasTest.cpp#newcode468
tests/CanvasTest.cpp:468: for (int i = 0; i < referenceRecord->fBitmaps.count();
i++) {
Nit:  pre-increment

[junov1, 2012-02-16 22:24:56]

http://codereview.appspot.com/5674077/diff/1/src/utils/SkDeferredCanvas.cpp#n...
> src/utils/SkDeferredCanvas.cpp:24: paint = &defaultPaint;
> If there is no paint, do we need to compute the rest of this function?  Or did
> you intend to be defensive, in-case the default-constructed paint changes?

You are right, if there is no paint I can make assumptions. I think that is safe
enough.

> Maybe this is bike-shedding, but I wonder if these really are TEST_CASES. 
These
> macros define what could be called canvas-operations.

I looked it up, and "test step" is the usual term for a break down of a test
case.  I applied that name in the latest patch.

[junov1, 2012-02-22 14:58:14]

Latest patch addresses comments by twiz.

[reed1, 2012-02-22 16:42:28]

There are many things going on in this CL beyond adding unittests. Its a little
overwhelming. Can we do this in stages?

e.g.
The change in saveLayer() may be correct, but I can't easily separate that from
the addition of making things virtual and other changes.

[junov1, 2012-02-22 20:15:10]

Ok, patch set 3, has just the test and code changes that are necessary to make
the test work. No fixes.  Failing tests were commented out, and issues were
logged.  New issues are 505 and 506

[reed1, 2012-02-22 20:45:53]

lgtm