auto merge of #3744 : cgaebel/servo/efficient-preorder-traversal, r=pcwalton

This also tackles some nearby FIXMEs. `traverse_preorder` is used in a LOT of
DOM functions..

r? @pcwalton

components/layout/wrapper.rs

@@ -214,28 +214,36 @@ impl<'ln> LayoutNode<'ln> {
         }
     }
 
-    /// Iterates over this node and all its descendants, in preorder.
-    ///
-    /// FIXME(pcwalton): Terribly inefficient. We should use parallelism.
     pub fn traverse_preorder(self) -> LayoutTreeIterator<'ln> {
-        let mut nodes = vec!();
+        LayoutTreeIterator::new(self)
-        gather_layout_nodes(self, &mut nodes, false);
+    }
-        LayoutTreeIterator::new(nodes)
+
+    fn last_child(self) -> Option<LayoutNode<'ln>> {
+        unsafe {
+            self.get_jsmanaged().last_child_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
     }
 
     /// Returns an iterator over this node's children.
     pub fn children(self) -> LayoutNodeChildrenIterator<'ln> {
         // FIXME(zwarich): Remove this when UFCS lands and there is a better way
         // of disambiguating methods.
-        fn first_child<T: TLayoutNode>(this: &T) -> Option<T> {
+        fn first_child<T: TLayoutNode>(this: T) -> Option<T> {
             this.first_child()
         }
 
         LayoutNodeChildrenIterator {
-            current_node: first_child(&self),
+            current: first_child(self),
         }
     }
 
+    pub fn rev_children(self) -> LayoutNodeReverseChildrenIterator<'ln> {
+        LayoutNodeReverseChildrenIterator {
+            current: self.last_child()
+        }
+
+    }
+
     pub unsafe fn get_jsmanaged<'a>(&'a self) -> &'a JS<Node> {
         &self.node
     }
@@ -291,6 +299,12 @@ impl<'ln> TNode<'ln, LayoutElement<'ln>> for LayoutNode<'ln> {
         }
     }
 
+    fn last_child(self) -> Option<LayoutNode<'ln>> {
+        unsafe {
+            self.node.last_child_ref().map(|node| self.new_with_this_lifetime(&node))
+        }
+    }
+
     fn prev_sibling(self) -> Option<LayoutNode<'ln>> {
         unsafe {
             self.node.prev_sibling_ref().map(|node| self.new_with_this_lifetime(&node))
@@ -390,59 +404,48 @@ impl<'ln> TNode<'ln, LayoutElement<'ln>> for LayoutNode<'ln> {
 }
 
 pub struct LayoutNodeChildrenIterator<'a> {
-    current_node: Option<LayoutNode<'a>>,
+    current: Option<LayoutNode<'a>>,
 }
 
 impl<'a> Iterator<LayoutNode<'a>> for LayoutNodeChildrenIterator<'a> {
     fn next(&mut self) -> Option<LayoutNode<'a>> {
-        let node = self.current_node.clone();
+        let node = self.current;
-        self.current_node = node.clone().and_then(|node| {
+        self.current = node.and_then(|node| node.next_sibling());
-            node.next_sibling()
+        node
-        });
+    }
+}
+
+pub struct LayoutNodeReverseChildrenIterator<'a> {
+    current: Option<LayoutNode<'a>>,
+}
+
+impl<'a> Iterator<LayoutNode<'a>> for LayoutNodeReverseChildrenIterator<'a> {
+    fn next(&mut self) -> Option<LayoutNode<'a>> {
+        let node = self.current;
+        self.current = node.and_then(|node| node.prev_sibling());
         node
     }
 }
 
-// FIXME: Do this without precomputing a vector of refs.
-// Easy for preorder; harder for postorder.
-//
-// FIXME(pcwalton): Parallelism! Eventually this should just be nuked.
 pub struct LayoutTreeIterator<'a> {
