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

added servojsprincipal skeleton #353

Merged
merged 1 commit into from Apr 18, 2017
Merged

Conversation

@avadacatavra
Copy link
Contributor

avadacatavra commented Apr 18, 2017

Cross origin wrappers (servo/servo#16501) require JSPrincipals. This PR adds the skeleton and basic functionality required.

CC: @jdm


This change is Reviewable

@@ -332,6 +332,30 @@ class WrapperProxyHandler : public js::Wrapper
}
};

//http://searchfox.org/mozilla-central/source/js/public/Principals.h#24

This comment has been minimized.

@jdm

jdm Apr 18, 2017

Member

I don't think this link adds much.

{
const void* origin; //box with origin in it
public:
ServoJSPrincipal(const void* origin)

This comment has been minimized.

@jdm

jdm Apr 18, 2017

Member

We will need to accept two function pointers as arguments here - one for destroy (that accepts a *mut JSPrincipals argument), and one for write. This will allow Rust code to provide callbacks that those C++ methods can invoke.

}

const void*
getPrincipalOrigin(JSPrincipals* principal) {

This comment has been minimized.

@jdm

jdm Apr 18, 2017

Member

Let's capitalize the first letter to match.

@@ -407,6 +431,16 @@ class ForwardingProxyHandler : public js::BaseProxyHandler

extern "C" {

JSPrincipals *

This comment has been minimized.

@jdm

jdm Apr 18, 2017

Member

nit: remove the space before *

@avadacatavra avadacatavra force-pushed the avadacatavra:jsprincipal branch 3 times, most recently from 2e7ae3c to 3fde743 Apr 18, 2017
src/glue.rs Outdated
@@ -201,7 +202,14 @@ extern "C" {
-> *const ::libc::c_void;
pub fn CreateWrapperProxyHandler(aTraps: *const ProxyTraps)
-> *const ::libc::c_void;
pub fn CreateServoJSPrincipal(origin: *const ::libc::c_void,
destroy: Option<*const ::libc::c_void>,
write: Option<bool>)

This comment has been minimized.

@jdm

jdm Apr 18, 2017

Member

The destroy and write arguments don't match the C types. See the ones in ProxyTypes for good models.

This comment has been minimized.

@avadacatavra

avadacatavra Apr 18, 2017

Author Contributor

i think i fixed this

@asajeffrey
Copy link
Member

asajeffrey commented Apr 18, 2017

Annoying bikeshedding... is rust-mozjs intended to be servo-specific, or general rust bindings to SM for anyone to play with? If the latter, should ServoJSPrincipal have a less servo-specific name?

@jdm
Copy link
Member

jdm commented Apr 18, 2017

RustJSPrincipal would be fine.

@jdm
Copy link
Member

jdm commented Apr 18, 2017

Squash and r=me.

added null checks to write/destroy jsprincipal functions

fixed function callbacks and changed name to be less Servo specific
@avadacatavra avadacatavra force-pushed the avadacatavra:jsprincipal branch from c57fa7c to dea774c Apr 18, 2017
@avadacatavra
Copy link
Contributor Author

avadacatavra commented Apr 18, 2017

r=@jdm

@emilio
Copy link
Member

emilio commented Apr 18, 2017

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

📌 Commit dea774c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

Testing commit dea774c with merge 3d8c269...

bors-servo added a commit that referenced this pull request Apr 18, 2017
added servojsprincipal skeleton

Cross origin wrappers (servo/servo#16501) require JSPrincipals. This PR adds the skeleton and basic functionality required.

CC: @jdm

<!-- 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/353)
<!-- Reviewable:end -->
@emilio emilio dismissed jdm’s stale review Apr 18, 2017

Josh approved it.

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing 3d8c269 to master...

@bors-servo bors-servo merged commit dea774c into servo:master Apr 18, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@fitzgen
Copy link
Member

fitzgen commented Apr 18, 2017

is rust-mozjs intended to be servo-specific, or general rust bindings to SM for anyone to play with?

Eventually, it should be for general SM embedding. There is a fair amount of work that needs to happen to make this a reality after the bindings land in m-c, still. A lot of the generic GC-related infrastructure is in Servo's dom component, for example.

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.