[webkit-reviews] review denied: [Bug 18356] Stroking of zero-length paths in SVG should change according to erratum : [Attachment 100962] 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 100962: Patch
https://bugs.webkit.org/attachment.cgi?id=100962&action=review

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