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

Support the updated spidermonkey bindings #7727

Merged
merged 2 commits into from Oct 14, 2015
Merged

Support the updated spidermonkey bindings #7727

merged 2 commits into from Oct 14, 2015

Conversation

@michaelwu
Copy link
Contributor

michaelwu commented Sep 24, 2015

Still need to finish the rust-mozjs update and make cargo use it, but it's close enough that I don't expect much to change on the servo side.

Some changes here

  • bools are properly translated now
  • char16_t is handled as u16 now
  • JS_GlobalObjectTraceHook isn't mangled now
  • JSJitInfo has been adjusted
  • A const fn is used to generate bitfields in JSJitInfo
  • Manually generating handles now requires calling an unsafe function. It's not actually required, but it's too much of a hassle to generate them manually now due to bindgen++ adding base classes now.

Review on Reviewable
@highfive
Copy link

highfive commented Sep 24, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@Manishearth
Copy link
Member

Manishearth commented Sep 24, 2015

Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

components/script/dom/bindings/codegen/CodegenRust.py, line 5623 [r1] (raw file):
if !ok ?

components/script/dom/bindings/utils.rs, line 646 [r1] (raw file):
Why not 6?

Comments from the review on Reviewable.io
@michaelwu
Copy link
Contributor Author

michaelwu commented Sep 24, 2015

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

components/script/dom/bindings/codegen/CodegenRust.py, line 5623 [r1] (raw file):
Oh, good catch. I was looking for that kind of mistake too..

components/script/dom/bindings/utils.rs, line 646 [r1] (raw file):
This is the latest you can specify. The JSVERSION_ECMA_6 enum doesn't exist yet. Right now, JSVERSION_LATEST == JSVERSION_ECMA_5, but rust won't let us define enum variants with identical values. I think I kept JSVERSION_LATEST last time, but I'm keeping JSVERSION_ECMA_5 this time. (IIRC because it's easier to do)

Comments from the review on Reviewable.io
@michaelwu michaelwu force-pushed the michaelwu:update-bindings branch from 4e04de5 to 9da167e Sep 24, 2015
@Manishearth
Copy link
Member

Manishearth commented Sep 25, 2015

Reviewed 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@Manishearth
Copy link
Member

Manishearth commented Sep 25, 2015

r+ from me, unless @Ms2ger wants to have a look first.
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 25, 2015

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2015

📌 Commit 9da167e has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Sep 25, 2015

⌛ Testing commit 9da167e with merge ecfc0d5...
bors-servo pushed a commit that referenced this pull request Sep 25, 2015
bors-servo
Auto merge of #7727 - michaelwu:update-bindings, r=Manishearth
ecfc0d5 
Support the updated spidermonkey bindings

Still need to finish the rust-mozjs update and make cargo use it, but it's close enough that I don't expect much to change on the servo side.

Some changes here
- bools are properly translated now
- char16_t is handled as u16 now
- JS_GlobalObjectTraceHook isn't mangled now
- JSJitInfo has been adjusted
- A const fn is used to generate bitfields in JSJitInfo
- Manually generating handles now requires calling an unsafe function. It's not actually required, but it's too much of a hassle to generate them manually now due to bindgen++ adding base classes now.

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

bors-servo commented Sep 25, 2015

💔 Test failed - mac-dev-ref-unit
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 25, 2015

Oh, duh.
@michaelwu
Copy link
Contributor Author

michaelwu commented Sep 25, 2015

For reference, the bindings update is in servo/rust-mozjs#198
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2015

☔ The latest upstream changes (presumably #7766) made this pull request unmergeable. Please resolve the merge conflicts.
@michaelwu michaelwu force-pushed the michaelwu:update-bindings branch from 9da167e to 7ad5e75 Sep 29, 2015
@Manishearth
Copy link
Member

Manishearth commented Sep 30, 2015

Reviewed 4 of 4 files at r3.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@Manishearth
Copy link
Member

Manishearth commented Sep 30, 2015

@bors-servo r+

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io
@bors-servo
Copy link
Contributor

bors-servo commented Sep 30, 2015

📌 Commit 7ad5e75 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Oct 12, 2015

☔ The latest upstream changes (presumably #7973) made this pull request unmergeable. Please resolve the merge conflicts.
@michaelwu michaelwu force-pushed the michaelwu:update-bindings branch from 4857a6a to ad0d6f6 Oct 12, 2015
@michaelwu
Copy link
Contributor Author

michaelwu commented Oct 12, 2015

Had to mark some previously passing tests as failing, since they were only accidentally passing. A ByteString conversion check was broken before. Now those tests fail in the same way Gecko fails them.
@jdm jdm removed the S-needs-rebase label Oct 12, 2015
michaelwu added 2 commits Sep 23, 2015
@michaelwu
Support the updated spidermonkey bindings
e733a7c
@michaelwu
Throw on bad ByteStrings
Loading status checks…
3129fb2
@michaelwu michaelwu force-pushed the michaelwu:update-bindings branch from ad0d6f6 to 3129fb2 Oct 14, 2015
@jdm
Copy link
Member

jdm commented Oct 14, 2015

@bors-servo: r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

📌 Commit 3129fb2 has been approved by jdm
@jdm
Copy link
Member

jdm commented Oct 14, 2015

Filed web-platform-tests/wpt#2256 about the incorrect tests.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

⌛ Testing commit 3129fb2 with merge 0a2b86a...
bors-servo pushed a commit that referenced this pull request Oct 14, 2015
bors-servo
Auto merge of #7727 - michaelwu:update-bindings, r=jdm
0a2b86a 
Support the updated spidermonkey bindings

Still need to finish the rust-mozjs update and make cargo use it, but it's close enough that I don't expect much to change on the servo side.

Some changes here
- bools are properly translated now
- char16_t is handled as u16 now
- JS_GlobalObjectTraceHook isn't mangled now
- JSJitInfo has been adjusted
- A const fn is used to generate bitfields in JSJitInfo
- Manually generating handles now requires calling an unsafe function. It's not actually required, but it's too much of a hassle to generate them manually now due to bindgen++ adding base classes now.

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

bors-servo commented Oct 14, 2015

💔 Test failed - mac-dev-ref-unit
@jdm
Copy link
Member

jdm commented Oct 14, 2015

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 14, 2015

⌛ Testing commit 3129fb2 with merge b34fd5b...
bors-servo pushed a commit that referenced this pull request Oct 14, 2015
bors-servo
Auto merge of #7727 - michaelwu:update-bindings, r=jdm
Loading status checks…
b34fd5b 
Support the updated spidermonkey bindings

Still need to finish the rust-mozjs update and make cargo use it, but it's close enough that I don't expect much to change on the servo side.

Some changes here
- bools are properly translated now
- char16_t is handled as u16 now
- JS_GlobalObjectTraceHook isn't mangled now
- JSJitInfo has been adjusted
- A const fn is used to generate bitfields in JSJitInfo
- Manually generating handles now requires calling an unsafe function. It's not actually required, but it's too much of a hassle to generate them manually now due to bindgen++ adding base classes now.

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

bors-servo commented Oct 14, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 3129fb2 into servo:master Oct 14, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Oct 14, 2015
Merged
@michaelwu michaelwu deleted the michaelwu:update-bindings branch Oct 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@michaelwu @highfive @Manishearth @Ms2ger @bors-servo @jdm @tschneidereit @mbrubeck
You can’t perform that action at this time.