Auto merge of #19164 - emilio:more-traversal-cleanup, r=nox

style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->

components/script/dom/document.rs

@@ -573,8 +573,14 @@ impl Document {
         self.encoding.set(encoding);
     }
 
-    pub fn content_and_heritage_changed(&self, node: &Node, damage: NodeDamage) {
+    pub fn content_and_heritage_changed(&self, node: &Node) {
-        node.dirty(damage);
+        if node.is_in_doc() {
+            node.note_dirty_descendants();
+        }
+
+        // FIXME(emilio): This is very inefficient, ideally the flag above would
+        // be enough and incremental layout could figure out from there.
+        node.dirty(NodeDamage::OtherNodeDamage);
     }
 
     /// Reflows and disarms the timer if the reflow timer has expired.

components/script/dom/node.rs

@@ -483,6 +483,19 @@ impl Node {
         self.flags.set(flags);
     }
 
+    // FIXME(emilio): This and the function below should move to Element.
+    pub fn note_dirty_descendants(&self) {
+        debug_assert!(self.is_in_doc());
+
+        for ancestor in self.inclusive_ancestors() {
+            if ancestor.get_flag(NodeFlags::HAS_DIRTY_DESCENDANTS) {
+                return;
+            }
+
+            ancestor.set_flag(NodeFlags::HAS_DIRTY_DESCENDANTS, true);
+        }
+    }
+
     pub fn has_dirty_descendants(&self) -> bool {
         self.get_flag(NodeFlags::HAS_DIRTY_DESCENDANTS)
     }
@@ -2499,7 +2512,7 @@ impl VirtualMethods for Node {
         if let Some(list) = self.child_list.get() {
             list.as_children_list().children_changed(mutation);
         }
-        self.owner_doc().content_and_heritage_changed(self, NodeDamage::OtherNodeDamage);
+        self.owner_doc().content_and_heritage_changed(self);
     }
 
     // This handles the ranges mentioned in steps 2-3 when removing a node.

components/style/context.rs

@@ -5,11 +5,11 @@
 //! The context within which style is calculated.
 
 #[cfg(feature = "servo")] use animation::Animation;
-#[cfg(feature = "servo")] use animation::PropertyAnimation;
 use app_units::Au;
 use bloom::StyleBloom;
 use data::{EagerPseudoStyles, ElementData};
-use dom::{OpaqueNode, TNode, TElement, SendElement};
+use dom::{TElement, SendElement};
+#[cfg(feature = "servo")] use dom::OpaqueNode;
 use euclid::ScaleFactor;
 use euclid::Size2D;
 use fnv::FnvHashMap;
@@ -285,23 +285,6 @@ impl ElementCascadeInputs {
     }
 }
 
-/// Information about the current element being processed. We group this
-/// together into a single struct within ThreadLocalStyleContext so that we can
-/// instantiate and destroy it easily at the beginning and end of element
-/// processing.
-pub struct CurrentElementInfo {
-    /// The element being processed. Currently we use an OpaqueNode since we
-    /// only use this for identity checks, but we could use SendElement if there
-    /// were a good reason to.
-    element: OpaqueNode,
-    /// Whether the element is being styled for the first time.
-    is_initial_style: bool,
-    /// A Vec of possibly expired animations. Used only by Servo.
-    #[allow(dead_code)]
-    #[cfg(feature = "servo")]
-    pub possibly_expired_animations: Vec<PropertyAnimation>,
-}
-
 /// Statistics gathered during the traversal. We gather statistics on each
 /// thread and then combine them after the threads join via the Add
 /// implementation below.
