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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=82791

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

------- 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
below.

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

convenience?

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

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:

transform(transform);
addArc(...);

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?