Follow up on the previous patch :

Follow up on the previous patch :
- Moved the SkStrokeRec class in its own file
- Replaced SkStroke by SkStrokeRec in Ganesh
- Moved path stroking to the Ganesh level in some cases (everytime it isn't required to do it directly in SkGpuDevice). PathEffect and MaskFilter still require path stroking at the SkGpuDevice for now.
- Renamed static functions in SkPath with proper names

* No functionality shold have changed with this patch. This is a step towards enabling Ganesh Path Renderers to decide whether or not to stroke the path rather than always receiving the stroked path as an input argument.

BUG=chromium:135111
TEST=Try path rendering tests from the gm
Committed: https://code.google.com/p/skia/source/detail?r=6861

[sugoi1, 2012-12-17 19:14:12]

https://codereview.appspot.com/6946072/diff/1/src/core/SkPicturePlayback.cpp
File src/core/SkPicturePlayback.cpp (left):

https://codereview.appspot.com/6946072/diff/1/src/core/SkPicturePlayback.cpp#...
src/core/SkPicturePlayback.cpp:646: static const int kDrawComplete = SK_MaxU32;
This was causing a "comparison between signed and unsigned values" warning later
in the file.

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrContext.cpp#newcode1074
src/gpu/GrContext.cpp:1074: const SkPath* pathPtr = &path;
I wanted to put the following code just before internalDrawPath (or even inside
it), but the call to "isOval" seems to be failing on a non-stroked path, so I
put it here for now.

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.cpp
File src/gpu/GrDefaultPathRenderer.cpp (left):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.c...
src/gpu/GrDefaultPathRenderer.cpp:503: return (stroke.getWidthIfStroked() <= 0)
&& !antiAlias;
The condition that was removed here was always true before this change and the
fact that it could be false now means that it would assert if no path renderer
can handle a given path.

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1023: if (!rec.isHairlineStyle()) {
I kept path stroking here because of the following "transform()" call which was
previously done after path stroking, and I didn't want to break the logic. I've
been told it is possible that the mask filter code could be moved to ganesh,
which would solve this issue.

[reed1, 2012-12-17 19:18:45]

nits around copyright for new files (use Google)

deferring all else to brian

https://codereview.appspot.com/6946072/diff/1/include/core/SkStrokeRec.h
File include/core/SkStrokeRec.h (right):

https://codereview.appspot.com/6946072/diff/1/include/core/SkStrokeRec.h#newc...
include/core/SkStrokeRec.h:1: /*
New files are copyright Google

https://codereview.appspot.com/6946072/diff/1/src/core/SkStrokeRec.cpp
File src/core/SkStrokeRec.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/core/SkStrokeRec.cpp#newcode1
src/core/SkStrokeRec.cpp:1: /*
copyright Google

[sugoi1, 2012-12-17 19:22:46]

https://codereview.appspot.com/6946072/diff/1/include/core/SkStrokeRec.h
File include/core/SkStrokeRec.h (right):

https://codereview.appspot.com/6946072/diff/1/include/core/SkStrokeRec.h#newc...
include/core/SkStrokeRec.h:1: /*
Changed to :
Copyright 2012 Google Inc.

https://codereview.appspot.com/6946072/diff/1/src/core/SkStrokeRec.cpp
File src/core/SkStrokeRec.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/core/SkStrokeRec.cpp#newcode1
src/core/SkStrokeRec.cpp:1: /*
Changed to :
Copyright 2012 Google Inc.

[bsalomon, 2012-12-17 19:33:58]

Mostly looks good.

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h#newcode549
include/core/SkPath.h:549: static FillType ConvertToNonInverseFillType(FillType
fill) {
MakeNonInverseFillType?

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrClipMaskManager.cpp#n...
src/gpu/GrClipMaskManager.cpp:65: const SkStrokeRec& rec,
I kinda like calling these stroke rather than rec... not a big deal just, rec
just seems ambiguous.

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrContext.cpp#newcode1078
src/gpu/GrContext.cpp:1078: if (rec.applyToPath(&tmpPath, *pathPtr)) {
I think the next step (in another CL) is to defer doing this until after we
first see if a PR can handle the rec as is.

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.cpp
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.c...
src/gpu/GrDefaultPathRenderer.cpp:502: return !antiAlias;
We need to say stroke.isFillStyle() here as this class doesn't do strokes or
hairlines.

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1010: if (pathEffect &&
pathEffect->filterPath(&tmpPath2, *pathPtr, &rec)) {
I didn't realize that the PE gets to output a Rec. Can't we just use the rec it
returned rather than applying it here?

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1023: if (!rec.isHairlineStyle()) {
On 2012/12/17 19:14:12, sugoi1 wrote:
> I kept path stroking here because of the following "transform()" call which
was
> previously done after path stroking, and I didn't want to break the logic.
I've
> been told it is possible that the mask filter code could be moved to ganesh,
> which would solve this issue.

Is it not valid to pass the same ptr as both params to rec.applyToPath()? If it
is allowed we could get rid of tmpPath2.

[sugoi1, 2012-12-17 19:48:24]

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h#newcode549
include/core/SkPath.h:549: static FillType ConvertToNonInverseFillType(FillType
fill) {
"ConvertToNonInverseFillType" was Mike's suggestion :)

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrClipMaskManager.cpp
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrClipMaskManager.cpp#n...
src/gpu/GrClipMaskManager.cpp:65: const SkStrokeRec& rec,
will do

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.cpp
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.c...
src/gpu/GrDefaultPathRenderer.cpp:502: return !antiAlias;
Ok, I'll need to make a few more changes to get this to work since it would now
be possible for a SkStrokeRec to get to this point simply after having been
initialized from the SkPaint object, which as not possible before (the previous
implementation with the SkStroke was always either 0 or <0 at this point and was
used essentially as a boolean)

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1010: if (pathEffect &&
pathEffect->filterPath(&tmpPath2, *pathPtr, &rec)) {
Good point, I'll investigate if it's possible.

https://codereview.appspot.com/6946072/diff/1/src/gpu/SkGpuDevice.cpp#newcode...
src/gpu/SkGpuDevice.cpp:1023: if (!rec.isHairlineStyle()) {
I thought it wasn't safe at first because SkPaint::getFillPath uses a temporary
path inside, but that was for applying the effect. SkStroke::strokePath() does
use a AutoTmpPath object, which should make it safe to have the same input and
output. I'll change it.

[bsalomon, 2012-12-17 19:50:37]

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.appspot.com/6946072/diff/1/include/core/SkPath.h#newcode549
include/core/SkPath.h:549: static FillType ConvertToNonInverseFillType(FillType
fill) {
On 2012/12/17 19:48:26, sugoi1 wrote:
> "ConvertToNonInverseFillType" was Mike's suggestion :)

ok :)

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.cpp
File src/gpu/GrDefaultPathRenderer.cpp (right):

https://codereview.appspot.com/6946072/diff/1/src/gpu/GrDefaultPathRenderer.c...
src/gpu/GrDefaultPathRenderer.cpp:502: return !antiAlias;
On 2012/12/17 19:48:26, sugoi1 wrote:
> Ok, I'll need to make a few more changes to get this to work since it would
now
> be possible for a SkStrokeRec to get to this point simply after having been
> initialized from the SkPaint object, which as not possible before (the
previous
> implementation with the SkStroke was always either 0 or <0 at this point and
was
> used essentially as a boolean)
> 

Aren't you calling applyToPath() in GrContext in the current version?

[sugoi1, 2012-12-17 20:18:33]

> Aren't you calling applyToPath() in GrContext in the current version?

Yes, but the gm test called "chrome_gradtext2" asserts because no renderer can
handle it. Apparently, currently, no PR handles a non AA hairline path. In the
default configuration, we either have :

GrStencilAndCoverPathRenderer (only fill style)
GrAAHairLinePathRenderer (hairline, but only AA)
GrAAConvexPathRenderer (only fill style)
GrDefaultPathRenderer (fill, no AA)

So, apparently, GrDefaultPathRenderer was (wrongly?) handling the hairline, no
AA case.

[bsalomon, 2012-12-17 20:21:15]

Oh you're right I should have said it should check for hairline or fill (it just
can't handle non-hairline stroke).

[sugoi1, 2012-12-17 20:30:58]

https://codereview.appspot.com/6946072/diff/7003/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6946072/diff/7003/src/gpu/GrContext.cpp#newcod...
src/gpu/GrContext.cpp:1084: /*switch(stroke.getStyle()) {
Oops, this was test code, it's deleted...

[bsalomon, 2012-12-17 20:40:11]

https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1010: if (pathEffect &&
pathEffect->filterPath(&effectPath, *pathPtr, &stroke)) {
Can't we use tmpPath here too and get rid of effectPath?

[sugoi1, 2012-12-17 20:46:12]

On 2012/12/17 20:40:11, bsalomon wrote:
> https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:1010: if (pathEffect &&
> pathEffect->filterPath(&effectPath, *pathPtr, &stroke)) {
> Can't we use tmpPath here too and get rid of effectPath?

I'm not so confident in doing that. Looking at SkPaint::getFillPath(), it does
create a temporary for that purpose, and the filterPath() function being
virtual, I don't think all derived versions handle have the same src and dst
paths.

[bsalomon, 2012-12-17 20:48:24]

On 2012/12/17 20:46:12, sugoi1 wrote:
> On 2012/12/17 20:40:11, bsalomon wrote:
> > https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp
> > File src/gpu/SkGpuDevice.cpp (right):
> > 
> >
>
https://codereview.appspot.com/6946072/diff/7003/src/gpu/SkGpuDevice.cpp#newc...
> > src/gpu/SkGpuDevice.cpp:1010: if (pathEffect &&
> > pathEffect->filterPath(&effectPath, *pathPtr, &stroke)) {
> > Can't we use tmpPath here too and get rid of effectPath?
> 
> I'm not so confident in doing that. Looking at SkPaint::getFillPath(), it does
> create a temporary for that purpose, and the filterPath() function being
> virtual, I don't think all derived versions handle have the same src and dst
> paths.

Ok lgtm