Last Comment Bug 630040 - Implement createImageData(ImageData)
: Implement createImageData(ImageData)
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: Yury Delendik (:yury)
:
: Milan Sreckovic [:milan]
Mentors:
http://www.whatwg.org/html/#dom-conte...
Depends on: post2.0 630052
Blocks: 622842
  Show dependency treegraph
 
Reported: 2011-01-30 10:20 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-05-13 08:04 PDT (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero (9.18 KB, patch)
2011-03-02 20:46 PST, Yury Delendik (:yury)
Ms2ger: feedback+
Details | Diff | Splinter Review
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero (9.63 KB, patch)
2011-03-03 16:01 PST, Yury Delendik (:yury)
bzbarsky: review-
Details | Diff | Splinter Review
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero, refactors putImageData (12.08 KB, patch)
2011-03-04 17:51 PST, Yury Delendik (:yury)
bzbarsky: review+
Details | Diff | Splinter Review
Patch for checkin (12.07 KB, patch)
2011-03-18 02:49 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch for checkin. (13.46 KB, patch)
2011-04-02 11:51 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review

Comment 1 Yury Delendik (:yury) 2011-03-02 16:05:17 PST
Some refactoring was done in bug 630052 -- it shall land before this one
Comment 2 Yury Delendik (:yury) 2011-03-02 20:46:05 PST
Created attachment 516499 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero

Fixes 
http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create1.basic.html
http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create1.initial.html
http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create1.zero.html

The http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create1.type.html test added with TODO check (similar to 2d.imageData.create.type one). 

http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create2.type.html does not pass as well due to absence of the ImageData and PixelsCanvasArray objects.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-03-03 08:39:53 PST
Comment on attachment 516499 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero

>diff -r 0f3654e8b637 content/canvas/src/CustomQS_Canvas2D.h
>--- a/content/canvas/src/CustomQS_Canvas2D.h	Wed Mar 02 21:11:03 2011 -0600
>+++ b/content/canvas/src/CustomQS_Canvas2D.h	Wed Mar 02 22:37:43 2011 -0600
>@@ -213,22 +213,46 @@
>+        // grab width, height, and the dense array from the dataObject

You don't grab the dense array.

>diff -r 0f3654e8b637 content/canvas/test/test_canvas.html
>--- a/content/canvas/test/test_canvas.html	Wed Mar 02 21:11:03 2011 -0600
>+++ b/content/canvas/test/test_canvas.html	Wed Mar 02 22:37:43 2011 -0600
>@@ -7457,16 +7504,48 @@
>     _thrown_outer = true;
> }
> todo(!_thrown_outer, 'should not throw exception');
> 
> 
> }
> </script>
> 
>+<!-- [[[ test_2d.imageData.create1.type.html ]]] -->
>+
>+<p>Canvas test: 2d.imageData.create1.type - bug 630040</p>
>+<!-- Testing: createImageData(imgdata) returns an ImageData object containing a CanvasPixelArray object -->
>+<canvas id="c261a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
>+<script>
>+
>+function test_2d_imageData_create1_type() {
>+
>+var canvas = document.getElementById('c261a');
>+var ctx = canvas.getContext('2d');
>+
>+var _thrown_outer = false;
>+try {
>+
>+todo(window.ImageData !== undefined, "window.ImageData !== undefined");
>+todo(window.CanvasPixelArray !== undefined, "window.CanvasPixelArray !== undefined");
>+window.ImageData.prototype.thisImplementsImageData = true;
>+window.CanvasPixelArray.prototype.thisImplementsCanvasPixelArray = true;
>+var imgdata = ctx.createImageData(ctx.createImageData(1, 1));
>+ok(imgdata.thisImplementsImageData, "imgdata.thisImplementsImageData");
>+ok(imgdata.data.thisImplementsCanvasPixelArray, "imgdata.data.thisImplementsCanvasPixelArray");

These oks confused me at first, because I didn't realize we'd throw before we hit them. Maybe todo?

>+
>+} catch (e) {
>+    _thrown_outer = true;
>+}
>+todo(!_thrown_outer, 'should not throw exception');
>+
>+
>+}
>+</script>
>+
> <!-- [[[ test_2d.imageData.create.zero.html ]]] -->
> 
> <p>Canvas test: 2d.imageData.create.zero - bug 433004</p>
> <!-- Testing: createImageData() throws INDEX_SIZE_ERR if size is zero -->
> <canvas id="c262" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
> <script>
> 
> function test_2d_imageData_create_zero() {
>@@ -7483,16 +7562,36 @@
> var _thrown = undefined; try {
>   ctx.createImageData(0, 0);
> } catch (e) { _thrown = e }; ok(_thrown && _thrown.code == DOMException.INDEX_SIZE_ERR, "should throw INDEX_SIZE_ERR");
> 
> 
> }
> </script>
> 
>+<!-- [[[ test_2d.imageData.create1.zero.html ]]] -->
>+
>+<p>Canvas test: 2d.imageData.create1.zero - bug 630040</p>
>+<!-- Testing: createImageData(null) throws INDEX_SIZE_ERR -->