-    nodes: Vec<LayoutNode<'a>>,
+    stack: Vec<LayoutNode<'a>>,
-    index: uint,
 }
 
 impl<'a> LayoutTreeIterator<'a> {
-    fn new(nodes: Vec<LayoutNode<'a>>) -> LayoutTreeIterator<'a> {
+    fn new(root: LayoutNode<'a>) -> LayoutTreeIterator<'a> {
+        let mut stack = vec!();
+        stack.push(root);
         LayoutTreeIterator {
-            nodes: nodes,
+            stack: stack
-            index: 0,
         }
     }
 }
 
 impl<'a> Iterator<LayoutNode<'a>> for LayoutTreeIterator<'a> {
     fn next(&mut self) -> Option<LayoutNode<'a>> {
-        if self.index >= self.nodes.len() {
+        let ret = self.stack.pop();
-            None
+        ret.map(|node| self.stack.extend(node.rev_children()));
-        } else {
+        ret
-            let v = self.nodes[self.index].clone();
-            self.index += 1;
-            Some(v)
-        }
-    }
-}
-
-/// FIXME(pcwalton): This is super inefficient.
-fn gather_layout_nodes<'a>(cur: LayoutNode<'a>, refs: &mut Vec<LayoutNode<'a>>, postorder: bool) {
-    if !postorder {
-        refs.push(cur.clone());
-    }
-    for kid in cur.children() {
-        gather_layout_nodes(kid, refs, postorder)
-    }
-    if postorder {
-        refs.push(cur.clone());
     }
 }
 

components/script/dom/node.rs

@@ -381,6 +381,7 @@ impl<'a> PrivateNodeHelpers for JSRef<'a, Node> {
 pub trait NodeHelpers<'a> {
     fn ancestors(self) -> AncestorIterator<'a>;
     fn children(self) -> AbstractNodeChildrenIterator<'a>;
+    fn rev_children(self) -> ReverseChildrenIterator<'a>;
     fn child_elements(self) -> ChildElementIterator<'a>;
     fn following_siblings(self) -> AbstractNodeChildrenIterator<'a>;
     fn is_in_doc(self) -> bool;
@@ -441,7 +442,6 @@ pub trait NodeHelpers<'a> {
     fn debug_str(self) -> String;
 
     fn traverse_preorder(self) -> TreeIterator<'a>;
-    fn sequential_traverse_postorder(self) -> TreeIterator<'a>;
     fn inclusively_following_siblings(self) -> AbstractNodeChildrenIterator<'a>;
 
     fn to_trusted_node_address(self) -> TrustedNodeAddress;
@@ -658,21 +658,12 @@ impl<'a> NodeHelpers<'a> for JSRef<'a, Node> {
 
     /// Iterates over this node and all its descendants, in preorder.
     fn traverse_preorder(self) -> TreeIterator<'a> {
-        let mut nodes = vec!();
+        TreeIterator::new(self)
-        gather_abstract_nodes(self, &mut nodes, false);
-        TreeIterator::new(nodes)
-    }
-
-    /// Iterates over this node and all its descendants, in postorder.
-    fn sequential_traverse_postorder(self) -> TreeIterator<'a> {
-        let mut nodes = vec!();
-        gather_abstract_nodes(self, &mut nodes, true);
-        TreeIterator::new(nodes)
     }
 
     fn inclusively_following_siblings(self) -> AbstractNodeChildrenIterator<'a> {
         AbstractNodeChildrenIterator {
-            current_node: Some(self.clone()),
+            current: Some(self.clone()),
         }
     }
 
@@ -682,7 +673,7 @@ impl<'a> NodeHelpers<'a> for JSRef<'a, Node> {
 
     fn following_siblings(self) -> AbstractNodeChildrenIterator<'a> {
         AbstractNodeChildrenIterator {
-            current_node: self.next_sibling().root().map(|next| next.clone()),
+            current: self.next_sibling().root().map(|next| next.clone()),
         }
     }
 
@@ -774,7 +765,13 @@ impl<'a> NodeHelpers<'a> for JSRef<'a, Node> {
 
     fn children(self) -> AbstractNodeChildrenIterator<'a> {
         AbstractNodeChildrenIterator {
-            current_node: self.first_child.get().map(|node| (*node.root()).clone()),
+            current: self.first_child.get().map(|node| (*node.root()).clone()),
+        }
+    }
+
+    fn rev_children(self) -> ReverseChildrenIterator<'a> {
+        ReverseChildrenIterator {
+            current: self.last_child.get().map(|node| *node.root().deref()),
         }
     }
 
@@ -974,15 +971,25 @@ pub type ChildElementIterator<'a> = Map<'a, JSRef<'a, Node>,
                                         Filter<'a, JSRef<'a, Node>, AbstractNodeChildrenIterator<'a>>>;
 
 pub struct AbstractNodeChildrenIterator<'a> {
-    current_node: Option<JSRef<'a, Node>>,
+    current: Option<JSRef<'a, Node>>,
 }
 
 impl<'a> Iterator<JSRef<'a, Node>> for AbstractNodeChildrenIterator<'a> {
     fn next(&mut self) -> Option<JSRef<'a, Node>> {
