Track the restyle root and use it to do less work during the traversal.

MozReview-Commit-ID: A8O3JOpsv4E

components/style/dom.rs

@@ -501,13 +501,20 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
     unsafe fn unset_animation_only_dirty_descendants(&self) {
     }
 
-    /// Clear all bits related to dirty descendant.
+    /// Clear all bits related describing the dirtiness of descendants.
     ///
     /// In Gecko, this corresponds to the regular dirty descendants bit, the
     /// animation-only dirty descendants bit, and the lazy frame construction
     /// descendants bit.
     unsafe fn clear_descendants_bits(&self) { self.unset_dirty_descendants(); }
 
+    /// Clear all element flags related to dirtiness.
+    ///
+    /// In Gecko, this corresponds to the regular dirty descendants bit, the
+    /// animation-only dirty descendants bit, the lazy frame construction bit,
+    /// and the lazy frame construction descendants bit.
+    unsafe fn clear_dirty_bits(&self) { self.unset_dirty_descendants(); }
+
     /// Returns true if this element is a visited link.
     ///
     /// Servo doesn't support visited styles yet.
@@ -718,28 +725,6 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
                           -> bool;
 }
 
-/// Trait abstracting over different kinds of dirty-descendants bits.
-pub trait DescendantsBit<E: TElement> {
-    /// Returns true if the Element has the bit.
-    fn has(el: E) -> bool;
-    /// Sets the bit on the Element.
-    unsafe fn set(el: E);
-}
-
-/// Implementation of DescendantsBit for the regular dirty descendants bit.
-pub struct DirtyDescendants;
-impl<E: TElement> DescendantsBit<E> for DirtyDescendants {
-    fn has(el: E) -> bool { el.has_dirty_descendants() }
-    unsafe fn set(el: E) { el.set_dirty_descendants(); }
-}
-
-/// Implementation of DescendantsBit for the animation-only dirty descendants bit.
-pub struct AnimationOnlyDirtyDescendants;
-impl<E: TElement> DescendantsBit<E> for AnimationOnlyDirtyDescendants {
-    fn has(el: E) -> bool { el.has_animation_only_dirty_descendants() }
-    unsafe fn set(el: E) { el.set_animation_only_dirty_descendants(); }
-}
-
 /// TNode and TElement aren't Send because we want to be careful and explicit
 /// about our parallel traversal. However, there are certain situations
 /// (including but not limited to the traversal) where we need to send DOM

components/style/gecko/data.rs