Not your fault, but could you make this say NOT_SUPPORTED_ERR?

>+<canvas id="c262a" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
>+<script>
>+
>+function test_2d_imageData_create1_zero() {
>+
>+var canvas = document.getElementById('c262a');
>+var ctx = canvas.getContext('2d');
>+
>+var _thrown = undefined; try {
>+  ctx.createImageData(null);
>+} catch (e) { _thrown = e }; ok(_thrown && _thrown.code == DOMException.NOT_SUPPORTED_ERR, "should throw NOT_SUPPORTED_ERR");
>+
>+
>+}
>+</script>
>+
> <!-- [[[ test_2d.imageData.get.basic.html ]]] -->
> 
> <p>Canvas test: 2d.imageData.get.basic</p>
> <!-- Testing: getImageData() exists and returns something -->
> <canvas id="c263" width="100" height="50"><p class="fallback">FAIL (fallback content)</p></canvas>
> <script>
> 
> function test_2d_imageData_get_basic() {

Looks good otherwise.

Boris, since you reviewed bug 630052, you get this one too :)
Comment 4 Yury Delendik (:yury) 2011-03-03 16:01:21 PST
Created attachment 516730 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero

Including feedback items and additional imgdata.width and .height check into the patch.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-03-04 11:17:35 PST
Comment on attachment 516730 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero

>+        if (JSVAL_IS_NULL(argv[0]))
>+            return xpc_qsThrow(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);

This is redundant.  null is JSVAL_IS_PRIMITIVE...  Or is the point that for some reason we need to throw a different exception here?  If so, that's .... annoying.

> +        // grab width and height
> +        js::AutoValueRooter tv(cx);

You shouldn't need a rooter here.  Just get into on-stack jsvals.

