switch maskfilters to sk_sp

switch maskfilters to sk_sp

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

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

[reed1, 2016-04-04 11:39:47]

Description was changed from

==========
switch maskfilters to sk_sp

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

to

==========
switch maskfilters to sk_sp

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

[reed1, 2016-04-04 13:27:25]

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

[commit-bot: I haz the power, 2016-04-04 13:27:29]

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

[reed1, 2016-04-04 13:28:13]

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

[reed1, 2016-04-04 13:42:17]

The CQ bit was unchecked by reed@google.com

[f(malita), 2016-04-04 13:46:57]

https://codereview.chromium.org/1852113003/diff/20001/gm/megalooper.cpp
File gm/megalooper.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/gm/megalooper.cpp#newco...
gm/megalooper.cpp:113: sk_sp<SkMaskFilter> createBlur() {
nit: should be static?

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cpp
File samplecode/SampleAll.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cp...
samplecode/SampleAll.cpp:423: paint.setMaskFilter(embossFilter);
nit: std::move

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp#newc...
tests/BlurTest.cpp:530: test_layerDrawLooper(reporter, mf, sigma, style,
quality, success);
nit: std::move(mf)

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:1150:
target->setMaskFilter(sk_ref_sp(maskFilter));
I think this leaks: local ptr already holds a ref (based on existing unref), so
we need to adopt into sk_sp, not ref.

[f(malita), 2016-04-04 13:50:15]

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:1150:
target->setMaskFilter(sk_ref_sp(maskFilter));
On 2016/04/04 13:46:57, f(malita) wrote:
> I think this leaks: local ptr already holds a ref (based on existing unref),
so
> we need to adopt into sk_sp, not ref.

(maybe refactor as sk_sp<SkMaskFilter> maskFilter(...) for clarity?)

[tomhudson, 2016-04-04 14:18:59]

LGTM modulo Florin's notes.

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cpp
File samplecode/SampleAll.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cp...
samplecode/SampleAll.cpp:423: paint.setMaskFilter(embossFilter);
On 2016/04/04 13:46:57, f(malita) wrote:
> nit: std::move

Or just construct it inline here?

https://codereview.chromium.org/1852113003/diff/20001/src/animator/SkDrawPain...
File src/animator/SkDrawPaint.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/src/animator/SkDrawPain...
src/animator/SkDrawPaint.cpp:232:
paint->setMaskFilter(sk_sp<SkMaskFilter>(maskFilter->getMaskFilter()));
getMaskFilter() is always returning a pointer that needs to be adopted like
this? (e.g. one already ref'd?)

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp#newc...
tests/BlurTest.cpp:546: if (mf) {
There shouldn't be a failure path returning NULL here, should there?

[reed1, 2016-04-04 16:34:48]

https://codereview.chromium.org/1852113003/diff/20001/gm/megalooper.cpp
File gm/megalooper.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/gm/megalooper.cpp#newco...
gm/megalooper.cpp:113: sk_sp<SkMaskFilter> createBlur() {
On 2016/04/04 13:46:57, f(malita) wrote:
> nit: should be static?

Done.

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cpp
File samplecode/SampleAll.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/samplecode/SampleAll.cp...
samplecode/SampleAll.cpp:423: paint.setMaskFilter(embossFilter);
On 2016/04/04 14:18:58, tomhudson wrote:
> On 2016/04/04 13:46:57, f(malita) wrote:
> > nit: std::move
> 
> Or just construct it inline here?

Done.

https://codereview.chromium.org/1852113003/diff/20001/src/animator/SkDrawPain...
File src/animator/SkDrawPaint.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/src/animator/SkDrawPain...
src/animator/SkDrawPaint.cpp:232:
paint->setMaskFilter(sk_sp<SkMaskFilter>(maskFilter->getMaskFilter()));
On 2016/04/04 14:18:58, tomhudson wrote:
> getMaskFilter() is always returning a pointer that needs to be adopted like
> this? (e.g. one already ref'd?)

I think so. I think the old code was leaking.

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp#newc...
tests/BlurTest.cpp:530: test_layerDrawLooper(reporter, mf, sigma, style,
quality, success);
On 2016/04/04 13:46:57, f(malita) wrote:
> nit: std::move(mf)

I find explicitly using move to be fragile feeling, esp. when we are not at the
scope-end of the variable's lifetime. I will add it here, but perhaps we can
have a group discussion about when to use move.

https://codereview.chromium.org/1852113003/diff/20001/tests/BlurTest.cpp#newc...
tests/BlurTest.cpp:546: if (mf) {
On 2016/04/04 14:18:58, tomhudson wrote:
> There shouldn't be a failure path returning NULL here, should there?

Our factories can return null if they think the inputs are invalid (or result in
an identity effect). If you think this test should require a filter, that may be
true, but this refactor isn't focused on that.

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
File tools/debugger/SkDrawCommand.cpp (right):

https://codereview.chromium.org/1852113003/diff/20001/tools/debugger/SkDrawCo...
tools/debugger/SkDrawCommand.cpp:1150:
target->setMaskFilter(sk_ref_sp(maskFilter));
On 2016/04/04 13:50:15, f(malita) wrote:
> On 2016/04/04 13:46:57, f(malita) wrote:
> > I think this leaks: local ptr already holds a ref (based on existing unref),
> so
> > we need to adopt into sk_sp, not ref.
> 
> (maybe refactor as sk_sp<SkMaskFilter> maskFilter(...) for clarity?)

Done.

[f(malita), 2016-04-04 16:41:43]

lgtm

[reed1, 2016-04-04 16:43:43]

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

[commit-bot: I haz the power, 2016-04-04 16:43:47]

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

[reed1, 2016-04-04 16:47:22]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-04-04 17:01:46]

The CQ bit was checked by reed@google.com

[reed1, 2016-04-04 17:01:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/1852113003/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-04-04 17:01:53]

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

[commit-bot: I haz the power, 2016-04-04 17:03:01]

Description was changed from

==========
switch maskfilters to sk_sp

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

to

==========
switch maskfilters to 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/+/efdfd51b68a300d5c6e28743fe0344ca05d1cec9
==========

[commit-bot: I haz the power, 2016-04-04 17:03:02]

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