@@ -152,7 +152,7 @@ impl PerDocumentStyleDataImpl {
         &mut self,
         guard: &SharedRwLockReadGuard,
         document_element: Option<E>,
-    )
+    ) -> bool
     where
         E: TElement,
     {

components/style/gecko/wrapper.rs

@@ -61,6 +61,7 @@ use gecko_bindings::structs::ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO;
 use gecko_bindings::structs::ELEMENT_HAS_SNAPSHOT;
 use gecko_bindings::structs::EffectCompositor_CascadeLevel as CascadeLevel;
 use gecko_bindings::structs::NODE_DESCENDANTS_NEED_FRAMES;
+use gecko_bindings::structs::NODE_NEEDS_FRAME;
 use gecko_bindings::structs::nsChangeHint;
 use gecko_bindings::structs::nsIDocument_DocumentTheme as DocumentTheme;
 use gecko_bindings::structs::nsRestyleHint;
@@ -493,6 +494,33 @@ impl<'le> GeckoElement<'le> {
         unsafe { Gecko_UnsetNodeFlags(self.as_node().0, flags) }
     }
 
+    /// Returns true if this element has descendants for lazy frame construction.
+    pub fn descendants_need_frames(&self) -> bool {
+        self.flags() & (NODE_DESCENDANTS_NEED_FRAMES  as u32) != 0
+    }
+
+    /// Returns true if this element needs lazy frame construction.
+    pub fn needs_frame(&self) -> bool {
+        self.flags() & (NODE_NEEDS_FRAME as u32) != 0
+    }
+
+    /// Returns true if a traversal starting from this element requires a post-traversal.
+    pub fn needs_post_traversal(&self) -> bool {
+        debug!("needs_post_traversal: dd={}, aodd={}, lfcd={}, lfc={}, restyle={:?}",
+               self.has_dirty_descendants(),
+               self.has_animation_only_dirty_descendants(),
+               self.descendants_need_frames(),
+               self.needs_frame(),
+               self.borrow_data().unwrap().restyle);
+
+        let has_flag =
+            self.flags() & (ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                            ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                            NODE_DESCENDANTS_NEED_FRAMES as u32 |
+                            NODE_NEEDS_FRAME as u32) != 0;
+        has_flag || self.borrow_data().unwrap().restyle.contains_restyle_data()
+    }
+
     /// Returns true if this element has a shadow root.
     fn has_shadow_root(&self) -> bool {
         self.get_extended_slots().map_or(false, |slots| !slots.mShadowRoot.mRawPtr.is_null())
@@ -1070,6 +1098,13 @@ impl<'le> TElement for GeckoElement<'le> {
     }
 
     #[inline]
+    unsafe fn clear_dirty_bits(&self) {
+        self.unset_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                         ELEMENT_HAS_ANIMATION_ONLY_DIRTY_DESCENDANTS_FOR_SERVO as u32 |
+                         NODE_DESCENDANTS_NEED_FRAMES as u32 |
+                         NODE_NEEDS_FRAME as u32)
+    }
+
     fn is_visited_link(&self) -> bool {
         use element_state::IN_VISITED_STATE;
         self.get_state().intersects(IN_VISITED_STATE)

components/style/invalidation/stylesheets.rs

@@ -117,13 +117,17 @@ impl StylesheetInvalidationSet {
 
     /// Clears the invalidation set, invalidating elements as needed if
     /// `document_element` is provided.
-    pub fn flush<E>(&mut self, document_element: Option<E>)
+    ///
+    /// Returns true if any invalidations ocurred.
+    pub fn flush<E>(&mut self, document_element: Option<E>) -> bool
         where E: TElement,
     {
-        if let Some(e) = document_element {
+        let have_invalidations = match document_element {
-            self.process_invalidations(e);
+            Some(e) => self.process_invalidations(e),
-        }
+            None => false,
+        };
         self.clear();
+        have_invalidations
     }
 
     /// Clears the invalidation set without processing.

components/style/stylesheet_set.rs

@@ -182,25 +182,28 @@ where
 
     /// Flush the current set, unmarking it as dirty, and returns an iterator
     /// over the new stylesheet list.
+    ///
+    /// Returns true if any elements were invalidated.
     pub fn flush<E>(
         &mut self,
         document_element: Option<E>,
-    ) -> (StylesheetIterator<S>, OriginSet)
+    ) -> (StylesheetIterator<S>, OriginSet, bool)
     where
         E: TElement,
     {
         debug!("StylesheetSet::flush");
 
         let mut origins = OriginSet::empty();
+        let mut have_invalidations = false;
         for (data, origin) in self.invalidation_data.iter_mut_origins() {
             if data.dirty {
-                data.invalidations.flush(document_element);
+                have_invalidations |= data.invalidations.flush(document_element);
                 data.dirty = false;
                 origins |= origin;
             }
         }
 
-        (self.iter(), origins)
+        (self.iter(), origins, have_invalidations)
     }
 
     /// Flush stylesheets, but without running any of the invalidation passes.

components/style/stylist.rs

@@ -481,19 +481,20 @@ impl Stylist {
         ua_sheets: Option<&UserAgentStylesheets>,
         extra_data: &mut PerOrigin<ExtraStyleData>,
         document_element: Option<E>,
-    )
+    ) -> bool
     where
         E: TElement,
     {
         if !self.stylesheets.has_changed() {
-            return;
+            return false;
         }
 
         let author_style_disabled = self.stylesheets.author_style_disabled();
-        let (doc_stylesheets, origins_to_rebuild) = self.stylesheets.flush(document_element);
+        let (doc_stylesheets, origins_to_rebuild, have_invalidations) =
+            self.stylesheets.flush(document_element);
 
         if origins_to_rebuild.is_empty() {
-            return;
+            return have_invalidations;
         }
 
         self.num_rebuilds += 1;
@@ -538,6 +539,8 @@ impl Stylist {
             extra_data,
             origins_to_rebuild,
         );
+
+        have_invalidations
     }
 
     /// Insert a given stylesheet before another stylesheet in the document.

components/style/traversal.rs

@@ -185,10 +185,16 @@ pub trait DomTraversal<E: TElement> : Sync {
         let should_traverse = Self::element_needs_traversal(
             root,
             flags,
-            data.map(|d| &*d),
+            data.as_mut().map(|d| &**d),
             parent_data.as_ref().map(|d| &**d)
         );
 
+        // If we're not going to traverse at all, we may need to clear some state
+        // off the root (which would normally be done at the end of recalc_style_at).
+        if !should_traverse && data.is_some() {
+            clear_state_after_traversing(root, data.unwrap(), flags);
+        }
+
         PreTraverseToken(should_traverse)
     }
 
@@ -592,6 +598,31 @@ where
         );
     }
 
