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

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