Make sp variants for SkData

Make sp variants for SkData

Committed: https://skia.googlesource.com/skia/+/fde05114e6c4107b36e1aa571c64d31def40f613

[reed1, 2016-03-11 00:24:28]

Description was changed from

==========
Make sp variants for SkData

BUG=skia:
==========

to

==========
Make sp variants for SkData

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-03-11 04:45:14]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 04:45:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779263003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779263003/1

[reed1, 2016-03-11 13:11:29]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 13:11:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779263003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779263003/40001

[commit-bot: I haz the power, 2016-03-11 13:14:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 13:14:49]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-03-11 13:21:12]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 13:21:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779263003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779263003/60001

[reed1, 2016-03-11 13:58:17]

reed@google.com changed reviewers:
+ msarett@google.com, scroggo@google.com

[reed1, 2016-03-11 13:58:35]

reed@google.com changed reviewers:
+ bungeman@google.com

[commit-bot: I haz the power, 2016-03-11 14:13:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 14:13:29]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-03-11 14:53:14]

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h
File include/core/SkData.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h#n...
include/core/SkData.h:226: typedef SkAutoTUnref<SkData> SkAutoDataUnref;
There's still one user of this in Chromium still... eck.

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h
File include/core/SkStream.h (left):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:11: #include "SkRefCnt.h"
Please don't remove this. You are directly using sk_sp here.

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:11: #include "SkData.h"
Was there a compiler that required this? It shouldn't be necessary to include
this here, the forward declaration should be enough. If it isn't we need to know
why, since that should work.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkWriteBuffer.cpp
File src/core/SkWriteBuffer.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkWriteBuffer....
src/core/SkWriteBuffer.cpp:189: SkAutoTUnref<SkData>
existingData(pixelRef->refEncodedData());
Not sk_sp?

[scroggo, 2016-03-11 15:28:35]

