update unittest EmptyPath -- do not inverse-stroke a path

update unittest EmptyPath -- do not inverse-stroke a path

BUG=skia:2222

[bsalomon, 2014-05-05 13:47:49]

https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
File tests/EmptyPathTest.cpp (right):

https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType() &&
what about kStrokeAndFill? Shouldn't it draw?

[yunchao, 2014-05-05 15:01:42]

https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
File tests/EmptyPathTest.cpp (right):

https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType() &&
On 2014/05/05 13:47:49, bsalomon wrote:
I think so. In my oponion, FillType(inverse or not) is supposed to be used for
kFill_Style SkPaint, not for stroke as well as strokeAndFill styles. Also see
the CL at https://codereview.chromium.org/183683010/. Seems that you also think
we should not inverse-fill for strokeAndFill style.

[bsalomon, 2014-05-05 17:53:19]

On 2014/05/05 15:01:42, Richard Ho wrote:
> https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
> File tests/EmptyPathTest.cpp (right):
> 
>
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
> tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType() &&
> On 2014/05/05 13:47:49, bsalomon wrote:
> I think so. In my oponion, FillType(inverse or not) is supposed to be used for
> kFill_Style SkPaint, not for stroke as well as strokeAndFill styles. Also see
> the CL at https://codereview.chromium.org/183683010/. Seems that you also
think
> we should not inverse-fill for strokeAndFill style.

Are you saying kStrokeAndFill should draw for inverse-filled empty path or not?

[yunchao, 2014-05-05 23:44:18]

On 2014/05/05 17:53:19, bsalomon wrote:
> On 2014/05/05 15:01:42, Richard Ho wrote:
> > https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
> > File tests/EmptyPathTest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
> > tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType() &&
> > On 2014/05/05 13:47:49, bsalomon wrote:
> > I think so. In my oponion, FillType(inverse or not) is supposed to be used
for
> > kFill_Style SkPaint, not for stroke as well as strokeAndFill styles. Also
see
> > the CL at https://codereview.chromium.org/183683010/. Seems that you also
> think
> > we should not inverse-fill for strokeAndFill style.
> 
> Are you saying kStrokeAndFill should draw for inverse-filled empty path or
not?

kStrokeAndFill should not draw inverse-filled empty path. We should be
consistent that inverse-fill will not inverse-stroke for kStroke and
kStrokeAndFill styles. The only exception is that hairline stroke + fill = fill,
so we inverse-fill for this edge case of kStrokeAndFill style.

[bsalomon, 2014-05-06 13:22:51]

On 2014/05/05 23:44:18, Richard Ho wrote:
> On 2014/05/05 17:53:19, bsalomon wrote:
> > On 2014/05/05 15:01:42, Richard Ho wrote:
> > > https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
> > > File tests/EmptyPathTest.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
> > > tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType() &&
> > > On 2014/05/05 13:47:49, bsalomon wrote:
> > > I think so. In my oponion, FillType(inverse or not) is supposed to be used
> for
> > > kFill_Style SkPaint, not for stroke as well as strokeAndFill styles. Also
> see
> > > the CL at https://codereview.chromium.org/183683010/. Seems that you also
> > think
> > > we should not inverse-fill for strokeAndFill style.
> > 
> > Are you saying kStrokeAndFill should draw for inverse-filled empty path or
> not?
> 
> kStrokeAndFill should not draw inverse-filled empty path. We should be
> consistent that inverse-fill will not inverse-stroke for kStroke and
> kStrokeAndFill styles. The only exception is that hairline stroke + fill =
fill,
> so we inverse-fill for this edge case of kStrokeAndFill style.

My understanding is that kStrokeAndFill will draw as though the path were
stroked and filled. That would mean the stroke draws nothing but the fill would
paint the entire plane.

[yunchao, 2014-05-06 13:44:38]

On 2014/05/06 13:22:51, bsalomon wrote:
> On 2014/05/05 23:44:18, Richard Ho wrote:
> > On 2014/05/05 17:53:19, bsalomon wrote:
> > > On 2014/05/05 15:01:42, Richard Ho wrote:
> > > > https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
> > > > File tests/EmptyPathTest.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
> > > > tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType()
&&
> > > > On 2014/05/05 13:47:49, bsalomon wrote:
> > > > I think so. In my oponion, FillType(inverse or not) is supposed to be
used
> > for
> > > > kFill_Style SkPaint, not for stroke as well as strokeAndFill styles.
Also
> > see
> > > > the CL at https://codereview.chromium.org/183683010/. Seems that you
also
> > > think
> > > > we should not inverse-fill for strokeAndFill style.
> > > 
> > > Are you saying kStrokeAndFill should draw for inverse-filled empty path or
> > not?
> > 
> > kStrokeAndFill should not draw inverse-filled empty path. We should be
> > consistent that inverse-fill will not inverse-stroke for kStroke and
> > kStrokeAndFill styles. The only exception is that hairline stroke + fill =
> fill,
> > so we inverse-fill for this edge case of kStrokeAndFill style.
> 
> My understanding is that kStrokeAndFill will draw as though the path were
> stroked and filled. That would mean the stroke draws nothing but the fill
would
> paint the entire plane.

