First pass at splitting out SkPictureRecord from SkPicture

First pass at splitting out SkPictureRecord from SkPicture

This patch begins the process of splitting apart SkPicture, SkPicturePlayback and SkPictureRecord.

This is still a bit messy. In a follow up CL I hope to delay the creation of SkPictureRecorder's SkPicture until endRecording time.

Committed: https://skia.googlesource.com/skia/+/643b8bd6617e333f7a970a57ad9166f3d6675d1a

[robertphillips, 2014-06-04 22:11:42]

Still a bit more testing to do but this is the bulk of it.

[mtklein, 2014-06-04 22:22:16]

mostly nits

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

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:17: fCanvas.reset(NULL);
You can probably drop this?  Either way, you're going to call fCanvas.reset()
below before the method's done.

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:25: SkPictureRecord* record = NULL;
Might skip this and just call fCanvas.reset inside each branch of the if?

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:41: if (NULL != fPicture.get()) {
I'd probably invert this so the normal case is outside?

if (NULL == fPicture.get()) {
   return NULL;
}
SkASSERT(NULL == fPicture->fPlayback);
...
return fPicture.detach();

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:45: SkPictureRecord* record =
static_cast<SkPictureRecord*>(fCanvas.get());
Store fCanvas as SkAutoTUnref<SkPictureRecord>?

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:50: fPicture->fPlayback =
SkNEW_ARGS(SkPicturePlayback, (fPicture, *record, info));
In the future this goes away too right?  Sort of spooky for SkPictureRecorder to
fiddle around inside fPicture.

[reed1, 2014-06-05 13:28:07]

api lgtm

[robertphillips, 2014-06-05 20:29:44]

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

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:17: fCanvas.reset(NULL);
This will eventually go away when SkPictureRecord doesn't need an SkPicture.
Right now a prior SkPictureRecord will have an (unreffed) pointer to the prior
picture.

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:25: SkPictureRecord* record = NULL;
All this goofiness was b.c. SkPictureRecord.h is private while
SkPictureRecorder.h is public. Additionally, using SkAutoTUnref wants the full
type. I've switched to using a raw pointer (we we're getting much mileage out of
the AutoTUnref anyway) which makes the code simpler but incurs some extra care.

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:41: if (NULL != fPicture.get()) {
On 2014/06/04 22:22:17, mtklein wrote:
> I'd probably invert this so the normal case is outside?
> 
> if (NULL == fPicture.get()) {
>    return NULL;
> }
> SkASSERT(NULL == fPicture->fPlayback);
> ...
> return fPicture.detach();

Done.

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:45: SkPictureRecord* record =
static_cast<SkPictureRecord*>(fCanvas.get());
SkPictureRecord.h is private.

https://codereview.chromium.org/318763004/diff/1/src/core/SkPictureRecorder.c...
src/core/SkPictureRecorder.cpp:50: fPicture->fPlayback =
SkNEW_ARGS(SkPicturePlayback, (fPicture, *record, info));
Right. At this point SkPictureRecorder should just call one of SkPicture's ctors
to create and return a new one.

[mtklein, 2014-06-05 20:50:19]

Looks good.  Only one concern remains.

https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
include/core/SkPictureRecorder.h:21: virtual ~SkPictureRecorder();
Drop virtual?  Nothing else here is virtual; this adds a vtable.

[robertphillips, 2014-06-05 21:01:53]

https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
include/core/SkPictureRecorder.h:21: virtual ~SkPictureRecorder();
On 2014/06/05 20:50:19, mtklein wrote:
> Drop virtual?  Nothing else here is virtual; this adds a vtable.

Done.

[mtklein, 2014-06-05 21:14:10]

On 2014/06/05 21:01:53, robertphillips wrote:
>
https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
> File include/core/SkPictureRecorder.h (right):
> 
>
https://codereview.chromium.org/318763004/diff/20001/include/core/SkPictureRe...
> include/core/SkPictureRecorder.h:21: virtual ~SkPictureRecorder();
> On 2014/06/05 20:50:19, mtklein wrote:
> > Drop virtual?  Nothing else here is virtual; this adds a vtable.
> 
> Done.

lgtm

[robertphillips, 2014-06-06 17:34:00]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-06 17:34:59]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/318763004/4...

[robertphillips, 2014-06-06 19:18:04]

The CQ bit was unchecked by robertphillips@google.com

[robertphillips, 2014-06-08 12:54:06]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-08 12:55:01]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/318763004/4...

[commit-bot: I haz the power, 2014-06-08 12:55:09]

Change committed as 643b8bd6617e333f7a970a57ad9166f3d6675d1a