[webkit-reviews] review denied: [Bug 71820] Linecaps wrong for zero length lines : [Attachment 122336] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Stephen Chenney
<schenney at chromium.org>'s request for review:
Bug 71820: Linecaps wrong for zero length lines
https://bugs.webkit.org/show_bug.cgi?id=71820

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122336&action=review


Sorry it took so long, this is a really nice patch - it only needs some style
tweaks I think.
Do you see any negative performance impact for common-cases? I think it doesn't
impose any burden for non-zero-length subpath.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> +    Vector<FloatPoint>::iterator iter =
m_zeroLengthLinecapLocations.begin();
> +    while (iter != m_zeroLengthLinecapLocations.end()) {

vectors should be accessed by index, this is preferred.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> +	   float strokeWidth = this->strokeWidth();

This shouldn't be inside the loop, or directly use strokeWidth().

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
> +				   const Color& fallbackColor, const bool
nonScalingStroke, const AffineTransform& nonScalingStrokeTransform,

We don't constify POD types usually.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:317
> +    Vector<FloatPoint>::iterator iter =
m_zeroLengthLinecapLocations.begin();
> +    while (iter != m_zeroLengthLinecapLocations.end()) {

Should walk vector by index.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:437
> +    Vector<FloatPoint>::iterator iter =
m_zeroLengthLinecapLocations.begin();

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:475
> +class ZeroLengthSubpathFinder {

This needs to go into its own file, similar to SVGMarkerFoobar.cpp etc.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:489
> +	   switch (element->type) {

Good job!

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:506
> +	       if (subpathFinder->m_lastPoint != element->points[0]
> +		   || element->points[0] != element->points[1])

I'd rather not wrap lines here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:515
> +	       if (subpathFinder->m_lastPoint != element->points[0]
> +		   || element->points[0] != element->points[1]

ditto.

> Source/WebCore/svg/SVGPathSegListBuilder.h:53
> +    // Used in UnalteredParsing/NormalizedParsing modes.

You could land this seperated, to make the patch a bit smaller.