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

ImageData.html::new_with_u8_clamped_array should accept immutable u8_clamped_array #2364

Closed
JAicewizard opened this issue Nov 20, 2020 · 3 comments · Fixed by #2400
Closed

Comments

@JAicewizard
Copy link
Contributor

Motivation

This requites piet-web to clone the entire image even when it is already in the right format, this is costly because images are big.

Proposed Solution

ImageData.html::new_with_u8_clamped_array should accept an imitable u8_clamped_array because I suspect it is not needed (I'm sure you know that better then I do).

This was originally mentioned in in #1005 by @raphlinus but that has since been closed.

@alexcrichton
Copy link
Contributor

Nah this is a known issue we have with WebIDL generation. We currently have a whitelist of methods that don't need mutable borrows and only need shared verisons, and should be fine to add this method to the whitelist!

@JAicewizard
Copy link
Contributor Author

I looked around in the repo a bit and at the immutable list in constants.rs, and it took me a while to figure it out.
The problem is that this is a constructor, and cunstructors warn't supported yes by the implementation. It wasnt hard to add support for constructors after figuring out the issue.
I will probably open up a PR later today to add support for constructors + the ImageData fix, this should probably have been split but its not a big patch.

@JAicewizard
Copy link
Contributor Author

I wanted to be extra sure that the clampedArray doesn't have to be mutable, and just playing around in JS would indicate it does need to be mutable: https://jsfiddle.net/ars6bz89/27/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants