Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and DrawMask

Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and DrawMask
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1955633002

Committed: https://skia.googlesource.com/skia/+/055e192adc0072ae2548ef5431ceee652945f9c2

[bsalomon, 2016-05-05 18:36:49]

Description was changed from

==========
Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and
DrawMask
==========

to

==========
Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and
DrawMask
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-05-05 19:09:57]

bsalomon@google.com changed reviewers:
+ reed@google.com, robertphillips@google.com

[bsalomon, 2016-05-05 19:09:58]

The current callers always pass SkPaint::kFill or SkPaint::kStyle. The
implementation puts the passed style on a vanilla paint which means kStyle gets
interpreted as kHairline. This just makes the fill vs hairline limitation
explicit in the signatures.

[bsalomon, 2016-05-05 19:10:18]

On 2016/05/05 19:09:58, bsalomon wrote:
> The current callers always pass SkPaint::kFill or SkPaint::kStyle. The
> implementation puts the passed style on a vanilla paint which means kStyle
gets
> interpreted as kHairline. This just makes the fill vs hairline limitation
> explicit in the signatures.

Follow up CL will move SkStrokeRec to include/private.

[robertphillips, 2016-05-05 19:52:06]

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2047: case SkStrokeRec::kHairline_InitStyle:
!paint.getStrokeWidth() ?

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2051: case SkStrokeRec::kFill_InitStyle:
xtra space ?

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:238: int rectCount = 0;
yoda speak ?

[robertphillips, 2016-05-05 19:57:20]

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:217: const SkRasterClip& clip, SkBlitter* blitter,
style is unused here ?
can we delete is as a parameter ?

[robertphillips, 2016-05-05 20:04:14]

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:208: 
count_nested_fill_rects ?

[robertphillips, 2016-05-05 20:09:37]

lgtm

[reed1, 2016-05-05 20:12:55]

I like making strokerec private.
However, SkDraw is public, so we can't have it take a private type as a
parameter...

[reed1, 2016-05-05 20:15:00]

... unless we can make the function(s) that take the private type.. private?

1. SkDraw is public only because chrome still has a subclass of SkDevice, whose
methods take a SkDraw (afaik)

2. Perhaps we can make lots more of SkDraw private (?)

[bsalomon, 2016-05-05 20:15:17]

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2047: case SkStrokeRec::kHairline_InitStyle:
On 2016/05/05 19:52:06, robertphillips wrote:
> !paint.getStrokeWidth() ?

Done.

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2051: case SkStrokeRec::kFill_InitStyle:
On 2016/05/05 19:52:06, robertphillips wrote:
> xtra space ?

Done. (actually other line is shy one space.)

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

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:217: const SkRasterClip& clip, SkBlitter* blitter,
On 2016/05/05 19:57:20, robertphillips wrote:
> style is unused here ?
> can we delete is as a parameter ?

I think so. The one caller (SkDraw::drawRRect) does take the path case if the
style is not fill and always passes kFill to this function. There is a comment
there about investigating optimizations for other cases but since the
implementation assumes kFill it's probably better to remove the parameter and
readd it in the glorious future when all TODOs are addressed.

https://codereview.chromium.org/1955633002/diff/20001/src/core/SkMaskFilter.c...
src/core/SkMaskFilter.cpp:238: int rectCount = 0;
On 2016/05/05 19:52:06, robertphillips wrote:
> yoda speak ?

Done.

[bsalomon, 2016-05-05 20:16:33]

On 2016/05/05 20:15:00, reed1 wrote:
> ... unless we can make the function(s) that take the private type.. private?
> 
> 1. SkDraw is public only because chrome still has a subclass of SkDevice,
whose
> methods take a SkDraw (afaik)
> 
> 2. Perhaps we can make lots more of SkDraw private (?)

I'll look into that... was wondering why SkDraw was public.

[reed1, 2016-05-05 20:18:03]

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

[reed1, 2016-05-05 20:18:04]

Tom is working on trying to eliminate chrome subclasses...

[bsalomon, 2016-05-05 20:26:52]

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

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

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

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

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

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

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[bsalomon, 2016-05-05 21:09:40]

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

[commit-bot: I haz the power, 2016-05-05 21:09:50]

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

[commit-bot: I haz the power, 2016-05-05 21:12:20]

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

[commit-bot: I haz the power, 2016-05-05 21:12:21]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bsalomon, 2016-05-06 13:24:55]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-06 13:24:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1955633002/#ps80001
(title: "change xps")

[commit-bot: I haz the power, 2016-05-06 13:25:02]

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

[commit-bot: I haz the power, 2016-05-06 13:27:53]

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

[commit-bot: I haz the power, 2016-05-06 13:27:54]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bsalomon, 2016-05-06 13:30:42]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-05-06 13:30:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1955633002/#ps100001
(title: "fix xps change")

[commit-bot: I haz the power, 2016-05-06 13:30:53]

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

[bsalomon, 2016-05-06 13:36:43]

On 2016/05/05 20:18:04, reed1 wrote:
> Tom is working on trying to eliminate chrome subclasses...

I realized SkStrokeRec is part of the SkPathEffect interface, so no hope of
making it private unless we remove SkPathEffect from the public API or change
its signature.

[commit-bot: I haz the power, 2016-05-06 14:23:00]

Description was changed from

==========
Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and
DrawMask
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Take SkStrokeRec::InitStyle rather than SkPaint::Style in mask filter and
DrawMask
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/055e192adc0072ae2548ef5431ceee652945f9c2
==========

[commit-bot: I haz the power, 2016-05-06 14:23:01]

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