Fixing gradient problem.

Fixing the problem with SVG gradients. Before, we were transforming paths when drawing the paths which would not work for gradients because scaling is applied during setup() in SVGPaintServerGradient and we would end up scaling again when drawing.

We now apply the transform when we add the path instead. 

This should fix a few SVG tests, which I am working on in a separate changelist.

[brettw, 2009-01-09 18:47:17]

http://codereview.chromium.org/17454/diff/1/5
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/1/5#newcode1043
Line 1043: if (!IsPathReasonable(getCTM().inverse(), path))
I think the right thing is to use SkMatrix() here instead of getCTM().inverse().

Save above.

[Evan Stade, 2009-01-09 18:58:49]

http://codereview.chromium.org/17454/diff/1/5
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/1/5#newcode302
Line 302: platformContext()->addPath(*transformed_path.platformPath());
nit: use the addPath(path, transform) method

[Evan Stade, 2009-01-09 19:00:46]

http://codereview.chromium.org/17454/diff/1/5
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/1/5#newcode302
Line 302: platformContext()->addPath(*transformed_path.platformPath());
nevermind, I misread this. Why is the transform taking place here rather than in
PlatformContextSkia's addPath?

[brettw, 2009-01-09 19:19:24]

http://codereview.chromium.org/17454/diff/1/5
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/1/5#newcode302
Line 302: platformContext()->addPath(*transformed_path.platformPath());
On 2009/01/09 19:00:46, Evan Stade wrote:
> nevermind, I misread this. Why is the transform taking place here rather than
in
> PlatformContextSkia's addPath?

I agree.

[Evan Stade, 2009-01-09 21:52:54]

http://codereview.chromium.org/17454/diff/16/206
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/16/206#newcode1054
Line 1054: if (!isPathSkiaSafe(SkMatrix(), path))
shouldn't this be

if (!isPathSkiaSafe(SkMatrix(),
platformContext()->currentPathInGlobalCoordinates())

?

http://codereview.chromium.org/17454/diff/16/203
File third_party/WebKit/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/16/203#newcode381
Line 381: void PlatformContextSkia::addPath(const SkPath& path)
nit: the body of this function should be:

{
m_path.addPath(path, m_canvas->getTotalMatrix());
}

http://codereview.chromium.org/17454/diff/16/203#newcode390
Line 390: WebCore::TransformationMatrix matrix = m_canvas->getTotalMatrix();
nit: you copy the matrix an extra time here. This should be 

const WebCore::TransformationMatrix& matrix = m_canvas->getTotalMatrix();
WebCore::TransformationMatrix matrix_inverse = matrix.inverse();

(maybe WebCore::TransformationMatrix matrix =
m_canvas->getTotalMatrix().inverse(); would work as well)

[Finnur, 2009-01-09 22:47:32]

I have uploaded a new patch. Unfortunately, I had to sync to tot because the
files keep changing from under me due to the upstreaming effort, but Dimitri
says they are done now so hopefully I can move forward with this. What this
means for you however, is that the diffs from patch to patch become confusing. I
recommend the View link in Rietveld instead of the patch deltas.

I did what you suggested, except for: 

> shouldn't this be
> if (!isPathSkiaSafe(SkMatrix(),
> platformContext()->currentPathInGlobalCoordinates())
> ?

My reply is: No, I don't think so, and when I try to apply it (for arguments
sake) then SVG objects are drawn in weird locations. The whole point with this
patch was to use paths in local coordinates in FillPath and StrokePath. :)

If you meant currentPathInLocalCoordinates() and you were implying that I should
do away with the path variable, I would still reply no because path is used
again at the bottom of the function.

[Evan Stade, 2009-01-09 22:53:58]

On 2009/01/09 22:47:32, Finnur wrote:
> I have uploaded a new patch. Unfortunately, I had to sync to tot because the
> files keep changing from under me due to the upstreaming effort, but Dimitri
> says they are done now so hopefully I can move forward with this. What this
> means for you however, is that the diffs from patch to patch become confusing.
I
> recommend the View link in Rietveld instead of the patch deltas.
> 
> I did what you suggested, except for: 
> 
> > shouldn't this be
> > if (!isPathSkiaSafe(SkMatrix(),
> > platformContext()->currentPathInGlobalCoordinates())
> > ?
> 
> My reply is: No, I don't think so, and when I try to apply it (for arguments
> sake) then SVG objects are drawn in weird locations. The whole point with this
> patch was to use paths in local coordinates in FillPath and StrokePath. :)
> 
> If you meant currentPathInLocalCoordinates() and you were implying that I
should
> do away with the path variable, I would still reply no because path is used
> again at the bottom of the function.

I don't think that changing just the if statement should affect where the svg
objects are drawn (aside from sometimes canceling drawing). If it does,
something is wrong.

