style: Refactor children handling.

Moving traversal_children away from TNode I can make TNode trivial enough, in
order to share a QuerySelector implementation between Servo and Gecko.

components/layout_thread/dom_wrapper.rs

@@ -68,7 +68,7 @@ use style::attr::AttrValue;
 use style::computed_values::display;
 use style::context::SharedStyleContext;
 use style::data::ElementData;
-use style::dom::{LayoutIterator, NodeInfo, OpaqueNode};
+use style::dom::{DomChildren, LayoutIterator, NodeInfo, OpaqueNode};
 use style::dom::{PresentationalHintsSynthesizer, TElement, TNode};
 use style::element_state::*;
 use style::font_metrics::ServoMetricsProvider;
@@ -159,7 +159,6 @@ impl<'ln> NodeInfo for ServoLayoutNode<'ln> {
 
 impl<'ln> TNode for ServoLayoutNode<'ln> {
     type ConcreteElement = ServoLayoutElement<'ln>;
-    type ConcreteChildrenIterator = ServoChildrenIterator<'ln>;
 
     fn parent_node(&self) -> Option<Self> {
         unsafe {
@@ -167,20 +166,34 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
         }
     }
 
-    fn children(&self) -> LayoutIterator<ServoChildrenIterator<'ln>> {
-        LayoutIterator(ServoChildrenIterator {
-            current: self.first_child(),
-        })
+    fn first_child(&self) -> Option<Self> {
+        unsafe {
+            self.node.first_child_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
+    }
+
+    fn last_child(&self) -> Option<Self> {
+        unsafe {
+            self.node.last_child_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
+    }
+
+    fn prev_sibling(&self) -> Option<Self> {
+        unsafe {
+            self.node.prev_sibling_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
+    }
+
+    fn next_sibling(&self) -> Option<Self> {
+        unsafe {
+            self.node.next_sibling_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
     }
 
     fn traversal_parent(&self) -> Option<ServoLayoutElement<'ln>> {
         self.parent_element()
     }
 
-    fn traversal_children(&self) -> LayoutIterator<ServoChildrenIterator<'ln>> {
-        self.children()
-    }
-
     fn opaque(&self) -> OpaqueNode {
         unsafe { self.get_jsmanaged().opaque() }
     }
@@ -206,19 +219,6 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
     }
 }
 
-pub struct ServoChildrenIterator<'a> {
-    current: Option<ServoLayoutNode<'a>>,
-}
-
-impl<'a> Iterator for ServoChildrenIterator<'a> {
-    type Item = ServoLayoutNode<'a>;
-    fn next(&mut self) -> Option<ServoLayoutNode<'a>> {
-        let node = self.current;
-        self.current = node.and_then(|node| node.next_sibling());
-        node
-    }
-}
-
 impl<'ln> LayoutNode for ServoLayoutNode<'ln> {
     type ConcreteThreadSafeLayoutNode = ServoThreadSafeLayoutNode<'ln>;
 
@@ -248,30 +248,6 @@ impl<'ln> LayoutNode for ServoLayoutNode<'ln> {
     unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData {
         self.get_jsmanaged().take_style_and_layout_data()
     }
-
-    fn first_child(&self) -> Option<ServoLayoutNode<'ln>> {
-        unsafe {
-            self.node.first_child_ref().map(|node| self.new_with_this_lifetime(&node))
-        }
-    }
-
-    fn last_child(&self) -> Option<ServoLayoutNode<'ln>> {
-        unsafe {
-            self.node.last_child_ref().map(|node| self.new_with_this_lifetime(&node))
-        }
-    }
-
-    fn prev_sibling(&self) -> Option<ServoLayoutNode<'ln>> {
-        unsafe {
-            self.node.prev_sibling_ref().map(|node| self.new_with_this_lifetime(&node))
-        }
-    }
-
-    fn next_sibling(&self) -> Option<ServoLayoutNode<'ln>> {
-        unsafe {
-            self.node.next_sibling_ref().map(|node| self.new_with_this_lifetime(&node))
-        }
-    }
 }
 
 impl<'ln> GetLayoutData for ServoLayoutNode<'ln> {
@@ -320,8 +296,8 @@ impl<'ld> ServoLayoutDocument<'ld> {
         ServoLayoutNode::from_layout_js(self.document.upcast())
     }
 
-    pub fn root_node(&self) -> Option<ServoLayoutNode<'ld>> {
-        self.as_node().children().find(ServoLayoutNode::is_element)
+    pub fn root_element(&self) -> Option<ServoLayoutElement<'ld>> {
+        self.as_node().dom_children().flat_map(|n| n.as_element()).next()
     }
 
     pub fn drain_pending_restyles(&self) -> Vec<(ServoLayoutElement<'ld>, PendingRestyle)> {
@@ -380,6 +356,7 @@ impl<'le> PresentationalHintsSynthesizer for ServoLayoutElement<'le> {
 
 impl<'le> TElement for ServoLayoutElement<'le> {
     type ConcreteNode = ServoLayoutNode<'le>;
+    type TraversalChildrenIterator = DomChildren<Self::ConcreteNode>;
 
     type FontMetricsProvider = ServoMetricsProvider;
 
@@ -387,6 +364,10 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         ServoLayoutNode::from_layout_js(self.element.upcast())
     }
 
+    fn traversal_children(&self) -> LayoutIterator<Self::TraversalChildrenIterator> {
+        LayoutIterator(self.as_node().dom_children())
+    }
+
     fn style_attribute(&self) -> Option<ArcBorrow<StyleLocked<PropertyDeclarationBlock>>> {
         unsafe {
             (*self.element.style_attribute()).as_ref().map(|x| x.borrow_arc())
@@ -629,7 +610,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
     }
 
     fn first_child_element(&self) -> Option<ServoLayoutElement<'le>> {
-        self.as_node().children().filter_map(|n| n.as_element()).next()
+        self.as_node().dom_children().filter_map(|n| n.as_element()).next()
     }
 
     fn last_child_element(&self) -> Option<ServoLayoutElement<'le>> {
@@ -690,7 +671,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
     }
 
     fn is_empty(&self) -> bool {
-        self.as_node().children().all(|node| match node.script_type_id() {
+        self.as_node().dom_children().all(|node| match node.script_type_id() {
             NodeTypeId::Element(..) => false,
             NodeTypeId::CharacterData(CharacterDataTypeId::Text) => unsafe {
                 node.node.downcast().unwrap().data_for_layout().is_empty()
@@ -850,20 +831,14 @@ impl<'ln> ServoThreadSafeLayoutNode<'ln> {
     }
 }
 
-// NB: The implementation here is a bit tricky because elements implementing
-// pseudos are supposed to return false for is_element().
 impl<'ln> NodeInfo for ServoThreadSafeLayoutNode<'ln> {
     fn is_element(&self) -> bool {
-        self.pseudo == PseudoElementType::Normal && self.node.is_element()
+        self.node.is_element()
     }
 
     fn is_text_node(&self) -> bool {
         self.node.is_text_node()
     }
-
-    fn needs_layout(&self) -> bool {
-        self.node.is_text_node() || self.node.is_element()
-    }
 }
 
 impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> {

components/layout_thread/lib.rs

@@ -1067,7 +1067,7 @@ impl LayoutThread {
 
         let mut rw_data = possibly_locked_rw_data.lock();
 
-        let element: ServoLayoutElement = match document.root_node() {
+        let element = match document.root_element() {
             None => {
                 // Since we cannot compute anything, give spec-required placeholders.
                 debug!("layout: No root node: bailing");
@@ -1112,7 +1112,7 @@ impl LayoutThread {
                 }
                 return;
             },
-            Some(x) => x.as_element().unwrap(),
+            Some(x) => x,
         };
 
         debug!("layout: processing reflow request for: {:?} ({}) (query={:?})",

components/script_layout_interface/wrapper_traits.rs

@@ -100,14 +100,6 @@ pub trait LayoutNode: Debug + GetLayoutData + TNode {
     fn traverse_preorder(self) -> TreeIterator<Self> {
         TreeIterator::new(self)
     }
-
-    fn first_child(&self) -> Option<Self>;
-
-    fn last_child(&self) -> Option<Self>;
-
-    fn prev_sibling(&self) -> Option<Self>;
-
-    fn next_sibling(&self) -> Option<Self>;
 }
 
 pub struct ReverseChildrenIterator<ConcreteNode> where ConcreteNode: LayoutNode {

components/style/dom.rs

@@ -64,29 +64,49 @@ pub trait NodeInfo {
     fn is_element(&self) -> bool;
     /// Whether this node is a text node.
     fn is_text_node(&self) -> bool;
-
-    /// Whether this node needs layout.
-    ///
-    /// Comments, doctypes, etc are ignored by layout algorithms.
-    fn needs_layout(&self) -> bool { self.is_element() || self.is_text_node() }
 }
 
 /// A node iterator that only returns node that don't need layout.
 pub struct LayoutIterator<T>(pub T);
 
-impl<T, I> Iterator for LayoutIterator<T>
-    where T: Iterator<Item=I>,
-          I: NodeInfo,
+impl<T, N> Iterator for LayoutIterator<T>
+where
+    T: Iterator<Item = N>,
+    N: NodeInfo,
 {
-    type Item = I;
-    fn next(&mut self) -> Option<I> {
+    type Item = N;
+
+    fn next(&mut self) -> Option<N> {
         loop {
+            match self.0.next() {
+                Some(n) => {
                     // Filter out nodes that layout should ignore.
-            let n = self.0.next();
-            if n.is_none() || n.as_ref().unwrap().needs_layout() {
-                return n
+                    if n.is_text_node() || n.is_element() {
+                        return Some(n)
                     }
                 }
+                None => return None,
+            }
+        }
+    }
+}
+
+/// An iterator over the DOM children of a node.
+pub struct DomChildren<N>(Option<N>);
+impl<N> Iterator for DomChildren<N>
+where
+    N: TNode
+{
+    type Item = N;
+
+    fn next(&mut self) -> Option<N> {
+        match self.0.take() {
+            Some(n) => {
+                self.0 = n.next_sibling();
+                Some(n)
+            }
+            None => None,
+        }
     }
 }
 
@@ -96,29 +116,34 @@ pub trait TNode : Sized + Copy + Clone + Debug + NodeInfo {
     /// The concrete `TElement` type.
     type ConcreteElement: TElement<ConcreteNode = Self>;
 
-    /// A concrete children iterator type in order to iterate over the `Node`s.
-    ///
-    /// TODO(emilio): We should eventually replace this with the `impl Trait`
-    /// syntax.
-    type ConcreteChildrenIterator: Iterator<Item = Self>;
-
     /// Get this node's parent node.
     fn parent_node(&self) -> Option<Self>;
 
-    /// Get this node's parent element if present.
-    fn parent_element(&self) -> Option<Self::ConcreteElement> {
-        self.parent_node().and_then(|n| n.as_element())
-    }
+    /// Get this node's first child.
+    fn first_child(&self) -> Option<Self>;
 
-    /// Returns an iterator over this node's children.
-    fn children(&self) -> LayoutIterator<Self::ConcreteChildrenIterator>;
+    /// Get this node's first child.
+    fn last_child(&self) -> Option<Self>;
+
+    /// Get this node's previous sibling.
+    fn prev_sibling(&self) -> Option<Self>;
+
+    /// Get this node's next sibling.
+    fn next_sibling(&self) -> Option<Self>;
+
+    /// Iterate over the DOM children of an element.
+    fn dom_children(&self) -> DomChildren<Self> {
+        DomChildren(self.first_child())
+    }
 
     /// Get this node's parent element from the perspective of a restyle
     /// traversal.
     fn traversal_parent(&self) -> Option<Self::ConcreteElement>;
 
-    /// Get this node's children from the perspective of a restyle traversal.
-    fn traversal_children(&self) -> LayoutIterator<Self::ConcreteChildrenIterator>;
+    /// Get this node's parent element if present.
+    fn parent_element(&self) -> Option<Self::ConcreteElement> {
+        self.parent_node().and_then(|n| n.as_element())
+    }
 
     /// Converts self into an `OpaqueNode`.
     fn opaque(&self) -> OpaqueNode;
@@ -223,10 +248,12 @@ fn fmt_subtree<F, N: TNode>(f: &mut fmt::Formatter, stringify: &F, n: N, indent:
         write!(f, "  ")?;
     }
     stringify(f, n)?;
-    for kid in n.traversal_children() {
+    if let Some(e) = n.as_element() {
+        for kid in e.traversal_children() {
             writeln!(f, "")?;
             fmt_subtree(f, stringify, kid, indent + 1)?;
         }
+    }
 
     Ok(())
 }
@@ -256,6 +283,12 @@ pub trait TElement
     /// The concrete node type.
     type ConcreteNode: TNode<ConcreteElement = Self>;
 
+    /// A concrete children iterator type in order to iterate over the `Node`s.
+    ///
+    /// TODO(emilio): We should eventually replace this with the `impl Trait`
+    /// syntax.
+    type TraversalChildrenIterator: Iterator<Item = Self::ConcreteNode>;
+
     /// Type of the font metrics provider
     ///
     /// XXXManishearth It would be better to make this a type parameter on
@@ -295,6 +328,9 @@ pub trait TElement
         self.as_node().traversal_parent()
     }
 
+    /// Get this node's children from the perspective of a restyle traversal.
+    fn traversal_children(&self) -> LayoutIterator<Self::TraversalChildrenIterator>;
+
     /// Returns the parent element we should inherit from.
     ///
     /// This is pretty much always the parent element itself, except in the case

components/style/gecko/wrapper.rs

@@ -158,32 +158,6 @@ impl<'ln> GeckoNode<'ln> {
         unsafe { &*self.node_info().mDocument }
     }
 
-    #[inline]
-    fn first_child(&self) -> Option<GeckoNode<'ln>> {
-        unsafe { self.0.mFirstChild.as_ref().map(GeckoNode::from_content) }
-    }
-
-    #[inline]
-    fn last_child(&self) -> Option<GeckoNode<'ln>> {
-        unsafe { Gecko_GetLastChild(self.0).map(GeckoNode) }
-    }
-
-    #[inline]
-    fn prev_sibling(&self) -> Option<GeckoNode<'ln>> {
-        unsafe { self.0.mPreviousSibling.as_ref().map(GeckoNode::from_content) }
-    }
-
-    #[inline]
-    fn next_sibling(&self) -> Option<GeckoNode<'ln>> {
-        unsafe { self.0.mNextSibling.as_ref().map(GeckoNode::from_content) }
-    }
-
-    /// Simple iterator over all this node's children.  Unlike `.children()`, this iterator does
-    /// not filter out nodes that don't need layout.
-    fn dom_children(self) -> GeckoChildrenIterator<'ln> {
-        GeckoChildrenIterator::Current(self.first_child())
-    }
-
     /// WARNING: This logic is duplicated in Gecko's FlattenedTreeParentIsParent.
     /// Make sure to mirror any modifications in both places.
     fn flattened_tree_parent_is_parent(&self) -> bool {
@@ -222,11 +196,6 @@ impl<'ln> GeckoNode<'ln> {
     fn contains_non_whitespace_content(&self) -> bool {
         unsafe { Gecko_IsSignificantChild(self.0, true, false) }
     }
-
-    #[inline]
-    fn may_have_anonymous_children(&self) -> bool {
-        self.get_bool_flag(nsINode_BooleanFlag::ElementMayHaveAnonymousChildren)
-    }
 }
 
 impl<'ln> NodeInfo for GeckoNode<'ln> {
@@ -244,40 +213,35 @@ impl<'ln> NodeInfo for GeckoNode<'ln> {
 
 impl<'ln> TNode for GeckoNode<'ln> {
     type ConcreteElement = GeckoElement<'ln>;
-    type ConcreteChildrenIterator = GeckoChildrenIterator<'ln>;
 
     fn parent_node(&self) -> Option<Self> {
         unsafe { self.0.mParent.as_ref().map(GeckoNode) }
     }
 
-    fn children(&self) -> LayoutIterator<GeckoChildrenIterator<'ln>> {
-        LayoutIterator(self.dom_children())
+    #[inline]
+    fn first_child(&self) -> Option<Self> {
+        unsafe { self.0.mFirstChild.as_ref().map(GeckoNode::from_content) }
+    }
+
+    #[inline]
+    fn last_child(&self) -> Option<Self> {
+        unsafe { Gecko_GetLastChild(self.0).map(GeckoNode) }
+    }
+
+    #[inline]
+    fn prev_sibling(&self) -> Option<Self> {
+        unsafe { self.0.mPreviousSibling.as_ref().map(GeckoNode::from_content) }
+    }
+
+    #[inline]
+    fn next_sibling(&self) -> Option<Self> {
+        unsafe { self.0.mNextSibling.as_ref().map(GeckoNode::from_content) }
     }
 
     fn traversal_parent(&self) -> Option<GeckoElement<'ln>> {
         self.flattened_tree_parent().and_then(|n| n.as_element())
     }
 
-    fn traversal_children(&self) -> LayoutIterator<GeckoChildrenIterator<'ln>> {
-        if let Some(element) = self.as_element() {
-            // This condition is similar to the check that
-            // StyleChildrenIterator::IsNeeded does, except that it might return
-            // true if we used to (but no longer) have anonymous content from
-            // ::before/::after, XBL bindings, or nsIAnonymousContentCreators.
-            if element.is_in_anonymous_subtree() ||
-               element.has_xbl_binding_with_content() ||
-               self.may_have_anonymous_children() {
-                unsafe {
-                    let mut iter: structs::StyleChildrenIterator = ::std::mem::zeroed();
-                    Gecko_ConstructStyleChildrenIterator(element.0, &mut iter);
-                    return LayoutIterator(GeckoChildrenIterator::GeckoIterator(iter));
-                }
-            }
-        }
-
-        LayoutIterator(self.dom_children())
-    }
-
     fn opaque(&self) -> OpaqueNode {
         let ptr: usize = self.0 as *const _ as usize;
         OpaqueNode(ptr)
@@ -447,6 +411,11 @@ impl<'le> fmt::Debug for GeckoElement<'le> {
 }
 
 impl<'le> GeckoElement<'le> {
+    #[inline]
+    fn may_have_anonymous_children(&self) -> bool {
+        self.as_node().get_bool_flag(nsINode_BooleanFlag::ElementMayHaveAnonymousChildren)
+    }
+
     /// Parse the style attribute of an element.
     pub fn parse_style_attribute<R>(
         value: &str,
@@ -883,6 +852,7 @@ impl structs::FontSizePrefs {
 impl<'le> TElement for GeckoElement<'le> {
     type ConcreteNode = GeckoNode<'le>;
     type FontMetricsProvider = GeckoFontMetricsProvider;
+    type TraversalChildrenIterator = GeckoChildrenIterator<'le>;
 
     fn inheritance_parent(&self) -> Option<Self> {
         if self.is_native_anonymous() {
@@ -894,6 +864,24 @@ impl<'le> TElement for GeckoElement<'le> {
         }
     }
 
+    fn traversal_children(&self) -> LayoutIterator<GeckoChildrenIterator<'le>> {
+        // This condition is similar to the check that
+        // StyleChildrenIterator::IsNeeded does, except that it might return
+        // true if we used to (but no longer) have anonymous content from
+        // ::before/::after, XBL bindings, or nsIAnonymousContentCreators.
+        if self.is_in_anonymous_subtree() ||
+           self.has_xbl_binding_with_content() ||
+           self.may_have_anonymous_children() {
+            unsafe {
+                let mut iter: structs::StyleChildrenIterator = ::std::mem::zeroed();
+                Gecko_ConstructStyleChildrenIterator(self.0, &mut iter);
+                return LayoutIterator(GeckoChildrenIterator::GeckoIterator(iter));
+            }
+        }
+
+        LayoutIterator(GeckoChildrenIterator::Current(self.as_node().first_child()))
+    }
+
     fn before_pseudo_element(&self) -> Option<Self> {
         self.get_before_or_after_pseudo(/* is_before = */ true)
     }

components/style/invalidation/element/invalidator.rs

@@ -382,7 +382,7 @@ where
         let mut any_descendant = false;
 
         let mut sibling_invalidations = InvalidationVector::new();
-        for child in parent.children() {
+        for child in parent.dom_children() {
             // TODO(emilio): We handle <xbl:children> fine, because they appear
             // in selector-matching (note bug 1374247, though).
             //

components/style/invalidation/stylesheets.rs

@@ -237,7 +237,7 @@ impl StylesheetInvalidationSet {
 
         let mut any_children_invalid = false;
 
-        for child in element.as_node().traversal_children() {
+        for child in element.traversal_children() {
             let child = match child.as_element() {
                 Some(e) => e,
                 None => continue,

components/style/traversal.rs

@@ -841,7 +841,7 @@ where
     let is_initial_style = context.thread_local.is_initial_style();
 
     // Loop over all the traversal children.
-    for child_node in element.as_node().traversal_children() {
+    for child_node in element.traversal_children() {
         let child = match child_node.as_element() {
             Some(el) => el,
             None => {
@@ -933,7 +933,7 @@ where
     let mut parents = SmallVec::<[E; 32]>::new();
     parents.push(root);
     while let Some(p) = parents.pop() {
-        for kid in p.as_node().traversal_children() {
+        for kid in p.traversal_children() {
             if let Some(kid) = kid.as_element() {
                 // We maintain an invariant that, if an element has data, all its
                 // ancestors have data as well.

ports/geckolib/glue.rs

@@ -3621,7 +3621,7 @@ pub extern "C" fn Servo_AssertTreeIsClean(root: RawGeckoElementBorrowed) {
         debug_assert!(!el.has_dirty_descendants() && !el.has_animation_only_dirty_descendants(),
                       "{:?} has still dirty bit {:?} or animation-only dirty bit {:?}",
                       el, el.has_dirty_descendants(), el.has_animation_only_dirty_descendants());
-        for child in el.as_node().traversal_children() {
+        for child in el.traversal_children() {
             if let Some(child) = child.as_element() {
                 assert_subtree_is_clean(child);
             }