return pictures as sk_sp

return pictures as sk_sp

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

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

[reed1, 2016-03-16 20:47:04]

Description was changed from

==========
return pictures as sk_sp

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

to

==========
return pictures as sk_sp

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

[reed1, 2016-03-17 19:52:12]

reed@google.com changed reviewers:
+ fmalita@chromium.org, robertphillips@google.com

[reed1, 2016-03-17 19:52:18]

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

[commit-bot: I haz the power, 2016-03-17 19:52:29]

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

[reed1, 2016-03-17 20:01:52]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-03-17 21:01:27]

Given ASAN + GM + tests, I mostly want the reviewers to like to direction/style
if the public API changes. A later CL will attempt to actually remove the guard
in the header.

[reed1, 2016-03-18 12:47:36]

shall I land as is? There are many follow-ups planned.

[f(malita), 2016-03-18 12:48:12]

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
src/core/SkPictureRecorder.cpp:202: return std::move(drawable);
IIUC move semantics clash with RVO, and the general consensus is to not use
std::move if RVO would apply - which I think is the case here.

(Ben commented on this subject the other week, we may even have some bots which
catch this pessimization with a warning)

[f(malita), 2016-03-18 12:54:54]

On 2016/03/18 12:47:36, reed1 wrote:
> shall I land as is? There are many follow-ups planned.

I haven't gone through all the gm/tests, but other than the RVO-disabling
std::move core changes LGTM.

[f(malita), 2016-03-18 13:02:40]

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
src/core/SkPictureRecorder.cpp:202: return std::move(drawable);
On 2016/03/18 12:48:12, f(malita) wrote:
> IIUC move semantics clash with RVO, and the general consensus is to not use
> std::move if RVO would apply - which I think is the case here.
> 
> (Ben commented on this subject the other week, we may even have some bots
which
> catch this pessimization with a warning)

Maybe I imagined that conversation since I can't find it, but here's an article
explaining the issue:
https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39...

[robertphillips, 2016-03-18 13:43:01]

I looked through the headers and the interface looks good to me

[reed1, 2016-03-18 14:11:51]

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
src/core/SkPictureRecorder.cpp:202: return std::move(drawable);
On 2016/03/18 12:48:12, f(malita) wrote:
> IIUC move semantics clash with RVO, and the general consensus is to not use
> std::move if RVO would apply - which I think is the case here.
> 
> (Ben commented on this subject the other week, we may even have some bots
which
> catch this pessimization with a warning)

Done.

https://codereview.chromium.org/1811703002/diff/20001/src/core/SkPictureRecor...
src/core/SkPictureRecorder.cpp:202: return std::move(drawable);
On 2016/03/18 13:02:40, f(malita) wrote:
> On 2016/03/18 12:48:12, f(malita) wrote:
> > IIUC move semantics clash with RVO, and the general consensus is to not use
> > std::move if RVO would apply - which I think is the case here.
> > 
> > (Ben commented on this subject the other week, we may even have some bots
> which
> > catch this pessimization with a warning)
> 
> Maybe I imagined that conversation since I can't find it, but here's an
article
> explaining the issue:
>
https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-39...

Thanks for the link

[reed1, 2016-03-18 14:12:25]

The CQ bit was checked by reed@google.com

[reed1, 2016-03-18 14:12:26]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1811703002/#ps40001
(title: "rely on RVO in picturerecorder")

[commit-bot: I haz the power, 2016-03-18 14:12:32]

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

[commit-bot: I haz the power, 2016-03-18 14:25:58]

Description was changed from

==========
return pictures as sk_sp

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

to

==========
return pictures as sk_sp

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

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

[commit-bot: I haz the power, 2016-03-18 14:26:00]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/ca2622ba051829fed5f30facd74c5b41cd4b931c