Implement createImageData(ImageData)

RESOLVED FIXED in mozilla5

Status

()

Core
Canvas: 2D
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: yury)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla5
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → async.processingjs
(Assignee)

Comment 1

6 years ago
Some refactoring was done in bug 630052 -- it shall land before this one
Depends on: 630052
(Assignee)

Comment 2

6 years ago
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.
Attachment #516499 - Flags: feedback?(Ms2ger)
(Reporter)

Comment 3

6 years ago
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 :)
Attachment #516499 - Flags: review?(bzbarsky)
Attachment #516499 - Flags: feedback?(Ms2ger)
Attachment #516499 - Flags: feedback+
(Assignee)

Comment 4

6 years ago
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.
Attachment #516499 - Attachment is obsolete: true
Attachment #516730 - Flags: review?(bzbarsky)
Attachment #516499 - Flags: review?(bzbarsky)
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?
Attachment #516730 - Flags: review?(bzbarsky) → review+
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-.
Attachment #516730 - Flags: review+ → review-
(Assignee)

Comment 7

6 years ago
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)
Attachment #516730 - Attachment is obsolete: true
Attachment #517063 - Flags: review?(bzbarsky)
(Reporter)

Comment 8

6 years ago
(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.
(Reporter)

Comment 9

6 years ago
(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.
(Reporter)

Comment 11

6 years ago
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....
(Assignee)

Comment 13

6 years ago
(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?
Attachment #517063 - Flags: review?(bzbarsky) → review+
Speaking of which, Ms2ger, want to just make sure this lands?  ;)
(Reporter)

Comment 16

6 years ago
Certainly. (I don't think it conflicted, actually.)
Status: NEW → ASSIGNED
Depends on: 610267
Whiteboard: [needs landing]
(Reporter)

Comment 17

6 years ago
Created attachment 520161 [details] [diff] [review]
Patch for checkin

Fixed that comment.
Attachment #517063 - Attachment is obsolete: true
Whiteboard: [needs landing] → [needs landing][not-ready-for-cedar]
(Reporter)

Updated

6 years ago
Keywords: dev-doc-needed
(Reporter)

Comment 18

6 years ago
Created attachment 523801 [details] [diff] [review]
Patch for checkin.
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
Whiteboard: [needs landing][not-ready-for-cedar]
(Reporter)

Updated

6 years ago
Attachment #520161 - Attachment is obsolete: true
http://hg.mozilla.org/projects/cedar/rev/7c9c29d52829
Flags: in-testsuite+
Whiteboard: fixed-in-cedar
http://hg.mozilla.org/mozilla-central/rev/7c9c29d52829

Yury, thank you for the patch!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
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.
Keywords: dev-doc-needed → dev-doc-complete
> Was it not actually supported yet?

That's correct.
You need to log in before you can comment on or make changes to this bug.