Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRewrote websocket array buffer handling to typed array API #14718
Conversation
|
@bors-servo try |
Rewrote websocket array buffer handling to typed array API <!-- Please describe your changes on the following line: -➜ Replaced existing code for handling `BinaryType::Arraybuffer` from `JS_NewArrayBuffer` to `Uint8Array::create`. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14675 (github issue number if applicable). <!-- Either: --> - [X] These changes do not require tests because they replace an existing implementation <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> I am not certain the test suite will adequately verify my implementation as I am not familiar with the architecture. It compiles and passes the current tests. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14718) <!-- Reviewable:end -->
| let len = data.len() as u32; | ||
| rooted!(in(cx) let mut array_buffer = ptr::null_mut()); | ||
| assert!(Uint8Array::create(cx, len, Some(data.as_slice()), | ||
| array_buffer.handle_mut()).is_ok()); |
This comment has been minimized.
This comment has been minimized.
| let mut is_shared = false; | ||
| let buf_data: *mut uint8_t = JS_GetArrayBufferData(buf, &mut is_shared, ptr::null()); | ||
| let buf_data: *mut uint8_t = | ||
| JS_GetUint8ArrayData(*array_buffer, &mut is_shared, ptr::null()); | ||
| assert!(!is_shared); | ||
| ptr::copy_nonoverlapping(data.as_ptr(), buf_data, len as usize); |
This comment has been minimized.
This comment has been minimized.
jdm
Dec 24, 2016
Member
All of the code to fill the buffer here is now unnecessary, since that's handled in by the create method.
| assert!(!is_shared); | ||
| ptr::copy_nonoverlapping(data.as_ptr(), buf_data, len as usize); | ||
| buf.to_jsval(cx, message.handle_mut()); | ||
| data.to_jsval(cx, message.handle_mut()); |
This comment has been minimized.
This comment has been minimized.
jdm
Dec 24, 2016
Member
This change means that we return the original string value, instead of the new array buffer. I'm actually rather concerned that this passes tests ;)
|
|
|
It turns out that our websocket arraybuffer tests are all failing because of #14761, hence why the bits in the original PR that worried me didn't cause any new test failures. |
|
I'd like to try to fix that issue first, so that we can be confident that these changes don't regress anything. |
jdm
added
S-blocked-on-external
S-awaiting-review
and removed
S-awaiting-review
S-blocked-on-external
labels
|
Nevermind, I figured out why #14761 is broken and we can't fix it yet. These changes look fine to me, in that case :) |
|
@bors-servo: r+ |
|
|
|
|
Rewrote websocket array buffer handling to typed array API <!-- Please describe your changes on the following line: -➜ Replaced existing code for handling `BinaryType::Arraybuffer` from `JS_NewArrayBuffer` to `Uint8Array::create`. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14675 (github issue number if applicable). <!-- Either: --> - [X] These changes do not require tests because they replace an existing implementation <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> I am not certain the test suite will adequately verify my implementation as I am not familiar with the architecture. It compiles and passes the current tests. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14718) <!-- Reviewable:end -->
|
|
dpyro commentedDec 24, 2016
•
edited by larsbergstrom
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsI am not certain the test suite will adequately verify my implementation as I am not familiar with the architecture. It compiles and passes the current tests.
This change is