[webkit-reviews] review denied: [Bug 82791] Add support for new canvas ellipse method : [Attachment 189028] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Dongseong Hwang
<dongseong.hwang at intel.com>'s request for review:
Bug 82791: Add support for new canvas ellipse method
https://bugs.webkit.org/show_bug.cgi?id=82791

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

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


This particular feature is still under discussion on the mailing lists. I even
doubt that it is that important for web authors at all. However, you do not
even follow the specification by making all values optional. I would like to
ask you to send a mail to webkit-dev and start a discussion there.

> Source/WebCore/ChangeLog:1
> +2013-02-18  Huang Dongsung  <luxtella at company100.net>

Is it Hwang or Huang? Are you doing this for company100 or intel?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:130
>	   m_path.addArcTo(p1, p2, r);

You may want to add your copyright.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:136
> +    // If 'sa' and 'ea' differ by more than 2Pi, just add a circle
starting/ending at 'sa'.
> +    if (anticlockwise && sa - ea >= 2 * piFloat)

This seems to change the behavior of arc as well. Do you have tests for arc?

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:163
> +    float adjustedEndAngle = adjustEndAngle(sa, ea, anticlockwise);

This seems unnecessary. Just pass it to the next function call.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:167
> +void CanvasPathMethods::ellipse(float x, float y, float rx, float ry, float
rot, float sa, float ea, bool anticlockwise, ExceptionCode& ec)

Can you rename ea, sa to endAngle and startAngle please? (In arc as well.)

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:174
> +    if (rx < 0 || ry < 0) {
> +	   ec = INDEX_SIZE_ERR;

I am not sure if we want to have an exception in Canvas at all. It may was an
mistake to do it in arc in the first place and should be fixed there.

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:179
> +	   // The ellipse is empty but we still need to draw the connecting
line to start point.

So, when you do it here, why do you have the same code in Path again? This
seems redundant.

> Source/WebCore/html/canvas/CanvasPathMethods.h:53
> +    void ellipse(float x, float y, float rx, float ry, float rot, float sa,
float ea, bool anticlockwise, ExceptionCode&);

startAngle and endAngle please.

> Source/WebCore/platform/graphics/Path.cpp:96
> +void Path::addEllipse(const FloatPoint& p, float radiusX, float radiusY,
float rotation, float startAngle, float endAngle, bool anticlockwise)

Hm. Some platforms have direct implementations for ellipse. It might be good to
support the platform ellipse implementations.

> Source/WebCore/platform/graphics/Path.cpp:104
> +	   else if (p1 != currentPoint())
> +	       addLineTo(p1);

Is that specified? Can you add a link to the ChangeLog to the paragraph of the
spec please?

> Source/WebCore/platform/graphics/Path.cpp:109
> +    AffineTransform ellipseTransform =
AffineTransform().rotate(rad2deg(rotation)).scale(radiusX, radiusY);

Is the origin in the middle of the ellipse? Otherwise, don't you need to set
the origin to p before rotating and scaling the ellipse?

> LayoutTests/fast/canvas/ellipse-crash.html:16
> +	   context.ellipse(10, 10, 20, 20, 0, 20, 1.0/0.0, true);

Can't you just use Math.NaN and Math.Infinite? That would make more sense here.


> LayoutTests/fast/canvas/script-tests/canvas-ellipse.js:20
> +ctx.ellipse(200, 200, 100, 150, deg2rad(20), deg2rad(-180), deg2rad(100),
false);

Why not use rad values directly like Math.PI * 2, Math.PI * 0.5 and so on. I
think you have by far not enough tests. You do not test all values and it is
not so easy to say if the drawing is correct even for this simple case. It is
also interesting to know how ellipse interacts with other path arguments. Maybe
it is possible to do a ref test. (Ref test would have a DRT call as well to
tell to wait for the completion of the rendering. But not sure how it would
look like.)