move setshader to sk_sp

move setshader to sk_sp, re-using SK_SUPPORT_LEGACY_CREATESHADER_PTR

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

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

[reed1, 2016-03-25 03:29:58]

Description was changed from

==========
move setshader to sk_sp

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

to

==========
move setshader to sk_sp

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

[reed1, 2016-03-25 12:49:54]

Description was changed from

==========
move setshader to sk_sp

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

to

==========
move setshader to sk_sp, re-using SK_SUPPORT_LEGACY_CREATESHADER_PTR

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

[reed1, 2016-03-25 12:50:38]

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

[reed1, 2016-03-25 14:22:42]

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

[commit-bot: I haz the power, 2016-03-25 14:22:50]

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

[reed1, 2016-03-25 14:24:45]

The CQ bit was unchecked by reed@google.com

[tomhudson, 2016-03-25 14:28:07]

https://codereview.chromium.org/1829303002/diff/20001/gm/pictureshader.cpp
File gm/pictureshader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/gm/pictureshader.cpp#ne...
gm/pictureshader.cpp:207: p.setShader(std::move(shader));
Nit: could fold shader construction into this line? Wasn't that a pattern you
wanted to encourage?

https://codereview.chromium.org/1829303002/diff/20001/samplecode/SampleLighti...
File samplecode/SampleLighting.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/samplecode/SampleLighti...
samplecode/SampleLighting.cpp:78: nullptr, nullptr);
We have to regenerate this every time the lights change angle and color? :(

But if it's never valid, is there a point to storing it in fShader? Or should we
just set it on the paint?

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
src/effects/SkPerlinNoiseShader.cpp:258: seed, tileSize));
This sk_sp<SkShader>(new ...(...)) is pretty verbose - are we doing it just to
force the return type correct? We can't just write sk_make_sp<...>(...)?

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:437: State(SkShader* shader, const SkMatrix&
canvasTransform,
Changing PDF to raw pointers instead of sk_sp<>?

[reed1, 2016-03-25 15:04:31]

https://codereview.chromium.org/1829303002/diff/20001/gm/pictureshader.cpp
File gm/pictureshader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/gm/pictureshader.cpp#ne...
gm/pictureshader.cpp:207: p.setShader(std::move(shader));
On 2016/03/25 14:28:07, tomhudson wrote:
> Nit: could fold shader construction into this line? Wasn't that a pattern you
> wanted to encourage?

Done.

https://codereview.chromium.org/1829303002/diff/20001/samplecode/SampleLighti...
File samplecode/SampleLighting.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/samplecode/SampleLighti...
samplecode/SampleLighting.cpp:78: nullptr, nullptr);
On 2016/03/25 14:28:07, tomhudson wrote:
> We have to regenerate this every time the lights change angle and color? :(
> 
> But if it's never valid, is there a point to storing it in fShader? Or should
we
> just set it on the paint?

The lights are animating, so we have to regenerate each time. I have removed
fShader as a field.

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
src/effects/SkPerlinNoiseShader.cpp:258: seed, tileSize));
On 2016/03/25 14:28:07, tomhudson wrote:
> This sk_sp<SkShader>(new ...(...)) is pretty verbose - are we doing it just to
> force the return type correct? We can't just write sk_make_sp<...>(...)?

We have to do this in places where the actual constructor is not public. A
larger refactor of just perlinnoise could do that, by moving the class
definition into src, and just exposing the factories to the public.

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:437: State(SkShader* shader, const SkMatrix&
canvasTransform,
On 2016/03/25 14:28:07, tomhudson wrote:
> Changing PDF to raw pointers instead of sk_sp<>?

There are lots of changes we could do internally to PDF to make it more sp aware
(like using sp for the PFObject dictionary instead of raw-ptrs). I'd rather do
those independent of this CL, which is focused on modernizing the public APIs
(SkPaint in particular).

[reed1, 2016-03-25 15:04:56]

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

[commit-bot: I haz the power, 2016-03-25 15:05:07]

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

[reed1, 2016-03-25 15:05:32]

The CQ bit was unchecked by reed@google.com

[tomhudson, 2016-03-25 15:39:50]

LGTM.

Agreed on the PDF thing being a separate CL; I pointed Hal at this one just to
make sure it was good with him.

I'll change https://codereview.chromium.org/1831883002/ to not actually remove
the flag you're reusing; will try to land it next week (while you're on
vacation?) and write a separate CL to adapt to this set of changes.

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
File src/effects/SkPerlinNoiseShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/effects/SkPerlinNoi...
src/effects/SkPerlinNoiseShader.cpp:258: seed, tileSize));
On 2016/03/25 15:04:31, reed1 wrote:
> On 2016/03/25 14:28:07, tomhudson wrote:
> > This sk_sp<SkShader>(new ...(...)) is pretty verbose - are we doing it just
to
> > force the return type correct? We can't just write sk_make_sp<...>(...)?
> 
> We have to do this in places where the actual constructor is not public. A
> larger refactor of just perlinnoise could do that, by moving the class
> definition into src, and just exposing the factories to the public.

Acknowledged.

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/1829303002/diff/20001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:437: State(SkShader* shader, const SkMatrix&
canvasTransform,
On 2016/03/25 15:04:31, reed1 wrote:
> On 2016/03/25 14:28:07, tomhudson wrote:
> > Changing PDF to raw pointers instead of sk_sp<>?
> 
> There are lots of changes we could do internally to PDF to make it more sp
aware
> (like using sp for the PFObject dictionary instead of raw-ptrs). I'd rather do
> those independent of this CL, which is focused on modernizing the public APIs
> (SkPaint in particular).

Acknowledged.

[reed1, 2016-03-25 16:06:53]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-03-25 16:07:02]

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

[commit-bot: I haz the power, 2016-03-25 16:08:03]

Description was changed from

==========
move setshader to sk_sp, re-using SK_SUPPORT_LEGACY_CREATESHADER_PTR

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

to

==========
move setshader to sk_sp, re-using SK_SUPPORT_LEGACY_CREATESHADER_PTR

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/+/fe63045f075165b1be5d3e7fc5c710021d85f53b
==========

[commit-bot: I haz the power, 2016-03-25 16:08:04]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/fe63045f075165b1be5d3e7fc5c710021d85f53b