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
: WebKit
Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-06-28 00:36 PST by
Modified: 2013-02-28 10:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-28 00:36:37 PST
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 From 2011-06-28 00:41:03 PST -------
Created an attachment (id=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 From 2011-06-28 00:58:16 PST -------
(From update of attachment 98872 [details])
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 From 2011-06-28 00:58:21 PST -------
Created an attachment (id=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 From 2011-06-28 13:19:06 PST -------
(From update of attachment 98872 [details])
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 From 2011-07-05 00:35:14 PST -------
Created an attachment (id=99671) [details]
patch

The 5 layout tests that were failed don't use Canvas arc api correctly.
I fixed those.
------- Comment #6 From 2011-07-05 00:40:58 PST -------
Created an attachment (id=99673) [details]
patch
------- Comment #7 From 2011-07-05 02:00:02 PST -------
(From update of attachment 99673 [details])
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 From 2011-07-05 02:00:07 PST -------
Created an attachment (id=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 From 2011-07-05 03:21:56 PST -------
Created an attachment (id=99691) [details]
patch
------- Comment #10 From 2011-07-06 11:05:39 PST -------
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 From 2011-07-07 03:12:21 PST -------
Thank you for good advice.
I did send email about it to Philip Taylor.
------- Comment #12 From 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 From 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 From 2013-02-16 00:22:13 PST -------
(From update of attachment 99691 [details])
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 From 2013-02-28 10:39:05 PST -------
(From update of attachment 99691 [details])
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).