[webkit-reviews] review denied: [Bug 18356] Stroking of zero-length paths in SVG should change according to erratum : [Attachment 100809] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Rob Buis
<rwlbuis at gmail.com>'s request for review:
Bug 18356: Stroking of zero-length paths in SVG should change according to
erratum
https://bugs.webkit.org/show_bug.cgi?id=18356

Attachment 100809: Patch
https://bugs.webkit.org/attachment.cgi?id=100809&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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?