Ensure playing back a picture always balances saves and restores

Ensure playing back a picture always balances saves and restores

This "fixes" the legacy interface's possible creation of pictures with unbalanced save/restores.

The Android dox will need to be updated once/if this lands.

Committed: http://code.google.com/p/skia/source/detail?r=14772

[scroggo, 2014-05-16 18:20:53]

https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:1324: canvas.restoreToCount(originalSaveCount);
There are a couple of other places where we return from this function. In one of
them (if the callback aborts the drawing), we also restoreToCount, and in the
other (Android only; another version of aborting playback - as a side note, it
seems like that could be implemented in Android as a callback and we can
eliminate some code?) we do not. It seems like we should in both cases (as long
as the latter sticks around), we should do the restore.

I think it would make sense to use a SkAutoCanvasRestore(&canvas, false) early
on. That would make sure that we catch all exit cases (also that the restore
happens last).

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:979: #if defined(SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD) &&
defined(SK_SUPPORT_LEGACY_DERIVED_PICTURE_CLASSES)
Aren't we going to change Android to use the newer version of SkPicture and
remove SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD? Maybe this test belongs in Android.

Also:
nit: 100 chars.

[scroggo, 2014-05-16 18:37:57]

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:979: #if defined(SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD) &&
defined(SK_SUPPORT_LEGACY_DERIVED_PICTURE_CLASSES)
On 2014/05/16 18:20:53, scroggo wrote:
> Aren't we going to change Android to use the newer version of SkPicture and
> remove SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD? Maybe this test belongs in
Android.
> 
> Also:
> nit: 100 chars.

Also, it appears we only define SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD for the
Android framework, so this test won't be run anywhere (yet; eventually we'll run
our test suite within the Android framework).

[robertphillips, 2014-05-16 19:18:15]

https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.c...
src/core/SkPicturePlayback.cpp:1324: canvas.restoreToCount(originalSaveCount);
On 2014/05/16 18:20:53, scroggo wrote:
> There are a couple of other places where we return from this function. In one
of
> them (if the callback aborts the drawing), we also restoreToCount, and in the
> other (Android only; another version of aborting playback - as a side note, it
> seems like that could be implemented in Android as a callback and we can
> eliminate some code?) we do not. It seems like we should in both cases (as
long
> as the latter sticks around), we should do the restore.
> 
> I think it would make sense to use a SkAutoCanvasRestore(&canvas, false) early
> on. That would make sure that we catch all exit cases (also that the restore
> happens last).

Done.

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp#newcod...
tests/PictureTest.cpp:979: #if defined(SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD) &&
defined(SK_SUPPORT_LEGACY_DERIVED_PICTURE_CLASSES)
On 2014/05/16 18:20:53, scroggo wrote:
> Aren't we going to change Android to use the newer version of SkPicture and
> remove SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD? Maybe this test belongs in
Android.

I'm trying to separate the fix for this SkPicture behavior from the switch to
the new recording interface. I think combining them in one (Android-side) CL
will obscure what is going on. I intend to add a unit test to Android that
verifies the new behavior of their Java Picture copy constructor.

The path I was envisioning was:
  Land something like this in Skia
  Update Android docs & add unit test
  Move Android to new recording API
  Delete legacy recording API in Skia

> Also:
> nit: 100 chars.

Done.

> Also, it appears we only define SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD for the
> Android framework, so this test won't be run anywhere (yet; eventually we'll
run
> our test suite within the Android framework).

Right. This was more so I could be confident I actually fixed it here. This will
be deleted once Android is moved to the new API.

[scroggo, 2014-05-16 19:28:02]

On 2014/05/16 19:18:15, robertphillips wrote:
>
https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.cpp
> File src/core/SkPicturePlayback.cpp (right):
> 
>
https://codereview.chromium.org/286033005/diff/1/src/core/SkPicturePlayback.c...
> src/core/SkPicturePlayback.cpp:1324: canvas.restoreToCount(originalSaveCount);
> On 2014/05/16 18:20:53, scroggo wrote:
> > There are a couple of other places where we return from this function. In
one
> of
> > them (if the callback aborts the drawing), we also restoreToCount, and in
the
> > other (Android only; another version of aborting playback - as a side note,
it
> > seems like that could be implemented in Android as a callback and we can
> > eliminate some code?) we do not. It seems like we should in both cases (as
> long
> > as the latter sticks around), we should do the restore.
> > 
> > I think it would make sense to use a SkAutoCanvasRestore(&canvas, false)
early
> > on. That would make sure that we catch all exit cases (also that the restore
> > happens last).
> 
> Done.
> 
> https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp
> File tests/PictureTest.cpp (right):
> 
>
https://codereview.chromium.org/286033005/diff/1/tests/PictureTest.cpp#newcod...
> tests/PictureTest.cpp:979: #if defined(SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD)
&&
> defined(SK_SUPPORT_LEGACY_DERIVED_PICTURE_CLASSES)
> On 2014/05/16 18:20:53, scroggo wrote:
> > Aren't we going to change Android to use the newer version of SkPicture and
> > remove SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD? Maybe this test belongs in
> Android.
> 
> I'm trying to separate the fix for this SkPicture behavior from the switch to
> the new recording interface. I think combining them in one (Android-side) CL
> will obscure what is going on. I intend to add a unit test to Android that
> verifies the new behavior of their Java Picture copy constructor.
> 
> The path I was envisioning was:
>   Land something like this in Skia
>   Update Android docs & add unit test
>   Move Android to new recording API
>   Delete legacy recording API in Skia
> 
> > Also:
> > nit: 100 chars.
> 
> Done.
> 
> > Also, it appears we only define SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD for the
> > Android framework, so this test won't be run anywhere (yet; eventually we'll
> run
> > our test suite within the Android framework).
> 
> Right. This was more so I could be confident I actually fixed it here. This
will
> be deleted once Android is moved to the new API.

lgtm

[reed1, 2014-05-19 12:18:48]

lgtm

[robertphillips, 2014-05-19 12:21:27]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-05-19 12:21:36]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/286033005/2...

[commit-bot: I haz the power, 2014-05-19 12:27:01]

Change committed as 14772