Bug 20435 - Canvas missing exceptioncode for gradients
Summary: Canvas missing exceptioncode for gradients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-18 15:40 PDT by Dirk Schulze
Modified: 2008-10-12 17:35 PDT (History)
0 users

See Also:


Attachments
Added ExceptionCode (4.23 KB, patch)
2008-08-18 15:44 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Exceptions in Canvas (6.46 KB, patch)
2008-08-19 00:47 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Exceptions in canvas (9.14 KB, patch)
2008-08-19 12:25 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Exceptions in canvas (10.28 KB, patch)
2008-08-19 13:48 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Exceptions in canvas (12.60 KB, patch)
2008-08-19 23:46 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
drawImage() update (4.75 KB, patch)
2008-08-20 12:33 PDT, Dirk Schulze
oliver: review-
Details | Formatted Diff | Diff
Canvas gradients exceptions (21.34 KB, patch)
2008-09-03 06:20 PDT, Dirk Schulze
sam: review+
Details | Formatted Diff | Diff
Canvas Gradients exception (21.32 KB, patch)
2008-09-05 12:18 PDT, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
Canvas Gradients exception (20.53 KB, patch)
2008-09-13 03:48 PDT, Dirk Schulze
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2008-08-18 15:40:27 PDT
Exceptions for wrong values (like NaN, infinity, negative radius) are missing for gradients in canvas.
Comment 1 Dirk Schulze 2008-08-18 15:44:14 PDT
Created attachment 22865 [details]
Added ExceptionCode

Added the missing exceptions.
Comment 2 Dirk Schulze 2008-08-19 00:47:39 PDT
Created attachment 22875 [details]
Exceptions in Canvas

Added exceptions for addColorStop.
Comment 3 Dirk Schulze 2008-08-19 12:25:54 PDT
Created attachment 22879 [details]
Exceptions in canvas

Added some more Exceptions to match and updated excisting ones to match the current specifications of Canvas.

I'm not sure about CanvasGradient and FloatRect.
I added the Exceptions to the CanvasGradient.cpp-code. I think thats ok so, since Pattern makes the same.
I don't know if FloatRect::contains() is used by another function of webkit beside Canvas. But after the changes on the Spec, rect's can be negative. And contains() wasn't able to check negative rects.
Comment 4 Dirk Schulze 2008-08-19 13:48:13 PDT
Created attachment 22881 [details]
Exceptions in canvas

Corrects some bugs in the patch above
Comment 5 Dirk Schulze 2008-08-19 23:46:34 PDT
Created attachment 22888 [details]
Exceptions in canvas

Added and corrected more exceptions.
Comment 6 Dirk Schulze 2008-08-20 12:33:18 PDT
Created attachment 22897 [details]
drawImage() update

Updated drawImage() to match the current specification. The thrown exceptions now match the specification as well as firefox.
Comment 7 Oliver Hunt 2008-08-20 13:06:27 PDT
Comment on attachment 22897 [details]
drawImage() update

fixing draw image should go in a separate bug, however issues i spotted, that make me r- this in its current form:
You changed FloatRect::contains -- i'm not entirely happy with this as FloatRect as the contains semantics would be subtly different from IntRect::contains.  A better approach would to normalise the rect before doing the contains check.

The code also now ends up checking for !srcRect.width(), etc multiple times throwing an exception in one case and silently returning in another.

Your testcase also isn't particularly great as you  don't actually test that you can specify a negative sized source rect (0-width/height for some of the components result in an exception), and you don't test the rendering behaviour of a negative source.  This last part is especially critical as you are adding new functionality, so ou need to confirm that the new functionality actually works.
Comment 8 Dirk Schulze 2008-09-03 06:20:21 PDT
Created attachment 23140 [details]
Canvas gradients exceptions

Added the exception on gradients that are thrown by FireFox (some times with a wrong text/exception code), Opera and match the specifications to WebKit.
Comment 9 Sam Weinig 2008-09-05 09:57:33 PDT
Comment on attachment 23140 [details]
Canvas gradients exceptions

+    if (!isfinite(value) || value < 0. || value > 1.) {
Please use 0 and 1.0f here.

r=me
Comment 10 Dirk Schulze 2008-09-05 12:18:06 PDT
Created attachment 23197 [details]
Canvas Gradients exception

see comment above
Comment 11 Eric Seidel (no email) 2008-09-12 12:13:42 PDT
Comment on attachment 23197 [details]
Canvas Gradients exception

This looks fine, but it *really* should use the new js test system.  see LayoutTests/fast/js for examples.

Then things like:

+        try{
+            var gradient = ctx.createLinearGradient(0, 0, 100, -Infinity);
+            print("FAIL");
+        } catch (e) {
+            print("PASS: a -Infinity value for y1, got exception as expected");
+        }

turn into:
shouldThrow("ctx.createLinearGradient(0, 0, 100, -Infinity)")

You could even re-write this as a loop if you really wanted to, which built up different arg arrays.
Comment 12 Dirk Schulze 2008-09-13 03:48:39 PDT
Created attachment 23390 [details]
Canvas Gradients exception

rewrote testcases.
Comment 13 Eric Seidel (no email) 2008-09-15 15:12:14 PDT
Comment on attachment 23390 [details]
Canvas Gradients exception

This looks fine.  It would be best if you could add a couple positive tests as well (to make sure that your exceptions aren't thrown too often, or that future added code doesn't make the exceptions thrown too often.
Comment 14 Darin Adler 2008-10-12 17:12:21 PDT
I decided not to wait for the positive tests. I'll land this now.
Comment 15 Darin Adler 2008-10-12 17:35:09 PDT
http://trac.webkit.org/changeset/37538