Bug 18356 - Stroking of zero-length paths in SVG should change according to erratum
Summary: Stroking of zero-length paths in SVG should change according to erratum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Rob Buis
URL: http://mcc.id.au/temp/2008/zero-path.svg
Keywords:
: 61243 (view as bug list)
Depends on: 65796
Blocks: 38785
  Show dependency treegraph
 
Reported: 2008-04-08 00:41 PDT by Cameron McCormack (:heycam)
Modified: 2011-08-05 23:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.70 KB, patch)
2011-07-14 08:07 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (22.68 KB, patch)
2011-07-14 10:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (5.96 MB, application/zip)
2011-07-14 12:01 PDT, WebKit Review Bot
no flags Details
Patch (25.13 KB, patch)
2011-07-15 06:14 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (25.86 KB, patch)
2011-07-15 08:08 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (5.85 MB, application/zip)
2011-07-15 08:28 PDT, WebKit Review Bot
no flags Details
Patch (27.22 KB, patch)
2011-07-16 07:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (27.22 KB, patch)
2011-07-16 09:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2008-04-08 00:41:42 PDT
According to http://www.w3.org/2003/01/REC-SVG11-20030114-errata#stroking-subpaths-zero-length a zero-length path with stroke-linecap="square" should result in a square being rendered.
Comment 1 Cameron McCormack (:heycam) 2008-04-08 00:42:40 PDT
There doesn't seem to be an easy way to get CG to draw stroke caps in this way, so I assume some preprocessing of the path data will have to be done before handing it off to CG to render.
Comment 2 Eric Seidel (no email) 2008-04-09 10:02:35 PDT
This is probably well into the "edge case" realm.  But a bug all the same.  Now if the W3C actually had a test case...
Comment 3 Jeff Schiller 2010-04-10 22:31:30 PDT
Speaking of test case, Microsoft seems to have put one together: http://samples.msdn.microsoft.com/ietestcenter/svg112/svg_harness.htm?url=./svg112/svg/chapter_11.4.svg
Comment 4 Dirk Schulze 2011-07-12 02:54:55 PDT
*** Bug 61243 has been marked as a duplicate of this bug. ***
Comment 5 Rob Buis 2011-07-14 08:07:39 PDT
Created attachment 100809 [details]
Patch
Comment 6 Nikolas Zimmermann 2011-07-14 09:07:13 PDT
Comment on attachment 100809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review

Good catch, some comments:

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:173
> +    Path* path = &m_path;
> +    Path tempPath;

I don't see the need here for these two. Just leave "Path path" as is.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:180
> +    // Spec: any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt

s/any/Any/. A link to the paragraph would be even better (plus your text).

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:184
> +        path = &tempPath;

When you leave "Path path" above, that line is obsolete.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:196
> +        tempPath = m_path;
> +        path = &tempPath;
> +        tempPath.transform(nonScalingStrokeTransform);

