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

Add task source name #21583

Merged
merged 1 commit into from Sep 4, 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

@@ -33,7 +33,7 @@ use js::rust::HandleValue;
use msg::constellation_msg::TopLevelBrowsingContextId;
use net_traits::{IpcSend, load_whole_resource};
use net_traits::request::{CredentialsMode, Destination, RequestInit};
use script_runtime::{CommonScriptMsg, ScriptChan, ScriptPort, ScriptThreadEventCategory, new_rt_and_cx, Runtime};
use script_runtime::{CommonScriptMsg, ScriptChan, ScriptPort, new_rt_and_cx, Runtime};
use script_runtime::ScriptThreadEventCategory::WorkerEvent;
use script_traits::{TimerEvent, TimerSource, WorkerGlobalScopeInit, WorkerScriptLoadOrigin};
use servo_rand::random;
@@ -45,6 +45,7 @@ use std::sync::mpsc::{Receiver, Sender, channel};
use std::thread;
use style::thread_state::{self, ThreadState};
use task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue};
use task_source::TaskSourceName;

/// Set the `worker` field of a related DedicatedWorkerGlobalScope object to a particular
/// value for the duration of this object's lifetime. This ensures that the related Worker
@@ -86,7 +87,7 @@ pub enum MixedMessage {
}

impl QueuedTaskConversion for DedicatedWorkerScriptMsg {
fn task_category(&self) -> Option<&ScriptThreadEventCategory> {
fn task_source_name(&self) -> Option<&TaskSourceName> {
let common_worker_msg = match self {
DedicatedWorkerScriptMsg::CommonWorker(_, common_worker_msg) => common_worker_msg,
_ => return None,
@@ -95,11 +96,10 @@ impl QueuedTaskConversion for DedicatedWorkerScriptMsg {
WorkerScriptMsg::Common(ref script_msg) => script_msg,
_ => return None,
};
let category = match script_msg {
CommonScriptMsg::Task(category, _boxed, _pipeline_id) => category,
match script_msg {
CommonScriptMsg::Task(_category, _boxed, _pipeline_id, source_name) => Some(&source_name),
_ => return None,
};
Some(category)
}
}

fn into_queued_task(self) -> Option<QueuedTask> {
@@ -111,17 +111,22 @@ impl QueuedTaskConversion for DedicatedWorkerScriptMsg {
WorkerScriptMsg::Common(script_msg) => script_msg,
_ => return None,
};
let (category, boxed, pipeline_id) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id) =>
(category, boxed, pipeline_id),
let (category, boxed, pipeline_id, task_source) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id, task_source) =>
(category, boxed, pipeline_id, task_source),
_ => return None,
};
Some((Some(worker), category, boxed, pipeline_id))
Some((Some(worker), category, boxed, pipeline_id, task_source))
}

fn from_queued_task(queued_task: QueuedTask) -> Self {
let (worker, category, boxed, pipeline_id) = queued_task;
let script_msg = CommonScriptMsg::Task(category, boxed, pipeline_id);
let (worker, category, boxed, pipeline_id, task_source) = queued_task;
let script_msg = CommonScriptMsg::Task(
category,
boxed,
pipeline_id,
task_source
);
DedicatedWorkerScriptMsg::CommonWorker(worker.unwrap(), WorkerScriptMsg::Common(script_msg))
}

@@ -295,7 +300,8 @@ impl DedicatedWorkerGlobalScope {
parent_sender.send(CommonScriptMsg::Task(
WorkerEvent,
Box::new(SimpleWorkerErrorHandler::new(worker)),
pipeline_id
pipeline_id,
TaskSourceName::DOMManipulation,
)).unwrap();
return;
}
@@ -446,8 +452,12 @@ impl DedicatedWorkerGlobalScope {
global.report_an_error(error_info, HandleValue::null());
}
}));
// TODO: Should use the DOM manipulation task source.
self.parent_sender.send(CommonScriptMsg::Task(WorkerEvent, task, Some(pipeline_id))).unwrap();
self.parent_sender.send(CommonScriptMsg::Task(
WorkerEvent,
task,
Some(pipeline_id),
TaskSourceName::DOMManipulation,
)).unwrap();
}
}

