Hoist StyleNew{Children,Subtree} into their own paths.

The buggy animation handling isn't a regression, since currently we pass
UnstyledChildrenOnly in those cases, which blocks the animation traversal
in Servo_TraverseSubtree.

In general I really wanted to handle these two paths together. But there's
enough broken with the NewChildren path that I wanted to scope the buginess
as tightly as possible. And I really need to separate the handling here from
StyleDocument() to make the restyle root stuff work.

MozReview-Commit-ID: 9F0mcQl7AAX

components/style/traversal.rs

@@ -31,23 +31,12 @@ pub struct PerLevelTraversalData {
     pub current_dom_depth: usize,
 }
 
-/// This structure exists to enforce that callers invoke pre_traverse, and also
-/// to pass information from the pre-traversal into the primary traversal.
-pub struct PreTraverseToken {
-    traverse: bool,
-    unstyled_children_only: bool,
-}
-
+/// We use this structure, rather than just returning a boolean from pre_traverse,
+/// to enfore that callers process root invalidations before starting the traversal.
+pub struct PreTraverseToken(bool);
 impl PreTraverseToken {
     /// Whether we should traverse children.
-    pub fn should_traverse(&self) -> bool {
-        self.traverse
-    }
-
-    /// Whether we should traverse only unstyled children.
-    pub fn traverse_unstyled_children_only(&self) -> bool {
-        self.unstyled_children_only
-    }
+    pub fn should_traverse(&self) -> bool { self.0 }
 }
 
 /// The kind of traversals we could perform.
@@ -157,33 +146,20 @@ pub trait DomTraversal<E: TElement> : Sync {
         }
     }
 
-    /// Must be invoked before traversing the root element to determine whether
-    /// a traversal is needed. Returns a token that allows the caller to prove
-    /// that the call happened.
-    ///
-    /// The traversal_flags is used in Gecko.
-    ///
-    /// If traversal_flag::UNSTYLED_CHILDREN_ONLY is specified, style newly-
-    /// appended children without restyling the parent.
-    ///
-    /// If traversal_flag::ANIMATION_ONLY is specified, style only elements for
-    /// animations.
+    /// Style invalidations happen when traversing from a parent to its children.
+    /// However, this mechanism can't handle style invalidations on the root. As
+    /// such, we have a pre-traversal step to handle that part and determine whether
+    /// a full traversal is needed.
     fn pre_traverse(
         root: E,
         shared_context: &SharedStyleContext,
         traversal_flags: TraversalFlags
     ) -> PreTraverseToken {
-        if traversal_flags.contains(traversal_flags::UnstyledChildrenOnly) {
-            if root.borrow_data().map_or(true, |d| d.has_styles() && d.styles.is_display_none()) {
-                return PreTraverseToken {
-                    traverse: false,
-                    unstyled_children_only: false,
-                };
-            }
-            return PreTraverseToken {
-                traverse: true,
-                unstyled_children_only: true,
-            };
+        // If this is an unstyled-only traversal, the caller has already verified
+        // that there's something to traverse, and we don't need to do any
+        // invalidation since we're not doing any restyling.
+        if traversal_flags.contains(traversal_flags::UnstyledOnly) {
+            return PreTraverseToken(true)
         }
 
         let flags = shared_context.traversal_flags;
@@ -205,10 +181,7 @@ pub trait DomTraversal<E: TElement> : Sync {
             parent_data.as_ref().map(|d| &**d)
         );
 
-        PreTraverseToken {
-            traverse: should_traverse,
-            unstyled_children_only: false,
-        }
+        PreTraverseToken(should_traverse)
     }
 
     /// Returns true if traversal should visit a text node. The style system
@@ -231,16 +204,32 @@ pub trait DomTraversal<E: TElement> : Sync {
     ) -> bool {
         debug!("element_needs_traversal({:?}, {:?}, {:?}, {:?})",
                el, traversal_flags, data, parent_data);
-        let data = match data {
-            Some(d) if d.has_styles() => d,
-            _ => return !traversal_flags.for_animation_only(),
-        };
+
+        if traversal_flags.contains(traversal_flags::UnstyledOnly) {
+            return data.map_or(true, |d| !d.has_styles()) || el.has_dirty_descendants();
+        }
+
+
+        // In case of animation-only traversal we need to traverse the element
+        // if the element has animation only dirty descendants bit,
+        // animation-only restyle hint or recascade.
+        if traversal_flags.for_animation_only() {
+            return data.map_or(false, |d| d.has_styles()) &&
+                   (el.has_animation_only_dirty_descendants() ||
+                    data.as_ref().unwrap().restyle.hint.has_animation_hint_or_recascade());
+        }
 
         // Non-incremental layout visits every node.
         if is_servo_nonincremental_layout() {
             return true;
         }
 
+        // Unwrap the data.
+        let data = match data {
+            Some(d) if d.has_styles() => d,
+            _ => 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...
@@ -283,14 +272,6 @@ pub trait DomTraversal<E: TElement> : Sync {
             }
         }
 
-        // In case of animation-only traversal we need to traverse the element
-        // if the element has animation only dirty descendants bit,
-        // animation-only restyle hint or recascade.
-        if traversal_flags.for_animation_only() {
-            return el.has_animation_only_dirty_descendants() ||
-                   data.restyle.hint.has_animation_hint_or_recascade();
-        }
-
         // If the dirty descendants bit is set, we need to traverse no matter
         // what. Skip examining the ElementData.
         if el.has_dirty_descendants() {
@@ -488,7 +469,7 @@ where
     let flags = context.shared.traversal_flags;
     context.thread_local.begin_element(element, data);
     context.thread_local.statistics.elements_traversed += 1;
-    debug_assert!(flags.for_animation_only() ||
+    debug_assert!(flags.intersects(AnimationOnly | UnstyledOnly) ||
                   !element.has_snapshot() || element.handled_snapshot(),
                   "Should've handled snapshots here already");
 
@@ -535,8 +516,12 @@ where
     }
 
     // Now that matching and cascading is done, clear the bits corresponding to
-    // those operations and compute the propagated restyle hint.
-    let mut propagated_hint = {
+    // those operations and compute the propagated restyle hint (unless we're
+    // not processing invalidations, in which case don't need to propagate it
+    // and must avoid clearing it).
+    let mut propagated_hint = if flags.contains(UnstyledOnly) {
+        RestyleHint::empty()
+    } else {
         debug_assert!(flags.for_animation_only() ||
                       !data.restyle.hint.has_animation_hint(),
                       "animation restyle hint should be handled during \