script: Update the rendering when receiving IPC messages instead of just reflowing (#34486)

This changes fixes two issues:

1. A reflow of all `Document`s currently done unconditionally after
   receving IPC messages in the `ScriptThread`. Reflowing without first
   updating the animation timeline can lead to transitions finshing as
   soon as they start (because it looks like time advancement is
   measaured between calls to `update-the-rendering`).
2. Fix an issue where not all `Pipeline`s were updated during *update
   the rendering*. The previous code only took into account top level
   frames and their children. It's not guaranteed that a particular
   `ScriptThread` is managing any top level frames, depending on the
   origens of those frames. We should update the rendering of those
   non-top-level iframes regardless.

   The new code attempts to order the frames according to the
   specification as much as possible without knowing the entire frame
   tree, without skipping any documents managed by the `ScriptThread` in
   question.

In addition, `Documents` is pulled out the `script_thread.rs` and
renamed to `DocumentCollection`.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>

components/script/devtools.rs

@@ -16,6 +16,7 @@ use js::jsval::UndefinedValue;
 use js::rust::ToString;
 use uuid::Uuid;
 
+use crate::document_collection::DocumentCollection;
 use crate::dom::bindings::codegen::Bindings::CSSRuleListBinding::CSSRuleListMethods;
 use crate::dom::bindings::codegen::Bindings::CSSStyleDeclarationBinding::CSSStyleDeclarationMethods;
 use crate::dom::bindings::codegen::Bindings::CSSStyleRuleBinding::CSSStyleRuleMethods;
@@ -41,7 +42,6 @@ use crate::dom::types::HTMLElement;
 use crate::realms::enter_realm;
 use crate::script_module::ScriptFetchOptions;
 use crate::script_runtime::CanGc;
-use crate::script_thread::Documents;
 
 #[allow(unsafe_code)]
 pub fn handle_evaluate_js(
@@ -98,7 +98,7 @@ pub fn handle_evaluate_js(
 }
 
 pub fn handle_get_root_node(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Option<NodeInfo>>,
 ) {
@@ -109,7 +109,7 @@ pub fn handle_get_root_node(
 }
 
 pub fn handle_get_document_element(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Option<NodeInfo>>,
 ) {
@@ -121,7 +121,7 @@ pub fn handle_get_document_element(
 }
 
 fn find_node_by_unique_id(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: &str,
 ) -> Option<DomRoot<Node>> {
@@ -134,7 +134,7 @@ fn find_node_by_unique_id(
 }
 
 pub fn handle_get_children(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Option<Vec<NodeInfo>>>,
@@ -186,7 +186,7 @@ pub fn handle_get_children(
 }
 
 pub fn handle_get_attribute_style(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Option<Vec<NodeStyle>>>,
@@ -218,7 +218,7 @@ pub fn handle_get_attribute_style(
 
 #[allow(crown::unrooted_must_root)]
 pub fn handle_get_stylesheet_style(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     selector: String,
@@ -265,7 +265,7 @@ pub fn handle_get_stylesheet_style(
 
 #[allow(crown::unrooted_must_root)]
 pub fn handle_get_selectors(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Option<Vec<(String, usize)>>>,
@@ -301,7 +301,7 @@ pub fn handle_get_selectors(
 }
 
 pub fn handle_get_computed_style(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Option<Vec<NodeStyle>>>,
@@ -335,7 +335,7 @@ pub fn handle_get_computed_style(
 }
 
 pub fn handle_get_layout(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Option<ComputedNodeLayout>>,
@@ -396,7 +396,7 @@ fn determine_auto_margins(node: &Node, can_gc: CanGc) -> AutoMargins {
 }
 
 pub fn handle_modify_attribute(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     modifications: Vec<AttrModification>,
@@ -436,7 +436,7 @@ pub fn handle_modify_attribute(
 }
 
 pub fn handle_modify_rule(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     modifications: Vec<RuleModification>,
@@ -474,7 +474,7 @@ pub fn handle_wants_live_notifications(global: &GlobalScope, send_notifications:
 }
 
 pub fn handle_set_timeline_markers(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     marker_types: Vec<TimelineMarkerType>,
     reply: IpcSender<Option<TimelineMarker>>,
@@ -486,7 +486,7 @@ pub fn handle_set_timeline_markers(
 }
 
 pub fn handle_drop_timeline_markers(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     marker_types: Vec<TimelineMarkerType>,
 ) {
@@ -495,13 +495,17 @@ pub fn handle_drop_timeline_markers(
     }
 }
 
-pub fn handle_request_animation_frame(documents: &Documents, id: PipelineId, actor_name: String) {
+pub fn handle_request_animation_frame(
+    documents: &DocumentCollection,
+    id: PipelineId,
+    actor_name: String,
+) {
     if let Some(doc) = documents.find_document(id) {
         doc.request_animation_frame(AnimationFrameCallback::DevtoolsFramerateTick { actor_name });
     }
 }
 
-pub fn handle_reload(documents: &Documents, id: PipelineId, can_gc: CanGc) {
+pub fn handle_reload(documents: &DocumentCollection, id: PipelineId, can_gc: CanGc) {
     if let Some(win) = documents.find_window(id) {
         win.Location().reload_without_origin_check(can_gc);
     }

components/script/document_collection.rs

@@ -0,0 +1,177 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+use std::collections::{hash_map, HashMap};
+
+use base::id::{BrowsingContextId, PipelineId};
+
+use crate::dom::bindings::inheritance::Castable;
+use crate::dom::bindings::root::{Dom, DomRoot};
+use crate::dom::bindings::trace::HashMapTracedValues;
+use crate::dom::document::Document;
+use crate::dom::globalscope::GlobalScope;
+use crate::dom::htmliframeelement::HTMLIFrameElement;
+use crate::dom::window::Window;
+
+/// The collection of all [`Document`]s managed by the [`crate::script_thread::SriptThread`].
+/// This is stored as a mapping of [`PipelineId`] to [`Document`], but for updating the
+/// rendering, [`Document`]s should be processed in order via [`Self::documents_in_order`].
+#[derive(JSTraceable)]
+#[crown::unrooted_must_root_lint::must_root]
+pub(crate) struct DocumentCollection {
+    map: HashMapTracedValues<PipelineId, Dom<Document>>,
+}
+
+impl DocumentCollection {
+    pub fn insert(&mut self, pipeline_id: PipelineId, doc: &Document) {
+        self.map.insert(pipeline_id, Dom::from_ref(doc));
+    }
+
+    pub fn remove(&mut self, pipeline_id: PipelineId) -> Option<DomRoot<Document>> {
+        self.map
+            .remove(&pipeline_id)
+            .map(|ref doc| DomRoot::from_ref(&**doc))
+    }
+
+    pub fn find_document(&self, pipeline_id: PipelineId) -> Option<DomRoot<Document>> {
+        self.map
+            .get(&pipeline_id)
+            .map(|doc| DomRoot::from_ref(&**doc))
+    }
+
+    pub fn find_window(&self, pipeline_id: PipelineId) -> Option<DomRoot<Window>> {
+        self.find_document(pipeline_id)
+            .map(|doc| DomRoot::from_ref(doc.window()))
+    }
+
+    pub fn find_global(&self, pipeline_id: PipelineId) -> Option<DomRoot<GlobalScope>> {
+        self.find_window(pipeline_id)
+            .map(|window| DomRoot::from_ref(window.upcast()))
+    }
+
+    pub fn find_iframe(
+        &self,
+        pipeline_id: PipelineId,
+        browsing_context_id: BrowsingContextId,
+    ) -> Option<DomRoot<HTMLIFrameElement>> {
+        self.find_document(pipeline_id)
+            .and_then(|doc| doc.find_iframe(browsing_context_id))
+    }
+
+    pub fn iter(&self) -> DocumentsIter<'_> {
+        DocumentsIter {
+            iter: self.map.iter(),
+        }
+    }
+
+    /// Return the documents managed by this [`crate::script_thread::ScriptThread`] in the
+    /// order specified by the *[update the rendering][update-the-rendering]* step of the
+    /// HTML specification:
+    ///
+    /// > Let docs be all fully active Document objects whose relevant agent's event loop is
+    /// > eventLoop, sorted arbitrarily except that the following conditions must be met:
+    /// >
+    /// > Any Document B whose container document is A must be listed after A in the list.
+    /// >
+    /// > If there are two documents A and B that both have the same non-null container
+    /// > document C, then the order of A and B in the list must match the shadow-including
+    /// > tree order of their respective navigable containers in C's node tree.
+    /// >
+    /// > In the steps below that iterate over docs, each Document must be processed in the
+    /// > order it is found in the list.
+    ///
+    /// [update-the-rendering]: https://html.spec.whatwg.org/multipage/#update-the-rendering
+    pub(crate) fn documents_in_order(&self) -> Vec<PipelineId> {
+        // TODO: This is a fairly expensive operation, because iterating iframes requires walking
+        // the *entire* DOM for a document. Instead this should be cached and marked as dirty when
+        // the DOM of a document changes or documents are added or removed from our set.
+        DocumentTree::new(self).documents_in_order()
+    }
+}
+
+impl Default for DocumentCollection {
+    #[allow(crown::unrooted_must_root)]
+    fn default() -> Self {
+        Self {
+            map: HashMapTracedValues::new(),
+        }
+    }
+}
+
+#[allow(crown::unrooted_must_root)]
+pub struct DocumentsIter<'a> {
+    iter: hash_map::Iter<'a, PipelineId, Dom<Document>>,
+}
+
+impl<'a> Iterator for DocumentsIter<'a> {
+    type Item = (PipelineId, DomRoot<Document>);
+
+    fn next(&mut self) -> Option<(PipelineId, DomRoot<Document>)> {
+        self.iter
+            .next()
+            .map(|(id, doc)| (*id, DomRoot::from_ref(&**doc)))
+    }
+}
+
+#[derive(Default)]
+struct DocumentTreeNode {
+    parent: Option<PipelineId>,
+    children: Vec<PipelineId>,
+}
+
+/// A tree representation of [`Document`]s managed by the [`ScriptThread`][st], which is used
+/// to generate an ordered set of [`Document`]s for the *update the rendering* step of the
+/// HTML5 specification.
+///
+/// FIXME: The [`ScriptThread`][st] only has a view of [`Document`]s managed by itself,
+/// so if there are interceding iframes managed by other `ScriptThread`s, then the
+/// order of the [`Document`]s may not be correct. Perhaps the Constellation could
+/// ensure that every [`ScriptThread`][st] has the full view of the frame tree.
+///
+/// [st]: crate::script_thread::ScriptThread
+#[derive(Default)]
+struct DocumentTree {
+    tree: HashMap<PipelineId, DocumentTreeNode>,
+}
+
+impl DocumentTree {
+    fn new(documents: &DocumentCollection) -> Self {
+        let mut tree = DocumentTree::default();
+        for (id, document) in documents.iter() {
+            let children: Vec<PipelineId> = document
+                .iter_iframes()
+                .filter_map(|iframe| iframe.pipeline_id())
+                .filter(|iframe_pipeline_id| documents.find_document(*iframe_pipeline_id).is_some())
+                .collect();
+            for child in &children {
+                tree.tree.entry(*child).or_default().parent = Some(id);
+            }
+            tree.tree.entry(id).or_default().children = children;
+        }
+        tree
+    }
+
+    fn documents_in_order(&self) -> Vec<PipelineId> {
+        let mut list = Vec::new();
+        for (id, node) in self.tree.iter() {
+            if node.parent.is_none() {
+                self.process_node_for_documents_in_order(*id, &mut list);
+            }
+        }
+        list
+    }
+
+    fn process_node_for_documents_in_order(&self, id: PipelineId, list: &mut Vec<PipelineId>) {
+        list.push(id);
+        for child in self
+            .tree
+            .get(&id)
+            .expect("Should have found child node")
+            .children
+            .iter()
+        {
+            self.process_node_for_documents_in_order(*child, list);
+        }
+    }
+}

components/script/dom/document.rs

@@ -2107,6 +2107,7 @@ impl Document {
 
     /// <https://html.spec.whatwg.org/multipage/#run-the-animation-frame-callbacks>
     pub(crate) fn run_the_animation_frame_callbacks(&self, can_gc: CanGc) {
+        let _realm = enter_realm(self);
         rooted_vec!(let mut animation_frame_list);
         mem::swap(
             &mut *animation_frame_list,

components/script/dom/window.rs

@@ -1983,6 +1983,7 @@ impl Window {
         let mut issued_reflow = false;
         let condition = self.Document().needs_reflow();
         if !for_display || condition.is_some() {
+            debug!("Reflowing document ({:?})", self.pipeline_id());
             issued_reflow = self.force_reflow(reflow_goal, reason, condition);
 
             // We shouldn't need a reflow immediately after a
@@ -2000,8 +2001,8 @@ impl Window {
             );
         } else {
             debug!(
-                "Document doesn't need reflow - skipping it (reason {:?})",
+                "Document ({:?}) doesn't need reflow - skipping it (reason {reason:?})",
-                reason
+                self.pipeline_id()
             );
         }
 

components/script/lib.rs

@@ -51,6 +51,8 @@ mod init;
 #[warn(deprecated)]
 mod layout_image;
 
+#[warn(deprecated)]
+pub mod document_collection;
 pub mod layout_dom;
 #[warn(deprecated)]
 mod mem;

components/script/script_thread.rs

@@ -19,7 +19,7 @@
 
 use std::borrow::Cow;
 use std::cell::{Cell, RefCell};
-use std::collections::{hash_map, HashMap, HashSet};
+use std::collections::{HashMap, HashSet};
 use std::default::Default;
 use std::option::Option;
 use std::rc::Rc;
@@ -99,6 +99,7 @@ use webgpu::{WebGPUDevice, WebGPUMsg};
 use webrender_api::DocumentId;
 use webrender_traits::CrossProcessCompositorApi;
 
+use crate::document_collection::DocumentCollection;
 use crate::document_loader::DocumentLoader;
 use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::DocumentBinding::{
@@ -443,81 +444,6 @@ impl OpaqueSender<CommonScriptMsg> for Sender<MainThreadScriptMsg> {
         self.send(MainThreadScriptMsg::Common(msg)).unwrap()
     }
 }
-
-/// The set of all documents managed by this script thread.
-#[derive(JSTraceable)]
-#[crown::unrooted_must_root_lint::must_root]
-pub struct Documents {
-    map: HashMapTracedValues<PipelineId, Dom<Document>>,
-}
-
-impl Documents {
-    pub fn insert(&mut self, pipeline_id: PipelineId, doc: &Document) {
-        self.map.insert(pipeline_id, Dom::from_ref(doc));
-    }
-
-    pub fn remove(&mut self, pipeline_id: PipelineId) -> Option<DomRoot<Document>> {
-        self.map
-            .remove(&pipeline_id)
-            .map(|ref doc| DomRoot::from_ref(&**doc))
-    }
-
-    pub fn find_document(&self, pipeline_id: PipelineId) -> Option<DomRoot<Document>> {
-        self.map
-            .get(&pipeline_id)
-            .map(|doc| DomRoot::from_ref(&**doc))
-    }
-
-    pub fn find_window(&self, pipeline_id: PipelineId) -> Option<DomRoot<Window>> {
-        self.find_document(pipeline_id)
-            .map(|doc| DomRoot::from_ref(doc.window()))
-    }
-
-    pub fn find_global(&self, pipeline_id: PipelineId) -> Option<DomRoot<GlobalScope>> {
-        self.find_window(pipeline_id)
-            .map(|window| DomRoot::from_ref(window.upcast()))
-    }
-
-    pub fn find_iframe(
-        &self,
-        pipeline_id: PipelineId,
-        browsing_context_id: BrowsingContextId,
-    ) -> Option<DomRoot<HTMLIFrameElement>> {
-        self.find_document(pipeline_id)
-            .and_then(|doc| doc.find_iframe(browsing_context_id))
-    }
-
-    pub fn iter(&self) -> DocumentsIter<'_> {
-        DocumentsIter {
-            iter: self.map.iter(),
-        }
-    }
-}
-
-impl Default for Documents {
-    #[allow(crown::unrooted_must_root)]
-    fn default() -> Self {
-        Self {
-            map: HashMapTracedValues::new(),
-        }
-    }
-}
-
-#[allow(crown::unrooted_must_root)]
-pub struct DocumentsIter<'a> {
-    iter: hash_map::Iter<'a, PipelineId, Dom<Document>>,
-}
-
-impl<'a> Iterator for DocumentsIter<'a> {
-    type Item = (PipelineId, DomRoot<Document>);
-
-    fn next(&mut self) -> Option<(PipelineId, DomRoot<Document>)> {
-        self.iter
-            .next()
-            .map(|(id, doc)| (*id, DomRoot::from_ref(&**doc)))
-    }
-}
-
 // We borrow the incomplete parser contexts mutably during parsing,
 // which is fine except that parsing can trigger evaluation,
 // which can trigger GC, and so we can end up tracing the script
@@ -537,7 +463,7 @@ pub struct ScriptThread {
     #[no_trace]
     update_the_rendering_task_queued_for_pipeline: DomRefCell<HashSet<PipelineId>>,
     /// The documents for pipelines managed by this thread
-    documents: DomRefCell<Documents>,
+    documents: DomRefCell<DocumentCollection>,
     /// The window proxies known by this thread
     /// TODO: this map grows, but never shrinks. Issue #15258.
     window_proxies: DomRefCell<HashMapTracedValues<BrowsingContextId, Dom<WindowProxy>>>,
@@ -1274,7 +1200,7 @@ impl ScriptThread {
         let control_port = ROUTER.route_ipc_receiver_to_new_crossbeam_receiver(state.control_port);
 
         ScriptThread {
-            documents: DomRefCell::new(Documents::default()),
+            documents: DomRefCell::new(DocumentCollection::default()),
             last_render_opportunity_time: Default::default(),
             update_the_rendering_task_queued_for_pipeline: Default::default(),
             window_proxies: DomRefCell::new(HashMapTracedValues::new()),
@@ -1594,13 +1520,15 @@ impl ScriptThread {
     }
 
     /// <https://html.spec.whatwg.org/multipage/#update-the-rendering>
-    fn update_the_rendering(&self, can_gc: CanGc) {
+    ///
+    /// Returns true if the rendering was actually updated.
+    fn update_the_rendering(&self, requested_by_compositor: bool, can_gc: CanGc) -> bool {
         self.update_the_rendering_task_queued_for_pipeline
             .borrow_mut()
             .clear();
 
         if !self.can_continue_running_inner() {
-            return;
+            return false;
         }
 
         // Run rafs for all pipeline, if a raf tick was received for any.
@@ -1611,34 +1539,47 @@ impl ScriptThread {
             .iter()
             .any(|(_, doc)| doc.has_received_raf_tick());
 
-        // TODO: The specification says to filter out non-renderable documents,
+        let any_animations_running = self
-        // as well as those for which a rendering update would be unnecessary,
-        // but this isn't happening here.
-        let pipelines_to_update: Vec<PipelineId> = self
             .documents
             .borrow()
             .iter()
-            .filter(|(_, document)| document.window().is_top_level())
+            .any(|(_, document)| document.animations().running_animation_count() != 0);
-            .flat_map(|(id, document)| {
+
-                std::iter::once(id).chain(
+        // TODO: The specification says to filter out non-renderable documents,
-                    document
+        // as well as those for which a rendering update would be unnecessary,
-                        .iter_iframes()
+        // but this isn't happening here.
-                        .filter_map(|iframe| iframe.pipeline_id()),
+
-                )
+        // If we aren't explicitly running rAFs, this update wasn't requested by the compositor,
-            })
+        // and we are running animations, then wait until the compositor tells us it is time to
-            .collect();
+        // update the rendering via a TickAllAnimations message.
+        if !requested_by_compositor && any_animations_running {
+            return false;
+        }
+
+        // > 2. Let docs be all fully active Document objects whose relevant agent's event loop
+        // > is eventLoop, sorted arbitrarily except that the following conditions must be
+        // > met:
+        //
+        // > Any Document B whose container document is A must be listed after A in the
+        // > list.
+        //
+        // > If there are two documents A and B that both have the same non-null container
+        // > document C, then the order of A and B in the list must match the
+        // > shadow-including tree order of their respective navigable containers in C's
+        // > node tree.
+        //
+        // > In the steps below that iterate over docs, each Document must be processed in
+        // > the order it is found in the list.
+        let documents_in_order = self.documents.borrow().documents_in_order();
+
         // Note: the spec reads: "for doc in docs" at each step
         // whereas this runs all steps per doc in docs.
-        for pipeline_id in pipelines_to_update {
+        for pipeline_id in documents_in_order {
-            // This document is not managed by this script thread. This can happen is the pipeline is
+            let document = self
-            // unexpectedly closed or simply that it is managed by a different script thread.
+                .documents
-            // TODO: It would be better if iframes knew whether or not their Document was managed
+                .borrow()
-            // by the same script thread.
+                .find_document(pipeline_id)
-            let Some(document) = self.documents.borrow().find_document(pipeline_id) else {
+                .expect("Got pipeline for Document not managed by this ScriptThread.");
-                continue;
-            };
-            // TODO(#32004): The rendering should be updated according parent and shadow root order
-            // in the specification, but this isn't happening yet.
 
             // TODO(#31581): The steps in the "Revealing the document" section need to be implemente
             // `process_pending_compositor_events` handles the focusing steps as well as other events
@@ -1708,9 +1649,19 @@ impl ScriptThread {
 
             // TODO(#31871): Update the rendering: consolidate all reflow calls into one here?
 
+            // > Step 22: For each doc of docs, update the rendering or user interface of
+            // > doc and its node navigable to reflect the current state.
+            let window = document.window();
+            let pending_reflows = window.get_pending_reflow_count();
+            if document.is_fully_active() && pending_reflows > 0 {
+                window.reflow(ReflowGoal::Full, ReflowReason::PendingReflow, can_gc);
+            }
+
             // TODO: Process top layer removals according to
             // https://drafts.csswg.org/css-position-4/#process-top-layer-removals.
         }
+
+        true
     }
 
     /// <https://html.spec.whatwg.org/multipage/#event-loop-processing-model:rendering-opportunity>
@@ -1744,7 +1695,7 @@ impl ScriptThread {
                 // Note: spec says to queue a task using the navigable's active window,
                 // but then updates the rendering for all docs in the same event-loop.
                 with_script_thread(|script_thread| {
-                    script_thread.update_the_rendering(CanGc::note());
+                    script_thread.update_the_rendering(false, CanGc::note());
                 })
             }),
             &canceller,
@@ -1798,13 +1749,14 @@ impl ScriptThread {
                 }
             },
         };
-        debug!("Got event.");
-
         // Prioritize only a single update of the rendering;
         // others will run together with the other sequential tasks.
         let mut rendering_update_already_prioritized = false;
+        let mut compositor_requested_update_the_rendering = false;
 
         loop {
+            debug!("Handling event: {event:?}");
+
             let pipeline_id = self.message_to_pipeline(&event);
             let _realm = pipeline_id.map(|id| {
                 let global = self.documents.borrow().find_global(id);
@@ -1863,9 +1815,9 @@ impl ScriptThread {
                     pipeline_id,
                     tick_type,
                 )) => {
-                    if let Some(doc) = self.documents.borrow().find_document(pipeline_id) {
+                    if let Some(document) = self.documents.borrow().find_document(pipeline_id) {
-                        self.rendering_opportunity(pipeline_id);
+                        document.note_pending_animation_tick(tick_type);
-                        doc.note_pending_animation_tick(tick_type);
+                        compositor_requested_update_the_rendering = true;
                     } else {
                         warn!(
                             "Trying to note pending animation tick for closed pipeline {}.",
@@ -1975,11 +1927,11 @@ impl ScriptThread {
                 continue;
             }
 
-            let result = self.profile_event(category, pipeline_id, move || {
+            let exiting = self.profile_event(category, pipeline_id, move || {
                 match msg {
                     FromConstellation(ConstellationControlMsg::ExitScriptThread) => {
                         self.handle_exit_script_thread_msg(can_gc);
-                        return Some(false);
+                        return true;
                     },
                     FromConstellation(inner_msg) => {
                         self.handle_msg_from_constellation(inner_msg, can_gc)
@@ -1993,11 +1945,12 @@ impl ScriptThread {
                     },
                 }
 
-                None
+                false
             });
 
-            if let Some(retval) = result {
+            // If an `ExitScriptThread` message was handled above, bail out now.
-                return retval;
+            if exiting {
+                return false;
             }
 
             // https://html.spec.whatwg.org/multipage/#event-loop-processing-model step 6
@@ -2018,39 +1971,13 @@ impl ScriptThread {
             docs.clear();
         }
 
-        // https://html.spec.whatwg.org/multipage/#event-loop-processing-model step 7.12
+        // Update the rendering whenever we receive an IPC message. This may not actually do anything if
-
+        // we are running animations and the compositor hasn't requested a new frame yet via a TickAllAnimatons
-        // Issue batched reflows on any pages that require it (e.g. if images loaded)
+        // message.
-        // TODO(gw): In the future we could probably batch other types of reflows
+        if self.update_the_rendering(compositor_requested_update_the_rendering, can_gc) {
-        // into this loop too, but for now it's only images.
+            // Perform a microtask checkpoint as the specifications says that *update the rendering* should be
-        debug!("Issuing batched reflows.");
+            // run in a task and a microtask checkpoint is always done when running tasks.
-        for (_, document) in self.documents.borrow().iter() {
+            self.perform_a_microtask_checkpoint(can_gc);
-            // Step 13
-            if !document.is_fully_active() {
-                continue;
-            }
-            let window = document.window();
-
-            let _realm = enter_realm(&*document);
-
-            window
-                .upcast::<GlobalScope>()
-                .perform_a_dom_garbage_collection_checkpoint();
-
-            let pending_reflows = window.get_pending_reflow_count();
-            if pending_reflows > 0 {
-                window.reflow(ReflowGoal::Full, ReflowReason::PendingReflow, can_gc);
-            } else {
-                // Reflow currently happens when explicitly invoked by code that
-                // knows the document could have been modified. This should really
-                // be driven by the compositor on an as-needed basis instead, to
-                // minimize unnecessary work.
-                window.reflow(
-                    ReflowGoal::Full,
-                    ReflowReason::MissingExplicitReflow,
-                    can_gc,
-                );
-            }
         }
 
         true

components/script/webdriver_handlers.rs

@@ -25,6 +25,7 @@ use servo_url::ServoUrl;
 use webdriver::common::{WebElement, WebFrame, WebWindow};
 use webdriver::error::ErrorStatus;
 
+use crate::document_collection::DocumentCollection;
 use crate::dom::bindings::codegen::Bindings::CSSStyleDeclarationBinding::CSSStyleDeclarationMethods;
 use crate::dom::bindings::codegen::Bindings::DOMRectBinding::DOMRectMethods;
 use crate::dom::bindings::codegen::Bindings::DocumentBinding::DocumentMethods;
@@ -61,10 +62,10 @@ use crate::dom::xmlserializer::XMLSerializer;
 use crate::realms::enter_realm;
 use crate::script_module::ScriptFetchOptions;
 use crate::script_runtime::{CanGc, JSContext as SafeJSContext};
-use crate::script_thread::{Documents, ScriptThread};
+use crate::script_thread::ScriptThread;
 
 fn find_node_by_unique_id(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
 ) -> Result<DomRoot<Node>, ErrorStatus> {
@@ -346,7 +347,7 @@ pub fn handle_execute_async_script(
 }
 
 pub fn handle_get_browsing_context_id(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     webdriver_frame_id: WebDriverFrameId,
     reply: IpcSender<Result<BrowsingContextId, ErrorStatus>>,
@@ -412,7 +413,7 @@ fn get_element_in_view_center_point(element: &Element, can_gc: CanGc) -> Option<
 }
 
 pub fn handle_get_element_in_view_center_point(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<Option<(i64, i64)>, ErrorStatus>>,
@@ -429,7 +430,7 @@ pub fn handle_get_element_in_view_center_point(
 }
 
 pub fn handle_find_element_css(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     reply: IpcSender<Result<Option<String>, ErrorStatus>>,
@@ -450,7 +451,7 @@ pub fn handle_find_element_css(
 }
 
 pub fn handle_find_element_link_text(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     partial: bool,
@@ -469,7 +470,7 @@ pub fn handle_find_element_link_text(
 }
 
 pub fn handle_find_element_tag_name(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     reply: IpcSender<Result<Option<String>, ErrorStatus>>,
@@ -491,7 +492,7 @@ pub fn handle_find_element_tag_name(
 }
 
 pub fn handle_find_elements_css(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     reply: IpcSender<Result<Vec<String>, ErrorStatus>>,
@@ -517,7 +518,7 @@ pub fn handle_find_elements_css(
 }
 
 pub fn handle_find_elements_link_text(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     partial: bool,
@@ -536,7 +537,7 @@ pub fn handle_find_elements_link_text(
 }
 
 pub fn handle_find_elements_tag_name(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     selector: String,
     reply: IpcSender<Result<Vec<String>, ErrorStatus>>,
@@ -558,7 +559,7 @@ pub fn handle_find_elements_tag_name(
 }
 
 pub fn handle_find_element_element_css(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -576,7 +577,7 @@ pub fn handle_find_element_element_css(
 }
 
 pub fn handle_find_element_element_link_text(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -592,7 +593,7 @@ pub fn handle_find_element_element_link_text(
 }
 
 pub fn handle_find_element_element_tag_name(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -615,7 +616,7 @@ pub fn handle_find_element_element_tag_name(
 }
 
 pub fn handle_find_element_elements_css(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -638,7 +639,7 @@ pub fn handle_find_element_elements_css(
 }
 
 pub fn handle_find_element_elements_link_text(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -654,7 +655,7 @@ pub fn handle_find_element_elements_link_text(
 }
 
 pub fn handle_find_element_elements_tag_name(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     selector: String,
@@ -677,7 +678,7 @@ pub fn handle_find_element_elements_tag_name(
 }
 
 pub fn handle_focus_element(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<(), ErrorStatus>>,
@@ -700,7 +701,7 @@ pub fn handle_focus_element(
 }
 
 pub fn handle_get_active_element(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Option<String>>,
 ) {
@@ -715,7 +716,7 @@ pub fn handle_get_active_element(
 }
 
 pub fn handle_get_page_source(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Result<String, ErrorStatus>>,
     can_gc: CanGc,
@@ -744,7 +745,7 @@ pub fn handle_get_page_source(
 }
 
 pub fn handle_get_cookies(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Vec<Serde<Cookie<'static>>>>,
 ) {
@@ -770,7 +771,7 @@ pub fn handle_get_cookies(
 
 // https://w3c.github.io/webdriver/webdriver-spec.html#get-cookie
 pub fn handle_get_cookie(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     name: String,
     reply: IpcSender<Vec<Serde<Cookie<'static>>>>,
@@ -801,7 +802,7 @@ pub fn handle_get_cookie(
 
 // https://w3c.github.io/webdriver/webdriver-spec.html#add-cookie
 pub fn handle_add_cookie(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     cookie: Cookie<'static>,
     reply: IpcSender<Result<(), WebDriverCookieError>>,
@@ -848,7 +849,7 @@ pub fn handle_add_cookie(
 }
 
 pub fn handle_delete_cookies(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     reply: IpcSender<Result<(), ErrorStatus>>,
 ) {
@@ -868,7 +869,11 @@ pub fn handle_delete_cookies(
     reply.send(Ok(())).unwrap();
 }
 
-pub fn handle_get_title(documents: &Documents, pipeline: PipelineId, reply: IpcSender<String>) {
+pub fn handle_get_title(
+    documents: &DocumentCollection,
+    pipeline: PipelineId,
+    reply: IpcSender<String>,
+) {
     reply
         .send(
             // TODO: Return an error if the pipeline doesn't exist
@@ -881,7 +886,7 @@ pub fn handle_get_title(documents: &Documents, pipeline: PipelineId, reply: IpcS
 }
 
 pub fn handle_get_rect(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<Rect<f64>, ErrorStatus>>,
@@ -927,7 +932,7 @@ pub fn handle_get_rect(
 }
 
 pub fn handle_get_bounding_client_rect(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<Rect<f32>, ErrorStatus>>,
@@ -952,7 +957,7 @@ pub fn handle_get_bounding_client_rect(
 }
 
 pub fn handle_get_text(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Result<String, ErrorStatus>>,
@@ -966,7 +971,7 @@ pub fn handle_get_text(
 }
 
 pub fn handle_get_name(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     reply: IpcSender<Result<String, ErrorStatus>>,
@@ -980,7 +985,7 @@ pub fn handle_get_name(
 }
 
 pub fn handle_get_attribute(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     name: String,
@@ -1000,7 +1005,7 @@ pub fn handle_get_attribute(
 
 #[allow(unsafe_code)]
 pub fn handle_get_property(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     name: String,
@@ -1039,7 +1044,7 @@ pub fn handle_get_property(
 }
 
 pub fn handle_get_css(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     node_id: String,
     name: String,
@@ -1061,7 +1066,11 @@ pub fn handle_get_css(
         .unwrap();
 }
 
-pub fn handle_get_url(documents: &Documents, pipeline: PipelineId, reply: IpcSender<ServoUrl>) {
+pub fn handle_get_url(
+    documents: &DocumentCollection,
+    pipeline: PipelineId,
+    reply: IpcSender<ServoUrl>,
+) {
     reply
         .send(
             // TODO: Return an error if the pipeline doesn't exist.
@@ -1075,7 +1084,7 @@ pub fn handle_get_url(documents: &Documents, pipeline: PipelineId, reply: IpcSen
 
 // https://w3c.github.io/webdriver/#element-click
 pub fn handle_element_click(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<Option<String>, ErrorStatus>>,
@@ -1172,7 +1181,7 @@ pub fn handle_element_click(
 }
 
 pub fn handle_is_enabled(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<bool, ErrorStatus>>,
@@ -1190,7 +1199,7 @@ pub fn handle_is_enabled(
 }
 
 pub fn handle_is_selected(
-    documents: &Documents,
+    documents: &DocumentCollection,
     pipeline: PipelineId,
     element_id: String,
     reply: IpcSender<Result<bool, ErrorStatus>>,