When cloning into an SkPicture, clear its data.

When cloning into an SkPicture, clear its data.

Prevents a memory leak if the caller clones into an SkPicture
that already has a playback or record.

Committed: https://code.google.com/p/skia/source/detail?r=6282

[Leon, 2012-11-02 22:13:07]

On 2012/11/02 20:36:35, scroggo-work wrote:

reed@ gave me verbal approval, but wondered if it would make more sense to have
clone() return new SkPictures (rather than requiring the caller to create them,
which gives them the opportunity to run into this problem).

[DerekS, 2012-11-05 13:19:34]

lgtm.

Mike and I decided to have the caller pass in the SkPicture so that they could
be created on the stack, but I would be open to revisiting the decision.

[reed1, 2012-11-05 13:31:04]

I like having the Array be on the stack (rather than the API returning a
SkTDArray), but I do favor having the API create new pictures to stuff into the
array.

[reed1, 2012-11-05 13:45:03]

Is there a bleeding-edge canary that will have this build available
somewhere?

On Mon, Nov 5, 2012 at 8:31 AM, <reed@google.com> wrote:

> I like having the Array be on the stack (rather than the API returning a
> SkTDArray), but I do favor having the API create new pictures to stuff
> into the array.
>
>
https://codereview.appspot.**com/6813081/<https://codereview.appspot.com/6813...
>

[reed1, 2012-11-05 13:45:14]

oops, wrong thread.

On Mon, Nov 5, 2012 at 8:44 AM, Mike Reed <reed@google.com> wrote:

> Is there a bleeding-edge canary that will have this build available
> somewhere?
>
>
> On Mon, Nov 5, 2012 at 8:31 AM, <reed@google.com> wrote:
>
>> I like having the Array be on the stack (rather than the API returning a
>> SkTDArray), but I do favor having the API create new pictures to stuff
>> into the array.
>>
>>
https://codereview.appspot.**com/6813081/<https://codereview.appspot.com/6813...
>>
>
>

[Leon, 2012-11-05 14:05:40]

On 2012/11/05 13:19:34, DerekS wrote:
> lgtm.
> 
> Mike and I decided to have the caller pass in the SkPicture so that they could
> be created on the stack, but I would be open to revisiting the decision.

Yeah, that makes sense to me. Thinking about it over the weekend, it occurred to
me that SkBitmap follows a similar pattern. SkBitmap::copyTo (and ::deepCopyTo)
allow the caller to allocate the SkBitmap how they wish and then copy into it.

[TomH, 2014-02-14 12:27:07]

15 months later, everybody in London is confused by this change: the previous
line of code sets clone->fRecord = NULL, and then the first line of this change
checks to see if it's non-NULL. Do we need a followup?

[DerekS, 2014-02-14 14:50:51]

Seems to me that I may have rubber stamped LGTM'd this one without doing a full
review.  I think we just need to remove line 158 for this CL to make sense, but
I'll let Leon confirm.

[mtklein, 2014-02-14 14:51:01]

On 2014/02/14 12:27:07, TomH wrote:
> 15 months later, everybody in London is confused by this change: the previous
> line of code sets clone->fRecord = NULL, and then the first line of this
change
> checks to see if it's non-NULL. Do we need a followup?

Ha!  Count me among the confused too.  Clearly this is just a fancy way to say
SkDELETE(clone->fPlayback);.

[iancottrell, 2014-02-14 14:59:57]

On 2014/02/14 14:51:01, mtklein wrote:
> On 2014/02/14 12:27:07, TomH wrote:
> > 15 months later, everybody in London is confused by this change: the
previous
> > line of code sets clone->fRecord = NULL, and then the first line of this
> change
> > checks to see if it's non-NULL. Do we need a followup?
> 
> Ha!  Count me among the confused too.  Clearly this is just a fancy way to say
> SkDELETE(clone->fPlayback);.

My suggested fix was this
https://codereview.chromium.org/163053005/

[Leon, 2014-02-14 15:11:05]

On 2014/02/14 14:50:51, DerekS wrote:
> Seems to me that I may have rubber stamped LGTM'd this one without doing a
full
> review.  I think we just need to remove line 158 for this CL to make sense,
but
> I'll let Leon confirm.

Yep. I'll let this be a reminder to write tests. Deleting the fPlayback fixed
the memory leak I happened to run into, and the other code was copied from
beginRecording.