From 40d9cd99b5267b7712c73e1b998c79280a53e113 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Oct 2017 17:04:47 +0200 Subject: [PATCH 1/5] style: Don't require a full SharedStyleContext for TreeStyleInvalidator. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We only use it to get the quirks mode, and a shared style context is a pretty heavy-weight struct. Signed-off-by: Emilio Cobos Álvarez --- components/style/data.rs | 5 ++-- .../style/invalidation/element/collector.rs | 22 ++++++++++++----- .../style/invalidation/element/invalidator.rs | 24 +++++++++---------- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index 7e50a67dbb3..defb0a49759 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -259,11 +259,12 @@ impl ElementData { return InvalidationResult::empty(); } - let mut processor = StateAndAttrInvalidationProcessor; + let mut processor = + StateAndAttrInvalidationProcessor::new(shared_context); let invalidator = TreeStyleInvalidator::new( element, Some(self), - shared_context, + shared_context.quirks_mode(), stack_limit_checker, nth_index_cache, &mut processor, diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs index b1971bfab19..2bdc5f4a0cc 100644 --- a/components/style/invalidation/element/collector.rs +++ b/components/style/invalidation/element/collector.rs @@ -51,9 +51,18 @@ where /// An invalidation processor for style changes due to state and attribute /// changes. -pub struct StateAndAttrInvalidationProcessor; +pub struct StateAndAttrInvalidationProcessor<'a, 'b: 'a> { + shared_context: &'a SharedStyleContext<'b>, +} -impl InvalidationProcessor for StateAndAttrInvalidationProcessor +impl<'a, 'b: 'a> StateAndAttrInvalidationProcessor<'a, 'b> { + /// Creates a new StateAndAttrInvalidationProcessor. + pub fn new(shared_context: &'a SharedStyleContext<'b>) -> Self { + Self { shared_context } + } +} + +impl<'a, 'b: 'a, E> InvalidationProcessor for StateAndAttrInvalidationProcessor<'a, 'b> where E: TElement, { @@ -67,15 +76,16 @@ where element: E, mut data: Option<&mut ElementData>, nth_index_cache: Option<&mut NthIndexCache>, - shared_context: &SharedStyleContext, + quirks_mode: QuirksMode, descendant_invalidations: &mut InvalidationVector, sibling_invalidations: &mut InvalidationVector, ) -> bool { debug_assert!(element.has_snapshot(), "Why bothering?"); debug_assert!(data.is_some(), "How exactly?"); + debug_assert_eq!(quirks_mode, self.shared_context.quirks_mode(), "How exactly?"); let wrapper = - ElementWrapper::new(element, &*shared_context.snapshot_map); + ElementWrapper::new(element, &*self.shared_context.snapshot_map); let state_changes = wrapper.state_changes(); let snapshot = wrapper.snapshot().expect("has_snapshot lied"); @@ -140,7 +150,7 @@ where state_changes, element, snapshot: &snapshot, - quirks_mode: shared_context.quirks_mode(), + quirks_mode: self.shared_context.quirks_mode(), removed_id: id_removed.as_ref(), added_id: id_added.as_ref(), classes_removed: &classes_removed, @@ -150,7 +160,7 @@ where invalidates_self: false, }; - shared_context.stylist.each_invalidation_map(|invalidation_map| { + self.shared_context.stylist.each_invalidation_map(|invalidation_map| { collector.collect_dependencies_in_invalidation_map(invalidation_map); }); diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 97edd7a4203..7ad87363066 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -5,12 +5,12 @@ //! The struct that takes care of encapsulating all the logic on where and how //! element styles need to be invalidated. -use context::{SharedStyleContext, StackLimitChecker}; +use context::StackLimitChecker; use data::ElementData; use dom::{TElement, TNode}; use selector_parser::SelectorImpl; use selectors::NthIndexCache; -use selectors::matching::{MatchingContext, MatchingMode, VisitedHandlingMode}; +use selectors::matching::{MatchingContext, MatchingMode, QuirksMode, VisitedHandlingMode}; use selectors::matching::CompoundSelectorMatchingResult; use selectors::matching::matches_compound_selector; use selectors::parser::{Combinator, Component, Selector}; @@ -38,7 +38,7 @@ where element: E, data: Option<&mut ElementData>, nth_index_cache: Option<&mut NthIndexCache>, - shared_context: &SharedStyleContext, + quirks_mode: QuirksMode, descendant_invalidations: &mut InvalidationVector, sibling_invalidations: &mut InvalidationVector, ) -> bool; @@ -77,7 +77,7 @@ where /// The struct that takes care of encapsulating all the logic on where and how /// element styles need to be invalidated. -pub struct TreeStyleInvalidator<'a, 'b: 'a, E, P: 'a> +pub struct TreeStyleInvalidator<'a, E, P: 'a> where E: TElement, P: InvalidationProcessor @@ -93,7 +93,7 @@ where // Seems like we could at least avoid running invalidation for the // descendants if an element has no data, though. data: Option<&'a mut ElementData>, - shared_context: &'a SharedStyleContext<'b>, + quirks_mode: QuirksMode, stack_limit_checker: Option<&'a StackLimitChecker>, nth_index_cache: Option<&'a mut NthIndexCache>, processor: &'a mut P, @@ -224,7 +224,7 @@ impl InvalidationResult { } } -impl<'a, 'b: 'a, E, P: 'a> TreeStyleInvalidator<'a, 'b, E, P> +impl<'a, E, P: 'a> TreeStyleInvalidator<'a, E, P> where E: TElement, P: InvalidationProcessor, @@ -233,7 +233,7 @@ where pub fn new( element: E, data: Option<&'a mut ElementData>, - shared_context: &'a SharedStyleContext<'b>, + quirks_mode: QuirksMode, stack_limit_checker: Option<&'a StackLimitChecker>, nth_index_cache: Option<&'a mut NthIndexCache>, processor: &'a mut P, @@ -241,7 +241,7 @@ where Self { element, data, - shared_context, + quirks_mode, stack_limit_checker, nth_index_cache, processor, @@ -259,7 +259,7 @@ where self.element, self.data.as_mut().map(|d| &mut **d), self.nth_index_cache.as_mut().map(|c| &mut **c), - self.shared_context, + self.quirks_mode, &mut descendant_invalidations, &mut sibling_invalidations, ); @@ -296,7 +296,7 @@ where let mut sibling_invalidator = TreeStyleInvalidator::new( sibling, sibling_data.as_mut().map(|d| &mut **d), - self.shared_context, + self.quirks_mode, self.stack_limit_checker, self.nth_index_cache.as_mut().map(|c| &mut **c), self.processor, @@ -364,7 +364,7 @@ where let mut child_invalidator = TreeStyleInvalidator::new( child, child_data.as_mut().map(|d| &mut **d), - self.shared_context, + self.quirks_mode, self.stack_limit_checker, self.nth_index_cache.as_mut().map(|c| &mut **c), self.processor, @@ -609,7 +609,7 @@ where None, self.nth_index_cache.as_mut().map(|c| &mut **c), VisitedHandlingMode::AllLinksVisitedAndUnvisited, - self.shared_context.quirks_mode(), + self.quirks_mode, ); matches_compound_selector( From 7c2265360f4153bd75da1cae46cd06a3c53d35f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Oct 2017 17:38:00 +0200 Subject: [PATCH 2/5] style: Stop threading the ElementData around the invalidator. --- components/style/data.rs | 9 ++- .../style/invalidation/element/collector.rs | 73 ++++++++----------- .../style/invalidation/element/invalidator.rs | 70 ++---------------- 3 files changed, 46 insertions(+), 106 deletions(-) diff --git a/components/style/data.rs b/components/style/data.rs index defb0a49759..b9aed397f23 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -259,11 +259,14 @@ impl ElementData { return InvalidationResult::empty(); } - let mut processor = - StateAndAttrInvalidationProcessor::new(shared_context); + let mut processor = StateAndAttrInvalidationProcessor::new( + shared_context, + element, + self, + ); + let invalidator = TreeStyleInvalidator::new( element, - Some(self), shared_context.quirks_mode(), stack_limit_checker, nth_index_cache, diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs index 2bdc5f4a0cc..ff74ad9f3a2 100644 --- a/components/style/invalidation/element/collector.rs +++ b/components/style/invalidation/element/collector.rs @@ -51,18 +51,24 @@ where /// An invalidation processor for style changes due to state and attribute /// changes. -pub struct StateAndAttrInvalidationProcessor<'a, 'b: 'a> { +pub struct StateAndAttrInvalidationProcessor<'a, 'b: 'a, E> { shared_context: &'a SharedStyleContext<'b>, + element: E, + data: &'a mut ElementData, } -impl<'a, 'b: 'a> StateAndAttrInvalidationProcessor<'a, 'b> { +impl<'a, 'b: 'a, E> StateAndAttrInvalidationProcessor<'a, 'b, E> { /// Creates a new StateAndAttrInvalidationProcessor. - pub fn new(shared_context: &'a SharedStyleContext<'b>) -> Self { - Self { shared_context } + pub fn new( + shared_context: &'a SharedStyleContext<'b>, + element: E, + data: &'a mut ElementData, + ) -> Self { + Self { shared_context, element, data } } } -impl<'a, 'b: 'a, E> InvalidationProcessor for StateAndAttrInvalidationProcessor<'a, 'b> +impl<'a, 'b: 'a, E> InvalidationProcessor for StateAndAttrInvalidationProcessor<'a, 'b, E> where E: TElement, { @@ -74,14 +80,12 @@ where fn collect_invalidations( &mut self, element: E, - mut data: Option<&mut ElementData>, nth_index_cache: Option<&mut NthIndexCache>, quirks_mode: QuirksMode, descendant_invalidations: &mut InvalidationVector, sibling_invalidations: &mut InvalidationVector, ) -> bool { debug_assert!(element.has_snapshot(), "Why bothering?"); - debug_assert!(data.is_some(), "How exactly?"); debug_assert_eq!(quirks_mode, self.shared_context.quirks_mode(), "How exactly?"); let wrapper = @@ -102,8 +106,7 @@ where trace!(" > visitedness change, force subtree restyle"); // We can't just return here because there may also be attribute // changes as well that imply additional hints. - let data = data.as_mut().unwrap(); - data.hint.insert(RestyleHint::restyle_subtree()); + self.data.hint.insert(RestyleHint::restyle_subtree()); } let mut classes_removed = SmallVec::<[Atom; 8]>::new(); @@ -183,52 +186,43 @@ where }; if invalidated_self { - if let Some(ref mut data) = data { - data.hint.insert(RESTYLE_SELF); - } + self.data.hint.insert(RESTYLE_SELF); } invalidated_self } - fn should_process_descendants( - &mut self, - _element: E, - data: Option<&mut ElementData>, - ) -> bool { - let data = match data { + fn should_process_descendants(&mut self, element: E) -> bool { + if element == self.element { + return !self.data.styles.is_display_none() && + !self.data.hint.contains(RESTYLE_DESCENDANTS) + } + + let data = match element.borrow_data() { None => return false, - Some(ref data) => data, + Some(data) => data, }; !data.styles.is_display_none() && !data.hint.contains(RESTYLE_DESCENDANTS) } - fn recursion_limit_exceeded( - &mut self, - _element: E, - data: Option<&mut ElementData>, - ) { - if let Some(data) = data { + fn recursion_limit_exceeded(&mut self, element: E) { + if element == self.element { + self.data.hint.insert(RESTYLE_DESCENDANTS); + return; + } + + if let Some(mut data) = element.mutate_data() { data.hint.insert(RESTYLE_DESCENDANTS); } } - fn invalidated_descendants( - &mut self, - element: E, - data: Option<&mut ElementData>, - child: E, - ) { + fn invalidated_descendants(&mut self, element: E, child: E) { if child.get_data().is_none() { return; } - if data.as_ref().map_or(true, |d| d.styles.is_display_none()) { - return; - } - // The child may not be a flattened tree child of the current element, // but may be arbitrarily deep. // @@ -245,12 +239,9 @@ where } } - fn invalidated_self( - &mut self, - _element: E, - data: Option<&mut ElementData>, - ) { - if let Some(data) = data { + fn invalidated_self(&mut self, element: E) { + debug_assert_ne!(element, self.element); + if let Some(mut data) = element.mutate_data() { data.hint.insert(RESTYLE_SELF); } } diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 7ad87363066..388fb36aae8 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/components/style/invalidation/element/invalidator.rs @@ -6,7 +6,6 @@ //! element styles need to be invalidated. use context::StackLimitChecker; -use data::ElementData; use dom::{TElement, TNode}; use selector_parser::SelectorImpl; use selectors::NthIndexCache; @@ -18,9 +17,6 @@ use smallvec::SmallVec; use std::fmt; /// A trait to abstract the collection of invalidations for a given pass. -/// -/// The `data` argument is a mutable reference to the element's style data, if -/// any. pub trait InvalidationProcessor where E: TElement, @@ -36,7 +32,6 @@ where fn collect_invalidations( &mut self, element: E, - data: Option<&mut ElementData>, nth_index_cache: Option<&mut NthIndexCache>, quirks_mode: QuirksMode, descendant_invalidations: &mut InvalidationVector, @@ -45,34 +40,17 @@ where /// Returns whether the invalidation process should process the descendants /// of the given element. - fn should_process_descendants( - &mut self, - element: E, - data: Option<&mut ElementData>, - ) -> bool; + fn should_process_descendants(&mut self, element: E) -> bool; /// Executes an arbitrary action when the recursion limit is exceded (if /// any). - fn recursion_limit_exceeded( - &mut self, - element: E, - data: Option<&mut ElementData>, - ); + fn recursion_limit_exceeded(&mut self, element: E); /// Executes an action when `Self` is invalidated. - fn invalidated_self( - &mut self, - element: E, - data: Option<&mut ElementData>, - ); + fn invalidated_self(&mut self, element: E); /// Executes an action when any descendant of `Self` is invalidated. - fn invalidated_descendants( - &mut self, - element: E, - data: Option<&mut ElementData>, - child: E, - ); + fn invalidated_descendants(&mut self, element: E, child: E); } /// The struct that takes care of encapsulating all the logic on where and how @@ -83,16 +61,6 @@ where P: InvalidationProcessor { element: E, - // TODO(emilio): It's tempting enough to just avoid running invalidation for - // elements without data. - // - // But that's be wrong for sibling invalidations when a new element has been - // inserted in the tree and still has no data (though I _think_ the slow - // selector bits save us, it'd be nice not to depend on them). - // - // Seems like we could at least avoid running invalidation for the - // descendants if an element has no data, though. - data: Option<&'a mut ElementData>, quirks_mode: QuirksMode, stack_limit_checker: Option<&'a StackLimitChecker>, nth_index_cache: Option<&'a mut NthIndexCache>, @@ -232,7 +200,6 @@ where /// Trivially constructs a new `TreeStyleInvalidator`. pub fn new( element: E, - data: Option<&'a mut ElementData>, quirks_mode: QuirksMode, stack_limit_checker: Option<&'a StackLimitChecker>, nth_index_cache: Option<&'a mut NthIndexCache>, @@ -240,7 +207,6 @@ where ) -> Self { Self { element, - data, quirks_mode, stack_limit_checker, nth_index_cache, @@ -257,7 +223,6 @@ where let invalidated_self = self.processor.collect_invalidations( self.element, - self.data.as_mut().map(|d| &mut **d), self.nth_index_cache.as_mut().map(|c| &mut **c), self.quirks_mode, &mut descendant_invalidations, @@ -291,11 +256,8 @@ where let mut any_invalidated = false; while let Some(sibling) = current { - let mut sibling_data = sibling.mutate_data(); - let mut sibling_invalidator = TreeStyleInvalidator::new( sibling, - sibling_data.as_mut().map(|d| &mut **d), self.quirks_mode, self.stack_limit_checker, self.nth_index_cache.as_mut().map(|c| &mut **c), @@ -359,11 +321,8 @@ where let mut invalidated_child = false; let invalidated_descendants = { - let mut child_data = child.mutate_data(); - let mut child_invalidator = TreeStyleInvalidator::new( child, - child_data.as_mut().map(|d| &mut **d), self.quirks_mode, self.stack_limit_checker, self.nth_index_cache.as_mut().map(|c| &mut **c), @@ -392,11 +351,7 @@ where // Since we keep the traversal flags in terms of the flattened tree, // we need to propagate it as appropriate. if invalidated_child || invalidated_descendants { - self.processor.invalidated_descendants( - self.element, - self.data.as_mut().map(|d| &mut **d), - child, - ); + self.processor.invalidated_descendants(self.element, child); } invalidated_child || invalidated_descendants @@ -467,10 +422,7 @@ where debug!(" > {:?}", invalidations); let should_process = - self.processor.should_process_descendants( - self.element, - self.data.as_mut().map(|d| &mut **d), - ); + self.processor.should_process_descendants(self.element); if !should_process { return false; @@ -478,10 +430,7 @@ where if let Some(checker) = self.stack_limit_checker { if checker.limit_exceeded() { - self.processor.recursion_limit_exceeded( - self.element, - self.data.as_mut().map(|d| &mut **d) - ); + self.processor.recursion_limit_exceeded(self.element); return true; } } @@ -771,10 +720,7 @@ where } if invalidated_self { - self.processor.invalidated_self( - self.element, - self.data.as_mut().map(|d| &mut **d), - ); + self.processor.invalidated_self(self.element); } SingleInvalidationResult { invalidated_self, matched, } From a11d268468e1bbb56f96ac8de61e8696b034df2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Oct 2017 14:25:25 +0200 Subject: [PATCH 3/5] 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 | 91 +++++++---------- components/layout_thread/lib.rs | 4 +- .../script_layout_interface/wrapper_traits.rs | 8 -- components/style/dom.rs | 98 +++++++++++++------ components/style/gecko/wrapper.rs | 96 ++++++++---------- .../style/invalidation/element/invalidator.rs | 2 +- components/style/invalidation/stylesheets.rs | 2 +- components/style/traversal.rs | 4 +- ports/geckolib/glue.rs | 2 +- 9 files changed, 149 insertions(+), 158 deletions(-) diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 08d628a6d6c..45db07f43de 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/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 { unsafe { @@ -167,20 +166,34 @@ impl<'ln> TNode for ServoLayoutNode<'ln> { } } - fn children(&self) -> LayoutIterator> { - LayoutIterator(ServoChildrenIterator { - current: self.first_child(), - }) + fn first_child(&self) -> Option { + unsafe { + self.node.first_child_ref().map(|node| self.new_with_this_lifetime(&node)) + } + } + + fn last_child(&self) -> Option { + unsafe { + self.node.last_child_ref().map(|node| self.new_with_this_lifetime(&node)) + } + } + + fn prev_sibling(&self) -> Option { + unsafe { + self.node.prev_sibling_ref().map(|node| self.new_with_this_lifetime(&node)) + } + } + + fn next_sibling(&self) -> Option { + unsafe { + self.node.next_sibling_ref().map(|node| self.new_with_this_lifetime(&node)) + } } fn traversal_parent(&self) -> Option> { self.parent_element() } - fn traversal_children(&self) -> LayoutIterator> { - 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>, -} - -impl<'a> Iterator for ServoChildrenIterator<'a> { - type Item = ServoLayoutNode<'a>; - fn next(&mut self) -> Option> { - 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> { - unsafe { - self.node.first_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } - - fn last_child(&self) -> Option> { - unsafe { - self.node.last_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } - - fn prev_sibling(&self) -> Option> { - unsafe { - self.node.prev_sibling_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } - - fn next_sibling(&self) -> Option> { - 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> { - self.as_node().children().find(ServoLayoutNode::is_element) + pub fn root_element(&self) -> Option> { + 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; 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 { + LayoutIterator(self.as_node().dom_children()) + } + fn style_attribute(&self) -> Option>> { 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> { - 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> { @@ -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> { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 3132eccf68a..528e06c6424 100644 --- a/components/layout_thread/lib.rs +++ b/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={:?})", diff --git a/components/script_layout_interface/wrapper_traits.rs b/components/script_layout_interface/wrapper_traits.rs index 8221a3f885c..a0571c66ff0 100644 --- a/components/script_layout_interface/wrapper_traits.rs +++ b/components/script_layout_interface/wrapper_traits.rs @@ -100,14 +100,6 @@ pub trait LayoutNode: Debug + GetLayoutData + TNode { fn traverse_preorder(self) -> TreeIterator { TreeIterator::new(self) } - - fn first_child(&self) -> Option; - - fn last_child(&self) -> Option; - - fn prev_sibling(&self) -> Option; - - fn next_sibling(&self) -> Option; } pub struct ReverseChildrenIterator where ConcreteNode: LayoutNode { diff --git a/components/style/dom.rs b/components/style/dom.rs index 1733b580184..91f51fdd7c2 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -64,61 +64,86 @@ 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(pub T); -impl Iterator for LayoutIterator - where T: Iterator, - I: NodeInfo, +impl Iterator for LayoutIterator +where + T: Iterator, + N: NodeInfo, { - type Item = I; - fn next(&mut self) -> Option { + type Item = N; + + fn next(&mut self) -> Option { loop { - // Filter out nodes that layout should ignore. - let n = self.0.next(); - if n.is_none() || n.as_ref().unwrap().needs_layout() { - return n + match self.0.next() { + Some(n) => { + // Filter out nodes that layout should ignore. + 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(Option); +impl Iterator for DomChildren +where + N: TNode +{ + type Item = N; + + fn next(&mut self) -> Option { + match self.0.take() { + Some(n) => { + self.0 = n.next_sibling(); + Some(n) + } + None => None, + } + } +} + /// The `TNode` trait. This is the main generic trait over which the style /// system can be implemented. pub trait TNode : Sized + Copy + Clone + Debug + NodeInfo { /// The concrete `TElement` type. type ConcreteElement: TElement; - /// 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; - /// Get this node's parent node. fn parent_node(&self) -> Option; - /// Get this node's parent element if present. - fn parent_element(&self) -> Option { - self.parent_node().and_then(|n| n.as_element()) - } + /// Get this node's first child. + fn first_child(&self) -> Option; - /// Returns an iterator over this node's children. - fn children(&self) -> LayoutIterator; + /// Get this node's first child. + fn last_child(&self) -> Option; + + /// Get this node's previous sibling. + fn prev_sibling(&self) -> Option; + + /// Get this node's next sibling. + fn next_sibling(&self) -> Option; + + /// Iterate over the DOM children of an element. + fn dom_children(&self) -> DomChildren { + DomChildren(self.first_child()) + } /// Get this node's parent element from the perspective of a restyle /// traversal. fn traversal_parent(&self) -> Option; - /// Get this node's children from the perspective of a restyle traversal. - fn traversal_children(&self) -> LayoutIterator; + /// Get this node's parent element if present. + fn parent_element(&self) -> Option { + self.parent_node().and_then(|n| n.as_element()) + } /// Converts self into an `OpaqueNode`. fn opaque(&self) -> OpaqueNode; @@ -223,9 +248,11 @@ fn fmt_subtree(f: &mut fmt::Formatter, stringify: &F, n: N, indent: write!(f, " ")?; } stringify(f, n)?; - for kid in n.traversal_children() { - writeln!(f, "")?; - fmt_subtree(f, stringify, kid, indent + 1)?; + 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; + /// 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; + /// 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; + /// Returns the parent element we should inherit from. /// /// This is pretty much always the parent element itself, except in the case diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index e259422c629..5b70f6207a0 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -158,32 +158,6 @@ impl<'ln> GeckoNode<'ln> { unsafe { &*self.node_info().mDocument } } - #[inline] - fn first_child(&self) -> Option> { - unsafe { self.0.mFirstChild.as_ref().map(GeckoNode::from_content) } - } - - #[inline] - fn last_child(&self) -> Option> { - unsafe { Gecko_GetLastChild(self.0).map(GeckoNode) } - } - - #[inline] - fn prev_sibling(&self) -> Option> { - unsafe { self.0.mPreviousSibling.as_ref().map(GeckoNode::from_content) } - } - - #[inline] - fn next_sibling(&self) -> Option> { - 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 { unsafe { self.0.mParent.as_ref().map(GeckoNode) } } - fn children(&self) -> LayoutIterator> { - LayoutIterator(self.dom_children()) + #[inline] + fn first_child(&self) -> Option { + unsafe { self.0.mFirstChild.as_ref().map(GeckoNode::from_content) } + } + + #[inline] + fn last_child(&self) -> Option { + unsafe { Gecko_GetLastChild(self.0).map(GeckoNode) } + } + + #[inline] + fn prev_sibling(&self) -> Option { + unsafe { self.0.mPreviousSibling.as_ref().map(GeckoNode::from_content) } + } + + #[inline] + fn next_sibling(&self) -> Option { + unsafe { self.0.mNextSibling.as_ref().map(GeckoNode::from_content) } } fn traversal_parent(&self) -> Option> { self.flattened_tree_parent().and_then(|n| n.as_element()) } - fn traversal_children(&self) -> LayoutIterator> { - 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( 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 { if self.is_native_anonymous() { @@ -894,6 +864,24 @@ impl<'le> TElement for GeckoElement<'le> { } } + fn traversal_children(&self) -> LayoutIterator> { + // 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.get_before_or_after_pseudo(/* is_before = */ true) } diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs index 388fb36aae8..99e42e6d860 100644 --- a/components/style/invalidation/element/invalidator.rs +++ b/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 fine, because they appear // in selector-matching (note bug 1374247, though). // diff --git a/components/style/invalidation/stylesheets.rs b/components/style/invalidation/stylesheets.rs index c65dfc056bc..2e2abdf4960 100644 --- a/components/style/invalidation/stylesheets.rs +++ b/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, diff --git a/components/style/traversal.rs b/components/style/traversal.rs index d31d1abff0e..7737ce044f0 100644 --- a/components/style/traversal.rs +++ b/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. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index 9e7c2f22f2d..33b95b53a09 100644 --- a/ports/geckolib/glue.rs +++ b/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); } From 7afe393f2392988d72d617a2fda2a5b89db0999c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Oct 2017 16:18:01 +0200 Subject: [PATCH 4/5] style: Remove a few unused functions. --- components/layout_thread/dom_wrapper.rs | 5 ----- components/script_layout_interface/wrapper_traits.rs | 12 ------------ 2 files changed, 17 deletions(-) diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 45db07f43de..1732d4cc3e8 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/components/layout_thread/dom_wrapper.rs @@ -858,11 +858,6 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> { } } - #[inline] - fn type_id_without_excluding_pseudo_elements(&self) -> LayoutNodeType { - self.node.type_id() - } - fn parent_style(&self) -> Arc { let parent = self.node.parent_node().unwrap().as_element().unwrap(); let parent_data = parent.get_data().unwrap().borrow(); diff --git a/components/script_layout_interface/wrapper_traits.rs b/components/script_layout_interface/wrapper_traits.rs index a0571c66ff0..b94ee469ecb 100644 --- a/components/script_layout_interface/wrapper_traits.rs +++ b/components/script_layout_interface/wrapper_traits.rs @@ -161,10 +161,6 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + Debug + GetLayoutData + NodeInfo /// Returns `None` if this is a pseudo-element; otherwise, returns `Some`. fn type_id(&self) -> Option; - /// Returns the type ID of this node, without discarding pseudo-elements as - /// `type_id` does. - fn type_id_without_excluding_pseudo_elements(&self) -> LayoutNodeType; - /// Returns the style for a text node. This is computed on the fly from the /// parent style to avoid traversing text nodes in the style system. /// @@ -175,14 +171,6 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + Debug + GetLayoutData + NodeInfo /// the parent until all the children have been processed. fn parent_style(&self) -> Arc; - #[inline] - fn is_element_or_elements_pseudo(&self) -> bool { - match self.type_id_without_excluding_pseudo_elements() { - LayoutNodeType::Element(..) => true, - _ => false, - } - } - fn get_before_pseudo(&self) -> Option { self.as_element().and_then(|el| el.get_before_pseudo()).map(|el| el.as_node()) } From 96b71754c3f02f11aadfda0039c1172a6ad90eed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 16 Oct 2017 14:36:13 +0200 Subject: [PATCH 5/5] style: Remove unused TNode::is_in_doc. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emilio Cobos Álvarez --- components/layout_thread/dom_wrapper.rs | 4 ---- components/style/dom.rs | 4 ---- components/style/gecko/wrapper.rs | 4 ---- 3 files changed, 12 deletions(-) diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 1732d4cc3e8..f19e5e13b78 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/components/layout_thread/dom_wrapper.rs @@ -213,10 +213,6 @@ impl<'ln> TNode for ServoLayoutNode<'ln> { unsafe fn set_can_be_fragmented(&self, value: bool) { self.node.set_flag(CAN_BE_FRAGMENTED, value) } - - fn is_in_doc(&self) -> bool { - unsafe { (*self.node.unsafe_get()).is_in_doc() } - } } impl<'ln> LayoutNode for ServoLayoutNode<'ln> { diff --git a/components/style/dom.rs b/components/style/dom.rs index 91f51fdd7c2..766b217863f 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -160,10 +160,6 @@ pub trait TNode : Sized + Copy + Clone + Debug + NodeInfo { /// Set whether this node can be fragmented. unsafe fn set_can_be_fragmented(&self, value: bool); - - /// Whether this node is in the document right now needed to clear the - /// restyle data appropriately on some forced restyles. - fn is_in_doc(&self) -> bool; } /// Wrapper to output the ElementData along with the node when formatting for diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index 5b70f6207a0..3ba4b5d470f 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -270,10 +270,6 @@ impl<'ln> TNode for GeckoNode<'ln> { // FIXME(SimonSapin): Servo uses this to implement CSS multicol / fragmentation // Maybe this isn’t useful for Gecko? } - - fn is_in_doc(&self) -> bool { - unsafe { bindings::Gecko_IsInDocument(self.0) } - } } /// A wrapper on top of two kind of iterators, depending on the parent being