Bug 18356 – Stroking of zero-length paths in SVG should change according to erratum

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Cameron McCormack (:heycam)]

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.

[Cameron McCormack (:heycam)]

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.

[Eric Seidel (no email)]

This is probably well into the "edge case" realm.  But a bug all the same.  Now if the W3C actually had a test case...

[Jeff Schiller]

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

[Dirk Schulze]

*** Bug 61243 has been marked as a duplicate of this bug. ***

[Rob Buis]

Created attachment 100809 [details]
Patch

[Nikolas Zimmermann]

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?

[Dirk Schulze]

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.

[Rob Buis]

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.

[Nikolas Zimmermann]

(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?

[Rob Buis]

Created attachment 100825 [details]
Patch

[Rob Buis]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Nikolas Zimmermann]

(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.

[Nikolas Zimmermann]

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;

[Rob Buis]

Created attachment 100962 [details]
Patch

[Rob Buis]

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.

[Nikolas Zimmermann]

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.

[Rob Buis]

Created attachment 100982 [details]
Patch

[Nikolas Zimmermann]

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?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Rob Buis]

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.

[Rob Buis]

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.

[Rob Buis]

This was fixed in r91125.

[James Robinson]

(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?

[Nikolas Zimmermann]

(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?

[Dirk Schulze]

(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?

[Rob Buis]

Created attachment 101092 [details]
Patch

[Rob Buis]

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.

[Nikolas Zimmermann]

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.

[Rob Buis]

Created attachment 101093 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

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.

[Nikolas Zimmermann]

Comment on attachment 101093 [details]
Patch

r=me, very elegant :-)

[WebKit Review Bot]

Comment on attachment 101093 [details]
Patch

Clearing flags on attachment: 101093

Committed r91191: <http://trac.webkit.org/changeset/91191>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

(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.

[Nikolas Zimmermann]

(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.

[James Robinson]

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.

[Dirk Schulze]

(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!

[Rob Buis]

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.

[Nico Weber]

This caused bug 65257

[Nico Weber]

This also caused bug 65796