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 dispatching generic runnables to script event loops #3735

Closed
jdm opened this issue Oct 20, 2014 · 9 comments
Closed

Implement dispatching generic runnables to script event loops #3735

jdm opened this issue Oct 20, 2014 · 9 comments
Labels
A-content/script Related to the script thread C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-cleanup No impact; the issue is one of maintainability or tidiness.

Comments

@jdm
Copy link
Member

jdm commented Oct 20, 2014

For events that are dispatched to the script event loop from the script crate it would be cleaner to avoid adding implementation knowledge to script_task.rs - events like the XHR ones belong entirely to the XHR code. If we had runnables with closures, the script task could just invoke the closure without having to know anything about its contents. Something like RunnableMsg(Box<Runnable>) where Runnable is a trait with a method to invoke the closure would make a lot of sense; any state the closure required could be stored in whatever type is implementing the Runnable trait for that particular event.

@jdm jdm added E-less-complex Straightforward. Recommended for a new contributor. A-content/script Related to the script thread I-cleanup No impact; the issue is one of maintainability or tidiness. labels Oct 20, 2014
@thiagopnts
Copy link
Contributor

You mean to replace the current ScriptMsg approach? I'll start to work on this.

@jdm
Copy link
Member Author

jdm commented Dec 16, 2014

I'm pretty sure we'll need to retain at least some of the messages in there, but I would also be happy to be proved wrong. I was imaging RunnableMsg being another variant of ScriptMsg.

@thiagopnts
Copy link
Contributor

Ok, I'll start to implement this approach with the XHR events you mentioned, then I'll ping you to check if I'm on the right track.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Dec 16, 2014
@thiagopnts
Copy link
Contributor

what I did so far: https://github.com/thiagopnts/servo/blob/b014a7417cc9e8abf06528653cafa89ab2fbbd9b/components/script/dom/xmlhttprequest.rs#L83-L95

https://github.com/thiagopnts/servo/blob/b014a7417cc9e8abf06528653cafa89ab2fbbd9b/components/script/script_task.rs#L123

The main problem is processing the progress on the handler, I end up getting a:

dom/xmlhttprequest.rs:92:42: 92:46 error: cannot move out of dereference of `&`-pointer
dom/xmlhttprequest.rs:92             xhr.process_partial_response(self.progress);

Any thoughts on how to workaround this? I would also appreciate any resources on how to deal with boxed traits on channels.

@jdm
Copy link
Member Author

jdm commented Dec 22, 2014

I would try making the handler method take self by value instead of reference.

@thiagopnts
Copy link
Contributor

This makes the object not safe:

dom/xmlhttprequest.rs:232:43: 232:92 error: cannot convert to a trait object because trait `script_task::Runnable` is not object-safe [E0038]
dom/xmlhttprequest.rs:232                     chan.send(RunnableMsg(box XHRProgressWrapper{addr: addr, progress: msg}));
                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: cannot call a method (`handler`) with a by-value receiver through a trait object
error: aborting due to previous error

@jdm
Copy link
Member Author

jdm commented Dec 22, 2014

:<

Maybe &mut self? If not, then just clone the needed values for now.

@jdm
Copy link
Member Author

jdm commented Dec 22, 2014

rust-lang/rust#19878 (comment) might be related (and/or suggest alternatives).

@thiagopnts
Copy link
Contributor

@gankro's post is also a good resource on the subject. The &mut self approach didn't work too, so I made a PR cloning the values.

bors-servo pushed a commit that referenced this issue Dec 24, 2014
This refs #3735. As discussed in the issue, I did it cloning when I couldn't dereference an attribute. The trait method should be on `&self` for object-safety reasons.
bors-servo pushed a commit that referenced this issue Dec 24, 2014
This refs #3735. As discussed in the issue, I did it cloning when I couldn't dereference an attribute. The trait method should be on `&self` for object-safety reasons.
@jdm jdm closed this as completed Jan 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread C-assigned There is someone working on resolving the issue E-less-complex Straightforward. Recommended for a new contributor. I-cleanup No impact; the issue is one of maintainability or tidiness.
Projects
None yet
Development

No branches or pull requests

2 participants