>+        if (!JS_GetProperty(cx, dataObject, "width", tv.jsval_addr()) ||

Somewhere in here you need to check that dataObject is ImageData object, no?  I guess that's hard for the moment because they're just vanilla objects in our implementation....  Let's not worry about it for now.

>+        if (data_wi <= 0 || data_hi <= 0)
>+            return xpc_qsThrow(cx, NS_ERROR_DOM_INDEX_SIZE_ERR);

The spec says nothing about throwing in this case.  Why are we doing this?

>-ok(imgdata.thisImplementsImageData, "imgdata.thisImplementsImageData");
>-ok(imgdata.data.thisImplementsCanvasPixelArray, "imgdata.data.thisImplementsCanvasPixelArray");
>+todo(imgdata.thisImplementsImageData, "imgdata.thisImplementsImageData");
>+todo(imgdata.data.thisImplementsCanvasPixelArray, "imgdata.data.thisImplementsCanvasPixelArray");

Why that change?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-03-04 12:49:05 PST
Comment on attachment 516730 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero

Er, I meant r-.
Comment 7 Yury Delendik (:yury) 2011-03-04 17:51:51 PST
Created attachment 517063 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero, refactors putImageData

(In reply to comment #5)

> >+        if (JSVAL_IS_NULL(argv[0]))
> >+            return xpc_qsThrow(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> 
> This is redundant.  null is JSVAL_IS_PRIMITIVE...  Or is the point that for
> some reason we need to throw a different exception here?  If so, that's ....
> annoying.

I could not find in the specification information about what to return in primitives case, it is talking only about null. However I found the "2d.missingargs" that tests if NOT_SUPPORTED exception raised if the argument is a number. The patch is fixed to reflect that. (The existing test_canvas.html in the "2d.missingargs" tests contains only TODO's, am I opening can of worms, and there a bug about this?)

> > +        // grab width and height
> > +        js::AutoValueRooter tv(cx);
> 
> You shouldn't need a rooter here.  Just get into on-stack jsvals. 

and

> >+        if (!JS_GetProperty(cx, dataObject, "width", tv.jsval_addr()) ||
> 
> Somewhere in here you need to check that dataObject is ImageData object, no?  
> guess that's hard for the moment because they're just vanilla objects in our
> implementation....  Let's not worry about it for now.
> 
> >+        if (data_wi <= 0 || data_hi <= 0)
> >+            return xpc_qsThrow(cx, NS_ERROR_DOM_INDEX_SIZE_ERR);
> 
> The spec says nothing about throwing in this case.  Why are we doing this?

logic was taken from the putImageData function below. We operate with object that has the width, height and data properties. Keeping the same logic but refactoring the createImageData and putImageData functions into single method.

> >-ok(imgdata.thisImplementsImageData, "imgdata.thisImplementsImageData");
> >-ok(imgdata.data.thisImplementsCanvasPixelArray, "imgdata.data.thisImplementsCanvasPixelArray");
> >+todo(imgdata.thisImplementsImageData, "imgdata.thisImplementsImageData");
> >+todo(imgdata.data.thisImplementsCanvasPixelArray, "imgdata.data.thisImplementsCanvasPixelArray");
> 
> Why that change?

Removing that from the patch. I was trying to make it less confissing. (See comment #3 above) Those ok's or todo's are not executed anyway -- it fails on one of the previous lines and intercepted by the enclosing try)
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2011-03-07 12:42:40 PST
(In reply to comment #5)
> Comment on attachment 516730 [details] [diff] [review]
> Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero
> 
> >+        if (JSVAL_IS_NULL(argv[0]))
> >+            return xpc_qsThrow(cx, NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> 
> This is redundant.  null is JSVAL_IS_PRIMITIVE...  Or is the point that for
> some reason we need to throw a different exception here?  If so, that's ....
> annoying.

Technically, I believe we need to throw a TypeError per WebIDL. (I note that some thoughts to disallow null at the WebIDL level, and if that happens, we'd probably need to throw TypeError for null as well.) If we can't throw TypeErrors here, I suppose it doesn't matter much which exceptions we do throw.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-03-07 12:44:54 PST
(In reply to comment #5)
> >+        if (data_wi <= 0 || data_hi <= 0)
> >+            return xpc_qsThrow(cx, NS_ERROR_DOM_INDEX_SIZE_ERR);
> 
> The spec says nothing about throwing in this case.  Why are we doing this?

This can't happen per spec, as those are unsigned longs.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-03-07 13:14:20 PST
There's no useful enforcement of that in the spec; anyone could define a getter on the ImageData object that returns arbitrary gunk.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-03-07 13:37:28 PST
See http://www.w3.org/Bugs/Public/show_bug.cgi?id=11670
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-03-07 14:16:52 PST
OK.  That still leaves the question of what to do in our implementation, where for the moment they can be negative....
Comment 13 Yury Delendik (:yury) 2011-03-07 16:34:35 PST
(In reply to comment #10)
> There's no useful enforcement of that in the spec; anyone could define a getter
> on the ImageData object that returns arbitrary gunk.

Agree. I used the putImageData function as a reference, it checks the all properties of the imgdata argument including width and height (also some info in bug 534467). The INDEX_SIZE_ERR error is good as any other. The new patch contains GetImageDataDimensions() code that was refactored from the putImageData's.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-03-16 08:34:24 PDT
Comment on attachment 517063 [details] [diff] [review]
Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero, refactors putImageData

>+        // The object is expected, so throwing the exception for all primitives.

"An object is expected, so throw an exception for all primitives."

This is going to conflict with the patches in bug 629894; please coordinate landing with ms2ger?
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-03-16 08:34:48 PDT
Speaking of which, Ms2ger, want to just make sure this lands?  ;)
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2011-03-16 13:41:49 PDT
Certainly. (I don't think it conflicted, actually.)
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2011-03-18 02:49:39 PDT
Created attachment 520161 [details] [diff] [review]
Patch for checkin

Fixed that comment.
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2011-04-02 11:51:26 PDT
Created attachment 523801 [details] [diff] [review]
Patch for checkin.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2011-04-04 22:39:56 PDT
http://hg.mozilla.org/projects/cedar/rev/7c9c29d52829
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-04-05 11:32:37 PDT
http://hg.mozilla.org/mozilla-central/rev/7c9c29d52829

Yury, thank you for the patch!
Comment 21 Eric Shepherd [:sheppy] 2011-04-12 12:59:52 PDT
Hm... what's new here? This page:

https://developer.mozilla.org/En/HTML/Canvas/Pixel_manipulation_with_canvas#Creating_an_ImageData_object

Already had createImageData(ImageData) documented. Was it not actually supported yet?

At any rate, I changed it to say that feature requires Gecko 5.0, and added it to the Firefox 5 for developers page.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-04-12 13:48:31 PDT
> Was it not actually supported yet?

That's correct.

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