You can revert this is as well.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:327
> +    if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {

m_fillBoundingBox.isEmpty() ?

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:332
> +        Path path;
> +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> +        float strokeWidth = svgStyle->strokeWidth().value(svgElement);
> +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> +        m_strokeAndMarkerBoundingBox = path.boundingRect();

Why do you need a temp rect for this? Just use the already calculatd Rect?
Comment 7 Dirk Schulze 2011-07-14 09:44:43 PDT
Comment on attachment 100809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:174
> +    int applyMode = ApplyToStrokeMode;

I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed.

>> Source/WebCore/rendering/svg/RenderSVGPath.cpp:327
>> +    if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {
> 
> m_fillBoundingBox.isEmpty() ?

Thats not the same, isEmpty is true for values smaller than 0. In our case this is unimportant than the rect never gets negative.
Comment 8 Rob Buis 2011-07-14 09:52:00 PDT
Hi Dirk,

(In reply to comment #7)
> (From update of attachment 100809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174
> > +    int applyMode = ApplyToStrokeMode;
> 
> I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed.

Agreed.

>>> Source/WebCore/rendering/svg/RenderSVGPath.cpp:327
>>> +    if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {
>>
>> m_fillBoundingBox.isEmpty() ?
>
> Thats not the same, isEmpty is true for values smaller than 0. In our case this is unimportant than the rect never gets negative.

Also isEmpty uses ||, where we want && for the width and height is zero condition. Thanks for the reaction Dirk. I hope I left the non-scaling stroke stuff correct and as efficient as before :)
Cheers,

Rob.
Comment 9 Nikolas Zimmermann 2011-07-14 10:12:21 PDT
(In reply to comment #7)
> (From update of attachment 100809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174
> > +    int applyMode = ApplyToStrokeMode;
> 
> I think Rob don't want to change the original path. This is necessary for JS calls like pathLength() and similar calls. But have to look at the SVG code first if this is needed.

I don't see how that is related? The current code doesn't change the original path at the moment either?
Comment 10 Rob Buis 2011-07-14 10:33:25 PDT
Created attachment 100825 [details]
Patch
Comment 11 Rob Buis 2011-07-14 10:49:06 PDT
Hi Niko,

(In reply to comment #6)
> (From update of attachment 100809 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100809&action=review
> 
> Good catch, some comments:
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:173
> > +    Path* path = &m_path;
> > +    Path tempPath;
> 
> I don't see the need here for these two. Just leave "Path path" as is.

Well I think we want a temp path for either non-scaling or linecap=square code paths, to leave m_path intact.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:180
> > +    // Spec: any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt
> 
> s/any/Any/. A link to the paragraph would be even better (plus your text).

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:184
> > +        path = &tempPath;
> 
> When you leave "Path path" above, that line is obsolete.

See above :)

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:196
> > +        tempPath = m_path;
> > +        path = &tempPath;
> > +        tempPath.transform(nonScalingStrokeTransform);
> 
> You can revert this is as well.

Ditto.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:327
> > +    if (svgStyle->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {
> 
> m_fillBoundingBox.isEmpty() ?

Does not do the same unfortunately :(

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332
> > +        Path path;
> > +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> > +        float strokeWidth = svgStyle->strokeWidth().value(svgElement);
> > +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> > +        m_strokeAndMarkerBoundingBox = path.boundingRect();
> 
> Why do you need a temp rect for this? Just use the already calculatd Rect?

That is in a different method.
So I think we want to keep m_path as it is, changing it at paint time feels wrong. I just now noticed strokeContains is probably wrong for the zero-length path + linecap=square, let me know if I should fix that...
Cheers,

Rob.
Comment 12 WebKit Review Bot 2011-07-14 12:01:51 PDT
Comment on attachment 100825 [details]
Patch

Attachment 100825 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9065267

New failing tests:
svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
Comment 13 WebKit Review Bot 2011-07-14 12:01:57 PDT
Created attachment 100836 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 14 Nikolas Zimmermann 2011-07-15 03:34:49 PDT
(In reply to comment #11)
Hi Rob,

> > 
> > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:173
> > > +    Path* path = &m_path;
> > > +    Path tempPath;
> > 
> > I don't see the need here for these two. Just leave "Path path" as is.
> 
> Well I think we want a temp path for either non-scaling or linecap=square code paths, to leave m_path intact.
Okay, but this looks super ugly. I'll think about a better way.

> > m_fillBoundingBox.isEmpty() ?
> 
> Does not do the same unfortunately :(
Oh yes, you are right.

> 
> > > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332
> > > +        Path path;
> > > +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> > > +        float strokeWidth = svgStyle->strokeWidth().value(svgElement);
> > > +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> > > +        m_strokeAndMarkerBoundingBox = path.boundingRect();
> > 
> > Why do you need a temp rect for this? Just use the already calculatd Rect?
> 
> That is in a different method.
> So I think we want to keep m_path as it is, changing it at paint time feels wrong. I just now noticed strokeContains is probably wrong for the zero-length path + linecap=square, let me know if I should fix that...
You misunderstood me.
The result of path.boundingRect() should be exactly the same that you've calculated in path.addRect, no?
So I'd just use
m_strokeAndMarkerBoundingBox = FloatRect(m_fillBo....

Will review the patch now.
Comment 15 Nikolas Zimmermann 2011-07-15 03:47:41 PDT
Comment on attachment 100825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100825&action=review

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:174
> +    Path* path = &m_path;
> +    Path tempPath;
> +    int applyMode = ApplyToStrokeMode;

The information we need to call strokePaingingResource->appyResource/postApplyResource is the Path, and the ApplyToFill/StrokeMode.
How about only storing "Path* usePath = 0;" here.
And then

if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height())
    setupSquareCapPath(usePath, applyMode);
else if (nonScalingStroke)
     setupNonScalingStrokePath(usePath, applyMode);
else {
    applyMode = ApplyToStrokeMode;
    usePath = &m_path;
}

static inline void setupSquareCapPath(Path*& usePath, int& applyMode) { .... } contains your new code
static inline void setupNonScalingStrokePath(Path*& usePath, int& applyMode) { .... } contains the old code to handle non-scaling-stroke.

That would avoid having to allocate a Path tempPath object even if we don't use it, it also makes the function more readable.
What do you think?

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:332
> +        Path path;
> +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> +        float strokeWidth = svgStyle->strokeWidth().value(svgElement);
> +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> +        m_strokeAndMarkerBoundingBox = path.boundingRect();

I still think this should be replaced by:
m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth);
m_repaintBoundingBox = m_strokeAndMarkerBoundingBox;
SVGRenderSupport::...
return;
Comment 16 Rob Buis 2011-07-15 06:14:50 PDT
Created attachment 100962 [details]
Patch
Comment 17 Rob Buis 2011-07-15 06:16:59 PDT
Hi Niko,

(In reply to comment #15)
> (From update of attachment 100825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100825&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:174
> > +    Path* path = &m_path;
> > +    Path tempPath;
> > +    int applyMode = ApplyToStrokeMode;
> 
> The information we need to call strokePaingingResource->appyResource/postApplyResource is the Path, and the ApplyToFill/StrokeMode.
> How about only storing "Path* usePath = 0;" here.
> And then
> 
> if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height())
>     setupSquareCapPath(usePath, applyMode);
> else if (nonScalingStroke)
>      setupNonScalingStrokePath(usePath, applyMode);
> else {
>     applyMode = ApplyToStrokeMode;
>     usePath = &m_path;
> }
> 
> static inline void setupSquareCapPath(Path*& usePath, int& applyMode) { .... } contains your new code
> static inline void setupNonScalingStrokePath(Path*& usePath, int& applyMode) { .... } contains the old code to handle non-scaling-stroke.
> 
> That would avoid having to allocate a Path tempPath object even if we don't use it, it also makes the function more readable.
> What do you think?

The temp Path bothered me as well. Great idea! I just didnt do setupNonScalingStrokePath since it requires 6 parameters.
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:332
> > +        Path path;
> > +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> > +        float strokeWidth = svgStyle->strokeWidth().value(svgElement);
> > +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> > +        m_strokeAndMarkerBoundingBox = path.boundingRect();
> 
> I still think this should be replaced by:
> m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth);
> m_repaintBoundingBox = m_strokeAndMarkerBoundingBox;
> SVGRenderSupport::...
> return;

Great idea, didnt see that one, fixed.
Also note that I fixes strokeContains this time. Now if you do inspect element on such a zero-length path the Inspector shows the correct one.
Cheers,

Rob.
Comment 18 Nikolas Zimmermann 2011-07-15 06:33:47 PDT
Comment on attachment 100962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100962&action=review

In general this is fine, but has way too much copy/paste code, which needs to be refactored. sorry for another round...

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:104
> +    if (style()->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {

That needs to be shared with the other duplicated checks, in a member function like shouldStrokeZeroLengthSubpath..

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:109
> +        Path path;
> +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> +        float strokeWidth = style()->svgStyle()->strokeWidth().value(svgElement);
> +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> +        return path.contains(point, style()->svgStyle()->fillRule());

Erm, this is again a trivial rect, you can implement the "contains" check without the need to use "Path". The fill rule won't matter here.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:162
> +// Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt
> +// but shall be stroked if the âstroke-linecapâ property has a value of round or square
> +void RenderSVGPath::setupSquareCapPath(Path*& usePath, int& applyMode)

I'd move this inline the function.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:167
> +    tempPath = Path();

This defeats the idea of a shared Path! This way you'd force eg. CG to create new underlying CGPathRefs.
You'd better leave this out and change ....

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:171
> +    usePath->addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));

.... to call usePath->clear(); first. That allows the underlying graphics platform to really maintain a shared path.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:204
> +    if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height())

This logic and the comment should be moved into a member function, as it's more than once. I'd add "bool shouldStrokeZeroLengthPath()".
I've just noticed it says _subpath_ not _path_ - what does that mean? sequence of segments? or does it really apply to the whole path only?

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:219
> +        DEFINE_STATIC_LOCAL(Path, tempPath, ());
> +
>          SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
>          AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
>          if (!nonScalingStrokeTransform.isInvertible())
>              return;
>  
> -        path = m_path;
> -        path.transform(nonScalingStrokeTransform);
> +        tempPath = m_path;
> +        usePath = &tempPath;
> +        tempPath.transform(nonScalingStrokeTransform);
>  
>          stateSaver.save();
>          context->concatCTM(nonScalingStrokeTransform.inverse());

Just make it a member function:

bool RenderSVGPath::setupNonScalingStrokePath(Path*& usePath, GraphicsContextStateSaver& stateSaver)
{
    SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
    AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
    if (!nonScalingStrokeTransform.isInvertible())
        return false;
    DEFINE_STATIC_LOCAL(Path, tempPath, ());
    tempPath = m_path;
    tempPath.transform(nonScalingStrokeTransform);
    usePath = &tempPath;
    stateSaver.save();
    stateSaver.context()->concatCTM(nonScalingStrokeTransform.inverse());
    return true;    
}

and use
(1):
else if (nonScalingStroke) {
    if (!setupNonScalingStrokePath(usePath, stateSaver))
        return;
}

or (2)
else if (nonScalingStroke && !setupNonScalingStrokePath(usePath, stateSaver))
    return;

Though I guess (1) looks nicer.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:350
> +        m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth);

The logic to create this rectangle needs to be shared as well in a member function. It's needed in multiple places.
Comment 19 Rob Buis 2011-07-15 08:08:54 PDT
Created attachment 100982 [details]
Patch
Comment 20 Nikolas Zimmermann 2011-07-15 08:27:15 PDT
Comment on attachment 100982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100982&action=review

Great, r=me. You may want to fix some last things before landing:

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:194
> +    context->concatCTM(nonScalingStrokeTransform.inverse());

it seems nicer to give the GraphicsContextStateSaver a "GraphicsContext* context" accessor, as it already stores it. You'd save having to hand over GraphicsContext as argument then.
I'd do it.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:358
> +    const SVGRenderStyle* svgStyle = style()->svgStyle();

This can be reverted to the old position.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:391
> +

Not needed?
Comment 21 WebKit Review Bot 2011-07-15 08:27:56 PDT
Comment on attachment 100982 [details]
Patch

Attachment 100982 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9093272

New failing tests:
svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
Comment 22 WebKit Review Bot 2011-07-15 08:28:02 PDT
Created attachment 100987 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 23 Rob Buis 2011-07-15 09:12:51 PDT
Hi Niko,

(In reply to comment #18)
> (From update of attachment 100962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100962&action=review
> 
> In general this is fine, but has way too much copy/paste code, which needs to be refactored. sorry for another round...
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:104
> > +    if (style()->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height()) {
> 
> That needs to be shared with the other duplicated checks, in a member function like shouldStrokeZeroLengthSubpath..

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:109
> > +        Path path;
> > +        SVGElement* svgElement = static_cast<SVGElement*>(node());
> > +        float strokeWidth = style()->svgStyle()->strokeWidth().value(svgElement);
> > +        path.addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> > +        return path.contains(point, style()->svgStyle()->fillRule());
> 
> Erm, this is again a trivial rect, you can implement the "contains" check without the need to use "Path". The fill rule won't matter here.

Right, fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:162
> > +// Spec(11.4): Any zero length subpath shall not be stroked if the âstroke-linecapâ property has a value of butt
> > +// but shall be stroked if the âstroke-linecapâ property has a value of round or square
> > +void RenderSVGPath::setupSquareCapPath(Path*& usePath, int& applyMode)
> 
> I'd move this inline the function.

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:167
> > +    tempPath = Path();
> 
> This defeats the idea of a shared Path! This way you'd force eg. CG to create new underlying CGPathRefs.
> You'd better leave this out and change ....
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:171
> > +    usePath->addRect(FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth));
> 
> .... to call usePath->clear(); first. That allows the underlying graphics platform to really maintain a shared path.

I see what you mean, fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:204
> > +    if (style->svgStyle()->capStyle() == SquareCap && !m_fillBoundingBox.width() && !m_fillBoundingBox.height())
> 
> This logic and the comment should be moved into a member function, as it's more than once. I'd add "bool shouldStrokeZeroLengthPath()".

Right, fixed.

> I've just noticed it says _subpath_ not _path_ - what does that mean? sequence of segments? or does it really apply to the whole path only?

Yes, this will unfortunatly not do for subpaths. OTOH I never saw an SVG testing or using that. Added a FIXME as agreed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:219
> > +        DEFINE_STATIC_LOCAL(Path, tempPath, ());
> > +
> >          SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
> >          AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
> >          if (!nonScalingStrokeTransform.isInvertible())
> >              return;
> >  
> > -        path = m_path;
> > -        path.transform(nonScalingStrokeTransform);
> > +        tempPath = m_path;
> > +        usePath = &tempPath;
> > +        tempPath.transform(nonScalingStrokeTransform);
> >  
> >          stateSaver.save();
> >          context->concatCTM(nonScalingStrokeTransform.inverse());
> 
> Just make it a member function:
> 
> bool RenderSVGPath::setupNonScalingStrokePath(Path*& usePath, GraphicsContextStateSaver& stateSaver)
> {
>     SVGStyledTransformableElement* element = static_cast<SVGStyledTransformableElement*>(node());
>     AffineTransform nonScalingStrokeTransform = element->getScreenCTM(SVGLocatable::DisallowStyleUpdate);
>     if (!nonScalingStrokeTransform.isInvertible())
>         return false;
>     DEFINE_STATIC_LOCAL(Path, tempPath, ());
>     tempPath = m_path;
>     tempPath.transform(nonScalingStrokeTransform);
>     usePath = &tempPath;
>     stateSaver.save();
>     stateSaver.context()->concatCTM(nonScalingStrokeTransform.inverse());
>     return true;    
> }
> 
> and use
> (1):
> else if (nonScalingStroke) {
>     if (!setupNonScalingStrokePath(usePath, stateSaver))
>         return;
> }
> 
> or (2)
> else if (nonScalingStroke && !setupNonScalingStrokePath(usePath, stateSaver))
>     return;
> 
> Though I guess (1) looks nicer.

Agreed, fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:350
> > +        m_strokeAndMarkerBoundingBox = FloatRect(m_fillBoundingBox.x() - strokeWidth / 2., m_fillBoundingBox.y() - strokeWidth / 2., strokeWidth, strokeWidth);
> 
> The logic to create this rectangle needs to be shared as well in a member function. It's needed in multiple places.

Fixed.
Cheers,

Rob.
Comment 24 Rob Buis 2011-07-15 09:14:32 PDT
Hi Niko,

(In reply to comment #20)
> (From update of attachment 100982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100982&action=review
> 
> Great, r=me. You may want to fix some last things before landing:
> 
> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:194
> > +    context->concatCTM(nonScalingStrokeTransform.inverse());
> 
> it seems nicer to give the GraphicsContextStateSaver a "GraphicsContext* context" accessor, as it already stores it. You'd save having to hand over GraphicsContext as argument then.
> I'd do it.

Ah, I first read 'i'll do it' :) I"ll try to show the final patch before landing(in a few hours).

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:358
> > +    const SVGRenderStyle* svgStyle = style()->svgStyle();
> 
> This can be reverted to the old position.

Fixed.

> > Source/WebCore/rendering/svg/RenderSVGPath.cpp:391
> > +
> 
> Not needed?

Fixed.
Cheers,

Rob.
Comment 25 Rob Buis 2011-07-15 17:59:20 PDT
This was fixed in r91125.
Comment 26 James Robinson 2011-07-15 18:37:37 PDT
(In reply to comment #21)
> (From update of attachment 100982 [details])
> Attachment 100982 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/9093272
> 
> New failing tests:
> svg/W3C-SVG-1.1-SE/painting-control-04-f.svg

This test failed in the chromium EWS, but it seems nobody did anything about it.  Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test.  Why did you land a patch that you knew would make a buildbot go red?
Comment 27 Nikolas Zimmermann 2011-07-15 23:46:37 PDT
(In reply to comment #26)
> (In reply to comment #21)
> > (From update of attachment 100982 [details] [details])
> > Attachment 100982 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/9093272
> > 
> > New failing tests:
> > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
> 
> This test failed in the chromium EWS, but it seems nobody did anything about it.  Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test.  Why did you land a patch that you knew would make a buildbot go red?

I did assume it's a new test that needs baselines for chromium? This isn't the case?
Comment 28 Dirk Schulze 2011-07-16 01:21:03 PDT
(In reply to comment #26)
> (In reply to comment #21)
> > (From update of attachment 100982 [details] [details])
> > Attachment 100982 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/9093272
> > 
> > New failing tests:
> > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
> 
> This test failed in the chromium EWS, but it seems nobody did anything about it.  Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test.  Why did you land a patch that you knew would make a buildbot go red?

General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium?
Comment 29 Rob Buis 2011-07-16 07:45:17 PDT
Created attachment 101092 [details]
Patch
Comment 30 Rob Buis 2011-07-16 07:48:47 PDT
Hi Niko,

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #21)
> > > (From update of attachment 100982 [details] [details] [details])
> > > Attachment 100982 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/9093272
> > > 
> > > New failing tests:
> > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
> > 
> > This test failed in the chromium EWS, but it seems nobody did anything about it.  Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test.  Why did you land a patch that you knew would make a buildbot go red?
> 
> I did assume it's a new test that needs baselines for chromium? This isn't the case?

It is a new test that needs a baseline. However, I noticed that the chromium result really was different, the circle was not drawn (stroke-linecap=round)! Maybe James meant that. To fix that we may as well do the rendering ourselves so it will be right regardless of toolkit. The attached patch should fix that, reopening :)
Cheers,

Rob.
Comment 31 Nikolas Zimmermann 2011-07-16 09:26:45 PDT
Comment on attachment 101092 [details]
Patch

Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now.
Comment 32 Rob Buis 2011-07-16 09:48:23 PDT
Created attachment 101093 [details]
Patch
Comment 33 Rob Buis 2011-07-16 09:49:05 PDT
Hi Niko,

(In reply to comment #31)
> (From update of attachment 101092 [details])
> Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now.

Done! So I did an update-webkit && webkit-patch upload, lets hope it works this time...
Cheers,

Rob.
Comment 34 Rob Buis 2011-07-18 09:11:30 PDT
I just checked on qt, the toolkit doesnt render the round cap either for zero-length, so this should really go in.
Cheers,

Rob.

(In reply to comment #33)
> Hi Niko,
> 
> (In reply to comment #31)
> > (From update of attachment 101092 [details] [details])
> > Looks great! It doesn't apply on cr-linux atm - can you recreate the patch? I'd like to get confirmation from cr-linux ews that it's correct on Chromium now.
> 
> Done! So I did an update-webkit && webkit-patch upload, lets hope it works this time...
> Cheers,
> 
> Rob.
Comment 35 Nikolas Zimmermann 2011-07-18 09:42:51 PDT
Comment on attachment 101093 [details]
Patch

r=me, very elegant :-)
Comment 36 WebKit Review Bot 2011-07-18 10:01:54 PDT
Comment on attachment 101093 [details]
Patch

Clearing flags on attachment: 101093

Committed r91191: <http://trac.webkit.org/changeset/91191>
Comment 37 WebKit Review Bot 2011-07-18 10:02:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 38 James Robinson 2011-07-18 12:48:31 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #21)
> > > (From update of attachment 100982 [details] [details] [details])
> > > Attachment 100982 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > > Output: http://queues.webkit.org/results/9093272
> > > 
> > > New failing tests:
> > > svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
> > 
> > This test failed in the chromium EWS, but it seems nobody did anything about it.  Unsurprisingly when this patch landed it made the real chromium testers go red on the exact same test.  Why did you land a patch that you knew would make a buildbot go red?
> 
> General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium?

They do, and the chromium EWS bots also run pixel tests and upload their results.  If you'd looked at the attachment layout-test-results from ec2-cr-linux-02, you would see that the circle didn't render at all.  It looks like Rob Buis found a better fix for this and landed it (thanks!)

Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red!  Add an appropriate entry to test_expectations.txt if a test will need new baselines.  If you aren't sure how to do this, ask.
Comment 39 Nikolas Zimmermann 2011-07-18 12:57:20 PDT
(In reply to comment #38)
> Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red!  Add an appropriate entry to test_expectations.txt if a test will need new baselines.  If you aren't sure how to do this, ask.

This is news to me - in the past chromium gardening was left to the chromium team. Over the past years, I never had to touch chromium specific NRWT stuff. Now times are changing, as we unify our test expectation management, so in the future we'll cover chromium as well.
Comment 40 James Robinson 2011-07-18 13:03:12 PDT
We didn't used to have EWS bots, either, so it was not always possible to predict what would happen when a patch landed.  In this case the EWS bot ran, told you that landing the patch would make bots turn red.  In that case it's pretty clear that you should take care of the issue before landing (one way or another), not just leave a mess for someone else to clean up.
Comment 41 Dirk Schulze 2011-07-18 13:05:07 PDT
(In reply to comment #38)
> > General question: Do the chromium bots provide pixel results? Can we you the rebaseline tool to upload new results for chromium?
> 
> They do, and the chromium EWS bots also run pixel tests and upload their results.  If you'd looked at the attachment layout-test-results from ec2-cr-linux-02, you would see that the circle didn't render at all.  It looks like Rob Buis found a better fix for this and landed it (thanks!)
Yes he did. Great that it is fixed now.

> 
> Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red!  Add an appropriate entry to test_expectations.txt if a test will need new baselines.  If you aren't sure how to do this, ask.
I never said that it is correct to land patches that break builds. I just was curious if it is possible to upload new baselines to the bots with webkit-patch --rebaseline, including pixel results.

Nevertheless, if you compare the 'failing' results with a revision before Robs patch, you'll see that Rob definitely did not broke chromium! The patch was a progression for chromium as well. Chromium just needed a bit more love and a followup in this case would be ok as well for me. Great that Rob found a solution to fix the test completely on all ports. Thank you Rob!
Comment 42 Rob Buis 2011-07-18 13:34:06 PDT
Hi Dirk,

(In reply to comment #41)
> (In reply to comment #38)
> > Even if a test needs new baselines, it is NOT valid to land a patch that makes a bot go red!  Add an appropriate entry to test_expectations.txt if a test will need new baselines.  If you aren't sure how to do this, ask.
> I never said that it is correct to land patches that break builds. I just was curious if it is possible to upload new baselines to the bots with webkit-patch --rebaseline, including pixel results.
> 
> Nevertheless, if you compare the 'failing' results with a revision before Robs patch, you'll see that Rob definitely did not broke chromium! The patch was a progression for chromium as well. Chromium just needed a bit more love and a followup in this case would be ok as well for me. Great that Rob found a solution to fix the test completely on all ports. Thank you Rob!

Well, that is what they pay me for, right? ;)
Cheers,

Rob.
Comment 43 Nico Weber 2011-07-27 07:50:51 PDT
This caused bug 65257
Comment 44 Nico Weber 2011-08-05 16:47:35 PDT
This also caused bug 65796