-        let node = self.current_node.clone();
+        let node = self.current;
-        self.current_node = node.clone().and_then(|node| {
+        self.current = node.and_then(|node| node.next_sibling().map(|node| *node.root().deref()));
-            node.next_sibling().map(|node| (*node.root()).clone())
+        node
-        });
+    }
+}
+
+pub struct ReverseChildrenIterator<'a> {
+    current: Option<JSRef<'a, Node>>,
+}
+
+impl<'a> Iterator<JSRef<'a, Node>> for ReverseChildrenIterator<'a> {
+    fn next(&mut self) -> Option<JSRef<'a, Node>> {
+        let node = self.current;
+        self.current = node.and_then(|node| node.prev_sibling().map(|node| *node.root().deref()));
         node
     }
 }
@@ -993,43 +1000,32 @@ pub struct AncestorIterator<'a> {
 
 impl<'a> Iterator<JSRef<'a, Node>> for AncestorIterator<'a> {
     fn next(&mut self) -> Option<JSRef<'a, Node>> {
-        if self.current.is_none() {
+        let node = self.current;
-            return None;
+        self.current = node.and_then(|node| node.parent_node().map(|node| *node.root().deref()));
-        }
+        node
-
-        // FIXME: Do we need two clones here?
-        let x = self.current.as_ref().unwrap().clone();
-        self.current = x.parent_node().map(|node| (*node.root()).clone());
-        Some(x)
     }
 }
 
-// FIXME: Do this without precomputing a vector of refs.
-// Easy for preorder; harder for postorder.
 pub struct TreeIterator<'a> {
-    nodes: Vec<JSRef<'a, Node>>,
+    stack: Vec<JSRef<'a, Node>>,
-    index: uint,
 }
 
 impl<'a> TreeIterator<'a> {
-    fn new(nodes: Vec<JSRef<'a, Node>>) -> TreeIterator<'a> {
+    fn new(root: JSRef<'a, Node>) -> TreeIterator<'a> {
+        let mut stack = vec!();
+        stack.push(root);
+
         TreeIterator {
-            nodes: nodes,
+            stack: stack,
-            index: 0,
         }
     }
 }
 
 impl<'a> Iterator<JSRef<'a, Node>> for TreeIterator<'a> {
     fn next(&mut self) -> Option<JSRef<'a, Node>> {
-        if self.index >= self.nodes.len() {
+        let ret = self.stack.pop();
-            None
+        ret.map(|node| self.stack.extend(node.rev_children()));
-        } else {
+        ret
-            let v = self.nodes[self.index];
-            let v = v.clone();
-            self.index += 1;
-            Some(v)
-        }
     }
 }
 
@@ -1116,18 +1112,6 @@ impl<'a> Iterator<JSRef<'a, Node>> for NodeIterator {
     }
 }
 
-fn gather_abstract_nodes<'a>(cur: JSRef<'a, Node>, refs: &mut Vec<JSRef<'a, Node>>, postorder: bool) {
-    if !postorder {
-        refs.push(cur.clone());
-    }
-    for kid in cur.children() {
-        gather_abstract_nodes(kid, refs, postorder)
-    }
-    if postorder {
-        refs.push(cur.clone());
-    }
-}
-
 /// Specifies whether children must be recursively cloned or not.
 #[deriving(PartialEq)]
 pub enum CloneChildrenFlag {
@@ -2209,6 +2193,16 @@ impl<'a> style::TNode<'a, JSRef<'a, Element>> for JSRef<'a, Node> {
         first_child(self).map(|node| *node.root())
     }
 
+    fn last_child(self) -> Option<JSRef<'a, Node>> {
+        // FIXME(zwarich): Remove this when UFCS lands and there is a better way
+        // of disambiguating methods.
+        fn last_child<'a, T: NodeHelpers<'a>>(this: T) -> Option<Temporary<Node>> {
+            this.last_child()
+        }
+
+        last_child(self).map(|node| *node.root())
+    }
+
     fn prev_sibling(self) -> Option<JSRef<'a, Node>> {
         // FIXME(zwarich): Remove this when UFCS lands and there is a better way
         // of disambiguating methods.

components/style/node.rs

@@ -13,6 +13,7 @@ use string_cache::{Atom, Namespace};
 pub trait TNode<'a, E: TElement<'a>> : Clone + Copy {
     fn parent_node(self) -> Option<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>;
     fn is_document(self) -> bool;
@@ -57,4 +58,3 @@ pub trait TElementAttributes : Copy {
     fn get_length_attribute(self, attribute: LengthAttribute) -> LengthOrPercentageOrAuto;
     fn get_integer_attribute(self, attribute: IntegerAttribute) -> Option<i32>;
 }
-