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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 10 00:30:47 PDT 2014

Dirk Schulze <krit at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 82791: Add support for new canvas ellipse method

Attachment 232674: Patch

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

The patch looks really great. Some snippets inline. Please fix the build
breakage for GTK and EFL before landing. Just move the code from PathCG to
Path.cpp as described later and it works everywhere. r=me with the changes

> Source/WebCore/ChangeLog:19
> +	   Convience for passing a FloatPoint instead of two floats.


> Source/WebCore/ChangeLog:23
> +	   is greater than 0 and less than 2pi, and the the endAngle is at most

s/the the/the/

> Source/WebCore/html/canvas/CanvasPathMethods.cpp:217
> +	   if (!anticlockwise) {
> +	       for (float angle = startAngle - fmodf(startAngle,
piOverTwoFloat) + piOverTwoFloat; angle < endAngle; angle += piOverTwoFloat)
> +		   lineTo(transform.mapPoint(FloatPoint(radiusX * cosf(angle),
radiusY * sinf(angle))));
> +	   } else {
> +	       for (float angle = startAngle - fmodf(startAngle,
piOverTwoFloat); angle > endAngle; angle -= piOverTwoFloat)
> +		   lineTo(transform.mapPoint(FloatPoint(radiusX * cosf(angle),
radiusY * sinf(angle))));
> +	   }

It took me a bit to understand why you are doing this. Especially to get dash
arrays and line caps right we must have the multiple moveTos across the
otherwise straight line, but a short comment would help :).

IMO the spec should have a comment as well. Just to warn implementations.
Couldn't find it in the spec when I reviewed the patch.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:285
> +void Path::addArc(const FloatPoint& p, float radius, float startAngle, float
endAngle, bool clockwise)

The other implementations (at least PathCairo) need the same function with
NotImplemented(). And the tests need to be skipped.

> Source/WebCore/platform/graphics/cg/PathCG.cpp:303
> +    AffineTransform transform;
> +    transform.translate(p.x(),
p.y()).rotate(rad2deg(rotation)).scale(radiusX, radiusY);
> +
> +    CGAffineTransform cgTransform = transform;
> +    CGPathAddArc(ensurePlatformPath(), &cgTransform, 0, 0, 1, startAngle,
endAngle, anticlockwise);

This could actually mode to Path.cpp. The other graphic libraries don't have
ellipse either and would do the same. This would also make the other bots
build. You would need to transform the Path context:


Then the comment above gets superfluous.

> LayoutTests/ChangeLog:5
> +

Some crazy tests :P

It seems that some of the tests were submitted to Blink but I couldn't find
them on this bug report. Could you add a link to the Blink commit (I think
https://chromiumcodereview.appspot.com/14298022). Or mention the author please?

More information about the webkit-reviews mailing list