Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix createImageData with sizes < 1 pixel #6974

Merged
merged 1 commit into from
Aug 5, 2015

Conversation

dzbarsky
Copy link
Contributor

@dzbarsky dzbarsky commented Aug 4, 2015

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 4, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Aug 4, 2015

Is there a test for this?

@dzbarsky
Copy link
Contributor Author

dzbarsky commented Aug 4, 2015

Yep, the one I deleted the metadata for :)

@jdm
Copy link
Member

jdm commented Aug 4, 2015

This seems odd to me. There's no explicit spec text for this, and Gecko doesn't appear to have a similar maximization step: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#5365

@jdm jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 4, 2015
@jdm
Copy link
Member

jdm commented Aug 4, 2015

@dzbarsky
Copy link
Contributor Author

dzbarsky commented Aug 5, 2015

Gecko does the equivalent thing here: https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#5341 (this is after calling abs).

The relevant spec text is a little lower:
"New ImageData objects must be initialised so that their width attribute is set to the number of pixels per row in the image data, their height attribute is set to the number of rows in the image data...At least one pixel's worth of image data must be returned."
So the minimum size is 1x1.

@dzbarsky
Copy link
Contributor Author

dzbarsky commented Aug 5, 2015

Unfortunately the canvas spec doesn't really list explicit steps for its algorithms, you have to infer them from various requirements. That's why we fail so many edge-case tests.

@jdm
Copy link
Member

jdm commented Aug 5, 2015

Delightful!

@jdm
Copy link
Member

jdm commented Aug 5, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 2fb5fda has been approved by jdm

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-answer Someone asked a question that requires an answer. labels Aug 5, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 2fb5fda with merge 6c4d9ad...

@bors-servo
Copy link
Contributor

💔 Test failed - linux1

@jdm
Copy link
Member

jdm commented Aug 5, 2015

@bors-servo: retry

  • infra issue

Manishearth added a commit to Manishearth/servo that referenced this pull request Aug 5, 2015
<!-- Reviewable:start -->
[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/servo/servo/6974)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

⌛ Testing commit 2fb5fda with merge debd7d8...

bors-servo pushed a commit that referenced this pull request Aug 5, 2015
Fix createImageData with sizes < 1 pixel



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6974)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 2fb5fda into servo:master Aug 5, 2015
@dzbarsky dzbarsky deleted the tiny-create branch August 5, 2015 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants