1347268 - Change Canvas2D {get,put,create}ImageData() arguments from double to long

Status: NEW

(Core :: Graphics: Canvas2D, enhancement, P3)

Description

[Fernando Serboncini]

This is a heads up that we are considering changing the spec of getImageData() to improve interoperability.

The gist of it is we are changing the IDL signature of the function from double to long, removing edge cases (dimensions in (0,1) range) and adding web platform tests.

This is the relevant issue: https://github.com/whatwg/html/issues/2433

I'll update this with the spec and web-platform change when appropriate.

Comment 1

[Anne (:annevk)]

Hey Jet, would be good if someone looked at this sooner rather than later. We're about to change the standard here.

Comment 2

[Fernando Serboncini]

Just FYI, the spec and web-platform changes are in:

https://github.com/whatwg/html/commit/362c9315971af63dbba49ef171644312c721444f
https://github.com/w3c/web-platform-tests/commit/28c13b6ab36f44faf4612d017666ab95aca5dc1b

Comment 3

[Jet Villegas (inactive)]

+Milan on this one. We'll also want to coordinate with the other vendors on shipping this change.

Comment 4

[Boris Zbarsky [:bzbarsky]]

On the IDL level, there's no problem with making this change.  The question is whether the resulting change in API behavior is one we're OK with, and the right place to sort that out is in the spec, right?

I assume that's what the needinfo was for: "can someone familiar with the canvas API weigh in on the spec issue with Gecko's pov?"  I'm not sure who on our side owns this stuff.  Trying Jeff, because if he doesn't, I bet he knows who does.

Comment 5

[Anne (:annevk)]

Yeah, we ended up changing the standard already (as well as corresponding tests, per comment 2) since it all seemed reasonable, but if there are any issues that would be good to know of course.

Comment 6

[Fernando Serboncini]

I know it may not count, but I did check the Gecko Canvas2D code before stating this. ;)

Basically, all higher level functions on CanvasRenderingContext2D already move from float -> long using ToInt32 manually. The only real change there would be to remove the "if (sw == 0) sw = 1" code. IMO, the Gecko changes are even simpler than the Blink ones (which were already very simple).

But feel free to take a closer look. :)))

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #4)
> On the IDL level, there's no problem with making this change.

That's the reason for the needinfo, so thanks!

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, but I'd still like someone familiar with the canvas code to take a look at this _before_ everyone else ships the change and we're locked into it whether we want it or not.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Sure thing.  Not a lot of active people in canvas, but Markus is probably a good choice as well.  I'm familiar with that front end part, and outside of making sure inf/NaN is properly handled, we should be OK with this change.  Module reviewing the actual patch :)

Comment 10

[Boris Zbarsky [:bzbarsky]]

inf/NaN would become 0, per the current spec bits, iirc.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Or something like that; currently inf/NaN throw "not supported" and 0 throws "index size".

Comment 12

[Jeff Muizelaar [:jrmuizel]]

I have no objection to doing this provided all of the other vendors do it to.

Comment 13

[Fernando Serboncini]

FYI, Chrome already follows the updated spec.

Comment 14

[Josh Matthews [:jdm]]

It appears as though other vendors did not update their IDL, given the results of https://github.com/whatwg/html/issues/5336. Every browser still throws a TypeError when passed inf values.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Per Riot conversation: Blink did, but then added [EnforceRange] stuff that's not in the spec but made some wpts pass...