+    // FIXME(bholley): Make these assertions pass for servo.
+    if cfg!(feature = "gecko") && cfg!(debug_assertions) && data.styles.is_display_none() {
+        debug_assert!(!element.has_dirty_descendants());
+        debug_assert!(!element.has_animation_only_dirty_descendants());
+    }
+
+    debug_assert!(flags.for_animation_only() ||
+                  !flags.contains(ClearDirtyBits) ||
+                  !element.has_animation_only_dirty_descendants(),
+                  "Should have cleared animation bits already");
+    clear_state_after_traversing(element, data, flags);
+
+    context.thread_local.end_element(element);
+}
+
+fn clear_state_after_traversing<E>(
+    element: E,
+    data: &mut ElementData,
+    flags: TraversalFlags
+)
+where
+    E: TElement,
+{
+    use traversal_flags::*;
+
     // If we are in a forgetful traversal, drop the existing restyle
     // data here, since we won't need to perform a post-traversal to pick up
     // any change hints.
@@ -599,32 +630,16 @@ where
         data.clear_restyle_flags_and_damage();
     }
 
-    // Optionally clear the descendants bits.
+    // Clear dirty bits as appropriate.
-    if data.styles.is_display_none() {
+    if flags.for_animation_only() {
-        // When this element is the root of a display:none subtree, we want to clear
+        if flags.intersects(ClearDirtyBits | ClearAnimationOnlyDirtyDescendants) {
-        // the bits even if the style didn't change (since, if the style did change,
-        // we'd have already cleared it above).
-        //
-        // This keeps the tree in a valid state without requiring the DOM to check
-        // display:none on the parent when inserting new children (which can be
-        // moderately expensive). Instead, DOM implementations can unconditionally
-        // set the dirty descendants bit on any styled parent, and let the traversal
-        // sort it out.
-        //
-        // Note that the NODE_DESCENDANTS_NEED_FRAMES bit should generally only be set
-        // when appending content beneath an element with a frame (i.e. not
-        // display:none), so clearing it here isn't strictly necessary, but good
-        // belt-and-suspenders.
-        unsafe { element.clear_descendants_bits(); }
-    } else if flags.for_animation_only() {
-        if flags.contains(ClearAnimationOnlyDirtyDescendants) {
             unsafe { element.unset_animation_only_dirty_descendants(); }
         }
-    } else if flags.contains(ClearDirtyDescendants) {
+    } else if flags.contains(ClearDirtyBits) {
-        unsafe { element.unset_dirty_descendants(); }
+        // The animation traversal happens first, so we don't need to guard against
+        // clearing the animation bit on the regular traversal.
+        unsafe { element.clear_dirty_bits(); }
     }
-
-    context.thread_local.end_element(element);
 }
 
 fn compute_style<E>(
@@ -855,6 +870,11 @@ where
                 }
             }
         }
+        if p == root {
+            // Make sure not to clear NODE_NEEDS_FRAME on the root.
             p.clear_descendants_bits();
+        } else {
+            p.clear_dirty_bits();
+        }
     }
 }

components/style/traversal_flags.rs

