[webkit-reviews] review denied: [Bug 65796] REGRESSION (r91125): Polyline tool in google docs is broken : [Attachment 104636] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 65796: REGRESSION (r91125): Polyline tool in google docs is broken
https://bugs.webkit.org/show_bug.cgi?id=65796

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104636&action=review


> LayoutTests/ChangeLog:11
> +	   *
platform/mac/svg/custom/zero-path-square-cap-rendering-expected.png:

Why is this a progression? I thought we should draw a rect for zero length
paths?

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:4
> +<?xml version="1.0" standalone="no"?>
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
> +"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg width="100%" height="100%" version="1.1"
xmlns="http://www.w3.org/2000/svg">

We try to minimize tests as much as possible. Remove the doctype declaration,
the xml notation.

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:9
> +<g transform="translate(200 200) scale(0.766666)">
> +<rect width="300" height="100"
> +style="fill:rgb(0,0,255);stroke-width:1;
> +stroke:rgb(0,0,0)"/>

Why is that test so complicated? Is it necessary to have different transforms
in the test? Why?

> LayoutTests/svg/custom/zero-path-square-cap-rendering3.svg:12
> +<path stroke="#000000" stroke-opacity="1" stroke-width="496.95"
stroke-linecap="square" stroke-linejoin="miter" stroke-miterlimit="8"
shape-rendering="optimizeSpeed" d="M 0 0"></path>
> +</g>

<path d='M50,50 z' stroke='green' stroke-width='100' stroke-linecap='square'/>
is the code on zero-path-square-cap-rendering.svg. Why isn't the first square
drawn, while it is drawn here? The only difference is, that we close the path
on the first example.

> Source/WebCore/ChangeLog:8
> +	   Don't follow zero-length path code path for paths that consist of
just one moveto.

path code path for paths? So the problem should be a path with just one
segment? Why does the first example change than? Can you link to the part of
the spec?

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:158
>      // 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

Why did you land your previous patch with ‘stroke-linecapâ? I thought the
reviewer wanted you to change this first? Nevertheless, this does not explain
why you don't stroke your first example.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:159
> +    return style()->svgStyle()->hasStroke() &&
style()->svgStyle()->capStyle() != ButtCap && !m_fillBoundingBox.width() &&
!m_fillBoundingBox.height() && m_path.numberOfSegments() > 1;

I might had problems to understand what you are doing here :) Your new test has
just one segment in the path, but you are still drawing a rect, no?