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 all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -129,6 +129,7 @@ impl Formattable for ProfilerCategory {
ProfilerCategory::ScriptDomEvent => "Script Dom Event",
ProfilerCategory::ScriptEvaluate => "Script JS Evaluate",
ProfilerCategory::ScriptFileRead => "Script File Read",
ProfilerCategory::ScriptHistoryEvent => "Script History Event",
ProfilerCategory::ScriptImageCacheMsg => "Script Image Cache Msg",
ProfilerCategory::ScriptInputEvent => "Script Input Event",
ProfilerCategory::ScriptNetworkEvent => "Script Network Event",
@@ -103,6 +103,7 @@ pub enum ProfilerCategory {
ScriptWebVREvent = 0x79,
ScriptWorkletEvent = 0x7a,
ScriptPerformanceEvent = 0x7b,
ScriptHistoryEvent = 0x7c,
TimeToFirstPaint = 0x80,
TimeToFirstContentfulPaint = 0x81,
TimeToInteractive = 0x82,
@@ -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;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.