This patch makes us pass http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create2.negative.html http://www.w3c-test.org/html/tests/approved/canvas/2d.imageData.create2.nonfinite.html I'm not entirely happy about NonFinite(), but I couldn't find something better.

Comment on attachment 508263 [details] [diff] [review] Patch v1 Yeah, the nonfinite part is bogus. In particular, if v is an object whose valueOf returns Infinity, this code will do the wrong thing. Worth adding a test for this (to our tests and the official test suite). What you probably want to do instead if use JS_ValueToNumber and then use the DoubleIsFinite helper in nsCanvasRenderingContext.cpp (which therefore needs to move elsewhere). Then of course you have to worry about how to convert the doubles into integers. It looks like the spec doesn't actually define the behavior here, right?

This code duplication was poking my eye out. (Also, now I can fit nsIDOMCanvasRenderingContext2D_CreateImageData on my screen.)

Comment on attachment 508843 [details] [diff] [review] Preparatory patch >+++ b/content/canvas/src/CustomQS_Canvas2D.h >+ // create the fast typed array Keep the comment about it being 0-filled by default? >+ nsresult rv = self->GetImageData_explicit(x, y, w, h, >+ static_cast<PRUint8*>(tdest->data), I'd prefer line-breaking after the '=' and lining up the arguments properly. r=me with those nits. Thanks for reducing the code duplication!

This should look a bit better. The test will need some more updates.

That's missing the changes to nsMathUtils.h. The new code is not equivalent to the old for various finite doubles. For example, consider width == 2^32 - 1. Compare these two programs: JS: var x = (1 << 16); x = x * x - 1; print(x | 0); C++: double x = (1 << 16); x = x * x - 1; printf("%d\n", (int)x); The former prints -1. The latter prints -2147483648. You can get the behavior of the former, maybe, if you cast through PRUint32 first... but you'd need to check, the spec. Though again, the behavior here is not exactly defined; what's the desired behavior?

This patch moves the DoubleIsFinite code. Who should I ask to review the code in nsMathUtils?

Comment on attachment 509746 [details] [diff] [review] MathUtils code v1 I can review this. Still need to figure out how the double-to-int conversion should work...

I think this is going to be better for my (limited) sanity.

Comment on attachment 510319 [details] [diff] [review] Patch v3 >+ if (width < 0) { >+ w = -wi; >+ x -= w; >+ } else { >+ w = wi; >+ } Maybe add a comment like "Handle negative width and height by flipping the rectangle over in the relevant direction" before this code or some such? r=me

Comment on attachment 508843 [details] [diff] [review] Preparatory patch >+ nsresult rv = self->GetImageData_explicit(x, y, w, h, >+ static_cast<PRUint8*>(tdest->data), The static CreateImageData has no arguments or variables with x and y names. Also, there is no "CheckedInt.h" include. Comment on attachment 510319 [details] [diff] [review] Patch v3 It looks like JS_ValueToECMAInt32 better way to validate the arguments than JS_ValueToNumber/JS_DoubleToInt32 combo (and the compiler fails on JS_DoubleToInt32).

> It looks like JS_ValueToECMAInt32 better way to validate the arguments No, it's not. It'll return 0 silently if passed a NaN, whereas the API says it needs to throw. > and the compiler fails on JS_DoubleToInt32 That's because you didn't apply the patch for bug 631132, which this one is clearly marked as depending on....

Yeah, I noticed that later :) Thanks

I've got that fixed locally. I'll try to upload tonight.

And adding a dependency for the test I missed.

http://hg.mozilla.org/projects/cedar/rev/8e8e6f2f737d http://hg.mozilla.org/projects/cedar/rev/2676052e13a8 http://hg.mozilla.org/projects/cedar/rev/c51360e5cc59

http://hg.mozilla.org/mozilla-central/rev/8e8e6f2f737d http://hg.mozilla.org/mozilla-central/rev/2676052e13a8 http://hg.mozilla.org/mozilla-central/rev/c51360e5cc59