From b3892b74f7367c67603eff049454b3e9135f8d78 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 17 Jul 2015 18:56:47 +0200 Subject: [PATCH 1/2] Simplify ThreadSafeLayoutNodeChildrenIterator::next(). --- components/layout/wrapper.rs | 75 +++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 747b0d6cd128..5d75b7a29fb1 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -670,15 +670,6 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { } } - /// Returns the next sibling of this node. Unsafe and private because this can lead to races. - unsafe fn next_sibling(&self) -> Option> { - if self.pseudo.is_before() { - return self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - - self.get_jsmanaged().next_sibling_ref().map(|node| self.new_with_this_lifetime(&node)) - } - /// Returns an iterator over this node's children. pub fn children(&self) -> ThreadSafeLayoutNodeChildrenIterator<'ln> { ThreadSafeLayoutNodeChildrenIterator { @@ -975,34 +966,48 @@ impl<'a> Iterator for ThreadSafeLayoutNodeChildrenIterator<'a> { fn next(&mut self) -> Option> { let node = self.current_node.clone(); - match node { - Some(ref node) => { - if node.pseudo.is_after() { - return None - } - - self.current_node = if self.parent_node.pseudo == PseudoElementType::Normal { - self.current_node.clone().and_then(|node| { - unsafe { - node.next_sibling() + if let Some(ref node) = node { + self.current_node = match node.pseudo { + PseudoElementType::Before(_) => { + match unsafe { self.parent_node.get_jsmanaged().first_child_ref() } { + Some(first) => { + Some(unsafe { + self.parent_node.new_with_this_lifetime(&first) + }) + }, + None => { + if self.parent_node.has_after_pseudo() { + let pseudo = PseudoElementType::After( + self.parent_node.get_after_display()); + Some(self.parent_node.with_pseudo(pseudo)) + } else { + None + } } - }) - } else { + } + }, + PseudoElementType::Normal => { + match unsafe { node.get_jsmanaged().next_sibling_ref() } { + Some(next) => { + Some(unsafe { + self.parent_node.new_with_this_lifetime(&next) + }) + }, + None => { + if self.parent_node.has_after_pseudo() { + let pseudo = PseudoElementType::After( + self.parent_node.get_after_display()); + Some(self.parent_node.with_pseudo(pseudo)) + } else { + None + } + } + } + }, + PseudoElementType::After(_) => { None - }; - } - None => { - if self.parent_node.has_after_pseudo() { - let pseudo_after_node = if self.parent_node.pseudo == PseudoElementType::Normal { - let pseudo = PseudoElementType::After(self.parent_node.get_after_display()); - Some(self.parent_node.with_pseudo(pseudo)) - } else { - None - }; - self.current_node = pseudo_after_node; - return self.current_node.clone() - } - } + }, + }; } node From 930e1117139f5c7da2eda5c70f8c07a1b455048c Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Fri, 17 Jul 2015 23:08:54 +0200 Subject: [PATCH 2/2] Scope ThreadSafeLayoutNode::first_child to ThreadSafeLayoutNodeChildrenIterator::new. It is only used there. --- components/layout/wrapper.rs | 45 +++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 5d75b7a29fb1..33414f646736 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -656,26 +656,9 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { self.node.flow_debug_id() } - fn first_child(&self) -> Option> { - if self.pseudo != PseudoElementType::Normal { - return None - } - - if self.has_before_pseudo() { - return Some(self.with_pseudo(PseudoElementType::Before(self.get_before_display()))); - } - - unsafe { - self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } - /// Returns an iterator over this node's children. pub fn children(&self) -> ThreadSafeLayoutNodeChildrenIterator<'ln> { - ThreadSafeLayoutNodeChildrenIterator { - current_node: self.first_child(), - parent_node: self.clone(), - } + ThreadSafeLayoutNodeChildrenIterator::new(*self) } /// If this is an element, accesses the element data. Fails if this is not an element node. @@ -961,6 +944,32 @@ pub struct ThreadSafeLayoutNodeChildrenIterator<'a> { parent_node: ThreadSafeLayoutNode<'a>, } +impl<'a> ThreadSafeLayoutNodeChildrenIterator<'a> { + fn new(parent: ThreadSafeLayoutNode<'a>) -> ThreadSafeLayoutNodeChildrenIterator<'a> { + fn first_child<'a>(parent: ThreadSafeLayoutNode<'a>) + -> Option> { + if parent.pseudo != PseudoElementType::Normal { + return None + } + + if parent.has_before_pseudo() { + let pseudo = PseudoElementType::Before(parent.get_before_display()); + return Some(parent.with_pseudo(pseudo)); + } + + unsafe { + parent.get_jsmanaged().first_child_ref() + .map(|node| parent.new_with_this_lifetime(&node)) + } + } + + ThreadSafeLayoutNodeChildrenIterator { + current_node: first_child(parent), + parent_node: parent, + } + } +} + impl<'a> Iterator for ThreadSafeLayoutNodeChildrenIterator<'a> { type Item = ThreadSafeLayoutNode<'a>; fn next(&mut self) -> Option> {