Switch to one-at-a-time SkPicture::clone interface

Switch to one-at-a-time SkPicture::clone interface

This updates Chromium to use the one-at-a-time SkPicture
clone interface since Skia is moving towards removing 
SkPicture's default and copy constructors.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282668

[mtklein, 2014-07-10 19:22:39]

LGTM

[danakj, 2014-07-10 19:57:05]

https://codereview.chromium.org/380323002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/380323002/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:234: scoped_ptr<SkPicture* []> clones(new SkPicture*
[num_threads - 1]);
This is an array of raw SkRefCnt pointers? Since skia won't adopt something like
skia::RefPtr I don't know what to suggest given this intended skia api. I just
wanted to say that this makes me super sad, cuz we can't immediately wrap the
raw pointers in RefPtr like this.

If this was more common I'd suggest a RefPtrVector similar to
ScopedPtrVector.....

[mtklein, 2014-07-10 20:09:51]

https://codereview.chromium.org/380323002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/380323002/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:234: scoped_ptr<SkPicture* []> clones(new SkPicture*
[num_threads - 1]);
Why don't we rewrite this to just use the other existing clone method,
  SkPicture* SkPicture::clone() const;
?

This still lets us remove the default constructor and the copy constructor, and
allows Picture to wrap the SkPicture* in a skia::AdoptRef right away.

This means we also don't have to land the Skia-side CL adding void
SkPicture::clone(SkPicture* [], int).

[robertphillips, 2014-07-11 12:45:58]

PTAL I have revised the CL to use the other clone method (as Mike suggested).

[mtklein, 2014-07-11 13:02:56]

On 2014/07/11 12:45:58, robertphillips wrote:
> PTAL I have revised the CL to use the other clone method (as Mike suggested).

Yup, that's what I was thinking.  LGTM

[danakj, 2014-07-11 15:30:40]

I assume doing one clone at a time is not somehow worse for performance than
doing N clones at a time like we did before? I guess there was some problem with
the old clone API?

https://codereview.chromium.org/380323002/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/380323002/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:236: make_scoped_refptr(new
Picture(skia::AdoptRef(picture_->clone()),
nit: FWIW make_scoped_refptr is redundant, that conversion happens implicitly.

[mtklein, 2014-07-11 15:45:10]

On 2014/07/11 15:30:40, danakj wrote:
> I assume doing one clone at a time is not somehow worse for performance than
> doing N clones at a time like we did before? I guess there was some problem
with
> the old clone API?

Right.  The many-clone method is no more or less efficient than looping over
one-clone.  It had the same for-loop inside.

I think this version of the cc/Picture code is now a titch more efficient, as we
used to create 2x the clones we needed, once in an array and then one by one
with the copy constructor.

This is a half-way step to removing cloning altogether: should be we can
actually implement clone as { return this; } very soon after landing this CL,
let that stew a bit, then remove all the cloning code.  We can't do that
incrementally without control over which SkPicture*s we're cloning into.

[danakj, 2014-07-11 15:46:20]

On 2014/07/11 15:45:10, mtklein wrote:
> On 2014/07/11 15:30:40, danakj wrote:
> > I assume doing one clone at a time is not somehow worse for performance than
> > doing N clones at a time like we did before? I guess there was some problem
> with
> > the old clone API?
> 
> Right.  The many-clone method is no more or less efficient than looping over
> one-clone.  It had the same for-loop inside.
> 
> I think this version of the cc/Picture code is now a titch more efficient, as
we
> used to create 2x the clones we needed, once in an array and then one by one
> with the copy constructor.
> 
> This is a half-way step to removing cloning altogether: should be we can
> actually implement clone as { return this; } very soon after landing this CL,
> let that stew a bit, then remove all the cloning code.  We can't do that
> incrementally without control over which SkPicture*s we're cloning into.

Great! Thanks for the explanation, LGTM

[danakj, 2014-07-11 15:46:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-07-11 15:47:58]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/robertphillips@google.com/380323002/40001

[robertphillips, 2014-07-11 15:59:18]

On 2014/07/11 15:30:40, danakj wrote:
> I assume doing one clone at a time is not somehow worse for performance than
> doing N clones at a time like we did before? I guess there was some problem
with
> the old clone API?

A lot is going on with SkPicture. We're replacing the old backend with the
SkRecord-based backend. We're making them threadsafe so cloning will no longer
be required. We're making it so
SkPictureRecorder::endRecording/CreateFromStream/CreateFromBuffer are the only
sources of SkPictures and they return a const SkPicture.

As part of this we're trying to narrow the SkPicture API to make the switch
easier/feasible. So, short-term it can be a bit slower to make each clone
individually (mainly when the source SkPicture contains non-thread-safe
SkPaints) but long term it will become a noop.

[robertphillips, 2014-07-11 15:59:43]

https://codereview.chromium.org/380323002/diff/20001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/380323002/diff/20001/cc/resources/picture.cc#...
cc/resources/picture.cc:236: make_scoped_refptr(new
Picture(skia::AdoptRef(picture_->clone()),
On 2014/07/11 15:30:40, danakj wrote:
> nit: FWIW make_scoped_refptr is redundant, that conversion happens implicitly.

Done.

[commit-bot: I haz the power, 2014-07-11 19:44:34]

Change committed as 282668