Skip to content

/ servo

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

Cleanup arraybufferview type #15653

Merged
merged 1 commit into from Feb 21, 2017
Merged

Cleanup arraybufferview type #15653

merged 1 commit into from Feb 21, 2017

Conversation

@samliu
Copy link
Contributor

samliu commented Feb 20, 2017 •
edited by larsbergstrom

Replace uses of spidermonkey-specific JS_GetArrayBufferViewType with ArrayBufferView impl's method get_array_type().

Tests pass:
./mach test-wpt tests/wpt/web-platform-tests/WebCrypto
./mach test-wpt tests/wpt/web-platform-tests/webgl

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15605 (github issue number if applicable).
  • There are tests for these changes

This change is Reviewable
@highfive
Copy link

highfive commented Feb 20, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @cbrewster (or someone else) soon.
@highfive
Copy link

highfive commented Feb 20, 2017

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/crypto.rs
  • @fitzgen: components/script/dom/webglrenderingcontext.rs, components/script/dom/crypto.rs
  • @emilio: components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented Feb 20, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@samliu
Copy link
Contributor Author

samliu commented Feb 20, 2017

Note: need to update the js crate (./mach cargo-update -p js) for tests to pass.

Furthermore no additional tests are necessary because no functionality changed, only the API used is different.
@cbrewster
cbrewster reviewed Feb 20, 2017
View changes
Copy link
Member

cbrewster left a comment

LGTM once nits are address, js is updated, and this is squashed
components/script/dom/webglrenderingcontext.rs Outdated
Err(_) => return Err(Error::Type("Not an ArrayBufferView".to_owned())),
};

if !self.validate_framebuffer_complete() {
return Ok(());
}

match { JS_GetArrayBufferViewType(pixels) } {
match { array_type } {

This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Feb 20, 2017

Member

Does this still need to be in { }?
components/script/dom/crypto.rs Outdated
return Err(Error::TypeMismatch);
}


This comment has been minimized.

Sign in to view
@cbrewster

cbrewster Feb 20, 2017

Member

nit: remove newline
@KiChjang
Copy link
Member

KiChjang commented Feb 20, 2017

The merge commit has to go away before we merge this.
@samliu samliu closed this Feb 21, 2017
@samliu samliu force-pushed the samliu:cleanup_arraybufferview_type branch from d8f9ed4 to ccc1df2 Feb 21, 2017
@samliu samliu reopened this Feb 21, 2017
@samliu
Copy link
Contributor Author

samliu commented Feb 21, 2017

Sorry in a botched squash I accidentally lost the edit history, but all comments addressed and merge commits are gone. PTAL
@cbrewster
Copy link
Member

cbrewster commented Feb 21, 2017

Looks good, please run ./mach cargo-update -p js so servo/rust-mozjs#340 is used.
@samliu
Copy link
Contributor Author

samliu commented Feb 21, 2017

I ran ./mach cargo-update -p js -- do I have to checkin my Cargo.lock, or was just running it enough?
@cbrewster
Copy link
Member

cbrewster commented Feb 21, 2017

You'll need to checkin the updated Cargo.lock
@samliu
Replace uses of spidermonkey-specific JS_GetArrayBufferViewType with …
Loading status checks…
4fc3e7e 
…ArrayBufferView impl's method get_array_type()
@samliu samliu force-pushed the samliu:cleanup_arraybufferview_type branch from 43b4292 to 4fc3e7e Feb 21, 2017
@samliu
Copy link
Contributor Author

samliu commented Feb 21, 2017

Done. Thanks for the help! 👍
@jdm
Copy link
Member

jdm commented Feb 21, 2017

@bors-servo: r=cbrewster
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2017

📌 Commit 4fc3e7e has been approved by cbrewster
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2017

⌛ Testing commit 4fc3e7e with merge 907c1ef...
bors-servo added a commit that referenced this pull request Feb 21, 2017
@bors-servo
Auto merge of #15653 - samliu:cleanup_arraybufferview_type, r=cbrewster
Loading status checks…
907c1ef 
Cleanup arraybufferview type

<!-- Please describe your changes on the following line: -->
Replace uses of spidermonkey-specific JS_GetArrayBufferViewType with ArrayBufferView impl's method get_array_type().

Tests pass:
./mach test-wpt tests/wpt/web-platform-tests/WebCrypto
./mach test-wpt tests/wpt/web-platform-tests/webgl

---
<!-- 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 #15605 (github issue number if applicable).
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/15653)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 21, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: cbrewster
Pushing 907c1ef to master...
@bors-servo bors-servo merged commit 4fc3e7e into servo:master Feb 21, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Feb 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrewster cbrewster

Assignees

@cbrewster cbrewster

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Replace uses of JS_GetArrayBufferViewType with high-level API
6 participants
@samliu @highfive @KiChjang @cbrewster @jdm @bors-servo
You can’t perform that action at this time.