Don't traverse text nodes during styling.

MozReview-Commit-ID: 6CtQMxbcLnF

components/layout/traversal.rs

@@ -18,8 +18,10 @@ use style::context::{LocalStyleContext, SharedStyleContext, StyleContext};
 use style::data::NodeData;
 use style::dom::TNode;
 use style::selector_impl::ServoSelectorImpl;
-use style::traversal::{DomTraversalContext, recalc_style_at, remove_from_bloom_filter};
+use style::traversal::{DomTraversalContext, put_thread_local_bloom_filter};
+use style::traversal::{recalc_style_at, remove_from_bloom_filter};
 use style::traversal::RestyleResult;
+use style::traversal::take_thread_local_bloom_filter;
 use util::opts;
 use wrapper::{LayoutNodeLayoutData, ThreadSafeLayoutNodeHelpers};
 
@@ -77,7 +79,36 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         // done by the HTML parser.
         node.initialize_data();
 
-        recalc_style_at::<_, _, Self>(&self.context, self.root, node)
+        if node.is_text_node() {
+            // FIXME(bholley): Stop doing this silly work to maintain broken bloom filter
+            // invariants.
+            //
+            // Longer version: The bloom filter is entirely busted for parallel traversal. Because
+            // parallel traversal is breadth-first, each sibling rejects the bloom filter set up
+            // by the previous sibling (which is valid for children, not siblings) and recreates
+            // it. Similarly, the fixup performed in the bottom-up traversal is useless, because
+            // threads perform flow construction up the parent chain until they find a parent with
+            // other unprocessed children, at which point they bail to the work queue and find a
+            // different node.
+            //
+            // Nevertheless, the remove_from_bloom_filter call at the end of flow construction
+            // asserts that the bloom filter is valid for the current node. This breaks when we
+            // stop calling recalc_style_at for text nodes, because the recursive chain of
+            // construct_flows_at calls is no longer necessarily rooted in a call that sets up the
+            // thread-local bloom filter for the leaf node.
+            //
+            // The bloom filter stuff is all going to be rewritten, so we just hackily duplicate
+            // the bloom filter manipulation from recalc_style_at to maintain invariants.
+            let parent = node.parent_node();
+            debug_assert!(parent.unwrap().is_element());
+            let bf = take_thread_local_bloom_filter(parent, self.root, self.context.shared_context());
+            put_thread_local_bloom_filter(bf, &node.to_unsafe(), self.context.shared_context());
+
+            RestyleResult::Stop
+        } else {
+            let el = node.as_element().unwrap();
+            recalc_style_at::<_, _, Self>(&self.context, self.root, el)
+        }
     }
 
     fn process_postorder(&self, node: N) {

components/script/layout_wrapper.rs

@@ -763,6 +763,9 @@ impl<'ln> NodeInfo for ServoThreadSafeLayoutNode<'ln> {
     }
 
     fn is_text_node(&self) -> bool {
+        // It's unlikely that text nodes will ever be used to implement a
+        // pseudo-element, but the type system doesn't really enforce that,
+        // so we check to be safe.
         self.pseudo == PseudoElementType::Normal && self.node.is_text_node()
     }
 
@@ -802,11 +805,18 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> {
     }
 
     fn style_for_text_node(&self) -> Arc<ComputedValues> {
+        // Text nodes get a copy of the parent style. Inheriting all non-
+        // inherited properties into the text node is odd from a CSS
+        // perspective, but makes fragment construction easier (by making
+        // properties like vertical-align on fragments have values that
+        // match the parent element). This is an implementation detail of
+        // Servo layout that is not central to how fragment construction
+        // works, but would be difficult to change. (Text node style is
+        // also not visible to script.)
         debug_assert!(self.is_text_node());
         let parent = self.node.parent_node().unwrap();
         let parent_data = parent.get_style_data().unwrap().borrow();
-        let parent_style = &parent_data.current_styles().primary;
-        ComputedValues::style_for_child_text_node(parent_style)
+        parent_data.current_styles().primary.clone()
     }
 
     fn debug_id(self) -> usize {

components/script_layout_interface/wrapper_traits.rs

@@ -323,6 +323,7 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + NodeInfo + PartialEq + Sized {
     /// element style is precomputed, not from general layout itself.
     #[inline]
     fn resolved_style(&self) -> Arc<ServoComputedValues> {
+        // FIXME(bholley): This should move to Element and lose the text node check.
         if self.is_text_node() {
             return self.style_for_text_node();
         }
@@ -338,6 +339,7 @@ pub trait ThreadSafeLayoutNode: Clone + Copy + NodeInfo + PartialEq + Sized {
 
     #[inline]
     fn selected_style(&self, _context: &SharedStyleContext) -> Arc<ServoComputedValues> {
+        // FIXME(bholley): This should move to Element and lose the text node check.
         if self.is_text_node() {
             return self.style_for_text_node();
         }

components/style/dom.rs

@@ -268,7 +268,8 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     fn existing_style_for_restyle_damage<'a>(&'a self,
                                              current_computed_values: Option<&'a Arc<ComputedValues>>,
                                              pseudo: Option<&PseudoElement>)
-        -> Option<&'a <<Self::ConcreteNode as TNode>::ConcreteRestyleDamage as TRestyleDamage>::PreExistingComputedValues>;
+        -> Option<&'a <<Self::ConcreteNode as TNode>::ConcreteRestyleDamage as TRestyleDamage>
+                      ::PreExistingComputedValues>;
 
     /// Properly marks nodes as dirty in response to restyle hints.
     fn note_restyle_hint<C: DomTraversalContext<Self::ConcreteNode>>(&self, hint: RestyleHint) {

components/style/gecko/traversal.rs

@@ -5,7 +5,7 @@
 use atomic_refcell::AtomicRefCell;
 use context::{LocalStyleContext, SharedStyleContext, StyleContext};
 use data::NodeData;
-use dom::OpaqueNode;
+use dom::{NodeInfo, OpaqueNode, TNode};
 use gecko::context::StandaloneStyleContext;
 use gecko::wrapper::GeckoNode;
 use std::mem;
@@ -31,7 +31,14 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
     }
 
     fn process_preorder(&self, node: GeckoNode<'ln>) -> RestyleResult {
-        recalc_style_at::<_, _, Self>(&self.context, self.root, node)
+        if node.is_text_node() {
+            // Text nodes don't have children, so save the traversal algorithm
+            // the trouble of iterating the children.
+            RestyleResult::Stop
+        } else {
+            let el = node.as_element().unwrap();
+            recalc_style_at::<_, _, Self>(&self.context, self.root, el)
+        }
     }
 
     fn process_postorder(&self, _: GeckoNode<'ln>) {

components/style/properties/properties.mako.rs

@@ -1084,18 +1084,6 @@ impl ComputedValues {
         }
     }
 
-    pub fn style_for_child_text_node(parent: &Arc<Self>) -> Arc<Self> {
-        // Text nodes get a copy of the parent style. Inheriting all non-
-        // inherited properties into the text node is odd from a CSS
-        // perspective, but makes fragment construction easier (by making
-        // properties like vertical-align on fragments have values that
-        // match the parent element). This is an implementation detail of
-        // Servo layout that is not central to how fragment construction
-        // works, but would be difficult to change. (Text node style is
-        // also not visible to script.)
-        parent.clone()
-    }
-
     pub fn initial_values() -> &'static Self { &*INITIAL_SERVO_VALUES }
 
     #[inline]

components/style/traversal.rs

@@ -7,7 +7,7 @@
 use atomic_refcell::AtomicRefCell;
 use context::{LocalStyleContext, SharedStyleContext, StyleContext};
 use data::NodeData;
-use dom::{OpaqueNode, StylingMode, TElement, TNode, UnsafeNode};
+use dom::{NodeInfo, OpaqueNode, StylingMode, TElement, TNode, UnsafeNode};
 use matching::{ApplicableDeclarations, ElementMatchMethods, MatchMethods, StyleSharingResult};
 use selectors::bloom::BloomFilter;
 use selectors::matching::StyleRelations;
@@ -62,11 +62,11 @@ thread_local!(
 ///
 /// If one does not exist, a new one will be made for you. If it is out of date,
 /// it will be cleared and reused.
-fn take_thread_local_bloom_filter<N>(parent_node: Option<N>,
-                                     root: OpaqueNode,
-                                     context: &SharedStyleContext)
-                                     -> Box<BloomFilter>
-                                     where N: TNode {
+pub fn take_thread_local_bloom_filter<N>(parent_node: Option<N>,
+                                         root: OpaqueNode,
+                                         context: &SharedStyleContext)
+                                         -> Box<BloomFilter>
+                                         where N: TNode {
     STYLE_BLOOM.with(|style_bloom| {
         match (parent_node, style_bloom.borrow_mut().take()) {
             // Root node. Needs new bloom filter.
@@ -98,8 +98,8 @@ fn take_thread_local_bloom_filter<N>(parent_node: Option<N>,
     })
 }
 
-fn put_thread_local_bloom_filter(bf: Box<BloomFilter>, unsafe_node: &UnsafeNode,
-                                 context: &SharedStyleContext) {
+pub fn put_thread_local_bloom_filter(bf: Box<BloomFilter>, unsafe_node: &UnsafeNode,
+                                     context: &SharedStyleContext) {
     STYLE_BLOOM.with(move |style_bloom| {
         assert!(style_bloom.borrow().is_none(),
                 "Putting into a never-taken thread-local bloom filter");
@@ -284,13 +284,15 @@ fn ensure_node_styled_internal<'a, N, C>(node: N,
 /// Calculates the style for a single node.
 #[inline]
 #[allow(unsafe_code)]
-pub fn recalc_style_at<'a, N, C, D>(context: &'a C,
+pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
                                     root: OpaqueNode,
-                                    node: N) -> RestyleResult
-    where N: TNode,
+                                    element: E) -> RestyleResult
+    where E: TElement,
           C: StyleContext<'a>,
-          D: DomTraversalContext<N>
+          D: DomTraversalContext<E::ConcreteNode>
 {
+    let node = element.as_node();
+
     // Get the parent node.
     let parent_opt = match node.parent_node() {
         Some(parent) if parent.is_element() => Some(parent),