Handle non-finite and negative arguments to createImageData according to spec

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

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

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 9 obsolete attachments)

2.78 KB, patch
Details | Diff | Splinter Review
7.51 KB, patch
Details | Diff | Splinter Review
26.46 KB, patch
Details | Diff | Splinter Review
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Flags: in-testsuite+
Attachment #508263 - Flags: review?(bzbarsky)
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?
Attachment #508263 - Flags: review?(bzbarsky) → review-
Posted patch Preparatory patch (obsolete) — Splinter Review
This code duplication was poking my eye out.

(Also, now I can fit nsIDOMCanvasRenderingContext2D_CreateImageData on my screen.)
Attachment #508843 - Flags: review?(bzbarsky)
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!
Attachment #508843 - Flags: review?(bzbarsky) → review+
Posted patch Patch v2 (obsolete) — Splinter Review
This should look a bit better. The test will need some more updates.
Attachment #508263 - Attachment is obsolete: true
Attachment #509199 - Flags: review?(bzbarsky)
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?
Posted patch MathUtils code v1 (obsolete) — Splinter Review
This patch moves the DoubleIsFinite code. Who should I ask to review the code in nsMathUtils?
Attachment #509746 - Flags: review?(bzbarsky)
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...
Attachment #509746 - Flags: review?(bzbarsky) → review+
Blocks: 629894
Posted patch Patch v3 (obsolete) — Splinter Review
I think this is going to be better for my (limited) sanity.
Attachment #509199 - Attachment is obsolete: true
Attachment #510319 - Flags: review?(bzbarsky)
Attachment #509199 - Flags: review?(bzbarsky)
Depends on: 631132
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
Attachment #510319 - Flags: review?(bzbarsky) → review+
Depends on: post2.0
Whiteboard: [needs review] → [needs landing]
Blocks: 630040
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.
Attachment #509746 - Attachment is obsolete: true
Attachment #508843 - Attachment is obsolete: true
Attachment #516764 - Attachment is obsolete: true
Attachment #516764 - Flags: feedback?(Ms2ger)
And adding a dependency for the test I missed.
Depends on: 494744
Whiteboard: [needs landing] → [needs landing][not-ready-for-cedar]
Attachment #516887 - Attachment is obsolete: true
Attachment #516888 - Attachment is obsolete: true
Attachment #516889 - Attachment is obsolete: true
Whiteboard: [needs landing][not-ready-for-cedar] → [needs landing]
Keywords: checkin-needed
Whiteboard: [needs landing]
Depends on: 654655
You need to log in before you can comment on or make changes to this bug.