@@ -27,8 +27,8 @@ bitflags! {
         /// Actively seeks out and clears change hints that may have been posted into
         /// the tree. Nonsensical without also passing Forgetful.
         const AggressivelyForgetful = 1 << 4,
-        /// Clears the dirty descendants bit in the subtree.
+        /// Clears all the dirty bits on the elements traversed.
-        const ClearDirtyDescendants = 1 << 5,
+        const ClearDirtyBits = 1 << 5,
         /// Clears the animation-only dirty descendants bit in the subtree.
         const ClearAnimationOnlyDirtyDescendants = 1 << 6,
         /// Allows the traversal to run in parallel if there are sufficient cores on
@@ -67,7 +67,7 @@ pub fn assert_traversal_flags_match() {
         ServoTraversalFlags_UnstyledOnly => UnstyledOnly,
         ServoTraversalFlags_Forgetful => Forgetful,
         ServoTraversalFlags_AggressivelyForgetful => AggressivelyForgetful,
-        ServoTraversalFlags_ClearDirtyDescendants => ClearDirtyDescendants,
+        ServoTraversalFlags_ClearDirtyBits => ClearDirtyBits,
         ServoTraversalFlags_ClearAnimationOnlyDirtyDescendants =>
             ClearAnimationOnlyDirtyDescendants,
         ServoTraversalFlags_ParallelTraversal => ParallelTraversal,

ports/geckolib/glue.rs

@@ -25,6 +25,7 @@ use style::gecko::restyle_damage::GeckoRestyleDamage;
 use style::gecko::selector_parser::PseudoElement;
 use style::gecko::traversal::RecalcStyleOnly;
 use style::gecko::wrapper::GeckoElement;
+use style::gecko_bindings::bindings;
 use style::gecko_bindings::bindings::{RawGeckoElementBorrowed, RawGeckoElementBorrowedOrNull};
 use style::gecko_bindings::bindings::{RawGeckoKeyframeListBorrowed, RawGeckoKeyframeListBorrowedMut};
 use style::gecko_bindings::bindings::{RawServoDeclarationBlockBorrowed, RawServoDeclarationBlockStrong};
@@ -291,14 +292,14 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
                      traversal_flags,
                      unsafe { &*snapshots });
 
-    debug!("Servo_TraverseSubtree complete (dd={}, aodd={}, restyle={:?})",
+    debug!("Servo_TraverseSubtree complete (dd={}, aodd={}, lfcd={}, lfc={}, restyle={:?})",
            element.has_dirty_descendants(),
            element.has_animation_only_dirty_descendants(),
+           element.descendants_need_frames(),
+           element.needs_frame(),
            element.borrow_data().unwrap().restyle);
 
-    element.has_dirty_descendants() ||
+    element.needs_post_traversal()
-    element.has_animation_only_dirty_descendants() ||
-    element.borrow_data().unwrap().restyle.contains_restyle_data()
 }
 
 /// Checks whether the rule tree has crossed its threshold for unused nodes, and
@@ -826,6 +827,14 @@ pub extern "C" fn Servo_Element_GetPseudoComputedValues(element: RawGeckoElement
         .clone().into()
 }
 
+#[no_mangle]
+pub extern "C" fn Servo_Element_IsDisplayNone(element: RawGeckoElementBorrowed) -> bool
+{
+    let element = GeckoElement(element);
+    let data = element.borrow_data().expect("Invoking Servo_Element_IsDisplayNone on unstyled element");
+    data.styles.is_display_none()
+}
+
 #[no_mangle]
 pub extern "C" fn Servo_StyleSheet_Empty(mode: SheetParsingMode) -> RawServoStyleSheetContentsStrong {
     let global_style_data = &*GLOBAL_STYLE_DATA;
@@ -991,7 +1000,12 @@ pub extern "C" fn Servo_StyleSet_FlushStyleSheets(
     let guard = global_style_data.shared_lock.read();
     let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let doc_element = doc_element.map(GeckoElement);
-    data.flush_stylesheets(&guard, doc_element);
+    let have_invalidations = data.flush_stylesheets(&guard, doc_element);
+    if have_invalidations && doc_element.is_some() {
+        // The invalidation machinery propagates the bits up, but we still
+        // need to tell the gecko restyle root machinery about it.
+        unsafe { bindings::Gecko_NoteDirtyElement(doc_element.unwrap().0); }
+    }
 }
 
 #[no_mangle]