Getting tests ready for enforcing balanced save/restore calls in SkPicture

[junov1, 2012-07-13 16:01:51]

PTAL

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

http://codereview.appspot.com/6354105/diff/1/src/core/SkPicturePlayback.cpp#n...
src/core/SkPicturePlayback.cpp:550: offsetToRestore >= fReader.offset());
truns out that it is OK for offsetToRestore to be equal to fReader.offset(). 
Happens when a clip op is immediately followed by a restore.

[reed1, 2012-07-13 18:12:59]

Should we be able to change SkCanvas::drawPicture() to *not* have to add
save/restore after this finally lands?

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

http://codereview.appspot.com/6354105/diff/1/src/core/SkPictureRecord.cpp#new...
src/core/SkPictureRecord.cpp:184: #if 0
or... we say its ok to have too-many saves, and we will just add the balancing
restores here (to match what Skcanvas::drawpicture does).

beginRecording() {
    fSaveCount = canvas->getSaveCount();
}

endRecording() {
    canvas->restoreToCount(fSaveCount);
}

[junov1, 2012-07-13 18:18:39]

I like the idea of hanging on to the extra save/restore in SkCanvas::drawPicture
because it still helps insulate the canvas's state from whater unrestored
changes may have been left behind by the SkPicture.

[reed1, 2012-07-13 18:20:40]

On 2012/07/13 18:18:39, junov1 wrote:
> I like the idea of hanging on to the extra save/restore in
SkCanvas::drawPicture
> because it still helps insulate the canvas's state from whater unrestored
> changes may have been left behind by the SkPicture.

... unless we take my inline-suggestion about cleaning that up explicitly in
endRecording()

[junov1, 2012-07-13 18:24:34]

come to think of it. putting it in begin/endRecording is nice because it also
covers the SkPicture::draw code path.

[reed1, 2012-07-13 18:25:29]

On 2012/07/13 18:24:34, junov1 wrote:
> come to think of it. putting it in begin/endRecording is nice because it also
> covers the SkPicture::draw code path.

zackly

[junov1, 2012-07-13 18:46:02]

Ok, patch set 2 haz the save/restore in the new location.  All tests pass.

[reed1, 2012-07-13 18:49:00]

http://codereview.appspot.com/6354105/diff/5002/include/core/SkPicture.h
File include/core/SkPicture.h (left):

http://codereview.appspot.com/6354105/diff/5002/include/core/SkPicture.h#oldc...
include/core/SkPicture.h:135: SkPicturePlayback* fPlayback;
Is it clearer to have this field here, or in SkPictureRecord?

http://codereview.appspot.com/6354105/diff/5002/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

http://codereview.appspot.com/6354105/diff/5002/src/core/SkPictureRecord.cpp#...
src/core/SkPictureRecord.cpp:178: }
Is this still needed, given that we've called restoreToCount() before
endRecording()?

[junov1, 2012-07-13 21:18:01]

> Is it clearer to have this field here, or in SkPictureRecord?

I moved it to SkPictureRecord. It was a little tricky to get this right. 

I also had to get rid of a bunch of unit test cases in CanvasTest that assumed
that an SkPictureRecord's state would be identical to that of an SkCanvas that
executed the same series of commands, which is no longer the case.

[reed1, 2012-07-16 13:54:40]

lgtm w/ tiny nit about reverting a (basically) unchanged file.

http://codereview.appspot.com/6354105/diff/10001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (left):

http://codereview.appspot.com/6354105/diff/10001/src/core/SkPicture.cpp#oldco...
src/core/SkPicture.cpp:162: fRecord->setBitmapDevice(bm);
We should prolly revert this file

[junov1, 2012-07-16 19:10:38]

Fixed with r4618