what about draw a rect by kStrokeAndFill SkPaint when the strokeWidth > 0?
Should we stroke the rect and inverse_fill the outer part for this rect? I think
we should be consistent, so I propose that Skia should not inverse-fill if the
SkPaint is kStroke and kStrokeAndFill styles.

[yunchao, 2014-05-08 13:28:19]

On 2014/05/06 13:44:38, Richard Ho wrote:
> On 2014/05/06 13:22:51, bsalomon wrote:
> > On 2014/05/05 23:44:18, Richard Ho wrote:
> > > On 2014/05/05 17:53:19, bsalomon wrote:
> > > > On 2014/05/05 15:01:42, Richard Ho wrote:
> > > > >
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp
> > > > > File tests/EmptyPathTest.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/262113003/diff/1/tests/EmptyPathTest.cpp#newc...
> > > > > tests/EmptyPathTest.cpp:83: bool shouldDraw = path.isInverseFillType()
> &&
> > > > > On 2014/05/05 13:47:49, bsalomon wrote:
> > > > > I think so. In my oponion, FillType(inverse or not) is supposed to be
> used
> > > for
> > > > > kFill_Style SkPaint, not for stroke as well as strokeAndFill styles.
> Also
> > > see
> > > > > the CL at https://codereview.chromium.org/183683010/. Seems that you
> also
> > > > think
> > > > > we should not inverse-fill for strokeAndFill style.
> > > > 
> > > > Are you saying kStrokeAndFill should draw for inverse-filled empty path
or
> > > not?
> > > 
> > > kStrokeAndFill should not draw inverse-filled empty path. We should be
> > > consistent that inverse-fill will not inverse-stroke for kStroke and
> > > kStrokeAndFill styles. The only exception is that hairline stroke + fill =
> > fill,
> > > so we inverse-fill for this edge case of kStrokeAndFill style.
> > 
> > My understanding is that kStrokeAndFill will draw as though the path were
> > stroked and filled. That would mean the stroke draws nothing but the fill
> would
> > paint the entire plane.
> 
> what about draw a rect by kStrokeAndFill SkPaint when the strokeWidth > 0?
> Should we stroke the rect and inverse_fill the outer part for this rect? I
think
> we should be consistent, so I propose that Skia should not inverse-fill if the
> SkPaint is kStroke and kStrokeAndFill styles.

Brian, what about your aponion about this? If you still think we should
inverse-fill the entire plane for empty path for kStrokeAndFill style, I will
upload a new patchset to review.

[bsalomon, 2014-05-08 18:31:12]

After talking with Mike today I think the plan is to remove kStrokeAndFill.

[yunchao, 2014-05-09 05:20:36]

On 2014/05/08 18:31:12, bsalomon wrote:
> After talking with Mike today I think the plan is to remove kStrokeAndFill.

Wow, this is a big change. It may be not difficult, but will impact lots of
code. I can help on this if you begin to do this. 
I think it can be done by 2 steps: 
1) make sure there are no callers use kStrokeAndFill style. use stroke then fill
instead if necessary. 
2) remove path rasterization code and other corresponding implementation code
for kStrokeAndFill in all backends(sw-raster, gpu, etc).

Well, this plan will not affect this small CL. Because we should not
inverse-fill if it is not fill style(It is stroke style if no kStrokeAndFill
style).

[yunchao, 2014-05-12 06:06:05]

On 2014/05/09 05:20:36, Richard Ho wrote:
> On 2014/05/08 18:31:12, bsalomon wrote:
> > After talking with Mike today I think the plan is to remove kStrokeAndFill.
> 
> Wow, this is a big change. It may be not difficult, but will impact lots of
> code. I can help on this if you begin to do this. 
> I think it can be done by 2 steps: 
> 1) make sure there are no callers use kStrokeAndFill style. use stroke then
fill
> instead if necessary. 
> 2) remove path rasterization code and other corresponding implementation code
> for kStrokeAndFill in all backends(sw-raster, gpu, etc).
> 
> Well, this plan will not affect this small CL. Because we should not
> inverse-fill if it is not fill style(It is stroke style if no kStrokeAndFill
> style).

Brian, what about this CL? If it is OK, I will land this CL
https://codereview.chromium.org/183683010/ again. I have update the unit test in
that CL too. That CL broke unit test EmptyPath, but I think It is this unit test
EmptyPath that doesn't obey the rule: we should not inverse-stroke.