@@ -712,8 +695,6 @@ pub struct ThreadLocalStyleContext<E: TElement> {
     pub selector_flags: SelectorFlagsMap<E>,
     /// Statistics about the traversal.
     pub statistics: TraversalStatistics,
-    /// Information related to the current element, non-None during processing.
-    pub current_element_info: Option<CurrentElementInfo>,
     /// The struct used to compute and cache font metrics from style
     /// for evaluation of the font-relative em/ch units and font-size
     pub font_metrics_provider: E::FontMetricsProvider,
@@ -736,7 +717,6 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
             tasks: SequentialTaskList(Vec::new()),
             selector_flags: SelectorFlagsMap::new(),
             statistics: TraversalStatistics::default(),
-            current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
             stack_limit_checker: StackLimitChecker::new(
                 (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024),
@@ -754,55 +734,16 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
             tasks: SequentialTaskList(Vec::new()),
             selector_flags: SelectorFlagsMap::new(),
             statistics: TraversalStatistics::default(),
-            current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
             stack_limit_checker: StackLimitChecker::new(
                 (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024),
             nth_index_cache: NthIndexCache::default(),
         }
     }
-
-    #[cfg(feature = "gecko")]
-    /// Notes when the style system starts traversing an element.
-    pub fn begin_element(&mut self, element: E, data: &ElementData) {
-        debug_assert!(self.current_element_info.is_none());
-        self.current_element_info = Some(CurrentElementInfo {
-            element: element.as_node().opaque(),
-            is_initial_style: !data.has_styles(),
-        });
-    }
-
-    #[cfg(feature = "servo")]
-    /// Notes when the style system starts traversing an element.
-    pub fn begin_element(&mut self, element: E, data: &ElementData) {
-        debug_assert!(self.current_element_info.is_none());
-        self.current_element_info = Some(CurrentElementInfo {
-            element: element.as_node().opaque(),
-            is_initial_style: !data.has_styles(),
-            possibly_expired_animations: Vec::new(),
-        });
-    }
-
-    /// Notes when the style system finishes traversing an element.
-    pub fn end_element(&mut self, element: E) {
-        debug_assert!(self.current_element_info.is_some());
-        debug_assert!(self.current_element_info.as_ref().unwrap().element ==
-                      element.as_node().opaque());
-        self.current_element_info = None;
-    }
-
-    /// Returns true if the current element being traversed is being styled for
-    /// the first time.
-    ///
-    /// Panics if called while no element is being traversed.
-    pub fn is_initial_style(&self) -> bool {
-        self.current_element_info.as_ref().unwrap().is_initial_style
-    }
 }
 
 impl<E: TElement> Drop for ThreadLocalStyleContext<E> {
     fn drop(&mut self) {
-        debug_assert!(self.current_element_info.is_none());
         debug_assert!(thread_state::get() == ThreadState::LAYOUT);
 
         // Apply any slow selector flags that need to be set on parents.

components/style/data.rs

@@ -38,15 +38,16 @@ bitflags! {
         /// traversed, so each traversal simply updates it with the appropriate
         /// value.
         const TRAVERSED_WITHOUT_STYLING = 1 << 1;
-        /// Whether we reframed/reconstructed any ancestor or self.
+
-        const ANCESTOR_WAS_RECONSTRUCTED = 1 << 2;
+        /// Whether the primary style of this element data was reused from
-        /// Whether the primary style of this element data was reused from another
+        /// another element via a rule node comparison. This allows us to
-        /// element via a rule node comparison. This allows us to differentiate
+        /// differentiate between elements that shared styles because they met
-        /// between elements that shared styles because they met all the criteria
+        /// all the criteria of the style sharing cache, compared to elements
-        /// of the style sharing cache, compared to elements that reused style
+        /// that reused style structs via rule node identity.
-        /// structs via rule node identity. The former gives us stronger transitive
+        ///
-        /// guarantees that allows us to apply the style sharing cache to cousins.
+        /// The former gives us stronger transitive guarantees that allows us to
-        const PRIMARY_STYLE_REUSED_VIA_RULE_NODE = 1 << 3;
+        /// apply the style sharing cache to cousins.
+        const PRIMARY_STYLE_REUSED_VIA_RULE_NODE = 1 << 2;
     }
 }
 
@@ -405,13 +406,7 @@ impl ElementData {
     #[inline]
     pub fn clear_restyle_flags_and_damage(&mut self) {
         self.damage = RestyleDamage::empty();
-        self.flags.remove(ElementDataFlags::WAS_RESTYLED | ElementDataFlags::ANCESTOR_WAS_RECONSTRUCTED)
+        self.flags.remove(ElementDataFlags::WAS_RESTYLED);
-    }
-
-    /// Returns whether this element or any ancestor is going to be
-    /// reconstructed.
-    pub fn reconstructed_self_or_ancestor(&self) -> bool {
-        self.reconstructed_ancestor() || self.reconstructed_self()
     }
 
     /// Returns whether this element is going to be reconstructed.
@@ -419,23 +414,6 @@ impl ElementData {
         self.damage.contains(RestyleDamage::reconstruct())
     }
 
-    /// Returns whether any ancestor of this element is going to be
-    /// reconstructed.
-    fn reconstructed_ancestor(&self) -> bool {
-        self.flags.contains(ElementDataFlags::ANCESTOR_WAS_RECONSTRUCTED)
-    }
-
-    /// Sets the flag that tells us whether we've reconstructed an ancestor.
-    pub fn set_reconstructed_ancestor(&mut self, reconstructed: bool) {
-        if reconstructed {
-            // If it weren't for animation-only traversals, we could assert
-            // `!self.reconstructed_ancestor()` here.
-            self.flags.insert(ElementDataFlags::ANCESTOR_WAS_RECONSTRUCTED);
-        } else {
-            self.flags.remove(ElementDataFlags::ANCESTOR_WAS_RECONSTRUCTED);
-        }
-    }
-
     /// Mark this element as restyled, which is useful to know whether we need
     /// to do a post-traversal.
     pub fn set_restyled(&mut self) {

components/style/matching.rs

@@ -13,6 +13,7 @@ use data::ElementData;
 use dom::TElement;
 use invalidation::element::restyle_hints::RestyleHint;
 use properties::ComputedValues;
+use properties::longhands::display::computed_value as display;
 use rule_tree::{CascadeLevel, StrongRuleNode};
 use selector_parser::{PseudoElement, RestyleDamage};
 use selectors::matching::ElementSelectorFlags;
@@ -142,8 +143,6 @@ trait PrivateMatchMethods: TElement {
         old_values: Option<&Arc<ComputedValues>>,
         new_values: &ComputedValues,
     ) -> bool {
-        use properties::longhands::display::computed_value as display;
-
         let new_box_style = new_values.get_box();
         let has_new_animation_style = new_box_style.specifies_animations();
         let has_animations = self.has_css_animations();
@@ -182,7 +181,6 @@ trait PrivateMatchMethods: TElement {
         restyle_hints: RestyleHint
     ) {
         use context::PostAnimationTasks;
-        use properties::longhands::display::computed_value as display;
 
         if !restyle_hints.intersects(RestyleHint::RESTYLE_SMIL) {
             return;
@@ -298,13 +296,11 @@ trait PrivateMatchMethods: TElement {
         use animation;
         use dom::TNode;
 
-        let possibly_expired_animations =
+        let mut possibly_expired_animations = vec![];
-            &mut context.thread_local.current_element_info.as_mut().unwrap()
-                        .possibly_expired_animations;
         let shared_context = context.shared;
         if let Some(ref mut old) = *old_values {
             self.update_animations_for_cascade(shared_context, old,
-                                               possibly_expired_animations,
+                                               &mut possibly_expired_animations,
                                                &context.thread_local.font_metrics_provider);
         }
 
@@ -367,6 +363,16 @@ trait PrivateMatchMethods: TElement {
                 let old_display = old_values.get_box().clone_display();
                 let new_display = new_values.get_box().clone_display();
 
+                // If we used to be a display: none element, and no longer are,
+                // our children need to be restyled because they're unstyled.
+                //
+                // NOTE(emilio): Gecko has the special-case of -moz-binding, but
+                // that gets handled on the frame constructor when processing
+                // the reframe, so no need to handle that here.
+                if old_display == display::T::none && old_display != new_display {
+                    return ChildCascadeRequirement::MustCascadeChildren
+                }
+
                 // Blockification of children may depend on our display value,
                 // so we need to actually do the recascade. We could potentially
                 // do better, but it doesn't seem worth it.

components/style/traversal.rs

@@ -159,13 +159,6 @@ pub trait DomTraversal<E: TElement> : Sync {
         let parent_data = parent.as_ref().and_then(|p| p.borrow_data());
 
         if let Some(ref mut data) = data {
-            // Make sure we don't have any stale RECONSTRUCTED_ANCESTOR bits
-            // from the last traversal (at a potentially-higher root).
-            //
-            // From the perspective of this traversal, the root cannot have
-            // reconstructed ancestors.
-            data.set_reconstructed_ancestor(false);
-
             if !traversal_flags.for_animation_only() {
                 // Invalidate our style, and that of our siblings and
                 // descendants as needed.
@@ -247,48 +240,6 @@ pub trait DomTraversal<E: TElement> : Sync {
             _ => return true,
         };
 
-        // If the element is native-anonymous and an ancestor frame will be
-        // reconstructed, the child and all its descendants will be destroyed.
-        // In that case, we wouldn't need to traverse the subtree...
-        //
-        // Except if there could be transitions of pseudo-elements, in which
-        // case we still need to process them, unfortunately.
-        //
-        // We need to conservatively continue the traversal to style the
-        // pseudo-element in order to properly process potentially-new
-        // transitions that we won't see otherwise.
-        //
-        // But it may be that we no longer match, so detect that case and act
-        // appropriately here.
-        if el.is_native_anonymous() {
-            if let Some(parent_data) = parent_data {
-                let going_to_reframe =
-                    parent_data.reconstructed_self_or_ancestor();
-
-                let mut is_before_or_after_pseudo = false;
-                if let Some(pseudo) = el.implemented_pseudo_element() {
-                    if pseudo.is_before_or_after() {
-                        is_before_or_after_pseudo = true;
-                        let still_match =
-                            parent_data.styles.pseudos.get(&pseudo).is_some();
-
-                        if !still_match {
-                            debug_assert!(going_to_reframe,
-                                          "We're removing a pseudo, so we \
-                                           should reframe!");
-                            return false;
-                        }
-                    }
-                }
-
-                if going_to_reframe && !is_before_or_after_pseudo {
-                    debug!("Element {:?} is in doomed NAC subtree, \
-                            culling traversal", el);
-                    return false;
-                }
-            }
-        }
-
         // If the dirty descendants bit is set, we need to traverse no matter
         // what. Skip examining the ElementData.
         if el.has_dirty_descendants() {
@@ -322,6 +273,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         &self,
         context: &mut StyleContext<E>,
         parent: E,
+        is_initial_style: bool,
         parent_data: &ElementData,
     ) -> bool {
         debug_assert!(cfg!(feature = "gecko") ||
@@ -353,7 +305,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         // happens, we may just end up doing wasted work, since Gecko
         // recursively drops Servo ElementData when the XBL insertion parent of
         // an Element is changed.
-        if cfg!(feature = "gecko") && context.thread_local.is_initial_style() &&
+        if cfg!(feature = "gecko") && is_initial_style &&
             parent_data.styles.primary().has_moz_binding()
         {
             debug!("Parent {:?} has XBL binding, deferring traversal", parent);
@@ -476,7 +428,8 @@ where
     use traversal_flags::TraversalFlags;
 
     let flags = context.shared.traversal_flags;
-    context.thread_local.begin_element(element, data);
+    let is_initial_style = !data.has_styles();
+
     context.thread_local.statistics.elements_traversed += 1;
     debug_assert!(flags.intersects(TraversalFlags::AnimationOnly | TraversalFlags::UnstyledOnly) ||
                   !element.has_snapshot() || element.handled_snapshot(),
@@ -555,26 +508,24 @@ where
     // Before examining each child individually, try to prove that our children
     // don't need style processing. They need processing if any of the following
     // conditions hold:
-    // * We have the dirty descendants bit.
+    //
-    // * We're propagating a hint.
+    //  * We have the dirty descendants bit.
-    // * This is the initial style.
+    //  * We're propagating a restyle hint.
-    // * We generated a reconstruct hint on self (which could mean that we
+    //  * We can't skip the cascade.
-    //   switched from display:none to something else, which means the children
+    //  * This is a servo non-incremental traversal.
-    //   need initial styling).
-    // * This is a servo non-incremental traversal.
     //
     // Additionally, there are a few scenarios where we avoid traversing the
     // subtree even if descendant styles are out of date. These cases are
     // enumerated in should_cull_subtree().
-    let mut traverse_children = has_dirty_descendants_for_this_restyle ||
+    let mut traverse_children =
-                                !propagated_hint.is_empty() ||
+        has_dirty_descendants_for_this_restyle ||
-                                !child_cascade_requirement.can_skip_cascade() ||
+        !propagated_hint.is_empty() ||
-                                context.thread_local.is_initial_style() ||
+        !child_cascade_requirement.can_skip_cascade() ||
-                                data.reconstructed_self() ||
+        is_servo_nonincremental_layout();
-                                is_servo_nonincremental_layout();
 
-    traverse_children = traverse_children &&
+    traverse_children =
-                        !traversal.should_cull_subtree(context, element, &data);
+        traverse_children &&
+        !traversal.should_cull_subtree(context, element, is_initial_style, &data);
 
     // Examine our children, and enqueue the appropriate ones for traversal.
     if traverse_children {
@@ -584,7 +535,7 @@ where
             data,
             propagated_hint,
             child_cascade_requirement,
-            data.reconstructed_self_or_ancestor(),
+            is_initial_style,
             note_child
         );
     }
@@ -600,8 +551,6 @@ where
                   !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>(
@@ -826,7 +775,7 @@ fn note_children<E, D, F>(
     data: &ElementData,
     propagated_hint: RestyleHint,
     cascade_requirement: ChildCascadeRequirement,
-    reconstructed_ancestor: bool,
+    is_initial_style: bool,
     mut note_child: F,
 )
 where
@@ -836,7 +785,6 @@ where
 {
     trace!("note_children: {:?}", element);
     let flags = context.shared.traversal_flags;
-    let is_initial_style = context.thread_local.is_initial_style();
 
     // Loop over all the traversal children.
     for child_node in element.traversal_children() {
@@ -866,10 +814,6 @@ where
         }
 
         if let Some(ref mut child_data) = child_data {
-            // Propagate the parent restyle hint, that may make us restyle the whole
-            // subtree.
-            child_data.set_reconstructed_ancestor(reconstructed_ancestor);
-
             let mut child_hint = propagated_hint;
             match cascade_requirement {
                 ChildCascadeRequirement::CanSkipCascade => {}