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

Unify the task source and task canceller API #21804

Merged
merged 2 commits into from Nov 16, 2018
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Prev

Unify the task source and task canceller API

I moved away from the `Window` struct all the logic to handle task
sources, into a new struct called `TaskManager`. In a happy world, I'd
be able to just have there two functions, of the types:

```rust
fn task_source<T: TaskSource>(&self, name: TaskSourceName) -> Box<T>
fn task_source_with_canceller<T: TaskSource>(&self, name: TaskSourceName)
  -> (Box<T>, TaskSourceCanceller)
```

And not so much duplicated code. However, because TaskSource can't be a
trait object (because it has generic type parameters), that's not
possible. Instead, I decided to reduce duplicated logic through macros.

For reasons[1], I have to pass both the name of the function with
canceller and the name of the function without, as I'm not able to
concatenate them in the macro itself. I could probably use
`concat_idents` to create both types already defined and reduce the
amount of arguments by one, but that macro is nightly only. At the same
time, not being able to declare macros inside `impl` forces me to pass
`self` as an argument.

All this makes this solution more verbose than it would be ideally. It
does reduce duplication, but it doesn't reduce the size of the file.

[1](rust-lang/rust#29599)
  • Loading branch information
AgustinCB committed Nov 14, 2018
commit 75eb94afcaae2f868ecccba5b5dcea4066998d7a
@@ -17,7 +17,7 @@ use crate::dom::bindings::refcounted::Trusted;
use crate::dom::bindings::reflector::reflect_dom_object;
use crate::dom::bindings::root::DomRoot;
use crate::dom::window::Window;
use crate::task_source::{TaskSource, TaskSourceName};
use crate::task_source::TaskSource;
use dom_struct::dom_struct;
use ipc_channel::ipc::{self, IpcReceiver};
use ipc_channel::router::ROUTER;
@@ -97,8 +97,9 @@ impl AnalyserNode {
) -> Fallible<DomRoot<AnalyserNode>> {
let (node, recv) = AnalyserNode::new_inherited(window, context, options)?;
let object = reflect_dom_object(Box::new(node), window, AnalyserNodeBinding::Wrap);
let source = window.dom_manipulation_task_source();
let canceller = window.task_canceller(TaskSourceName::DOMManipulation);
let (source, canceller) = window
.task_manager()
.dom_manipulation_task_source_with_canceller();
let this = Trusted::new(&*object);

ROUTER.add_route(
@@ -126,7 +126,7 @@ impl AudioContextMethods for AudioContext {

// Steps 4 and 5.
let window = DomRoot::downcast::<Window>(self.global()).unwrap();
let task_source = window.dom_manipulation_task_source();
let task_source = window.task_manager().dom_manipulation_task_source();
let trusted_promise = TrustedPromise::new(promise.clone());
match self.context.audio_context_impl().suspend() {
Ok(_) => {
@@ -141,7 +141,7 @@ impl AudioContextMethods for AudioContext {
if base_context.State() != AudioContextState::Suspended {
base_context.set_state_attribute(AudioContextState::Suspended);
let window = DomRoot::downcast::<Window>(context.global()).unwrap();
window.dom_manipulation_task_source().queue_simple_event(
window.task_manager().dom_manipulation_task_source().queue_simple_event(

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 29, 2018

Member

nit: I'm wondering if task_source could be used instead here.

This comment has been minimized.

Copy link
@AgustinCB

AgustinCB Sep 29, 2018

Author Contributor

Not really, since task_source is borrowed in line 131.

context.upcast(),
atom!("statechange"),
&window
@@ -188,7 +188,7 @@ impl AudioContextMethods for AudioContext {

// Steps 4 and 5.
let window = DomRoot::downcast::<Window>(self.global()).unwrap();
let task_source = window.dom_manipulation_task_source();
let task_source = window.task_manager().dom_manipulation_task_source();
let trusted_promise = TrustedPromise::new(promise.clone());
match self.context.audio_context_impl().close() {
Ok(_) => {
@@ -203,7 +203,7 @@ impl AudioContextMethods for AudioContext {
if base_context.State() != AudioContextState::Closed {
base_context.set_state_attribute(AudioContextState::Closed);
let window = DomRoot::downcast::<Window>(context.global()).unwrap();
window.dom_manipulation_task_source().queue_simple_event(
window.task_manager().dom_manipulation_task_source().queue_simple_event(

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 29, 2018

Member

same here

This comment has been minimized.

Copy link
@AgustinCB

AgustinCB Sep 29, 2018

Author Contributor

In here, it's borrowed in line 193.

context.upcast(),
atom!("statechange"),
&window
@@ -10,7 +10,7 @@ use crate::dom::bindings::inheritance::Castable;
use crate::dom::bindings::num::Finite;
use crate::dom::bindings::refcounted::Trusted;
use crate::dom::bindings::reflector::DomObject;
use crate::task_source::{TaskSource, TaskSourceName};
use crate::task_source::TaskSource;
use dom_struct::dom_struct;
use servo_media::audio::node::OnEndedCallback;
use servo_media::audio::node::{AudioNodeInit, AudioNodeMessage, AudioScheduledSourceNodeMessage};
@@ -71,15 +71,16 @@ impl AudioScheduledSourceNodeMethods for AudioScheduledSourceNode {
let this = Trusted::new(self);
let global = self.global();
let window = global.as_window();
let task_source = window.dom_manipulation_task_source();
let canceller = window.task_canceller(TaskSourceName::DOMManipulation);
let (task_source, canceller) = window
.task_manager()
.dom_manipulation_task_source_with_canceller();
let callback = OnEndedCallback::new(move || {
let _ = task_source.queue_with_canceller(
task!(ended: move || {
let this = this.root();
let global = this.global();
let window = global.as_window();
window.dom_manipulation_task_source().queue_simple_event(
window.task_manager().dom_manipulation_task_source().queue_simple_event(

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 29, 2018

Member

same here

This comment has been minimized.

Copy link
@AgustinCB

AgustinCB Sep 29, 2018

Author Contributor

Similar, is borrowed before.

this.upcast(),
atom!("ended"),
&window
@@ -40,7 +40,7 @@ use crate::dom::oscillatornode::OscillatorNode;
use crate::dom::pannernode::PannerNode;
use crate::dom::promise::Promise;
use crate::dom::window::Window;
use crate::task_source::{TaskSource, TaskSourceName};
use crate::task_source::TaskSource;
use dom_struct::dom_struct;
use js::rust::CustomAutoRooterGuard;
use js::typedarray::ArrayBuffer;
@@ -213,7 +213,7 @@ impl BaseAudioContext {
pub fn resume(&self) {
let global = self.global();
let window = global.as_window();
let task_source = window.dom_manipulation_task_source();
let task_source = window.task_manager().dom_manipulation_task_source();
let this = Trusted::new(self);
// Set the rendering thread state to 'running' and start
// rendering the audio graph.
@@ -227,7 +227,7 @@ impl BaseAudioContext {
if this.state.get() != AudioContextState::Running {
this.state.set(AudioContextState::Running);
let window = DomRoot::downcast::<Window>(this.global()).unwrap();
window.dom_manipulation_task_source().queue_simple_event(
window.task_manager().dom_manipulation_task_source().queue_simple_event(

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 29, 2018

Member

same here

This comment has been minimized.

Copy link
@AgustinCB

AgustinCB Sep 29, 2018

Author Contributor

It's also borrowed before.

To be fair, this parts also bothered me. I suspect that there's some way in which this code can be refactor to call less clones. But I'm not sure it's immediately obvious.

Maybe I can offer you to open an issue to track the tech debt?

This comment has been minimized.

Copy link
@gterzian

gterzian Oct 1, 2018

Member

Ok, thanks for the info. I'm not sure at this point how to make a potential issue actionable, so I will not open one. If you happen to have a clearer idea, it could be useful if you were to open one indeed...

This comment has been minimized.

Copy link
@AgustinCB

AgustinCB Oct 1, 2018

Author Contributor

No, not really.

this.upcast(),
atom!("statechange"),
&window
@@ -428,10 +428,12 @@ impl BaseAudioContextMethods for BaseAudioContext {
let decoded_audio__ = decoded_audio.clone();
let this = Trusted::new(self);
let this_ = this.clone();
let task_source = window.dom_manipulation_task_source();
let task_source_ = window.dom_manipulation_task_source();
let canceller = window.task_canceller(TaskSourceName::DOMManipulation);
let canceller_ = window.task_canceller(TaskSourceName::DOMManipulation);
let (task_source, canceller) = window
.task_manager()
.dom_manipulation_task_source_with_canceller();
let (task_source_, canceller_) = window
.task_manager()
.dom_manipulation_task_source_with_canceller();
let callbacks = AudioDecoderCallbacks::new()
.ready(move |channel_count| {
decoded_audio
@@ -521,6 +521,7 @@ impl Document {
if self.ready_state.get() == DocumentReadyState::Complete {
let document = Trusted::new(self);
self.window
.task_manager()
.dom_manipulation_task_source()
.queue(
task!(fire_pageshow_event: move || {
@@ -1869,6 +1870,7 @@ impl Document {
debug!("Document loads are complete.");
let document = Trusted::new(self);
self.window
.task_manager()
.dom_manipulation_task_source()
.queue(
task!(fire_load_event: move || {
@@ -1922,6 +1924,7 @@ impl Document {
let document = Trusted::new(self);
if document.root().browsing_context().is_some() {
self.window
.task_manager()
.dom_manipulation_task_source()
.queue(
task!(fire_pageshow_event: move || {
@@ -2104,13 +2107,16 @@ impl Document {

// Step 4.1.
let window = self.window();
window.dom_manipulation_task_source().queue_event(
self.upcast(),
atom!("DOMContentLoaded"),
EventBubbles::Bubbles,
EventCancelable::NotCancelable,
window,
);
window
.task_manager()
.dom_manipulation_task_source()
.queue_event(
self.upcast(),
atom!("DOMContentLoaded"),
EventBubbles::Bubbles,
EventCancelable::NotCancelable,
window,
);

window.reflow(ReflowGoal::Full, ReflowReason::DOMContentLoaded);
update_with_current_time_ms(&self.dom_content_loaded_event_end);
@@ -476,7 +476,7 @@ impl GlobalScope {
/// this global scope.
pub fn networking_task_source(&self) -> NetworkingTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.networking_task_source();
return window.task_manager().networking_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.networking_task_source();
@@ -488,7 +488,7 @@ impl GlobalScope {
/// this global scope.
pub fn remote_event_task_source(&self) -> RemoteEventTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.remote_event_task_source();
return window.task_manager().remote_event_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.remote_event_task_source();
@@ -500,7 +500,7 @@ impl GlobalScope {
/// this global scope.
pub fn websocket_task_source(&self) -> WebsocketTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.websocket_task_source();
return window.task_manager().websocket_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.websocket_task_source();
@@ -635,7 +635,7 @@ impl GlobalScope {
/// properly cancelled when the global scope is destroyed.
pub fn task_canceller(&self, name: TaskSourceName) -> TaskCanceller {
if let Some(window) = self.downcast::<Window>() {
return window.task_canceller(name);
return window.task_manager().task_canceller(name);
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
// Note: the "name" is not passed to the worker,
@@ -691,7 +691,7 @@ impl GlobalScope {

pub fn dom_manipulation_task_source(&self) -> DOMManipulationTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.dom_manipulation_task_source();
return window.task_manager().dom_manipulation_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.dom_manipulation_task_source();
@@ -703,7 +703,7 @@ impl GlobalScope {
/// this of this global scope.
pub fn file_reading_task_source(&self) -> FileReadingTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.file_reading_task_source();
return window.task_manager().file_reading_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.file_reading_task_source();
@@ -756,7 +756,7 @@ impl GlobalScope {
/// of this global scope.
pub fn performance_timeline_task_source(&self) -> PerformanceTimelineTaskSource {
if let Some(window) = self.downcast::<Window>() {
return window.performance_timeline_task_source();
return window.task_manager().performance_timeline_task_source();
}
if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
return worker.performance_timeline_task_source();
@@ -76,7 +76,7 @@ impl VirtualMethods for HTMLDetailsElement {
let window = window_from_node(self);
let this = Trusted::new(self);
// FIXME(nox): Why are errors silenced here?
let _ = window.dom_manipulation_task_source().queue(
let _ = window.task_manager().dom_manipulation_task_source().queue(
task!(details_notification_task_steps: move || {
let this = this.root();
if counter == this.toggle_counter.get() {
@@ -90,7 +90,8 @@ impl HTMLDialogElementMethods for HTMLDialogElement {
// TODO: Step 4 implement pending dialog stack removal

// Step 5
win.dom_manipulation_task_source()
win.task_manager()
.dom_manipulation_task_source()
.queue_simple_event(target, atom!("close"), &win);
}
}
@@ -523,6 +523,7 @@ impl HTMLFormElement {

// Step 3.
target
.task_manager()
.dom_manipulation_task_source()
.queue(task, target.upcast())
.unwrap();
@@ -237,7 +237,7 @@ impl HTMLIFrameElement {
let this = Trusted::new(self);
let pipeline_id = self.pipeline_id().unwrap();
// FIXME(nox): Why are errors silenced here?
let _ = window.dom_manipulation_task_source().queue(
let _ = window.task_manager().dom_manipulation_task_source().queue(
task!(iframe_load_event_steps: move || {
this.root().iframe_load_event_steps(pipeline_id);
}),
@@ -40,7 +40,7 @@ use crate::dom::window::Window;
use crate::microtask::{Microtask, MicrotaskRunnable};
use crate::network_listener::{NetworkListener, PreInvoke};
use crate::script_thread::ScriptThread;
use crate::task_source::{TaskSource, TaskSourceName};
use crate::task_source::TaskSource;
use cssparser::{Parser, ParserInput};
use dom_struct::dom_struct;
use euclid::Point2D;
@@ -237,8 +237,9 @@ impl HTMLImageElement {
let (responder_sender, responder_receiver) = ipc::channel().unwrap();

let window = window_from_node(elem);
let task_source = window.networking_task_source();
let task_canceller = window.task_canceller(TaskSourceName::Networking);
let (task_source, canceller) = window
.task_manager()
.networking_task_source_with_canceller();
let generation = elem.generation.get();
ROUTER.add_route(
responder_receiver.to_opaque(),
@@ -257,7 +258,7 @@ impl HTMLImageElement {
element.process_image_response(image);
}
}),
&task_canceller,
&canceller,
);
}),
);
@@ -308,10 +309,14 @@ impl HTMLImageElement {
}));

let (action_sender, action_receiver) = ipc::channel().unwrap();
let (task_source, canceller) = document
.window()
.task_manager()
.networking_task_source_with_canceller();
let listener = NetworkListener {
context: context,
task_source: window.networking_task_source(),
canceller: Some(window.task_canceller(TaskSourceName::Networking)),
context,
task_source,
canceller: Some(canceller),
};
ROUTER.add_route(
action_receiver.to_opaque(),
@@ -780,7 +785,7 @@ impl HTMLImageElement {
fn update_the_image_data_sync_steps(&self) {
let document = document_from_node(self);
let window = document.window();
let task_source = window.dom_manipulation_task_source();
let task_source = window.task_manager().dom_manipulation_task_source();
let this = Trusted::new(self);
let (src, pixel_density) = match self.select_image_source() {
// Step 8
@@ -938,7 +943,7 @@ impl HTMLImageElement {
current_request.current_pixel_density = pixel_density;
let this = Trusted::new(self);
let src = String::from(src);
let _ = window.dom_manipulation_task_source().queue(
let _ = window.task_manager().dom_manipulation_task_source().queue(
task!(image_load_event: move || {
let this = this.root();
{
@@ -989,8 +994,9 @@ impl HTMLImageElement {
let (responder_sender, responder_receiver) = ipc::channel().unwrap();

let window = window_from_node(elem);
let task_source = window.networking_task_source();
let task_canceller = window.task_canceller(TaskSourceName::Networking);
let (task_source, canceller) = window
.task_manager()
.networking_task_source_with_canceller();
let generation = elem.generation.get();
ROUTER.add_route(responder_receiver.to_opaque(), Box::new(move |message| {
debug!("Got image {:?}", message);
@@ -1008,7 +1014,7 @@ impl HTMLImageElement {
DOMString::from_string(selected_source_clone), generation, selected_pixel_density);
}
}),
&task_canceller,
&canceller,
);
}));

@@ -1133,7 +1139,7 @@ impl HTMLImageElement {
let this = Trusted::new(self);
let window = window_from_node(self);
let src = src.to_string();
let _ = window.dom_manipulation_task_source().queue(
let _ = window.task_manager().dom_manipulation_task_source().queue(
task!(image_load_event: move || {
let this = this.root();
let relevant_mutation = this.generation.get() != generation;
@@ -1515,13 +1515,16 @@ impl VirtualMethods for HTMLInputElement {
{
if event.IsTrusted() {
let window = window_from_node(self);
let _ = window.user_interaction_task_source().queue_event(
&self.upcast(),
atom!("input"),
EventBubbles::Bubbles,
EventCancelable::NotCancelable,
&window,
);
let _ = window
.task_manager()
.user_interaction_task_source()
.queue_event(
&self.upcast(),
atom!("input"),
EventBubbles::Bubbles,
EventCancelable::NotCancelable,
&window,
);
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.