Push drawPath() call into PlatformContextSkia to avoid juggling two copies of...

* Update |path|'s fill rule.
* Change a currentPathInGlobalCoordinates() call to currentPathInLocalCoordinates().
* Get rid of currentPathInGlobalCoordinates().


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=7917

[brettw, 2009-01-13 01:03:48]

http://codereview.chromium.org/17633/diff/1/4
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(left):

http://codereview.chromium.org/17633/diff/1/4#oldcode1053
Line 1053: SkPath path = platformContext()->currentPathInLocalCoordinates();
Your new way of doing it seems slightly less clear to me. The current
PlatformContext is really just the "current graphics state" and currently has no
drawing commands.

And this patch pushes the actual drawing one level deeper, which makes it a
little more complex. I think eventually the state in PlatformContextSkia will be
pushed into WebKit more, so this actually puts us in a worse state for that.

If we're worried about optimizing path copies (and there's a good chance this
isn't actually a copy since VS will optimize this case), there is a lot more we
can/should do. I suggest leaving it the way it was until we decide it's a
problem.

[Evan Stade, 2009-01-13 01:12:13]

http://codereview.chromium.org/17633/diff/1/4
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(left):

http://codereview.chromium.org/17633/diff/1/4#oldcode1053
Line 1053: SkPath path = platformContext()->currentPathInLocalCoordinates();
> If we're worried about optimizing path copies (and there's a good chance this
> isn't actually a copy since VS will optimize this case), there is a lot more
we
> can/should do. I suggest leaving it the way it was until we decide it's a
> problem.

It's not really about reducing the times we copy the path. If we copy the path
then we have two paths floating about and any change we make on one won't affect
the other. This introduces subtle bugs like the one that Finnur's last patch
created, where any changes made to the current path in the body of fillPath()
are not propagated to the local |path| variable.

> Your new way of doing it seems slightly less clear to me. The current
> PlatformContext is really just the "current graphics state" and currently has
no
> drawing commands.

The path we are drawing here is part of the current graphics state. Creating
drawCurrentPath() is not only more convenient but also more mistake-proof.

[Evan Stade, 2009-01-13 01:47:12]

http://codereview.chromium.org/17633/diff/7/210
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17633/diff/7/210#newcode409
Line 409: SkPath path = platformContext()->currentPathInLocalCoordinates();
this fixes LayoutTests/svg/W3C-SVG-1.1/masking-path-02-b.svg . Perhaps we should
add a comment where the currentPathIn*Coordinates functions are defined that
makes it clear when to use one and when to use the other. I know I  for one have
to think hard to remember which is right.

[brettw, 2009-01-13 02:14:42]

Actually, I think you should delete currentPathInGlobalCoordinates. The only
user is the single place you now call it (which can use the old code). I think
this will be even more clear, and would not require the extra drawPath call.

[Evan Stade, 2009-01-13 02:37:19]

made Brett's suggested changes.

[brettw, 2009-01-13 02:54:36]

LGTM, thanks

http://codereview.chromium.org/17633/diff/217/218
File third_party/WebKit/WebCore/platform/graphics/skia/PlatformContextSkia.h
(right):

http://codereview.chromium.org/17633/diff/217/218#newcode166
Line 166: SkPath m_path;
Can you add to this comment that this patch is in global coordinates not
relative to the current transform?