switch xfermodes over to sk_sp

switch xfermodes over to sk_sp

waiting on https://codereview.chromium.org/1835163002/

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

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

[reed1, 2016-03-25 20:57:30]

Description was changed from

==========
switch xfermodes over to sk_sp

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

to

==========
switch xfermodes over 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 21:10:11]

Description was changed from

==========
switch xfermodes over to sk_sp

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

to

==========
switch xfermodes over to sk_sp

WIP - have not yet completed the switch-over, nor have I added a guard for
chrome/android/google3

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

[reed1, 2016-03-25 21:10:31]

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

[reed1, 2016-03-26 02:59:40]

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

[commit-bot: I haz the power, 2016-03-26 02:59:49]

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

[commit-bot: I haz the power, 2016-03-26 03:01:49]

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

[commit-bot: I haz the power, 2016-03-26 03:01:51]

Dry run: Try jobs failed on following builders:
  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...)

[reed1, 2016-03-26 03:44:30]

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

[commit-bot: I haz the power, 2016-03-26 03:44:41]

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

[reed1, 2016-03-26 03:51:05]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-03-26 03:55:35]

Description was changed from

==========
switch xfermodes over to sk_sp

WIP - have not yet completed the switch-over, nor have I added a guard for
chrome/android/google3

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

to

==========
switch xfermodes over 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-26 03:58:02]

Description was changed from

==========
switch xfermodes over to sk_sp

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

to

==========
switch xfermodes over to sk_sp

waiting on https://codereview.chromium.org/1835163002/

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

[tomhudson, 2016-03-28 17:46:25]

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

[tomhudson, 2016-03-28 17:46:26]

lgtm

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkComposeShader.h
File src/core/SkComposeShader.h (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkComposeShade...
src/core/SkComposeShader.h:31: SkComposeShader(sk_sp<SkShader> sA,
sk_sp<SkShader> sB, sk_sp<SkXfermode> mode)
Nit: Removed the default argument; will need to update callers?

[f(malita), 2016-03-28 18:44:59]

lgtm

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:1912: xmode->unref();
Nit: looks a bit convoluted, plus we unnecessarily ref/unref when creating a
local xmode.  How about something like this instead

  sk_sp<SkXferMode> mode = xmode
      ? sk_ref_sp(xmode)
      : SkXfermode::Make(SkXfermode::kModulate_Mode);
  p.setShader(SkShader::MakeComposeShader(..., std::move(mode));

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:318: canvas->drawPatch(cubics, colors, texCoords,
xfer, paint);
nit: std::move(xfer)?

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:465: canvas->drawVertices(vmode, vCount, verts,
texs, colors, xfer, indices, iCount, paint);
nit: ditto

https://codereview.chromium.org/1832223002/diff/60001/src/effects/SkXfermodeI...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/effects/SkXfermodeI...
src/effects/SkXfermodeImageFilter.cpp:43: return Make(mode, common.getInput(0),
common.getInput(1), &common.cropRect()).release();
nit: std::move(mode), or inline?

[reed1, 2016-03-29 16:13:52]

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkComposeShader.h
File src/core/SkComposeShader.h (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkComposeShade...
src/core/SkComposeShader.h:31: SkComposeShader(sk_sp<SkShader> sA,
sk_sp<SkShader> sB, sk_sp<SkXfermode> mode)
On 2016/03/28 17:46:26, tomhudson wrote:
> Nit: Removed the default argument; will need to update callers?

This class is private (in src/core) so all the callers have been updated.

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:1912: xmode->unref();
On 2016/03/28 18:44:59, f(malita) wrote:
> Nit: looks a bit convoluted, plus we unnecessarily ref/unref when creating a
> local xmode.  How about something like this instead
> 
>   sk_sp<SkXferMode> mode = xmode
>       ? sk_ref_sp(xmode)
>       : SkXfermode::Make(SkXfermode::kModulate_Mode);
>   p.setShader(SkShader::MakeComposeShader(..., std::move(mode));

Good suggestion. Done.

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:318: canvas->drawPatch(cubics, colors, texCoords,
xfer, paint);
On 2016/03/28 18:44:59, f(malita) wrote:
> nit: std::move(xfer)?

Done.

https://codereview.chromium.org/1832223002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:465: canvas->drawVertices(vmode, vCount, verts,
texs, colors, xfer, indices, iCount, paint);
On 2016/03/28 18:44:59, f(malita) wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1832223002/diff/60001/src/effects/SkXfermodeI...
File src/effects/SkXfermodeImageFilter.cpp (right):

https://codereview.chromium.org/1832223002/diff/60001/src/effects/SkXfermodeI...
src/effects/SkXfermodeImageFilter.cpp:43: return Make(mode, common.getInput(0),
common.getInput(1), &common.cropRect()).release();
On 2016/03/28 18:44:59, f(malita) wrote:
> nit: std::move(mode), or inline?

Done.

[reed1, 2016-03-29 18:21:14]

The CQ bit was checked by reed@google.com

[reed1, 2016-03-29 18:21:15]

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

[commit-bot: I haz the power, 2016-03-29 18:21:25]

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

[commit-bot: I haz the power, 2016-03-29 18:32:53]

Description was changed from

==========
switch xfermodes over to sk_sp

waiting on https://codereview.chromium.org/1835163002/

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

to

==========
switch xfermodes over to sk_sp

waiting on https://codereview.chromium.org/1835163002/

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

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

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

[dogben, 2016-04-01 20:08:50]

benjaminwagner@google.com changed reviewers:
+ benjaminwagner@google.com

[dogben, 2016-04-01 20:08:52]

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
File include/effects/SkXfermodeImageFilter.h (right):

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
include/effects/SkXfermodeImageFilter.h:24: static sk_sp<SkImageFilter>
Make(sk_sp<SkXfermode> mode, SkImageFilter* background,
Shouldn't this accept background and foreground as sk_sp?

[reed1, 2016-04-02 14:26:56]

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
File include/effects/SkXfermodeImageFilter.h (right):

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
include/effects/SkXfermodeImageFilter.h:24: static sk_sp<SkImageFilter>
Make(sk_sp<SkXfermode> mode, SkImageFilter* background,
On 2016/04/01 20:08:52, Ben Wagner wrote:
> Shouldn't this accept background and foreground as sk_sp?

Indeed. Subsequent changes will fix those. Since (at the time of this CL)
imagefilter factories had not been converted to return sk_sp, that aspect was
left alone for now.

[robertphillips, 2016-04-03 18:26:46]

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
File include/effects/SkXfermodeImageFilter.h (right):

https://codereview.chromium.org/1832223002/diff/100001/include/effects/SkXfer...
include/effects/SkXfermodeImageFilter.h:24: static sk_sp<SkImageFilter>
Make(sk_sp<SkXfermode> mode, SkImageFilter* background,
Yep - I'm slowly churning my way through all the ImageFilters.