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

Feedback for servo-based app runtime #19950

Closed
wants to merge 16 commits into from
Closed

Conversation

@justinmichaud
Copy link

justinmichaud commented Feb 5, 2018

I would like to take a crack at #7379. I have built a demo application (https://github.com/justinmichaud/ion/blob/master/src/app.rs) that uses servo as a runtime, although it requires some pretty hacky things to work (see below) that obviously shouldn't be merged.
image

I would like to see an application runtime that allows me to write apps in rust, but with the full power of html/css and the dom. Ideally, I would like to have an api into servo that is expressive enough for me to be able to build a react-like framework in rust outside of servo.

I was wondering if someone with more experience could help me understand what an application runtime would look like that:

  1. Doesn't break encapsulation like this mess
  2. Might have a chance of actually getting merged

If this is something that servo would consider supporting, then I would love to do this the right way. Thanks for the help!


This change is Reviewable

@highfive
Copy link

highfive commented Feb 5, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

@highfive
Copy link

highfive commented Feb 5, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/lib.rs, components/constellation/pipeline.rs, components/script/dom/bindings/mod.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs and 5 more
  • @cbrewster: components/constellation/pipeline.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs, components/constellation/lib.rs
  • @paulrouget: components/constellation/pipeline.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs, components/servo/lib.rs, components/constellation/lib.rs
  • @fitzgen: components/script/lib.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/root.rs, components/script_traits/lib.rs, components/script/script_thread.rs and 2 more
  • @KiChjang: components/script/lib.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/root.rs, components/script_traits/lib.rs, components/script/script_thread.rs and 2 more
@highfive
Copy link

highfive commented Feb 5, 2018

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Feb 5, 2018

@paulrouget You probably have some thoughts here.

@paulrouget
Copy link
Contributor

paulrouget commented Feb 5, 2018

I don't think this is what #7379 is about. In #7379, we are talking about a JS-based runtime. Like Node+Electron. What you built is different.

That being said, this is a very interesting project. For embedding purposes, this is something we need. And I think being able to use Servo+HTML as a UI toolkit for a Rust applications makes total sense. And like you stated, it needs to:

  1. not break encapsulation
  2. have a chance of actually getting merged

Maybe a better approach would be to expose the webdriver interface to the embedder. We do have a partial implementation of webdriver, so that should not be too hard. I don't know how far we can manipulate the document via webdriver, but it's possible to execute scripts.

If webdriver is not good enough, and full DOM access is needed, then a better approach might be to add a new method to libservo to execute a callback for a specific document. A document has an id (pipeline_id) but these are not accessible from the embedder (yet). So for now, I would recommend using the top level browsing context id (browser_id in the embedder code). So this would look like servo.run_script(browser_id, callback).

But I'd rather leverage the webdriver interface as it goes beyond the DOM API.

@justinmichaud
Copy link
Author

justinmichaud commented Feb 6, 2018

Hey @paulrouget, thanks for the fast response!

I guess the optimal way to solve this would be to make a JS runtime, and then replace spidermonkey with something that executes pure rust (although that sounds very hard).

For your suggestion, it seems to me that webdriver is read-only. I think that the servo.run_script approach sounds interesting (or perhaps a webdriver run message), and I am trying to implement it now.

The final problem is how to eliminate the coupling between the callback and script. Are there any existing traits that represent the stable dom interface provided to JS, that I could perhaps move to script_traits? If not, is there some kind of idiomatic way that I could create a "writable" version of the webdriver dom api, without duplicating a bunch of code?

Thanks again!

@justinmichaud
Copy link
Author

justinmichaud commented Feb 8, 2018

I tried adding a message that receives a callback. It worked, although I had to mem::transmute the pointer to make it serializable so it could reach the script process. Do you know of a safer way?

@paulrouget
Copy link
Contributor

paulrouget commented Feb 8, 2018

@justinmichaud for the technical details, I'd recommend you to file an issue that describes precisely what you're trying to achieve, and ask @asajeffrey for feedback.

Also, maybe look at the worker code.

@justinmichaud justinmichaud force-pushed the justinmichaud:master branch 2 times, most recently from 1cc51e9 to ffb3e91 Feb 8, 2018
@asajeffrey
Copy link
Member

asajeffrey commented Feb 8, 2018

@justinmichaud: the problem with just serializing the pointer is that it doesn't stop the pointer from being GC'd. For cases like this (e.g. history state or document ids) we've used a lookup table mapping object ids to objects, and making sure that table is stored in a DOM object that's kept alive. Then you can serialize object ids. For example the data structure that does this for documents is https://github.com/servo/servo/blob/master/components/script/script_thread.rs#L322

@justinmichaud
Copy link
Author

justinmichaud commented Feb 8, 2018

Thanks for the example @asajeffrey. Does this still mean that I have to serialize the fn(&Document)->() pointer with transmute? Also, I am a bit confused about how this pointer would be GC'd. What lifetime do fn pointers have?

Currently, for my callback (on head parsed), I made a thread-local variable and copied it across to the constellation and then script threads at their creation. I wonder, will there will be a more general need for embedding applications to receive callbacks that run on different threads? If so, what is the best way? I found that using a message was confusing, because there is no easy way to find out when the script thread has started, but I am sure I am missing something.

My other question is, what is the best way would be to decouple the application handler from the script crate? I can't seem to find an existing interface, but essentially I would like to be able to expose the same interface as the one that is exposed to Javascript. I was thinking I would create a new crate, something like embedder_traits, that would contain a new interface that was implemented using the dom interface.

Thanks again for helping me out with this. I really appreciate it.

@justinmichaud justinmichaud force-pushed the justinmichaud:master branch from ffb3e91 to c40cd45 Feb 26, 2018
@justinmichaud
Copy link
Author

justinmichaud commented Feb 26, 2018

img

Quick update: I have made a demo here of what I want to build.

I am wondering what the smallest, most flexible api is that could be exposed from servo without breaking encapsulation. Any ideas?

Thanks again!

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.