@@ -472,7 +482,13 @@ impl DedicatedWorkerGlobalScopeMethods for DedicatedWorkerGlobalScope {
let task = Box::new(task!(post_worker_message: move || {
Worker::handle_message(worker, data);
}));
self.parent_sender.send(CommonScriptMsg::Task(WorkerEvent, task, Some(pipeline_id))).unwrap();
// TODO: Change this task source to a new `unshipped-port-message-queue` task source
self.parent_sender.send(CommonScriptMsg::Task(
WorkerEvent,
task,
Some(pipeline_id),
TaskSourceName::DOMManipulation,

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 3, 2018

Member

Could you please add a TODO, and file a corresponding issue, pointing out that we should be using a new unshipped-port-message-queue task source that should be used here?

See https://html.spec.whatwg.org/multipage/#unshipped-port-message-queue (I got to this following the spec link just above the fn PostMessage and looking for the task source that would correspond to this specific tasks, it's often mentioned somewhere else than where the task itself is mentioned, often at the bottom of the relevant section...)

)).unwrap();
Ok(())
}

@@ -2837,7 +2837,14 @@ impl Document {
let trusted_pending = Trusted::new(pending);
let trusted_promise = TrustedPromise::new(promise.clone());
let handler = ElementPerformFullscreenEnter::new(trusted_pending, trusted_promise, error);
let script_msg = CommonScriptMsg::Task(ScriptThreadEventCategory::EnterFullscreen, handler, pipeline_id);
// NOTE: This steps should be running in parallel
// https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen
let script_msg = CommonScriptMsg::Task(
ScriptThreadEventCategory::EnterFullscreen,
handler,
pipeline_id,
TaskSourceName::DOMManipulation,

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 3, 2018

Member

Please add a NOTE here that the spec mentions running these steps "in parallel", not necessarily as a queued task. For now using the DOMManipulation seems OK however. See Step 7 of https://fullscreen.spec.whatwg.org/#dom-element-requestfullscreen

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 4, 2018

Member

I've filed a bit more of an explanation at #21600

);
let msg = MainThreadScriptMsg::Common(script_msg);
window.main_thread_script_chan().send(msg).unwrap();

@@ -2870,7 +2877,14 @@ impl Document {
let trusted_promise = TrustedPromise::new(promise.clone());
let handler = ElementPerformFullscreenExit::new(trusted_element, trusted_promise);
let pipeline_id = Some(global.pipeline_id());
let script_msg = CommonScriptMsg::Task(ScriptThreadEventCategory::ExitFullscreen, handler, pipeline_id);
// NOTE: This steps should be running in parallel
// https://fullscreen.spec.whatwg.org/#exit-fullscreen
let script_msg = CommonScriptMsg::Task(
ScriptThreadEventCategory::ExitFullscreen,
handler,
pipeline_id,
TaskSourceName::DOMManipulation,

This comment has been minimized.

Copy link
@gterzian
);
let msg = MainThreadScriptMsg::Common(script_msg);
window.main_thread_script_chan().send(msg).unwrap();

@@ -27,7 +27,7 @@ use js::jsapi::{JSAutoCompartment, JSContext, JS_AddInterruptCallback};
use js::jsval::UndefinedValue;
use net_traits::{load_whole_resource, IpcSend, CustomResponseMediator};
use net_traits::request::{CredentialsMode, Destination, RequestInit};
use script_runtime::{CommonScriptMsg, ScriptChan, ScriptThreadEventCategory, new_rt_and_cx, Runtime};
use script_runtime::{CommonScriptMsg, ScriptChan, new_rt_and_cx, Runtime};
use script_traits::{TimerEvent, WorkerGlobalScopeInit, ScopeThings, ServiceWorkerMsg, WorkerScriptLoadOrigin};
use servo_config::prefs::PREFS;
use servo_rand::random;
@@ -37,6 +37,7 @@ use std::thread;
use std::time::Duration;
use style::thread_state::{self, ThreadState};
use task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue};
use task_source::TaskSourceName;

/// Messages used to control service worker event loop
pub enum ServiceWorkerScriptMsg {
@@ -49,34 +50,33 @@ pub enum ServiceWorkerScriptMsg {
}

impl QueuedTaskConversion for ServiceWorkerScriptMsg {
fn task_category(&self) -> Option<&ScriptThreadEventCategory> {
fn task_source_name(&self) -> Option<&TaskSourceName> {
let script_msg = match self {
ServiceWorkerScriptMsg::CommonWorker(WorkerScriptMsg::Common(script_msg)) => script_msg,
_ => return None,
};
let category = match script_msg {
CommonScriptMsg::Task(category, _boxed, _pipeline_id) => category,
match script_msg {
CommonScriptMsg::Task(_category, _boxed, _pipeline_id, task_source) => Some(&task_source),
_ => return None,
};
Some(&category)
}
}

fn into_queued_task(self) -> Option<QueuedTask> {
let script_msg = match self {
ServiceWorkerScriptMsg::CommonWorker(WorkerScriptMsg::Common(script_msg)) => script_msg,
_ => return None,
};
let (category, boxed, pipeline_id) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id) =>
(category, boxed, pipeline_id),
let (category, boxed, pipeline_id, task_source) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id, task_source) =>
(category, boxed, pipeline_id, task_source),
_ => return None,
};
Some((None, category, boxed, pipeline_id))
Some((None, category, boxed, pipeline_id, task_source))
}

fn from_queued_task(queued_task: QueuedTask) -> Self {
let (_worker, category, boxed, pipeline_id) = queued_task;
let script_msg = CommonScriptMsg::Task(category, boxed, pipeline_id);
let (_worker, category, boxed, pipeline_id, task_source) = queued_task;
let script_msg = CommonScriptMsg::Task(category, boxed, pipeline_id, task_source);
ServiceWorkerScriptMsg::CommonWorker(WorkerScriptMsg::Common(script_msg))
}

@@ -42,6 +42,7 @@ use std::ops::Deref;
use std::rc::Rc;
use std::sync::mpsc;
use std::thread;
use task_source::TaskSourceName;
use webvr_traits::{WebVRDisplayData, WebVRDisplayEvent, WebVRFrameData, WebVRLayer, WebVRMsg};

#[dom_struct]
@@ -517,7 +518,14 @@ impl VRDisplay {
let task = Box::new(task!(handle_vrdisplay_raf: move || {
this.root().handle_raf(&sender);
}));
js_sender.send(CommonScriptMsg::Task(WebVREvent, task, Some(pipeline_id))).unwrap();
// NOTE: WebVR spec doesn't specify what task source we should use. Is
// dom-manipulation a good choice long term?
js_sender.send(CommonScriptMsg::Task(
WebVREvent,
task,
Some(pipeline_id),
TaskSourceName::DOMManipulation,

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 3, 2018

Member

Please add a NOTE asking whether long-term we want to be using the DOMManipulation task source here. The WebVR spec doesn't seem to point to s specific task source.

)).unwrap();

// Run Sync Poses in parallell on Render thread
let msg = WebVRCommand::SyncPoses(display_id, near, far, sync_sender.clone());
@@ -268,7 +268,13 @@ impl WebSocket {
let pipeline_id = self.global().pipeline_id();
self.global()
.script_chan()
.send(CommonScriptMsg::Task(WebSocketEvent, task, Some(pipeline_id)))
// TODO: Use a dedicated `websocket-task-source` task source instead.
.send(CommonScriptMsg::Task(
WebSocketEvent,
task,
Some(pipeline_id),
TaskSourceName::Networking,

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 3, 2018

Member

Please add a TODO, and file an issue, noting that we should be using a dedicated websocket-task-source here, see https://html.spec.whatwg.org/multipage/web-sockets.html#websocket-task-source (I got to this by first looking up this BufferedAmountTask, following the spec link, and then scrolling down until I found the words "The task source for all tasks queued in this section is the WebSocket task source.")

))
.unwrap();
}

@@ -1666,7 +1666,8 @@ impl Window {
let _ = self.script_chan.send(CommonScriptMsg::Task(
ScriptThreadEventCategory::DomEvent,
Box::new(self.task_canceller(TaskSourceName::DOMManipulation).wrap_task(task)),
self.pipeline_id()
self.pipeline_id(),
TaskSourceName::DOMManipulation,
));
doc.set_url(url.clone());
return
@@ -2123,7 +2124,8 @@ impl Window {
let _ = self.script_chan.send(CommonScriptMsg::Task(
ScriptThreadEventCategory::DomEvent,
Box::new(self.task_canceller(TaskSourceName::DOMManipulation).wrap_task(task)),
self.pipeline_id()
self.pipeline_id(),
TaskSourceName::DOMManipulation,
));
}
}
@@ -397,7 +397,7 @@ impl WorkerGlobalScope {

pub fn process_event(&self, msg: CommonScriptMsg) {
match msg {
CommonScriptMsg::Task(_, task, _) => {
CommonScriptMsg::Task(_, task, _, _) => {
task.run_box()
},
CommonScriptMsg::CollectReports(reports_chan) => {
@@ -66,6 +66,7 @@ use style::thread_state::{self, ThreadState};
use swapper::Swapper;
use swapper::swapper;
use task::TaskBox;
use task_source::TaskSourceName;
use uuid::Uuid;

// Magic numbers
@@ -644,7 +645,14 @@ impl WorkletThread {
where
T: TaskBox + 'static,
{
let msg = CommonScriptMsg::Task(ScriptThreadEventCategory::WorkletEvent, Box::new(task), None);
// NOTE: It's unclear which task source should be used here:
// https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule
let msg = CommonScriptMsg::Task(
ScriptThreadEventCategory::WorkletEvent,
Box::new(task),
None,
TaskSourceName::DOMManipulation,

This comment has been minimized.

Copy link
@gterzian

gterzian Sep 3, 2018

Member

Please add a NOTE that it's unclear from the Spec which task source should be used here, see https://drafts.css-houdini.org/worklets/#dom-worklet-addmodule

);
let msg = MainThreadScriptMsg::Common(msg);
self.global_init.to_script_thread_sender.send(msg).expect("Worklet thread outlived script thread.");
}
@@ -43,6 +43,7 @@ use std::panic::AssertUnwindSafe;
use std::ptr;
use style::thread_state::{self, ThreadState};
use task::TaskBox;
use task_source::TaskSourceName;
use time::{Tm, now};

/// Common messages used to control the event loops in both the script and the worker
@@ -51,14 +52,14 @@ pub enum CommonScriptMsg {
/// supplied channel.
CollectReports(ReportsChan),
/// Generic message that encapsulates event handling.
Task(ScriptThreadEventCategory, Box<TaskBox>, Option<PipelineId>),
Task(ScriptThreadEventCategory, Box<TaskBox>, Option<PipelineId>, TaskSourceName),
}

impl fmt::Debug for CommonScriptMsg {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match *self {
CommonScriptMsg::CollectReports(_) => write!(f, "CollectReports(...)"),
CommonScriptMsg::Task(ref category, ref task, _) => {
CommonScriptMsg::Task(ref category, ref task, _, _) => {
f.debug_tuple("Task").field(category).field(task).finish()
},
}
@@ -117,6 +117,7 @@ use std::sync::mpsc::{Receiver, Select, Sender, channel};
use std::thread;
use style::thread_state::{self, ThreadState};
use task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue};
use task_source::TaskSourceName;
use task_source::dom_manipulation::DOMManipulationTaskSource;
use task_source::file_reading::FileReadingTaskSource;
use task_source::history_traversal::HistoryTraversalTaskSource;
@@ -248,34 +249,33 @@ pub enum MainThreadScriptMsg {
}

impl QueuedTaskConversion for MainThreadScriptMsg {
fn task_category(&self) -> Option<&ScriptThreadEventCategory> {
fn task_source_name(&self) -> Option<&TaskSourceName> {
let script_msg = match self {
MainThreadScriptMsg::Common(script_msg) => script_msg,
_ => return None,
};
let category = match script_msg {
CommonScriptMsg::Task(category, _boxed, _pipeline_id) => category,
match script_msg {
CommonScriptMsg::Task(_category, _boxed, _pipeline_id, task_source) => Some(&task_source),
_ => return None,
};
Some(&category)
}
}

fn into_queued_task(self) -> Option<QueuedTask> {
let script_msg = match self {
MainThreadScriptMsg::Common(script_msg) => script_msg,
_ => return None,
};
let (category, boxed, pipeline_id) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id) =>
(category, boxed, pipeline_id),
let (category, boxed, pipeline_id, task_source) = match script_msg {
CommonScriptMsg::Task(category, boxed, pipeline_id, task_source) =>
(category, boxed, pipeline_id, task_source),
_ => return None,
};
Some((None, category, boxed, pipeline_id))
Some((None, category, boxed, pipeline_id, task_source))
}

fn from_queued_task(queued_task: QueuedTask) -> Self {
let (_worker, category, boxed, pipeline_id) = queued_task;
let script_msg = CommonScriptMsg::Task(category, boxed, pipeline_id);
let (_worker, category, boxed, pipeline_id, task_source) = queued_task;
let script_msg = CommonScriptMsg::Task(category, boxed, pipeline_id, task_source);
MainThreadScriptMsg::Common(script_msg)
}

@@ -1279,7 +1279,7 @@ impl ScriptThread {
MixedMessage::FromDevtools(_) => None,
MixedMessage::FromScript(ref inner_msg) => {
match *inner_msg {
MainThreadScriptMsg::Common(CommonScriptMsg::Task(_, _, pipeline_id)) =>
MainThreadScriptMsg::Common(CommonScriptMsg::Task(_, _, pipeline_id, _)) =>
pipeline_id,
MainThreadScriptMsg::Common(CommonScriptMsg::CollectReports(_)) => None,
MainThreadScriptMsg::Navigate(pipeline_id, ..) => Some(pipeline_id),
@@ -1433,7 +1433,7 @@ impl ScriptThread {
MainThreadScriptMsg::Navigate(parent_pipeline_id, load_data, replace) => {
self.handle_navigate(parent_pipeline_id, None, load_data, replace)
},
MainThreadScriptMsg::Common(CommonScriptMsg::Task(_, task, _)) => {
MainThreadScriptMsg::Common(CommonScriptMsg::Task(_, task, _, _)) => {
task.run_box()
}
MainThreadScriptMsg::Common(CommonScriptMsg::CollectReports(chan)) => {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.