Implement ImageData constructor

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-complete})

unspecified
mozilla29
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
See bug 959925 for the use case. .createImageData is unavailable from workers.
This isn't in the spec, though.
(Assignee)

Comment 2

5 years ago
Actually it is.
http://www.w3.org/html/wg/drafts/2dcontext/html5_canvas/#hitregionoptions
> [Constructor (Uint8ClampedArray data, unsigned long width, unsigned long height)]   
> interface ImageData {
>   readonly attribute unsigned long width;
>   readonly attribute unsigned long height;
>   readonly attribute Uint8ClampedArray data;
> };
(Assignee)

Comment 4

5 years ago
OTOH, WHATWG's ImageData has resolution but W3C's one doesn't.
I don't understand why W3C bugzilla 21045 and 21046 were closed.
I think we should do this, fwiw.

Comment 6

5 years ago
(In reply to comment #5)
> I think we should do this, fwiw.

Yeah, and fix the whatwg spec to add a ctor there too!
(Assignee)

Comment 7

5 years ago
The problem is, the WHATWG spec has an attribute what the W3C spec does not.
How the constructor should be?
[Constructor (Uint8ClampedArray data, unsigned long width, unsigned long height, optional double resolution)]?
[Constructor (ImageDataInit initDict)]?
Or whatever else?

Updated

5 years ago
Keywords: dev-doc-needed
resolution's going away; the constructor as specced on the w3c side is probably ok.
Actually the W3C one's arguments don't make much sense... I'll spec something on the WHATWG side.
Done. There's two constructors:

   new ImageData(w, h); // returns a blank ImageData of the given dimensions

   new ImageData(data, w[, h]); // returns a new ImageData that references the given data, using the given width

See: http://www.whatwg.org/html#dom-imagedata
No longer blocks: 959925
(Assignee)

Comment 11

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=953f0204d579
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(Assignee)

Comment 13

5 years ago
ping?
Comment on attachment 8362161 [details] [diff] [review]
Implement ImageData constructor

>+  if (aWidth <= 0 || aHeight <= 0) {

They're unsigned, so just == 0, please.

>+  if (length <= 0 || length & 3) {

Likewise.  Also, "% 4" would be clearer than "& 3" here, I think.

>+  length >>= 2;

length /= 4, please.  The compiler will strength-reduce as needed, and the code is clearer.

>+  if (aWidth <= 0) {

Again, just ==.  Also, please file a spec issue; the spec doesn't do this check, but should.

r=me with that
Attachment #8362161 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 15

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #14)
> >+  if (aWidth <= 0) {
> 
> Again, just ==.  Also, please file a spec issue; the spec doesn't do this
> check, but should.

It is covered by the text "If length is not an integral multiple of sw," in the spec. If sw is zero, length will never be an integral multiple of sw. Our explicit zero check is just an implementation detail.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a801fc05819
Flags: in-testsuite+
> It is covered by the text "If length is not an integral multiple of sw," in the spec.

I think that assumes an unwarranted level of mathematical sophistication on the part of spec readers.  The spec should just have an explicit check for 0 here before that test.
(Assignee)

Comment 17

5 years ago
Hm, indeed I forgot the check until the test crashed despite that programmers must be careful about the zero division. Filed <https://www.w3.org/Bugs/Public/show_bug.cgi?id=24429>.
https://hg.mozilla.org/mozilla-central/rev/4a801fc05819
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Comment 19

5 years ago
This API was implemented incorrectly.
The spec says that imagedata should keep a reference to the data but it seems to create a copy.
Flags: needinfo?(bzbarsky)

Comment 20

5 years ago
(In reply to Hixie (not reading bugmail) from comment #9)
> Actually the W3C one's arguments don't make much sense... I'll spec
> something on the WHATWG side.

The W3C level 2 version should just be a copy of the WhatWG. Not sure what was there before...
(Assignee)

Comment 21

5 years ago
(In reply to Rik Cabanier from comment #19)
> This API was implemented incorrectly.
> The spec says that imagedata should keep a reference to the data but it
> seems to create a copy.

Our implementation doesn't create a copy.
 < var a=new Uint8ClampedArray([1,2,3,4]);var i=new ImageData(a,1);i.data.set([5,6,7,8]);a;
 > Uint8ClampedArray [ 5, 6, 7, 8 ]
I even added an automated test to verify that. Why do you think it creates a copy?

(In reply to Rik Cabanier from comment #20)
> (In reply to Hixie (not reading bugmail) from comment #9)
> > Actually the W3C one's arguments don't make much sense... I'll spec
> > something on the WHATWG side.
> 
> The W3C level 2 version should just be a copy of the WhatWG. Not sure what
> was there before...

Because I referred a wrong version on the W3C site.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24298#c6
I agree with Masatoshi Kimura.  The code (which is _really_ simple), and basic tests, all indicate the constructor takes a reference.  Why do you think it creates a copy?
Flags: needinfo?(bzbarsky) → needinfo?(cabanier)

Comment 23

5 years ago
I looked at the wrong constructor.
Sorry about that!
Flags: needinfo?(cabanier)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.