The reason I recommended the change is because we should be validating the same
set of coordinates as we end up drawing. We draw local coordinates transformed
by getCTM(), so that is what we should be checking, but this is equivalent to
(and slightly slower than) checking the global coordinates transformed by the
identity matrix.

[Finnur, 2009-01-09 23:22:37]

My bad. Misunderstood you. Sorry. I uploaded a new patch.

[brettw, 2009-01-09 23:28:06]

http://codereview.chromium.org/17454/diff/213/42
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/213/42#newcode679
Line 679: if (!isPathSkiaSafe(SkMatrix(),
*platformContext()->currentPathInGlobalCoordinates()))
It seems weird that you get the path out twice in two different coordinate
systems. How about getting it once like the old one? Actually the old code (with
the getCTM() call) will work just fine if the initial path was constructed in
"local" coordinates.

http://codereview.chromium.org/17454/diff/213/42#newcode1052
Line 1052: if (!isPathSkiaSafe(SkMatrix(),
*platformContext()->currentPathInGlobalCoordinates()))
Same comment as above.

http://codereview.chromium.org/17454/diff/213/40
File third_party/WebKit/WebCore/platform/graphics/skia/PathSkia.cpp (right):

http://codereview.chromium.org/17454/diff/213/40#newcode278
Line 278: *context->platformContext()->currentPathInGlobalCoordinates(),
I would have thought you want local coordinates here? Am I wrong?

[Evan Stade, 2009-01-09 23:28:53]

1 nit

LGTM with or without addressing the nit

http://codereview.chromium.org/17454/diff/213/42
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/213/42#newcode701
Line 701:
platformContext()->canvas()->drawPath(platformContext()->currentPathInLocalCoordinates(),
paint);
nit: you could put a drawCurrentPath(paint) function in platformContext()

[Finnur, 2009-01-10 00:09:22]

I did what Brett suggested and also removed the WebCore TransformationMatrix
type from the AddPath call, in order to please the mac build which was failing
on line 387 on the try bots. Hopefully, this is the last of it :)

[brettw, 2009-01-10 00:18:03]

LGTM

[Evan Stade, 2009-01-13 00:19:17]

http://codereview.chromium.org/17454/diff/47/51
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/47/51#newcode689
Line 689: platformContext()->setFillRule(state.fillRule == RULE_EVENODD ?
found the bug :(

since path is no longer a reference to the platformContext's path, you need to
change this line to

    path.setFillType(state.fillRule == RULE_EVENODD ?
        SkPath::kEvenOdd_FillType : kWinding_FillType);

*OR*

you can get totally remove |path| and create/use a drawPath(SkPaint) function in
PlatformContextSkia. I think that this is a better solution overall--there's no
reason to be messing around with a local copy of the path, and will likely lead
to similar bugs in the future. (you can change the first use of |path| as we
discussed before)

[brettw, 2009-01-13 17:44:36]

http://codereview.chromium.org/17454/diff/47/51
File third_party/WebKit/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp
(right):

http://codereview.chromium.org/17454/diff/47/51#newcode689
Line 689: platformContext()->setFillRule(state.fillRule == RULE_EVENODD ?
On 2009/01/13 00:19:18, Evan Stade wrote:
> found the bug :(
> 
> since path is no longer a reference to the platformContext's path, you need to
> change this line to
> 
>     path.setFillType(state.fillRule == RULE_EVENODD ?
>         SkPath::kEvenOdd_FillType : kWinding_FillType);
> 
> *OR*
> 
> you can get totally remove |path| and create/use a drawPath(SkPaint) function
in
> PlatformContextSkia. I think that this is a better solution overall--there's
no
> reason to be messing around with a local copy of the path, and will likely
lead
> to similar bugs in the future. (you can change the first use of |path| as we
> discussed before)

Actually, I think this current code is pretty weird. It's globally and
persistently changing the rule of the path when you fill it, which seems
counter-intuitive and weird. It seems to me this is a reason to specifically
want a local copy.

In looking into platformContext()->setFillRule, I think it should be deleted. It
looks like this is the only place it's called, and we don't keep our fill rule
in sync with the one in GraphicsContext, unlike with the other properties
(webkit doesn't call us with the value). So keeping it there is misleading as
you might think it's correct. I think the only way to get the real fill rule is
calling GraphicsContext.fillRule().

[Finnur, 2009-01-13 18:23:38]

This is getting a little hard to keep track of. This changelist is checked in
and these look like a comment for a future changelist for estade, since this is
referring to changes he made on top of my changelist. I will assume this is for
him.

[Evan Stade, 2009-01-13 18:37:25]

> In looking into platformContext()->setFillRule, I think it should be deleted.

Agreed.

> This is getting a little hard to keep track of. This changelist is checked in
> and these look like a comment for a future changelist for estade, since this
is
> referring to changes he made on top of my changelist. I will assume this is
for
> him.

Yea, you should close this issue so we stop posting to it :)