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

Implement maybe_wrap_object_value without OOL calls #301

Merged
merged 1 commit into from Nov 15, 2016

Conversation

@nox
Copy link
Member

nox commented Aug 30, 2016

The only remaining OOL calls are ToWindowIfWindowProxy and JS_WrapValue,
like in the equivalent Gecko function.


This change is Reviewable

@nox nox changed the title Implement maybe_wrap_object_value without OOL calls (Do not merge) Implement maybe_wrap_object_value without OOL calls Aug 30, 2016
@nox nox force-pushed the nox:maybe-wrap branch from 4711175 to 5fa24d6 Aug 31, 2016
@nox nox changed the title (Do not merge) Implement maybe_wrap_object_value without OOL calls Implement maybe_wrap_object_value without OOL calls Aug 31, 2016
src/rust.rs Outdated
#[inline]
pub unsafe fn maybe_wrap_value(cx: *mut JSContext, rval: MutableHandleValue) {
if rval.is_object_or_null() {
maybe_wrap_object_value(cx, rval);

This comment has been minimized.

@Ms2ger

Ms2ger Aug 31, 2016

Collaborator

maybe_wrap_object_or_null_value

@nox nox force-pushed the nox:maybe-wrap branch from 5fa24d6 to d7e2f59 Aug 31, 2016
src/rust.rs Outdated
#[inline]
unsafe fn get_object_group(obj: *mut JSObject) -> *mut ObjectGroup {
assert!(!obj.is_null());
(*(obj as *mut Object)).group

This comment has been minimized.

@Ms2ger

Ms2ger Aug 31, 2016

Collaborator

I think this would be more readable if you added a separate let binding for obj as *mut Object.

src/rust.rs Outdated

#[inline]
pub unsafe fn get_context_compartment(cx: *mut JSContext) -> *mut JSCompartment {
(*(cx as *mut ContextFriendFields)).compartment_

This comment has been minimized.

@Ms2ger

Ms2ger Aug 31, 2016

Collaborator

Ditto

@nox nox force-pushed the nox:maybe-wrap branch from d7e2f59 to 0732c26 Aug 31, 2016
@nox
Copy link
Member Author

nox commented Aug 31, 2016

Amended all the things.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Aug 31, 2016

Code looks okay to me; I'd like @jdm to agree we want to go this way, though.

(Also, some documentation might be useful.)

}

#[inline]
pub unsafe fn maybe_wrap_value(cx: *mut JSContext, rval: MutableHandleValue) {

This comment has been minimized.

@jdm

jdm Sep 14, 2016

Member

This is different than Gecko's, which special-cases strings and objects but otherwise avoids wrapping: https://dxr.mozilla.org/mozilla-central/rev/82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc/dom/bindings/BindingUtils.h#852

src/rust.rs Outdated
return try_to_outerize(rval);
}
}
assert!(JS_WrapValue(cx, rval));

This comment has been minimized.

@jdm

jdm Sep 14, 2016

Member

We miss a fast path here where the compartment is the same and it's a non-WebIDL object.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 15, 2016

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

@jdm
Copy link
Member

jdm commented Nov 14, 2016

Planning to finish this up, @nox?

The only remaining OOL calls are ToWindowIfWindowProxy and JS_WrapValue,
like in the equivalent Gecko function.
@nox nox force-pushed the nox:maybe-wrap branch from 0732c26 to f32c5ac Nov 15, 2016
@jdm
Copy link
Member

jdm commented Nov 15, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 15, 2016

📌 Commit f32c5ac has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Nov 15, 2016

Testing commit f32c5ac with merge 3ac1492...

bors-servo added a commit that referenced this pull request Nov 15, 2016
Implement maybe_wrap_object_value without OOL calls

The only remaining OOL calls are ToWindowIfWindowProxy and JS_WrapValue,
like in the equivalent Gecko function.

<!-- Reviewable:start -->

---

This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/301)

<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 15, 2016

☀️ Test successful - status-appveyor, status-travis

@bors-servo bors-servo merged commit f32c5ac into servo:master Nov 15, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
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

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