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

Allow evaluate_script to return values #258

Merged
merged 2 commits into from Apr 28, 2016
Merged

Conversation

@metajack
Copy link
Contributor

metajack commented Apr 22, 2016

This includes some other minor cleanups. This is a port of @tschneidereit's commits from this branch:
https://github.com/tschneidereit/rust-mozjs/tree/update-bindings

This is need to get https://github.com/tschneidereit/mozjs-shell working.


This change is Reviewable

@metajack
Copy link
Contributor Author

metajack commented Apr 22, 2016

@jdm
Copy link
Member

jdm commented Apr 22, 2016

The last commit will require nontrivial changes in Servo, I think. I'd rather have it separate from the rest of these changes.

@metajack
Copy link
Contributor Author

metajack commented Apr 22, 2016

I'm not actually sure why it is need here. @till? I'm happy to remove it from this PR.

@metajack
Copy link
Contributor Author

metajack commented Apr 22, 2016

Sorry @till. I meant @tschneidereit.

@tschneidereit
Copy link
Contributor

tschneidereit commented Apr 22, 2016

The last commit will require nontrivial changes in Servo, I think. I'd rather have it separate from the rest of these changes.

It's not required. We should do the required changes to Servo anyway because long-term contexts will go away (and their uses be replaced by JSRuntime), and because there's no upside to using multiple contexts in Servo. But there's no need to do this now.

@tschneidereit
Copy link
Contributor

tschneidereit commented Apr 22, 2016

I think only the first commit is needed. The second one makes sense, though, so we should keep it.

@jdm
Copy link
Member

jdm commented Apr 22, 2016

@tschneidereit We only use one context. The commit in question is about requests.

@tschneidereit
Copy link
Contributor

tschneidereit commented Apr 22, 2016

@tschneidereit We only use one context. The commit in question is about requests.

Hrm, sorry - it's been a while since I looked at all this and I misremembered stuff. What I really meant was that, AFAICT, there's no need to ever enter more than one request - or leave that request. We don't call js::SetActivityCallback, and I'm pretty sure that's the only thing triggered by rt->requestDepth being set to or changed from 0.

Then again, it doesn't matter much at all whether we do or do not do this, so we can just ignore it.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Apr 23, 2016

We should call into jsapi with null for perf reasons, iirc; this PR doesn't change that, though.

@nox
Copy link
Member

nox commented Apr 25, 2016

Can we mark evaluate_script as unsafe? Especially if it doesn't use JSAutoRequest even though it crashes without it.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Apr 25, 2016

Not entirely relatedly: maybe we should make evaluate_script a free function that takes *mut JSContext? The implementation shouldn't need a Runtime, and it can be annoying to get your hands on one.

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2016

The latest upstream changes (presumably #260) made this pull request unmergeable. Please resolve the merge conflicts.

@metajack metajack force-pushed the metajack:eval-return-val branch from 8526a44 to b5d22b5 Apr 28, 2016
@metajack metajack force-pushed the metajack:eval-return-val branch from b5d22b5 to 73028b8 Apr 28, 2016
@metajack
Copy link
Contributor Author

metajack commented Apr 28, 2016

Rebased and dropped the last commit. Ready for rereview.

@jdm
Copy link
Member

jdm commented Apr 28, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2016

📌 Commit 73028b8 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2016

Testing commit 73028b8 with merge 8ad6c11...

bors-servo added a commit that referenced this pull request Apr 28, 2016
Allow evaluate_script to return values

This includes some other minor cleanups. This is a port of @tschneidereit's commits from this branch:
https://github.com/tschneidereit/rust-mozjs/tree/update-bindings

This is need to get https://github.com/tschneidereit/mozjs-shell  working.

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

bors-servo commented Apr 28, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 73028b8 into servo:master Apr 28, 2016
1 check passed
1 check passed
homu Test successful
Details
@metajack
Copy link
Contributor Author

metajack commented May 9, 2016

@nox did you include this in your smup somehow? This hasn't landed but now mozjs-shell is working as expected, so I'm totally confused.

@metajack
Copy link
Contributor Author

metajack commented May 9, 2016

oh, nm. github hadn't updated the page, and I was looking at stale data.

@metajack metajack deleted the metajack:eval-return-val branch May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.