Bug 55696 - canvas arc() missing line to start of arc if swing is zero
Summary: canvas arc() missing line to start of arc if swing is zero
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tom Zakrajsek
URL:
Keywords:
Depends on: 66832
Blocks: 67301
  Show dependency treegraph
 
Reported: 2011-03-03 12:09 PST by Cary Clark
Modified: 2011-08-31 10:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.46 KB, patch)
2011-08-24 22:38 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Patch - fix compare to 0 style error (5.46 KB, patch)
2011-08-24 22:53 PDT, Tom Zakrajsek
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (5.46 KB, application/octet-stream)
2011-08-24 22:57 PDT, Tom Zakrajsek
no flags Details
Patch (5.86 KB, patch)
2011-08-25 07:25 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2011-08-25 14:27 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2011-08-26 11:07 PDT, Tom Zakrajsek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cary Clark 2011-03-03 12:09:13 PST
In https://bugs.webkit.org/show_bug.cgi?id=41420 Andreas Kling quotes
http://www.whatwg.org/specs/web-apps/current-work/#dom-context-2d-stroke
 "Zero-length line segments must be pruned before stroking a path.
Empty subpaths must be ignored."

And implemented this in CanvasRenderingContext2D::arc() by adding:
+    if (sa == ea)
+        return;

However, the context.arc description in the Canvas element spec at
http://dev.w3.org/html5/2dcontext/ reads:

"The arc(x, y, radius, startAngle, endAngle, anticlockwise) method
draws an arc. If the context has any subpaths, then the method must
add a straight line from the last point in the subpath to the start
point of the arc. In any case, it must draw the arc between the start
point of the arc and the end point of the arc, and add the start and
end points of the arc to the subpath."

and:

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

This bug http://code.google.com/p/chromium/issues/detail?id=52059
containing this fragment:
   <canvas id="cvs" width="600" height="250" style="border: 1px
dashed black">[No canvas support]</canvas>
   <script>
       co = document.getElementById("cvs").getContext('2d');
       co.beginPath();
           co.moveTo(100,100);
           co.arc(100,100,100,0.785,0.785,0);
           co.lineTo(100,100);
       co.stroke();
   </script>

which shows that this change prevents the line from being drawn from
(100,100) to the arc start.

Opera, Firefox 3 and 4 draw this line, but Chrome and nightly Safari do not.
Comment 1 Tom Zakrajsek 2011-08-24 22:38:26 PDT
Created attachment 105132 [details]
Patch

This patch also has the fix for related bug https://bugs.webkit.org/show_bug.cgi?id=66832.
Comment 2 WebKit Review Bot 2011-08-24 22:40:38 PDT
Attachment 105132 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/canv..." exit_code: 1

Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:830:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tom Zakrajsek 2011-08-24 22:53:39 PDT
Created attachment 105133 [details]
Patch - fix compare to 0 style error

For the record, I don't like this.  Having to do a boolean check to see if a float is 0 or not, for "style" reasons, doesn't feel right.
Comment 4 Tom Zakrajsek 2011-08-24 22:57:33 PDT
Created attachment 105134 [details]
Patch
Comment 5 WebKit Review Bot 2011-08-24 23:17:36 PDT
Comment on attachment 105133 [details]
Patch - fix compare to 0 style error

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

New failing tests:
canvas/philip/tests/2d.path.arc.zeroradius.html
fast/canvas/canvas-arc-zero-lineto.html
Comment 6 Kent Tamura 2011-08-24 23:56:56 PDT
(In reply to comment #4)
> Created an attachment (id=105134) [details]
> Patch

Do you want it is reviewed?  If so, open "Details" page of the attachment, change MIME type, check "Patch" checkbox, and select '?' for review flag.  You had better use "webkit-patch upload" script. It takes care of them.
Comment 7 Tom Zakrajsek 2011-08-25 07:25:57 PDT
Created attachment 105183 [details]
Patch
Comment 8 Tom Zakrajsek 2011-08-25 07:32:31 PDT
Comment on attachment 105183 [details]
Patch

Missed a test failure from previous bot run.
Comment 9 Tom Zakrajsek 2011-08-25 14:27:31 PDT
Created attachment 105242 [details]
Patch
Comment 10 Andreas Kling 2011-08-26 08:49:10 PDT
Comment on attachment 105242 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:834
> +        FloatPoint pt(x + r * cos(sa), y + r * sin(sa));

Shouldn't that be cosf() and sinf()?
Comment 11 Tom Zakrajsek 2011-08-26 11:07:51 PDT
Created attachment 105382 [details]
Patch
Comment 12 Andreas Kling 2011-08-29 09:14:14 PDT
Comment on attachment 105382 [details]
Patch

LGTM.
Comment 13 WebKit Review Bot 2011-08-29 10:15:11 PDT
Comment on attachment 105382 [details]
Patch

Clearing flags on attachment: 105382

Committed r93982: <http://trac.webkit.org/changeset/93982>
Comment 14 WebKit Review Bot 2011-08-29 10:15:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Ryosuke Niwa 2011-08-30 00:01:01 PDT
canvas/philip/tests/2d.path.stroke.prune.arc.html started failing on Snow Leopard after this patch:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r93982%20(1898)/canvas/philip/tests/2d.path.stroke.prune.arc-pretty-diff.html