Skip to content

Commit

Permalink
Auto merge of #16861 - gterzian:use_microtask_to_await_stable_state, …
Browse files Browse the repository at this point in the history
…r=jdm

Use microtasks to await a stable state.

<!-- Please describe your changes on the following line: -->

@jdm @KiChjang First pass at using microtasks to await a stable state. I ran into all sorts of problems to get it to compile, I think it's mainly related to the fact that the microtasks are stored in a `Vec`, which meant the `Runnalbe.handler(self: Box<Self>)` couldn't be called while iterating over the Vec... It compiles now although I haven't run any tests. I'm assuming I'm missing something fundamental and was hoping my changes would highlight the problems I run into, and you had a better idea of how to implement this... Perhaps we shouldn't pass a `Runnable` to `await_stable_state` at all?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #15375 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16861)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed May 20, 2017
2 parents 60682cf + a8390ae commit f054911
Show file tree
Hide file tree
Showing 12 changed files with 47 additions and 97 deletions.
83 changes: 36 additions & 47 deletions components/script/dom/htmlmediaelement.rs
Expand Up @@ -31,6 +31,7 @@ use dom_struct::dom_struct;
use html5ever::{LocalName, Prefix};
use ipc_channel::ipc;
use ipc_channel::router::ROUTER;
use microtask::{Microtask, MicrotaskRunnable};
use net_traits::{FetchResponseListener, FetchMetadata, Metadata, NetworkError};
use net_traits::request::{CredentialsMode, Destination, RequestInit, Type as RequestType};
use network_listener::{NetworkListener, PreInvoke};
Expand Down Expand Up @@ -429,7 +430,11 @@ impl HTMLMediaElement {

// Step 4
let doc = document_from_node(self);
ScriptThread::await_stable_state(ResourceSelectionTask::new(self, doc.base_url()));
let task = MediaElementMicrotask::ResourceSelectionTask {
elem: Root::from_ref(self),
base_url: doc.base_url()
};
ScriptThread::await_stable_state(Microtask::MediaElement(task));
}

// https://html.spec.whatwg.org/multipage/#concept-media-load-algorithm
Expand Down Expand Up @@ -781,7 +786,36 @@ impl VirtualMethods for HTMLMediaElement {
self.super_type().unwrap().unbind_from_tree(context);

if context.tree_in_doc {
ScriptThread::await_stable_state(PauseIfNotInDocumentTask::new(self));
let task = MediaElementMicrotask::PauseIfNotInDocumentTask {
elem: Root::from_ref(self)
};
ScriptThread::await_stable_state(Microtask::MediaElement(task));
}
}
}

#[derive(JSTraceable, HeapSizeOf)]
pub enum MediaElementMicrotask {
ResourceSelectionTask {
elem: Root<HTMLMediaElement>,
base_url: ServoUrl
},
PauseIfNotInDocumentTask {
elem: Root<HTMLMediaElement>,
}
}

impl MicrotaskRunnable for MediaElementMicrotask {
fn handler(&self) {
match self {
&MediaElementMicrotask::ResourceSelectionTask { ref elem, ref base_url } => {
elem.resource_selection_algorithm_sync(base_url.clone());
},
&MediaElementMicrotask::PauseIfNotInDocumentTask { ref elem } => {
if !elem.upcast::<Node>().is_in_doc() {
elem.internal_pause_steps();
}
},
}
}
}
Expand Down Expand Up @@ -809,28 +843,6 @@ impl Runnable for FireSimpleEventTask {
}
}

struct ResourceSelectionTask {
elem: Trusted<HTMLMediaElement>,
base_url: ServoUrl,
}

impl ResourceSelectionTask {
fn new(elem: &HTMLMediaElement, url: ServoUrl) -> ResourceSelectionTask {
ResourceSelectionTask {
elem: Trusted::new(elem),
base_url: url,
}
}
}

impl Runnable for ResourceSelectionTask {
fn name(&self) -> &'static str { "ResourceSelectionTask" }

fn handler(self: Box<ResourceSelectionTask>) {
self.elem.root().resource_selection_algorithm_sync(self.base_url);
}
}

struct DedicatedMediaSourceFailureTask {
elem: Trusted<HTMLMediaElement>,
}
Expand All @@ -851,29 +863,6 @@ impl Runnable for DedicatedMediaSourceFailureTask {
}
}

struct PauseIfNotInDocumentTask {
elem: Trusted<HTMLMediaElement>,
}

impl PauseIfNotInDocumentTask {
fn new(elem: &HTMLMediaElement) -> PauseIfNotInDocumentTask {
PauseIfNotInDocumentTask {
elem: Trusted::new(elem),
}
}
}

impl Runnable for PauseIfNotInDocumentTask {
fn name(&self) -> &'static str { "PauseIfNotInDocumentTask" }

fn handler(self: Box<PauseIfNotInDocumentTask>) {
let elem = self.elem.root();
if !elem.upcast::<Node>().is_in_doc() {
elem.internal_pause_steps();
}
}
}

enum ResourceSelectionMode {
Object,
Attribute(String),
Expand Down
9 changes: 9 additions & 0 deletions components/script/microtask.rs
Expand Up @@ -11,6 +11,7 @@ use dom::bindings::cell::DOMRefCell;
use dom::bindings::codegen::Bindings::PromiseBinding::PromiseJobCallback;
use dom::bindings::js::Root;
use dom::globalscope::GlobalScope;
use dom::htmlmediaelement::MediaElementMicrotask;
use dom::mutationobserver::MutationObserver;
use msg::constellation_msg::PipelineId;
use std::cell::Cell;
Expand All @@ -29,9 +30,14 @@ pub struct MicrotaskQueue {
#[derive(JSTraceable, HeapSizeOf)]
pub enum Microtask {
Promise(EnqueuedPromiseCallback),
MediaElement(MediaElementMicrotask),
NotifyMutationObservers,
}

pub trait MicrotaskRunnable {
fn handler(&self) {}
}

/// A promise callback scheduled to run during the next microtask checkpoint (#4283).
#[derive(JSTraceable, HeapSizeOf)]
pub struct EnqueuedPromiseCallback {
Expand Down Expand Up @@ -72,6 +78,9 @@ impl MicrotaskQueue {
if let Some(target) = target_provider(job.pipeline) {
let _ = job.callback.Call_(&*target, ExceptionHandling::Report);
}
},
Microtask::MediaElement(ref task) => {
task.handler();
}
Microtask::NotifyMutationObservers => {
MutationObserver::notify_mutation_observers();
Expand Down
7 changes: 2 additions & 5 deletions components/script/script_thread.rs
Expand Up @@ -670,14 +670,11 @@ impl ScriptThread {
}

// https://html.spec.whatwg.org/multipage/#await-a-stable-state
pub fn await_stable_state<T: Runnable + Send + 'static>(task: T) {
//TODO use microtasks when they exist
pub fn await_stable_state(task: Microtask) {
SCRIPT_THREAD_ROOT.with(|root| {
if let Some(script_thread) = root.get() {
let script_thread = unsafe { &*script_thread };
let _ = script_thread.chan.send(CommonScriptMsg::RunnableMsg(
ScriptThreadEventCategory::DomEvent,
box task));
script_thread.microtask_queue.enqueue(task);
}
});
}
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

0 comments on commit f054911

Please sign in to comment.