Why do we still use a raw pointer when passing to some factories?

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h
File include/core/SkData.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h#n...
include/core/SkData.h:83: static sk_sp<SkData> MakeWithCopy(const void* data,
size_t length);
I don't feel strongly about the change from New to Make, but I am curious about
the reasoning.

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:291: *  The stream will call ref() on the data (assuming
it is not NULL).
IIUC, this is not necessarily true for the sk_sp version. Maybe put a blank line
between the two versions, and/or add a comment for the second one?

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:145: SkImageGenerator::NewFromEncoded(encoded.get()),
dst);
Will NewFromEncoded be updated separately, or is the assumption that it may not
need to share it so no need to pass an sk_sp?

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureData.h
File src/core/SkPictureData.h (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureData....
src/core/SkPictureData.h:80: const sk_sp<SkData>& opData() const { return
fOpData; }
We used to return a pointer to a const SkData, and now we return a const pointer
to an SkData. Is that intentional?

(I think the only non-const method on SkData is writable_data(), which will
clearly assert if called on the return value, since it will have at least two
owners.)

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:54: return
SkData::MakeWithCopy(fWriter.contiguousArray(), fWriter.bytesWritten());
Not related to your change, but it appears that this function does the same
thing whether deepCopy is true or not.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkStream.cpp#n...
src/core/SkStream.cpp:335: SkData* SkMemoryStream::copyToData() const {
Why does this not return an sk_sp?

https://codereview.chromium.org/1779263003/diff/60001/src/images/SkImageDecod...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/images/SkImageDecod...
src/images/SkImageDecoder_ktx.cpp:58: SkKTXFile ktxFile(data.get());
Should this class take an sk_sp?

[reed1, 2016-03-11 19:23:35]

updated

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h
File include/core/SkData.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h#n...
include/core/SkData.h:83: static sk_sp<SkData> MakeWithCopy(const void* data,
size_t length);
On 2016/03/11 15:28:35, scroggo wrote:
> I don't feel strongly about the change from New to Make, but I am curious
about
> the reasoning.

1. To allow us to stage the transition with clients that are already using the
ptr versions
2. Somewhat an attempt to unify (currently we have lots of Creates and lots of
News)

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkData.h#n...
include/core/SkData.h:226: typedef SkAutoTUnref<SkData> SkAutoDataUnref;
On 2016/03/11 14:53:13, bungeman1 wrote:
> There's still one user of this in Chromium still... eck.

Acknowledged.

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h
File include/core/SkStream.h (right):

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:11: #include "SkData.h"
On 2016/03/11 14:53:13, bungeman1 wrote:
> Was there a compiler that required this? It shouldn't be necessary to include
> this here, the forward declaration should be enough. If it isn't we need to
know
> why, since that should work.

Needed for destructor

https://codereview.chromium.org/1779263003/diff/60001/include/core/SkStream.h...
include/core/SkStream.h:291: *  The stream will call ref() on the data (assuming
it is not NULL).
On 2016/03/11 15:28:35, scroggo wrote:
> IIUC, this is not necessarily true for the sk_sp version. Maybe put a blank
line
> between the two versions, and/or add a comment for the second one?

Done.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:145: SkImageGenerator::NewFromEncoded(encoded.get()),
dst);
On 2016/03/11 15:28:35, scroggo wrote:
> Will NewFromEncoded be updated separately, or is the assumption that it may
not
> need to share it so no need to pass an sk_sp?

Updated separately, just to keep this CL finite.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureData.h
File src/core/SkPictureData.h (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureData....
src/core/SkPictureData.h:80: const sk_sp<SkData>& opData() const { return
fOpData; }
On 2016/03/11 15:28:35, scroggo wrote:
> We used to return a pointer to a const SkData, and now we return a const
pointer
> to an SkData. Is that intentional?
> 
> (I think the only non-const method on SkData is writable_data(), which will
> clearly assert if called on the return value, since it will have at least two
> owners.)

1. data or const_data is all the same (to me). In general I prefer data, since
90% of the time we just say type for immutables (e.g. shaders, etc.)

2. const sp is just because the method itself is const.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkPictureRecor...
src/core/SkPictureRecord.h:54: return
SkData::MakeWithCopy(fWriter.contiguousArray(), fWriter.bytesWritten());
On 2016/03/11 15:28:35, scroggo wrote:
> Not related to your change, but it appears that this function does the same
> thing whether deepCopy is true or not.

Acknowledged.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkStream.cpp
File src/core/SkStream.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkStream.cpp#n...
src/core/SkStream.cpp:335: SkData* SkMemoryStream::copyToData() const {
On 2016/03/11 15:28:35, scroggo wrote:
> Why does this not return an sk_sp?

Saving for another (also large) CL ... just trying to enable the SkData
factories to be sp-able in this one.

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkWriteBuffer.cpp
File src/core/SkWriteBuffer.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/core/SkWriteBuffer....
src/core/SkWriteBuffer.cpp:189: SkAutoTUnref<SkData>
existingData(pixelRef->refEncodedData());
On 2016/03/11 14:53:13, bungeman1 wrote:
> Not sk_sp?

PixelRef (and PixelSerializer below) are not yet changed over to sk_sp, wasn't
driven to do this yet. It will happen (by necessity) when those APIs are
converted.

https://codereview.chromium.org/1779263003/diff/60001/src/images/SkImageDecod...
File src/images/SkImageDecoder_ktx.cpp (right):

https://codereview.chromium.org/1779263003/diff/60001/src/images/SkImageDecod...
src/images/SkImageDecoder_ktx.cpp:58: SkKTXFile ktxFile(data.get());
On 2016/03/11 15:28:35, scroggo wrote:
> Should this class take an sk_sp?

Yes (in a separate CL)

[reed1, 2016-03-11 19:24:28]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-11 19:24:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779263003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779263003/80001

[commit-bot: I haz the power, 2016-03-11 19:37:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:37:51]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-03-11 19:44:48]

lgtm

[bungeman-skia, 2016-03-11 20:49:41]

Description was changed from

==========
Make sp variants for SkData

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Make sp variants for SkData
==========

[bungeman-skia, 2016-03-11 20:50:07]

I don't see anything here which probably can't be fixed later. lgtm

[reed1, 2016-03-11 21:01:21]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-03-11 21:01:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1779263003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1779263003/80001

[commit-bot: I haz the power, 2016-03-11 21:02:31]

Description was changed from

==========
Make sp variants for SkData
==========

to

==========
Make sp variants for SkData

Committed:
https://skia.googlesource.com/skia/+/fde05114e6c4107b36e1aa571c64d31def40f613
==========

[commit-bot: I haz the power, 2016-03-11 21:02:32]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/fde05114e6c4107b36e1aa571c64d31def40f613