This is required to pass 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.type.html http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create1.zero.html

Some refactoring was done in bug 630052 -- it shall land before this one

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 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 :)

Including feedback items and additional imgdata.width and .height check into the patch.

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 on attachment 516730 [details] [diff] [review] Adds the failed tests to the test_canvas and fixes -basic, -initial and -zero Er, I meant r-.

(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)

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

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

There's no useful enforcement of that in the spec; anyone could define a getter on the ImageData object that returns arbitrary gunk.

See http://www.w3.org/Bugs/Public/show_bug.cgi?id=11670

OK. That still leaves the question of what to do in our implementation, where for the moment they can be negative....

(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 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?

Speaking of which, Ms2ger, want to just make sure this lands? ;)

Certainly. (I don't think it conflicted, actually.)

Fixed that comment.

http://hg.mozilla.org/mozilla-central/rev/7c9c29d52829 Yury, thank you for the patch!

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.

> Was it not actually supported yet? That's correct.