Bug 63516 - Some canvas arc tests fail because the arc implementaion does not match the HTML5 canvas specification.
: Some canvas arc tests fail because the arc implementaion does not match the H...
Status: UNCONFIRMED
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-28 00:36 PDT by Dongseong Hwang
Modified: 2013-02-28 10:39 PST (History)
7 users (show)

See Also:


Attachments
patch (3.98 KB, patch)
2011-06-28 00:41 PDT, Dongseong Hwang
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.28 MB, application/zip)
2011-06-28 00:58 PDT, WebKit Review Bot
no flags Details
patch (8.10 KB, patch)
2011-07-05 00:35 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
patch (12.55 KB, patch)
2011-07-05 00:40 PDT, Dongseong Hwang
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.39 MB, application/zip)
2011-07-05 02:00 PDT, WebKit Review Bot
no flags Details
patch (18.57 KB, patch)
2011-07-05 03:21 PDT, Dongseong Hwang
jchaffraix: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2011-06-28 00:36:37 PDT
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.
Comment 1 Dongseong Hwang 2011-06-28 00:41:03 PDT
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."
Comment 2 WebKit Review Bot 2011-06-28 00:58:16 PDT
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
Comment 3 WebKit Review Bot 2011-06-28 00:58:21 PDT
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
Comment 4 Matthew Delaney 2011-06-28 13:19:06 PDT
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.
Comment 5 Dongseong Hwang 2011-07-05 00:35:14 PDT
Created attachment 99671 [details]
patch

The 5 layout tests that were failed don't use Canvas arc api correctly.
I fixed those.
Comment 6 Dongseong Hwang 2011-07-05 00:40:58 PDT
Created attachment 99673 [details]
patch
Comment 7 WebKit Review Bot 2011-07-05 02:00:02 PDT
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
Comment 8 WebKit Review Bot 2011-07-05 02:00:07 PDT
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
Comment 9 Dongseong Hwang 2011-07-05 03:21:56 PDT
Created attachment 99691 [details]
patch
Comment 10 Matthew Delaney 2011-07-06 11:05:39 PDT
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.
Comment 11 Dongseong Hwang 2011-07-07 03:12:21 PDT
Thank you for good advice.
I did send email about it to Philip Taylor.
Comment 12 Chang Shu 2012-02-13 05:48:46 PST
(In reply to comment #11)
> Thank you for good advice.
> I did send email about it to Philip Taylor.

any updates?
Comment 13 Dongseong Hwang 2012-02-16 03:14:04 PST
(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.
Comment 14 Dirk Schulze 2013-02-16 00:22:13 PST
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?
Comment 15 Julien Chaffraix 2013-02-28 10:39:05 PST
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).