Rearrange SkRecord public API to fit better with cc/resources/picture

Rearrange SkRecord public API to fit better with cc/resources/picture

BUG=skia:2378
Committed: http://code.google.com/p/skia/source/detail?r=14351

[mtklein, 2014-04-22 19:34:58]

This is Skia-side changes for what we've been discussing on
https://codereview.chromium.org/225023031/

[mtklein, 2014-04-22 19:52:38]

On 2014/04/22 19:46:35, enne wrote:
> Could ReleasePlayback also clear the fRecord off of the SkRecorder and
> SkRecording? (It seems like its an ownership transfer.)  If something tried to
> use the SkCanvas/SkRecorder after that it'd crash on the NULL pointer, but
> that's a clearer failure than thread raciness.

Added SkRecorder::forgetRecord() and calling it in
SkRecorder::releasePlayback(), as suggested in the other thread.

[enne (OOO), 2014-04-22 20:06:04]

Thanks, this lgtm.

[mtklein, 2014-04-23 20:04:00]

Mike, ping for (at least) include/record change?

[reed1, 2014-04-23 21:53:30]

Can you describe more why the change (i.e. why the caller gets to keep its
"record" object after closing down?

https://codereview.chromium.org/248033002/diff/2/include/record/SkRecording.h
File include/record/SkRecording.h (right):

https://codereview.chromium.org/248033002/diff/2/include/record/SkRecording.h...
include/record/SkRecording.h:24: *  scoped_ptr<SkRecording> recording(new
SkRecording(1920, 1080));
its not wrong, but I don't think our pseudo code in dox needs to use these
scoping helpers.

[mtklein, 2014-04-23 22:05:54]

These changes are for requests from cc/resources/picture owners, to make it fit
in the idiom they're used to working with, and particularly to minimize surprise
when converting back and forth between Skia thinking and Chrome thinking.

They'd like to hang onto the SkRecording not so much to use it (it disables that
and will crash loudly if you try) but only so they don't have to think about
another kind of pointer lifetime interaction crossing between Skia and Chrome. 
We've got unowned pointers, ref counted pointers, exclusively owned pointers,
and now this current custom lifetime that requires a call to Delete.  This makes
SkRecording work as an exclusively owned pointer so no one has to learn that
last new pattern.

I'm happy to think of this header as Chrome's code.  It's interface can work
however they like as far as I care, as long as we control its .cpp file and all
the other implementation bits.

https://codereview.chromium.org/248033002/diff/2/include/record/SkRecording.h
File include/record/SkRecording.h (right):

https://codereview.chromium.org/248033002/diff/2/include/record/SkRecording.h...
include/record/SkRecording.h:24: *  scoped_ptr<SkRecording> recording(new
SkRecording(1920, 1080));
On 2014/04/23 21:53:30, reed1 wrote:
> its not wrong, but I don't think our pseudo code in dox needs to use these
> scoping helpers.

It's intended for Chrome readers.  Want me to rewrite it in terms of
SkAutoTUnref and co?

[reed1, 2014-04-23 22:41:03]

Looking at SkPictureRecorder, it *seems* to follow this...

SkPIctureRecorder recorder;
SkCanvas* canvas = recorder.begin(width, height, ...);
...
SkPicture* recorder.end();

// at this point the returned canvas is dead, but you can "re"use the recorder

SkCanvas* anotherCanvas = recorder.begin(...);

Can we make your interface follow this pattern? If so, then we and our clients
don't have to learn two different models.

[mtklein, 2014-04-23 23:44:20]

On 2014/04/23 22:41:03, reed1 wrote:
> Looking at SkPictureRecorder, it *seems* to follow this...
> 
> SkPIctureRecorder recorder;
> SkCanvas* canvas = recorder.begin(width, height, ...);
> ...
> SkPicture* recorder.end();
> 
> // at this point the returned canvas is dead, but you can "re"use the recorder
> 
> SkCanvas* anotherCanvas = recorder.begin(...);
> 
> Can we make your interface follow this pattern? If so, then we and our clients
> don't have to learn two different models.

Sure, we can do that.  Will ping the thread again when the patch is up.

Or, given that we don't appear to be reusing the recorder, we might want to go
the other way and make everything single-use?

[reed1, 2014-04-23 23:49:45]

On 2014/04/23 23:44:20, mtklein wrote:
> On 2014/04/23 22:41:03, reed1 wrote:
> > Looking at SkPictureRecorder, it *seems* to follow this...
> > 
> > SkPIctureRecorder recorder;
> > SkCanvas* canvas = recorder.begin(width, height, ...);
> > ...
> > SkPicture* recorder.end();
> > 
> > // at this point the returned canvas is dead, but you can "re"use the
recorder
> > 
> > SkCanvas* anotherCanvas = recorder.begin(...);
> > 
> > Can we make your interface follow this pattern? If so, then we and our
clients
> > don't have to learn two different models.
> 
> Sure, we can do that.  Will ping the thread again when the patch is up.
> 
> Or, given that we don't appear to be reusing the recorder, we might want to go
> the other way and make everything single-use?

Yea, I couldn't tell for sure what Robert's thinking was. My read of the code is
that it can be re-used, and perhaps, if there is some interesting "setup"
options before recording, it may be a useful feature.

If your CL as is is a strict subset of that pattern (i.e. currently you assert
if we try to re-use), then we could land this now and discuss the larger plan
later.

[mtklein, 2014-04-24 01:16:55]

> If your CL as is is a strict subset of that pattern (i.e. currently you assert
> if we try to re-use), then we could land this now and discuss the larger plan
> later.

Just so.  It's enforced strict single-use.

Once you releasePlayback(), you can't access the canvas anymore, and if you
somehow were holding onto it, it'd blow up in your face when you try to write
more to it.

[reed1, 2014-04-24 13:25:08]

lgtm

[mtklein, 2014-04-24 13:47:59]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-24 13:48:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/248033002/2

[commit-bot: I haz the power, 2014-04-24 15:22:58]

Change committed as 14351