servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Allow evaluate_script to return values #258

Closed metajack closed 8 years ago

metajack commented 8 years ago

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 commented 8 years ago

r? @nox or @jdm or @tschneidereit

jdm commented 8 years ago

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

metajack commented 8 years ago

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

metajack commented 8 years ago

Sorry @till. I meant @tschneidereit.

tschneidereit commented 8 years ago

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 commented 8 years ago

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

jdm commented 8 years ago

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

tschneidereit commented 8 years ago

@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 commented 8 years ago

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

nox commented 8 years ago

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

Ms2ger commented 8 years ago

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 commented 8 years ago

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

metajack commented 8 years ago

Rebased and dropped the last commit. Ready for rereview.

jdm commented 8 years ago

@bors-servo: r+

bors-servo commented 8 years ago

:pushpin: Commit 73028b8 has been approved by jdm

bors-servo commented 8 years ago

:hourglass: Testing commit 73028b8 with merge 8ad6c11...

bors-servo commented 8 years ago

:sunny: Test successful - travis

metajack commented 8 years ago

@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 commented 8 years ago

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