[webkit-reviews] review denied: [Bug 97333] Implement Canvas Path object : [Attachment 184104] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 97333: Implement Canvas Path object
https://bugs.webkit.org/show_bug.cgi?id=97333

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=184104&action=review


> Source/WebCore/html/canvas/CanvasPathMethods.cpp:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY


APPLE -> ADOBE ?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:39
> +void CanvasPathMethods::closePath()
> +{
> +    if (m_path.isEmpty())
> +	   return;
> +

Should you check the transform here as well?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:47
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:56
> +    if (!isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:65
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (!m_path.hasCurrentPoint())
> +	   m_path.moveTo(p1);
> +    else if (p1 != m_path.currentPoint())
> +	   m_path.addLineTo(FloatPoint(x, y));

Use p1 in addLineTo since you have it?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:70
> +    if (!isfinite(cpx) | !isfinite(cpy) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:79
> +    if (!m_path.hasCurrentPoint())
> +	   m_path.moveTo(FloatPoint(cpx, cpy));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +	   m_path.addQuadCurveTo(FloatPoint(cpx, cpy), p1);

Is it defined behaviour that if (cpx, cpy) == (x, y) and you didn't have a
control point then you still do the moveTo? In fact, it seems a little weird to
move to the control point at all.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:84
> +    if (!isfinite(cp1x) | !isfinite(cp1y) | !isfinite(cp2x) |
!isfinite(cp2y) | !isfinite(x) | !isfinite(y))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:93
> +    if (!m_path.hasCurrentPoint())
> +	   m_path.moveTo(FloatPoint(cp1x, cp1y));
> +
> +    FloatPoint p1 = FloatPoint(x, y);
> +    if (p1 != m_path.currentPoint())
> +	   m_path.addBezierCurveTo(FloatPoint(cp1x, cp1y), FloatPoint(cp2x,
cp2y), p1);

Same question as for quad above.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:99
> +    if (!isfinite(x1) | !isfinite(y1) | !isfinite(x2) | !isfinite(y2) |
!isfinite(r))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:114
> +    if (!m_path.hasCurrentPoint())
> +	   m_path.moveTo(p1);

Again, is this defined? In this case you're moving to the start of the arc,
then not adding the arc, so subsequent calls would not draw from the end. I
have no idea what is correct, but that seems weird.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:124
> +    if (!isfinite(x) | !isfinite(y) | !isfinite(r) | !isfinite(sa) |
!isfinite(ea))

||

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:133
> +	   // The arc is empty but we still need to draw the connecting line

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:141
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle
starting/ending at 'sa'

Nit: trailing .

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:151
> +    if (anticlockwise && sa - ea >= 2 * piFloat) {
> +	   m_path.addArc(FloatPoint(x, y), r, sa, sa - 2 * piFloat,
anticlockwise);
> +	   return;
> +    }
> +    if (!anticlockwise && ea - sa >= 2 * piFloat) {
> +	   m_path.addArc(FloatPoint(x, y), r, sa, sa + 2 * piFloat,
anticlockwise);
> +	   return;
> +    }
> +
> +    m_path.addArc(FloatPoint(x, y), r, sa, ea, anticlockwise);

Might as well create a variable FloatPoint(x, y)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:159
> +    if (!isfinite(x) || !isfinite(y) || !isfinite(width) ||
!isfinite(height))

This one is ok!! :)

> Source/WebCore/html/canvas/CanvasPathMethods.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY


APPLE!

> Source/WebCore/html/canvas/CanvasPathMethods.h:49
> +    void arc(float x, float y, float r, float sa, float ea, bool clockwise,
ExceptionCode&);

Watch out - you name this clockwise here and anticlockwise in the
implementation!

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:133
>		in [Optional=DefaultIsUndefined] boolean anticlockwise)
>	   raises (DOMException);
> +
>      void fill(in [Optional] DOMString winding);

Intentional?

> Source/WebCore/html/canvas/DOMPath.h:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY


APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:17
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED

> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY


APPLE!

> Source/WebCore/html/canvas/DOMPath.idl:63
> +		in [Optional=DefaultIsUndefined] boolean anticlockwise)

And anticlockwise here.