Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

Committed: https://skia.googlesource.com/skia/+/372177ee115d46dfb5bfb881a408e6c37ae83678

[robertphillips, 2016-03-29 16:17:01]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp
==========

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-29 16:41:08]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-29 16:41:08]

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

[robertphillips, 2016-03-29 17:22:36]

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

[commit-bot: I haz the power, 2016-03-29 17:22:40]

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

[commit-bot: I haz the power, 2016-03-29 17:39:20]

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

[commit-bot: I haz the power, 2016-03-29 17:39:21]

Dry run: This issue passed the CQ dry run.

[f(malita), 2016-03-29 18:10:07]

https://codereview.chromium.org/1842793002/diff/20001/gm/localmatriximagefilt...
File gm/localmatriximagefilter.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/gm/localmatriximagefilt...
gm/localmatriximagefilter.cpp:91: show_image(canvas, image0.get(), localFilter);
nit: std::move(localFilter)

https://codereview.chromium.org/1842793002/diff/20001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:563: filter =
sk_sp<SkImageFilter>(SkMergeImageFilter::Create(make_image_filter().release(),
Hmm...

Mixing old-style Create methods with sk_sps is a bit subtle: Create doesn't
normally adopt a ref but grabs a new one, so doing something like
Create(sp.release()) usually results in a leak.

Now granted this CL is correct in that it doesn't seem to change ownership
semantics, and preserves what I suspect is an existing leak.  Do we have ASAN
coverage for this code at all?

I think the safe pattern is something like

  sk_sp<SkImageFilter> subfilter = make_image_filter();
  filter = sk_sk<SkImageFilter>(SkFooImageFilter::Create(subfilter.get(), ...));

(same ref scoping as the other Create args)

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1166: p.setImageFilter(localF);
std::move(localF) or inline

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:579: return SkLocalMatrixImageFilter::Make(matrix,
sk_sp<SkImageFilter>(SkRef(nonConstThis)));
Nit: sk_ref_sp(nonConstThis)?

[robertphillips, 2016-03-29 19:31:40]

https://codereview.chromium.org/1842793002/diff/20001/gm/localmatriximagefilt...
File gm/localmatriximagefilter.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/gm/localmatriximagefilt...
gm/localmatriximagefilter.cpp:91: show_image(canvas, image0.get(), localFilter);
On 2016/03/29 18:10:06, f(malita) wrote:
> nit: std::move(localFilter)

Done.

https://codereview.chromium.org/1842793002/diff/20001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:563: filter =
sk_sp<SkImageFilter>(SkMergeImageFilter::Create(make_image_filter().release(),
On 2016/03/29 18:10:06, f(malita) wrote:
> Hmm...
> 
> Mixing old-style Create methods with sk_sps is a bit subtle: Create doesn't
> normally adopt a ref but grabs a new one, so doing something like
> Create(sp.release()) usually results in a leak.
> 
> Now granted this CL is correct in that it doesn't seem to change ownership
> semantics, and preserves what I suspect is an existing leak.  Do we have ASAN
> coverage for this code at all?
> 
> I think the safe pattern is something like
> 
>   sk_sp<SkImageFilter> subfilter = make_image_filter();
>   filter = sk_sk<SkImageFilter>(SkFooImageFilter::Create(subfilter.get(),
...));
> 
> (same ref scoping as the other Create args)

Done.

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1166: p.setImageFilter(localF);
On 2016/03/29 18:10:06, f(malita) wrote:
> std::move(localF) or inline

Done.

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1842793002/diff/20001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:579: return SkLocalMatrixImageFilter::Make(matrix,
sk_sp<SkImageFilter>(SkRef(nonConstThis)));
On 2016/03/29 18:10:06, f(malita) wrote:
> Nit: sk_ref_sp(nonConstThis)?

Done.

[f(malita), 2016-03-29 19:57:24]

lgtm

[robertphillips, 2016-03-29 20:25:33]

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

[commit-bot: I haz the power, 2016-03-29 20:25:42]

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

[commit-bot: I haz the power, 2016-03-29 20:38:04]

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

[commit-bot: I haz the power, 2016-03-29 20:38:05]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-29 20:41:59]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2016-03-29 20:42:06]

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

[commit-bot: I haz the power, 2016-03-29 20:44:18]

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

[commit-bot: I haz the power, 2016-03-29 20:44:19]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[robertphillips, 2016-03-29 20:52:44]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

[robertphillips, 2016-03-29 20:52:57]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2016-03-29 20:53:05]

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

[commit-bot: I haz the power, 2016-03-29 20:54:28]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

[commit-bot: I haz the power, 2016-03-29 20:54:29]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/a33cf07a2273315363c0b6fb5d3ce811742f5a85

[djsollen, 2016-03-30 02:06:50]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1837293003/ by djsollen@google.com.

The reason for reverting is: This CL is causing the autoroll into Chromium &
google3 to fail..

[robertphillips, 2016-03-30 12:12:06]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

[robertphillips, 2016-03-30 12:13:37]

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

[commit-bot: I haz the power, 2016-03-30 12:13:48]

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

[commit-bot: I haz the power, 2016-03-30 12:24:38]

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

[commit-bot: I haz the power, 2016-03-30 12:24:40]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-30 14:31:14]

The CQ bit was checked by robertphillips@google.com

[robertphillips, 2016-03-30 14:31:14]

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/1842793002/#ps120001
(title: "Ooops")

[commit-bot: I haz the power, 2016-03-30 14:31:26]

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

[commit-bot: I haz the power, 2016-03-30 14:32:31]

Description was changed from

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

to

==========
Switch SkLocalMatrixImageFilter and SkPaintImageFilter over to sk_sp

TBR=reed@google.com

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

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

Committed:
https://skia.googlesource.com/skia/+/372177ee115d46dfb5bfb881a408e6c37ae83678
==========

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

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/372177ee115d46dfb5bfb881a408e6c37ae83678