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

Improve typed array support. #304

Merged
merged 3 commits into from Sep 22, 2016

Conversation

Projects
None yet
3 participants
@Ms2ger
Collaborator

Ms2ger commented Sep 13, 2016

This change is Reviewable

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 13, 2016

Collaborator

r? @jdm

Collaborator

Ms2ger commented Sep 13, 2016

r? @jdm

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 13, 2016

Member

I am super pumped to look at this later today!

Member

jdm commented Sep 13, 2016

I am super pumped to look at this later today!

@jdm

I would like to see some tests added for these new APIs.

Show outdated Hide outdated src/typedarray.rs
/// Create a typed array representation that wraps an existing JS reflector.
/// This operation will fail if attempted on a JS object that does not match
/// the expected typed array details.
pub fn new(cx: *mut JSContext, root: &'a mut Rooted<*mut JSObject>) -> Result<Self, ()> {

This comment has been minimized.

@jdm

jdm Sep 16, 2016

Member

I'm inclined to call this from or something similar; it's easy to confuse it with create in my mind.

@jdm

jdm Sep 16, 2016

Member

I'm inclined to call this from or something similar; it's easy to confuse it with create in my mind.

unsafe fn get_data(obj: *mut JSObject) -> *mut Self::Element;
}
macro_rules! typed_array_element {

This comment has been minimized.

@jdm

jdm Sep 16, 2016

Member

I propose one macro branch that accepts $t, $element, $unwrap, and $length_and_data, which declares the struct and implements TypedArrayElement, and another branch that accepts all the existing arguments, invokes the previous branch, and implements TypedArrayElementCreator.

@jdm

jdm Sep 16, 2016

Member

I propose one macro branch that accepts $t, $element, $unwrap, and $length_and_data, which declares the struct and implements TypedArrayElement, and another branch that accepts all the existing arguments, invokes the previous branch, and implements TypedArrayElementCreator.

Show outdated Hide outdated src/typedarray.rs
/// The ArrayBufferView type.
pub struct ArrayBufferViewU8;
impl TypedArrayElement for ArrayBufferViewU8 {

This comment has been minimized.

@jdm

jdm Sep 16, 2016

Member

We shouldn't need this if we modify the typed_array_element macro as suggested above.

@jdm

jdm Sep 16, 2016

Member

We shouldn't need this if we modify the typed_array_element macro as suggested above.

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 16, 2016

Member

As for the neutering stuff, I've spent a while looking at Gecko's implementation and thinking about whether we can do anything better, and I haven't come up with any good ideas. APIs like TypedArray::with_slice could accept a closure, but that wouldn't prevent people from running JS that could invalidate the slice inside of the closure, so it would still be unsafe and less pleasant to use.

Member

jdm commented Sep 16, 2016

As for the neutering stuff, I've spent a while looking at Gecko's implementation and thinking about whether we can do anything better, and I haven't come up with any good ideas. APIs like TypedArray::with_slice could accept a closure, but that wouldn't prevent people from running JS that could invalidate the slice inside of the closure, so it would still be unsafe and less pleasant to use.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 20, 2016

Collaborator

Still needs tests.

Collaborator

Ms2ger commented Sep 20, 2016

Still needs tests.

@Ms2ger

This comment has been minimized.

Show comment
Hide comment
@Ms2ger

Ms2ger Sep 22, 2016

Collaborator

Tests done, r? @jdm

Collaborator

Ms2ger commented Sep 22, 2016

Tests done, r? @jdm

@jdm

This comment has been minimized.

Show comment
Hide comment
@jdm

jdm Sep 22, 2016

Member

@bors-servo: r+
Thanks!

Member

jdm commented Sep 22, 2016

@bors-servo: r+
Thanks!

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

📌 Commit da0665d has been approved by jdm

Contributor

bors-servo commented Sep 22, 2016

📌 Commit da0665d has been approved by jdm

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

⌛️ Testing commit da0665d with merge fc380e0...

Contributor

bors-servo commented Sep 22, 2016

⌛️ Testing commit da0665d with merge fc380e0...

bors-servo added a commit that referenced this pull request Sep 22, 2016

Auto merge of #304 - servo:typedarray, r=jdm
Improve typed array support.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/304)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 22, 2016

Contributor

☀️ Test successful - status-appveyor, status-travis

Contributor

bors-servo commented Sep 22, 2016

☀️ Test successful - status-appveyor, status-travis

@bors-servo bors-servo merged commit da0665d into master Sep 22, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
homu Test successful
Details

@Ms2ger Ms2ger deleted the typedarray branch Sep 23, 2016

@jdm jdm referenced this pull request Oct 14, 2016

Merged

Implemented FileReader::readAsArrayBuffer #13729

7 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment