Swap SkPictureImageFilter's factories over to smart pointers

Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium first
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1831323003

Committed: https://skia.googlesource.com/skia/+/5ff17b13524eea03e4b672ee3b3a49b649dd09fb

[robertphillips, 2016-03-25 18:54:35]

Description was changed from

==========
Swap SkPictureImageFilter's factories over to smart pointers
==========

to

==========
Swap SkPictureImageFilter's factories over to smart pointers
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-25 18:59:00]

Description was changed from

==========
Swap SkPictureImageFilter's factories over to smart pointers
==========

to

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest
==========

[robertphillips, 2016-03-25 18:59:00]

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

[robertphillips, 2016-03-25 18:59:44]

Description was changed from

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest
==========

to

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add
SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium
first
==========

[reed1, 2016-03-25 19:32:36]

api lgtm

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
include/effects/SkPictureImageFilter.h:19: static sk_sp<SkImageFilter>
Make(sk_sp<SkPicture> picture) {
This style is fine.

However, at some point, I'd like us to also consider making the factories
actually perform checks on their inputs (e.g. null picture input) so they can
return null instead of creating imagefilters that either crash or don't work.

[robertphillips, 2016-03-28 12:02:22]

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
include/effects/SkPictureImageFilter.h:19: static sk_sp<SkImageFilter>
Make(sk_sp<SkPicture> picture) {
On 2016/03/25 19:32:36, reed1 wrote:
> This style is fine.
> 
> However, at some point, I'd like us to also consider making the factories
> actually perform checks on their inputs (e.g. null picture input) so they can
> return null instead of creating imagefilters that either crash or don't work.

The picture image filter is actually defined to work with no picture. We can
check with Florin but I believe it is to handle corner cases with filters that
affect transparent black.

[f(malita), 2016-03-28 13:00:08]

lgtm

https://codereview.chromium.org/1831323003/diff/1/gm/pictureimagefilter.cpp
File gm/pictureimagefilter.cpp (right):

https://codereview.chromium.org/1831323003/diff/1/gm/pictureimagefilter.cpp#n...
gm/pictureimagefilter.cpp:61: auto
pictureSource(SkPictureImageFilter::Make(fPicture));
I recently found that Chromium discourages auto for smart pointers (discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0).

Not sure whether we should do the same in Skia, but I agree with the sentiment
there that auto makes ownership non-obvious (and possibly fragile if we ever
change the type).

Just making a note, if we decide to disallow auto sk_sps we'll have to do a
cleanup pass.

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
File include/effects/SkPictureImageFilter.h (right):

https://codereview.chromium.org/1831323003/diff/1/include/effects/SkPictureIm...
include/effects/SkPictureImageFilter.h:19: static sk_sp<SkImageFilter>
Make(sk_sp<SkPicture> picture) {
On 2016/03/28 12:02:22, robertphillips wrote:
> On 2016/03/25 19:32:36, reed1 wrote:
> > This style is fine.
> > 
> > However, at some point, I'd like us to also consider making the factories
> > actually perform checks on their inputs (e.g. null picture input) so they
can
> > return null instead of creating imagefilters that either crash or don't
work.
> 
> The picture image filter is actually defined to work with no picture. We can
> check with Florin but I believe it is to handle corner cases with filters that
> affect transparent black.

I don't recall the image filter specifics, but that sounds reasonable.

[robertphillips, 2016-03-28 17:22:41]

Description was changed from

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add
SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium
first
==========

to

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add
SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium
first
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-28 17:25:36]

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

[robertphillips, 2016-03-28 17:25:43]

https://codereview.chromium.org/1831323003/diff/1/gm/pictureimagefilter.cpp
File gm/pictureimagefilter.cpp (right):

https://codereview.chromium.org/1831323003/diff/1/gm/pictureimagefilter.cpp#n...
gm/pictureimagefilter.cpp:61: auto
pictureSource(SkPictureImageFilter::Make(fPicture));
On 2016/03/28 13:00:08, f(malita) wrote:
> I recently found that Chromium discourages auto for smart pointers
(discussion:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0).
> 
> Not sure whether we should do the same in Skia, but I agree with the sentiment
> there that auto makes ownership non-obvious (and possibly fragile if we ever
> change the type).
> 
> Just making a note, if we decide to disallow auto sk_sps we'll have to do a
> cleanup pass.

Done.

[commit-bot: I haz the power, 2016-03-28 17:25:43]

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

[commit-bot: I haz the power, 2016-03-28 17:27:13]

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

[commit-bot: I haz the power, 2016-03-28 17:27:14]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  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...)

[robertphillips, 2016-03-28 19:42:59]

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

[commit-bot: I haz the power, 2016-03-28 19:43:06]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-28 20:12:36]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2016-03-28 20:12:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
fmalita@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1831323003/#ps40001
(title: "Fix sk_sp bug")

[commit-bot: I haz the power, 2016-03-28 20:12:47]

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

[commit-bot: I haz the power, 2016-03-28 20:13:44]

Description was changed from

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add
SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium
first
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Swap SkPictureImageFilter's factories over to smart pointers

A trial balloon before converting the rest

This requires https://codereview.chromium.org/1836443003/ (add
SK_SUPPORT_LEGACY_IMAGEFILTER_PTR flag for future skia CL) to land in Chromium
first
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/5ff17b13524eea03e4b672ee3b3a49b649dd09fb
==========

[commit-bot: I haz the power, 2016-03-28 20:13:45]

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