Bug 63516 – Some canvas arc tests fail because the arc implementaion does not match the HTML5 canvas specification.

• Component: Canvas
• Status: RESOLVED CONFIGURATION CHANGED

[Dongseong Hwang]

Following tests are failed in Chromium port.
* LayoutTests/canvas/philip/tests/2d.path.arc.angle.3.html
* LayoutTests/canvas/philip/tests/2d.path.arc.angle.5.html

If the anticlockwise argument is omitted or false and endAngle-startAngle is less than -2Pi, Chromium draws the whole circumference.

[Dongseong Hwang]

Created attachment 98872 [details]
patch

Following tests are failed in Chromium port.
* LayoutTests/canvas/philip/tests/2d.path.arc.angle.3.html
* LayoutTests/canvas/philip/tests/2d.path.arc.angle.5.html

It is not because of skia bug.
Other ports pass above test because their graphics backends are familiar with HTML5 Canvas arc specification by chance.

This patch makes CanvasRenderingContext2D::arc match following paragraph in HTML5 Canvas spec.
http://dev.w3.org/html5/2dcontext/#dom-context-2d-createpattern

"Otherwise, the arc is the path along the circumference of this circle
from the start point to the end point, going anti-clockwise if the
anticlockwise argument is true, and clockwise otherwise. Since the
points are on the circle, as opposed to being simply angles from zero,
the arc can never cover an angle greater than 2π radians. If the two
points are the same, or if the radius is zero, then the arc is defined
as being of zero length in both directions."

[WebKit Review Bot]

Comment on attachment 98872 [details]
patch

Attachment 98872 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8954269

New failing tests:
fast/canvas/canvas-arc-360-winding.html
fast/canvas/canvas-strokePath-alpha-shadow.html
fast/canvas/canvas-fillPath-alpha-shadow.html
fast/canvas/canvas-fillPath-pattern-shadow.html
canvas/philip/tests/2d.line.join.round.html
fast/canvas/canvas-fillPath-gradient-shadow.html

[WebKit Review Bot]

Created attachment 98876 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Matthew Delaney]

Comment on attachment 98872 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98872&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:904
> +        ea = fmod(ea , 2 * piFloat);

These are just floats here, so I believe you just need fmodf. Same for below.

[Dongseong Hwang]

Created attachment 99671 [details]
patch

The 5 layout tests that were failed don't use Canvas arc api correctly.
I fixed those.

[Dongseong Hwang]

Created attachment 99673 [details]
patch

[WebKit Review Bot]

Comment on attachment 99673 [details]
patch

Attachment 99673 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8983557

New failing tests:
canvas/philip/tests/2d.path.arc.nonempty.html

[WebKit Review Bot]

Created attachment 99684 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dongseong Hwang]

Created attachment 99691 [details]
patch

[Matthew Delaney]

The tests in the canvas/philip directory are basically just snapshots of the tests created by Philip Taylor which he has hosted here: http://philip.html5.org/tests/canvas/suite/tests/

IIRC, they've all been submitted to the w3c as well: http://w3c-test.org/html/tests/submission/PhilipTaylor/

So, as a general rule we avoid modifying our local copies of the tests if we find any issues. Instead, if you are 100% positive that a test has issues or is incorrect/outdated then you should contact Philip directly and tell him what's wrong with the test. If he agrees, he'll update the tests, and then we'll grab the latest copies and skip the faulty tests in the meantime.

I'm doubtful that Philip had that many errors in his tests as your latest patch shows. So, either we have old versions of those tests and the spec has changed or something (also unlikely) then you might want to take a second look at the validity of your changes to the tests.

[Dongseong Hwang]

Thank you for good advice.
I did send email about it to Philip Taylor.

[Chang Shu]

(In reply to comment #11)
> Thank you for good advice.
> I did send email about it to Philip Taylor.

any updates?

[Dongseong Hwang]

(In reply to comment #12)
> (In reply to comment #11)
> > Thank you for good advice.
> > I did send email about it to Philip Taylor.
> 
> any updates?

I'm sorry. Mr Taylor did not response to m, yet.

[Dirk Schulze]

Comment on attachment 99691 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99691&action=review

> LayoutTests/ChangeLog:16
> +        the arc can never cover an angle greater than 2Ï radians. If the two

The pi signe looks strange. Just use pi.

> LayoutTests/ChangeLog:18
> +        points are the same, or if the radius is zero, then the arc is defined
> +        as being of zero length in both directions."

I don't like this text at all. It basically says: You can not draw a full circle.

> LayoutTests/ChangeLog:29
> +        Fix 9 canvas layout tests because those are failed after this patch. The layout
> +        tests use arc api like this, "ctx.arc(x, y, r, 0, Math.PI*2, true);".
> +        Above code should not draw circle to conform Canvas spec. Above code draw
> +        nothing. Above code should be changed to following code to draw circle.
> +            ctx.arc(x, y, r, 0, Math.PI*2, false);
> +            ctx.arc(x, y, r, Math.PI*2, 0, true);

I don't get your point. The necessary quote is 

"If the two points are the same,..., then the arc is defined as being of zero length in both directions."

So it doesn't draw anything, independent of clockwise or anti-clockwise, no?

[Julien Chaffraix]

Comment on attachment 99691 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99691&action=review

The patch doesn't apply anymore and there were some issues to be sorted out. If this gets cleared out, feel free to resubmit an updated patch.

> LayoutTests/ChangeLog:5
> +

This ChangeLog doesn't have the "Reviewed by NOBODY (OOPS!)." line so your patch cannot land (our tools will choke on it).

[Brent Fulgham]

WebKit is currently passing all WPT for 2d.path.arc.angle, so I believe this is resolved.