Bug 20435 – Canvas missing exceptioncode for gradients

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Schulze]

Exceptions for wrong values (like NaN, infinity, negative radius) are missing for gradients in canvas.

[Dirk Schulze]

Created attachment 22865 [details]
Added ExceptionCode

Added the missing exceptions.

[Dirk Schulze]

Created attachment 22875 [details]
Exceptions in Canvas

Added exceptions for addColorStop.

[Dirk Schulze]

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.

[Dirk Schulze]

Created attachment 22881 [details]
Exceptions in canvas

Corrects some bugs in the patch above

[Dirk Schulze]

Created attachment 22888 [details]
Exceptions in canvas

Added and corrected more exceptions.

[Dirk Schulze]

Created attachment 22897 [details]
drawImage() update

Updated drawImage() to match the current specification. The thrown exceptions now match the specification as well as firefox.

[Oliver Hunt]

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.

[Dirk Schulze]

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.

[Sam Weinig]

Comment on attachment 23140 [details]
Canvas gradients exceptions

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

r=me

[Dirk Schulze]

Created attachment 23197 [details]
Canvas Gradients exception

see comment above

[Eric Seidel (no email)]

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.

[Dirk Schulze]

Created attachment 23390 [details]
Canvas Gradients exception

rewrote testcases.

[Eric Seidel (no email)]

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.

[Darin Adler]

I decided not to wait for the positive tests. I'll land this now.

[Darin Adler]

http://trac.webkit.org/changeset/37538