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 FromJSValConvertible for *mut JSObject #321

Closed
wants to merge 1 commit into from

Conversation

@KiChjang
Copy link
Member

KiChjang commented Dec 12, 2016

r? @Ms2ger or @nox


This change is Reviewable

@Ms2ger
Copy link
Collaborator

Ms2ger commented Dec 12, 2016

Doesn't compile

@KiChjang
Copy link
Member Author

KiChjang commented Dec 12, 2016

Uhh right, stupid me.

@KiChjang KiChjang force-pushed the KiChjang:convert-from-object branch 3 times, most recently from 541b7e2 to 24e1a18 Dec 12, 2016
@Ms2ger
Copy link
Collaborator

Ms2ger commented Dec 13, 2016

These implementations crash if handed the wrong type of value. Is that intentional?

@KiChjang
Copy link
Member Author

KiChjang commented Dec 17, 2016

I essentially copied the implementation from JSVal. Shall I instead check that it is an object first, and if not, return an Err(()) or a ConversionResult::Failure?

@Ms2ger
Copy link
Collaborator

Ms2ger commented Dec 19, 2016

Let's take a step back. Why did you make this PR?

@KiChjang
Copy link
Member Author

KiChjang commented Dec 19, 2016

MessagePort contains a WebIDL method that looks like this:

  void postMessage(any message, optional sequence<object> transfer = []);

And so CodegenRust.py is trying to convert a JS HandleValue to a JS object, which currently doesn't exist in rust-mozjs bindings.

@jdm
Copy link
Member

jdm commented Dec 22, 2016

Given that this is only needed because of codegen for a feature we don't support yet (transferrable objects), I propose we use optional any transfer and close this PR.

@jdm jdm closed this Dec 22, 2016
@KiChjang KiChjang deleted the KiChjang:convert-from-object branch Dec 22, 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

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