Auto merge of #14010 - bholley:element_data_management, r=emilio

incremental restyle: Centralize pre-styling setup, eliminate RestyleResult, and drop data for display:none subtrees

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14010)
<!-- Reviewable:end -->

components/layout/traversal.rs

@@ -19,16 +19,16 @@ use style::data::ElementData;
 use style::dom::{StylingMode, TElement, TNode};
 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::{LayoutNodeHelpers, LayoutNodeLayoutData};
+use wrapper::{GetRawData, LayoutNodeHelpers, LayoutNodeLayoutData};
 
 pub struct RecalcStyleAndConstructFlows<'lc> {
     context: LayoutContext<'lc>,
     root: OpaqueNode,
 }
 
+#[allow(unsafe_code)]
 impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
     where N: LayoutNode + TNode,
           N::ConcreteElement: LayoutElement
@@ -73,7 +73,7 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         }
     }
 
-    fn process_preorder(&self, node: N) -> RestyleResult {
+    fn process_preorder(&self, node: N) {
         // FIXME(pcwalton): Stop allocating here. Ideally this should just be
         // done by the HTML parser.
         node.initialize_data();
@@ -101,11 +101,9 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
             let parent = node.parent_node().unwrap().as_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)
+            recalc_style_at::<_, _, Self>(&self.context, self.root, el);
         }
     }
 
@@ -114,8 +112,13 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
     }
 
     fn should_traverse_child(parent: N::ConcreteElement, child: N) -> bool {
+        // If the parent is display:none, we don't need to do anything.
+        if parent.is_display_none() {
+            return false;
+        }
+
         // If this node has been marked as damaged in some way, we need to
-        // traverse it unconditionally for layout.
+        // traverse it for layout.
         if child.has_changed() {
             return true;
         }
@@ -131,9 +134,13 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         }
     }
 
-    fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData> {
+    unsafe fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData> {
         element.as_node().initialize_data();
-        element.get_style_data().unwrap()
+        element.get_data().unwrap()
+    }
+
+    unsafe fn clear_element_data(element: &N::ConcreteElement) {
+        element.as_node().clear_data();
     }
 
     fn local_context(&self) -> &LocalStyleContext {

components/layout/wrapper.rs

@@ -33,13 +33,21 @@
 use core::nonzero::NonZero;
 use data::{LayoutDataFlags, PersistentLayoutData};
 use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
-use script_layout_interface::wrapper_traits::{ThreadSafeLayoutElement, ThreadSafeLayoutNode};
+use script_layout_interface::wrapper_traits::{LayoutNode, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use script_layout_interface::wrapper_traits::GetLayoutData;
 use style::atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 use style::computed_values::content::{self, ContentItem};
+use style::dom::TElement;
+use style::traversal::prepare_for_styling;
 
 pub type NonOpaqueStyleAndLayoutData = AtomicRefCell<PersistentLayoutData>;
 
+pub unsafe fn drop_style_and_layout_data(data: OpaqueStyleAndLayoutData) {
+    let ptr: *mut AtomicRefCell<PartialPersistentLayoutData> = *data.ptr;
+    let non_opaque: *mut NonOpaqueStyleAndLayoutData = ptr as *mut _;
+    let _ = Box::from_raw(non_opaque);
+}
+
 pub trait LayoutNodeLayoutData {
     /// Similar to borrow_data*, but returns the full PersistentLayoutData rather
     /// than only the style::data::ElementData.
@@ -62,12 +70,25 @@ impl<T: GetLayoutData> LayoutNodeLayoutData for T {
     }
 }
 
-pub trait LayoutNodeHelpers {
+pub trait GetRawData {
-    fn initialize_data(&self);
     fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData>;
 }
 
-impl<T: GetLayoutData> LayoutNodeHelpers for T {
+impl<T: GetLayoutData> GetRawData for T {
+    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData> {
+        self.get_style_and_layout_data().map(|opaque| {
+            let container = *opaque.ptr as *mut NonOpaqueStyleAndLayoutData;
+            unsafe { &*container }
+        })
+    }
+}
+
+pub trait LayoutNodeHelpers {
+    fn initialize_data(&self);
+    fn clear_data(&self);
+}
+
+impl<T: LayoutNode> LayoutNodeHelpers for T {
     fn initialize_data(&self) {
         if self.get_raw_data().is_none() {
             let ptr: *mut NonOpaqueStyleAndLayoutData =
@@ -75,15 +96,17 @@ impl<T: GetLayoutData> LayoutNodeHelpers for T {
             let opaque = OpaqueStyleAndLayoutData {
                 ptr: unsafe { NonZero::new(ptr as *mut AtomicRefCell<PartialPersistentLayoutData>) }
             };
-            self.init_style_and_layout_data(opaque);
+            unsafe { self.init_style_and_layout_data(opaque) };
+            if let Some(el) = self.as_element() {
+                let _ = prepare_for_styling(el, el.get_data().unwrap());
+            }
         };
     }
 
-    fn get_raw_data(&self) -> Option<&NonOpaqueStyleAndLayoutData> {
+    fn clear_data(&self) {
-        self.get_style_and_layout_data().map(|opaque| {
+        if self.get_raw_data().is_some() {
-            let container = *opaque.ptr as *mut NonOpaqueStyleAndLayoutData;
+            unsafe { drop_style_and_layout_data(self.take_style_and_layout_data()) };
-            unsafe { &*container }
+        }
-        })
     }
 }
 

components/layout_thread/lib.rs

@@ -77,7 +77,8 @@ use layout::query::process_offset_parent_query;
 use layout::sequential;
 use layout::traversal::{ComputeAbsolutePositions, RecalcStyleAndConstructFlows};
 use layout::webrender_helpers::{WebRenderDisplayListConverter, WebRenderFrameBuilder};
-use layout::wrapper::{LayoutNodeLayoutData, NonOpaqueStyleAndLayoutData};
+use layout::wrapper::LayoutNodeLayoutData;
+use layout::wrapper::drop_style_and_layout_data;
 use layout_traits::LayoutThreadFactory;
 use msg::constellation_msg::PipelineId;
 use net_traits::image_cache_thread::{ImageCacheChan, ImageCacheResult, ImageCacheThread};
@@ -86,7 +87,6 @@ use profile_traits::mem::{self, Report, ReportKind, ReportsChan};
 use profile_traits::time::{self, TimerMetadata, profile};
 use profile_traits::time::{TimerMetadataFrameType, TimerMetadataReflowType};
 use script::layout_wrapper::{ServoLayoutDocument, ServoLayoutNode};
-use script_layout_interface::{OpaqueStyleAndLayoutData, PartialPersistentLayoutData};
 use script_layout_interface::message::{Msg, NewLayoutThreadInfo, Reflow, ReflowQueryType, ScriptReflow};
 use script_layout_interface::reporter::CSSErrorReporter;
 use script_layout_interface::restyle_damage::{REFLOW, REFLOW_OUT_OF_FLOW, REPAINT, REPOSITION};
@@ -105,7 +105,6 @@ use std::sync::{Arc, Mutex, MutexGuard, RwLock};
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::sync::mpsc::{Receiver, Sender, channel};
 use style::animation::Animation;
-use style::atomic_refcell::AtomicRefCell;
 use style::computed_values::{filter, mix_blend_mode};
 use style::context::{LocalStyleContextCreationInfo, ReflowGoal, SharedStyleContext};
 use style::dom::{TDocument, TElement, TNode};
@@ -648,7 +647,7 @@ impl LayoutThread {
             }
             Msg::ReapStyleAndLayoutData(dead_data) => {
                 unsafe {
-                    self.handle_reap_style_and_layout_data(dead_data)
+                    drop_style_and_layout_data(dead_data)
                 }
             }
             Msg::CollectReports(reports_chan) => {
@@ -756,7 +755,7 @@ impl LayoutThread {
             match self.port.recv().unwrap() {
                 Msg::ReapStyleAndLayoutData(dead_data) => {
                     unsafe {
-                        self.handle_reap_style_and_layout_data(dead_data)
+                        drop_style_and_layout_data(dead_data)
                     }
                 }
                 Msg::ExitNow => {
@@ -1483,14 +1482,6 @@ impl LayoutThread {
         }
     }
 
-    /// Handles a message to destroy layout data. Layout data must be destroyed on *this* thread
-    /// because the struct type is transmuted to a different type on the script side.
-    unsafe fn handle_reap_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        let ptr: *mut AtomicRefCell<PartialPersistentLayoutData> = *data.ptr;
-        let non_opaque: *mut NonOpaqueStyleAndLayoutData = ptr as *mut _;
-        let _ = Box::from_raw(non_opaque);
-    }
-
     /// Returns profiling information which is passed to the time profiler.
     fn profiler_metadata(&self) -> Option<TimerMetadata> {
         Some(TimerMetadata {

components/script/dom/node.rs

@@ -952,6 +952,7 @@ pub trait LayoutNodeHelpers {
 
     unsafe fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData>;
     unsafe fn init_style_and_layout_data(&self, OpaqueStyleAndLayoutData);
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData;
 
     fn text_content(&self) -> String;
     fn selection(&self) -> Option<Range<usize>>;
@@ -1051,6 +1052,14 @@ impl LayoutNodeHelpers for LayoutJS<Node> {
         (*self.unsafe_get()).style_and_layout_data.set(Some(val));
     }
 
+    #[inline]
+    #[allow(unsafe_code)]
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData {
+        let val = (*self.unsafe_get()).style_and_layout_data.get().unwrap();
+        (*self.unsafe_get()).style_and_layout_data.set(None);
+        val
+    }
+
     #[allow(unsafe_code)]
     fn text_content(&self) -> String {
         if let Some(text) = self.downcast::<Text>() {

components/script/layout_wrapper.rs

@@ -56,7 +56,7 @@ use std::mem::transmute;
 use std::sync::Arc;
 use std::sync::atomic::Ordering;
 use string_cache::{Atom, Namespace};
-use style::atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
+use style::atomic_refcell::{AtomicRef, AtomicRefCell};
 use style::attr::AttrValue;
 use style::computed_values::display;
 use style::context::SharedStyleContext;
@@ -252,6 +252,14 @@ impl<'ln> LayoutNode for ServoLayoutNode<'ln> {
         self.script_type_id().into()
     }
 
+    unsafe fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
+        self.get_jsmanaged().init_style_and_layout_data(data);
+    }
+
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData {
+        self.get_jsmanaged().take_style_and_layout_data()
+    }
+
     fn has_changed(&self) -> bool {
         unsafe { self.node.get_flag(HAS_CHANGED) }
     }
@@ -269,42 +277,24 @@ impl<'ln> GetLayoutData for ServoLayoutNode<'ln> {
             self.get_jsmanaged().get_style_and_layout_data()
         }
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        unsafe {
-            self.get_jsmanaged().init_style_and_layout_data(data);
-        }
-    }
 }
 
 impl<'le> GetLayoutData for ServoLayoutElement<'le> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.as_node().get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.as_node().init_style_and_layout_data(data)
-    }
 }
 
 impl<'ln> GetLayoutData for ServoThreadSafeLayoutNode<'ln> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.node.get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.node.init_style_and_layout_data(data)
-    }
 }
 
 impl<'le> GetLayoutData for ServoThreadSafeLayoutElement<'le> {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData> {
         self.element.as_node().get_style_and_layout_data()
     }
-
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData) {
-        self.element.as_node().init_style_and_layout_data(data)
-    }
 }
 
 impl<'ln> ServoLayoutNode<'ln> {
@@ -506,20 +496,11 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         old_value - 1
     }
 
-    fn begin_styling(&self) -> AtomicRefMut<ElementData> {
-        let mut data = self.mutate_data().unwrap();
-        data.gather_previous_styles(|| None);
-        data
-    }
-
     fn borrow_data(&self) -> Option<AtomicRef<ElementData>> {
-        self.get_style_data().map(|d| d.borrow())
+        self.get_data().map(|d| d.borrow())
     }
 
-}
+    fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
-
-impl<'le> LayoutElement for ServoLayoutElement<'le> {
-    fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| {
                 let ppld: &AtomicRefCell<PartialPersistentLayoutData> = &**d.ptr;
@@ -551,10 +532,6 @@ impl<'le> ServoLayoutElement<'le> {
         }
     }
 
-    fn mutate_data(&self) -> Option<AtomicRefMut<ElementData>> {
-        self.get_style_data().map(|d| d.borrow_mut())
-    }
-
     fn get_partial_layout_data(&self) -> Option<&AtomicRefCell<PartialPersistentLayoutData>> {
         unsafe {
             self.get_style_and_layout_data().map(|d| &**d.ptr)
@@ -836,7 +813,7 @@ impl<'ln> ThreadSafeLayoutNode for ServoThreadSafeLayoutNode<'ln> {
         // also not visible to script.)
         debug_assert!(self.is_text_node());
         let parent = self.node.parent_node().unwrap().as_element().unwrap();
-        let parent_data = parent.get_style_data().unwrap().borrow();
+        let parent_data = parent.get_data().unwrap().borrow();
         parent_data.current_styles().primary.clone()
     }
 
@@ -1089,10 +1066,12 @@ impl<'le> ThreadSafeLayoutElement for ServoThreadSafeLayoutElement<'le> {
     }
 
     fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>> {
-        self.element.get_style_data()
+        self.element.get_data()
     }
 }
 
+impl<'le> LayoutElement for ServoLayoutElement<'le> {}
+
 /// This implementation of `::selectors::Element` is used for implementing lazy
 /// pseudo-elements.
 ///

components/script_layout_interface/wrapper_traits.rs

@@ -73,7 +73,6 @@ impl<T> PseudoElementType<T> {
 /// Trait to abstract access to layout data across various data structures.
 pub trait GetLayoutData {
     fn get_style_and_layout_data(&self) -> Option<OpaqueStyleAndLayoutData>;
-    fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData);
 }
 
 /// A wrapper so that layout can access only the methods that it should have access to. Layout must
@@ -85,6 +84,9 @@ pub trait LayoutNode: GetLayoutData + TNode {
     /// Returns the type ID of this node.
     fn type_id(&self) -> LayoutNodeType;
 
+    unsafe fn init_style_and_layout_data(&self, data: OpaqueStyleAndLayoutData);
+    unsafe fn take_style_and_layout_data(&self) -> OpaqueStyleAndLayoutData;
+
     fn has_changed(&self) -> bool;
 
     unsafe fn clear_dirty_bits(&self);
@@ -274,7 +276,6 @@ pub trait DangerousThreadSafeLayoutNode: ThreadSafeLayoutNode {
 }
 
 pub trait LayoutElement: Clone + Copy + Sized + Debug + GetLayoutData + TElement {
-    fn get_style_data(&self) -> Option<&AtomicRefCell<ElementData>>;
 }
 
 pub trait ThreadSafeLayoutElement: Clone + Copy + Sized + Debug +

components/style/atomic_refcell.rs

@@ -27,10 +27,10 @@ impl<T> AtomicRefCell<T> {
         AtomicRefCell(RwLock::new(value))
     }
     pub fn borrow(&self) -> AtomicRef<T> {
-        self.0.try_read().unwrap()
+        self.0.try_read().expect("already mutably borrowed")
     }
     pub fn borrow_mut(&self) -> AtomicRefMut<T> {
-        self.0.try_write().unwrap()
+        self.0.try_write().expect("already borrowed")
     }
 }
 

components/style/dom.rs

@@ -6,11 +6,12 @@
 
 #![allow(unsafe_code)]
 
-use atomic_refcell::{AtomicRef, AtomicRefMut};
+use atomic_refcell::{AtomicRef, AtomicRefCell};
 use data::{ElementStyles, ElementData};
 use element_state::ElementState;
 use parking_lot::RwLock;
 use properties::{ComputedValues, PropertyDeclarationBlock};
+use properties::longhands::display::computed_value as display;
 use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RestyleHint};
 use selector_impl::{ElementExt, PseudoElement};
 use selector_matching::ApplicableDeclarationBlock;
@@ -213,6 +214,13 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     /// traversal. Returns the number of children left to process.
     fn did_process_child(&self) -> isize;
 
+    /// Returns true if this element's current style is display:none. Only valid
+    /// to call after styling.
+    fn is_display_none(&self) -> bool {
+        self.borrow_data().unwrap()
+            .current_styles().primary
+            .get_box().clone_display() == display::T::none
+    }
 
     /// Returns true if this node has a styled layout frame that owns the style.
     fn frame_has_style(&self) -> bool { false }
@@ -243,30 +251,43 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
             Stop
         };
 
-        match self.borrow_data() {
+        let mut mode = match self.borrow_data() {
-            // No node data, no style on the frame.
+            // No element data, no style on the frame.
             None if !self.frame_has_style() => Initial,
-            // No node data, style on the frame.
+            // No element data, style on the frame.
             None => mode_for_descendants,
+            // We have element data. Decide below.
             Some(d) => {
-                if d.restyle_data.is_some() || self.deprecated_dirty_bit_is_set() {
+                if d.has_current_styles() {
-                    Restyle
+                    // The element has up-to-date style.
-                } else {
+                    debug_assert!(!self.frame_has_style());
-                    debug_assert!(!self.frame_has_style()); // display:none etc
+                    debug_assert!(d.restyle_data.is_none());
                     mode_for_descendants
+                } else {
+                    // The element needs processing.
+                    if d.previous_styles().is_some() {
+                        Restyle
+                    } else {
+                        Initial
+                    }
                 }
             },
-        }
+        };
-    }
 
-    /// Sets up the appropriate data structures to style a node, returing a
+        // Handle the deprecated dirty bit. This should go away soon.
-    /// mutable handle to the node data upon which further style calculations
+        if mode != Initial && self.deprecated_dirty_bit_is_set() {
-    /// can be performed.
+            mode = Restyle;
-    fn begin_styling(&self) -> AtomicRefMut<ElementData>;
+        }
+        mode
+
+    }
 
     /// Immutable borrows the ElementData.
     fn borrow_data(&self) -> Option<AtomicRef<ElementData>>;
 
+    /// Gets a reference to the ElementData container.
+    fn get_data(&self) -> Option<&AtomicRefCell<ElementData>>;
+
     /// Properly marks nodes as dirty in response to restyle hints.
     fn note_restyle_hint<C: DomTraversalContext<Self::ConcreteNode>>(&self, hint: RestyleHint) {
         // Bail early if there's no restyling to do.
@@ -285,13 +306,13 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
 
         // Process hints.
         if hint.contains(RESTYLE_SELF) {
-            unsafe { C::ensure_element_data(self).borrow_mut().ensure_restyle_data(); }
+            unsafe { let _ = C::prepare_for_styling(self); }
         // XXX(emilio): For now, dirty implies dirty descendants if found.
         } else if hint.contains(RESTYLE_DESCENDANTS) {
             unsafe { self.set_dirty_descendants(); }
             let mut current = self.first_child_element();
             while let Some(el) = current {
-                unsafe { C::ensure_element_data(&el).borrow_mut().ensure_restyle_data(); }
+                unsafe { let _ = C::prepare_for_styling(&el); }
                 current = el.next_sibling_element();
             }
         }
@@ -299,7 +320,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
         if hint.contains(RESTYLE_LATER_SIBLINGS) {
             let mut next = ::selectors::Element::next_sibling_element(self);
             while let Some(sib) = next {
-                unsafe { C::ensure_element_data(&sib).borrow_mut().ensure_restyle_data() };
+                unsafe { let _ = C::prepare_for_styling(&sib); }
                 next = ::selectors::Element::next_sibling_element(&sib);
             }
         }

components/style/gecko/traversal.rs

@@ -10,7 +10,6 @@ use gecko::context::StandaloneStyleContext;
 use gecko::wrapper::{GeckoElement, GeckoNode};
 use std::mem;
 use traversal::{DomTraversalContext, recalc_style_at};
-use traversal::RestyleResult;
 
 pub struct RecalcStyleOnly<'lc> {
     context: StandaloneStyleContext<'lc>,
@@ -30,14 +29,10 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
         }
     }
 
-    fn process_preorder(&self, node: GeckoNode<'ln>) -> RestyleResult {
+    fn process_preorder(&self, node: GeckoNode<'ln>) {
-        if node.is_text_node() {
+        if node.is_element() {
-            // 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)
+            recalc_style_at::<_, _, Self>(&self.context, self.root, el);
         }
     }
 
@@ -48,17 +43,25 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
     /// We don't use the post-order traversal for anything.
     fn needs_postorder_traversal(&self) -> bool { false }
 
-    fn should_traverse_child(_parent: GeckoElement<'ln>, child: GeckoNode<'ln>) -> bool {
+    fn should_traverse_child(parent: GeckoElement<'ln>, child: GeckoNode<'ln>) -> bool {
+        if parent.is_display_none() {
+            return false;
+        }
+
         match child.as_element() {
             Some(el) => el.styling_mode() != StylingMode::Stop,
             None => false, // Gecko restyle doesn't need to traverse text nodes.
         }
     }
 
-    fn ensure_element_data<'a>(element: &'a GeckoElement<'ln>) -> &'a AtomicRefCell<ElementData> {
+    unsafe fn ensure_element_data<'a>(element: &'a GeckoElement<'ln>) -> &'a AtomicRefCell<ElementData> {
         element.ensure_data()
     }
 
+    unsafe fn clear_element_data<'a>(element: &'a GeckoElement<'ln>) {
+        element.clear_data()
+    }
+
     fn local_context(&self) -> &LocalStyleContext {
         self.context.local_context()
     }

components/style/gecko/wrapper.rs

@@ -5,7 +5,7 @@
 #![allow(unsafe_code)]
 
 
-use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
+use atomic_refcell::{AtomicRef, AtomicRefCell};
 use data::ElementData;
 use dom::{LayoutIterator, NodeInfo, TDocument, TElement, TNode, TRestyleDamage, UnsafeNode};
 use dom::{OpaqueNode, PresentationalHintsSynthetizer};
@@ -126,7 +126,12 @@ impl<'ln> TNode for GeckoNode<'ln> {
     }
 
     fn dump(self) {
-        unimplemented!()
+        if self.is_text_node() {
+            println!("Text ({:?})", &self.0 as *const _);
+        } else {
+            let el = self.as_element().unwrap();
+            println!("Element {} ({:?})", el.get_local_name(), &el.0 as *const _);
+        }
     }
 
     fn dump_style(self) {
@@ -332,12 +337,8 @@ impl<'le> GeckoElement<'le> {
                                                .get(pseudo).map(|c| c.clone()))
     }
 
-    fn get_node_data(&self) -> Option<&AtomicRefCell<ElementData>> {
-        unsafe { self.raw_node().mServoData.get().as_ref() }
-    }
-
     pub fn ensure_data(&self) -> &AtomicRefCell<ElementData> {
-        match self.get_node_data() {
+        match self.get_data() {
             Some(x) => x,
             None => {
                 let ptr = Box::into_raw(Box::new(AtomicRefCell::new(ElementData::new())));
@@ -426,7 +427,7 @@ impl<'le> TElement for GeckoElement<'le> {
     fn has_dirty_descendants(&self) -> bool {
         // Return true unconditionally if we're not yet styled. This is a hack
         // and should go away soon.
-        if self.get_node_data().is_none() {
+        if self.get_data().is_none() {
             return true;
         }
         self.flags() & (NODE_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) != 0
@@ -444,15 +445,14 @@ impl<'le> TElement for GeckoElement<'le> {
         panic!("Atomic child count not implemented in Gecko");
     }
 
-    fn begin_styling(&self) -> AtomicRefMut<ElementData> {
+    fn borrow_data(&self) -> Option<AtomicRef<ElementData>> {
-        let mut data = self.ensure_data().borrow_mut();
+        self.get_data().map(|x| x.borrow())
-        data.gather_previous_styles(|| self.get_styles_from_frame());
-        data
     }
 
-    fn borrow_data(&self) -> Option<AtomicRef<ElementData>> {
+    fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
-        self.get_node_data().map(|x| x.borrow())
+        unsafe { self.raw_node().mServoData.get().as_ref() }
     }
+
 }
 
 impl<'le> PartialEq for GeckoElement<'le> {

components/style/matching.rs

@@ -8,10 +8,11 @@
 
 use animation;
 use arc_ptr_eq;
+use atomic_refcell::AtomicRefMut;
 use cache::{LRUCache, SimpleHashCache};
 use cascade_info::CascadeInfo;
 use context::{SharedStyleContext, StyleContext};
-use data::{ElementStyles, PseudoStyles};
+use data::{ElementData, ElementStyles, PseudoStyles};
 use dom::{TElement, TNode, TRestyleDamage, UnsafeNode};
 use properties::{CascadeFlags, ComputedValues, SHAREABLE, cascade};
 use properties::longhands::display::computed_value as display;
@@ -29,7 +30,6 @@ use std::ops::Deref;
 use std::slice::IterMut;
 use std::sync::Arc;
 use string_cache::Atom;
-use traversal::RestyleResult;
 use util::opts;
 
 fn create_common_style_affecting_attributes_from_element<E: TElement>(element: &E)
@@ -423,6 +423,7 @@ impl StyleSharingCandidateCache {
 
     pub fn insert_if_possible<E: TElement>(&mut self,
                                            element: &E,
+                                           style: &Arc<ComputedValues>,
                                            relations: StyleRelations) {
         use traversal::relations_are_shareable;
 
@@ -441,9 +442,6 @@ impl StyleSharingCandidateCache {
             return;
         }
 
-        let data = element.borrow_data().unwrap();
-        let style = &data.current_styles().primary;
-
         let box_style = style.get_box();
         if box_style.transition_property_count() > 0 {
             debug!("Failing to insert to the cache: transitions");
@@ -483,7 +481,7 @@ pub enum StyleSharingResult<ConcreteRestyleDamage: TRestyleDamage> {
     /// LRU cache that was hit and the damage that was done, and the restyle
     /// result the original result of the candidate's styling, that is, whether
     /// it should stop the traversal or not.
-    StyleWasShared(usize, ConcreteRestyleDamage, RestyleResult),
+    StyleWasShared(usize, ConcreteRestyleDamage),
 }
 
 // Callers need to pass several boolean flags to cascade_node_pseudo_element.
@@ -695,7 +693,8 @@ pub trait MatchMethods : TElement {
     unsafe fn share_style_if_possible(&self,
                                       style_sharing_candidate_cache:
                                         &mut StyleSharingCandidateCache,
-                                      shared_context: &SharedStyleContext)
+                                      shared_context: &SharedStyleContext,
+                                      data: &mut AtomicRefMut<ElementData>)
                                       -> StyleSharingResult<Self::ConcreteRestyleDamage> {
         if opts::get().disable_share_style_cache {
             return StyleSharingResult::CannotShare
@@ -715,7 +714,6 @@ pub trait MatchMethods : TElement {
             match sharing_result {
                 Ok(shared_style) => {
                     // Yay, cache hit. Share the style.
-                    let mut data = self.begin_styling();
 
                     // TODO: add the display: none optimisation here too! Even
                     // better, factor it out/make it a bit more generic so Gecko
@@ -731,15 +729,9 @@ pub trait MatchMethods : TElement {
                             }
                         };
 
-                    let restyle_result = if shared_style.get_box().clone_display() == display::T::none {
-                        RestyleResult::Stop
-                    } else {
-                        RestyleResult::Continue
-                    };
-
                     data.finish_styling(ElementStyles::new(shared_style));
 
-                    return StyleSharingResult::StyleWasShared(i, damage, restyle_result)
+                    return StyleSharingResult::StyleWasShared(i, damage)
                 }
                 Err(miss) => {
                     debug!("Cache miss: {:?}", miss);
@@ -855,22 +847,21 @@ pub trait MatchMethods : TElement {
 
     unsafe fn cascade_node<'a, Ctx>(&self,
                                     context: &Ctx,
+                                    mut data: AtomicRefMut<ElementData>,
                                     parent: Option<Self>,
                                     applicable_declarations: &ApplicableDeclarations)
-                                    -> RestyleResult
         where Ctx: StyleContext<'a>
     {
         // Get our parent's style.
         let parent_data = parent.as_ref().map(|x| x.borrow_data().unwrap());
         let parent_style = parent_data.as_ref().map(|x| &x.current_styles().primary);
 
-        let mut data = self.begin_styling();
         let mut new_styles;
 
         let mut applicable_declarations_cache =
             context.local_context().applicable_declarations_cache.borrow_mut();
 
-        let (damage, restyle_result) = {
+        let damage = {
             // Update animations before the cascade. This may modify the value of the old primary
             // style.
             let cacheable = data.previous_styles_mut().map_or(true,
@@ -894,7 +885,7 @@ pub trait MatchMethods : TElement {
                                                      animate: true,
                                                  }));
 
-            let (damage, restyle_result) =
+            let damage =
                 self.compute_damage_and_cascade_pseudos(old_primary,
                                                         old_pseudos,
                                                         &new_styles.primary,
@@ -907,15 +898,13 @@ pub trait MatchMethods : TElement {
                 parent_style.unwrap().is_multicol()
             }));
 
-            (damage, restyle_result)
+            damage
         };
 
         data.finish_styling(new_styles);
         // Drop the mutable borrow early, since Servo's set_restyle_damage also borrows.
         mem::drop(data);
         self.set_restyle_damage(damage);
-
-        restyle_result
     }
 
     fn compute_damage_and_cascade_pseudos<'a, Ctx>(&self,
@@ -926,7 +915,7 @@ pub trait MatchMethods : TElement {
                                                    context: &Ctx,
                                                    applicable_declarations: &ApplicableDeclarations,
                                                    mut applicable_declarations_cache: &mut ApplicableDeclarationsCache)
-                                                   -> (Self::ConcreteRestyleDamage, RestyleResult)
+                                                   -> Self::ConcreteRestyleDamage
         where Ctx: StyleContext<'a>
     {
         // Here we optimise the case of the style changing but both the
@@ -951,14 +940,18 @@ pub trait MatchMethods : TElement {
             debug!("Short-circuiting traversal: {:?} {:?} {:?}",
                    this_display, old_display, damage);
 
-            return (damage, RestyleResult::Stop);
+            return damage
         }
 
-        // Otherwise, we just compute the damage normally, and sum up the damage
+        // Compute the damage and sum up the damage related to pseudo-elements.
-        // related to pseudo-elements.
         let mut damage =
             self.compute_restyle_damage(old_primary, new_primary, None);
 
+        // If the new style is display:none, we don't need pseudo-elements styles.
+        if new_primary.get_box().clone_display() == display::T::none {
+            return damage;
+        }
+
         let rebuild_and_reflow =
             Self::ConcreteRestyleDamage::rebuild_and_reflow();
         let no_damage = Self::ConcreteRestyleDamage::empty();
@@ -1017,7 +1010,7 @@ pub trait MatchMethods : TElement {
             }
         });
 
-        (damage, RestyleResult::Continue)
+        damage
     }
 }
 

components/style/parallel.rs

@@ -11,8 +11,8 @@
 use dom::{OpaqueNode, StylingMode, TElement, TNode, UnsafeNode};
 use std::mem;
 use std::sync::atomic::Ordering;
-use traversal::{RestyleResult, DomTraversalContext};
 use traversal::{STYLE_SHARING_CACHE_HITS, STYLE_SHARING_CACHE_MISSES};
+use traversal::DomTraversalContext;
 use util::opts;
 use workqueue::{WorkQueue, WorkUnit, WorkerProxy};
 
@@ -83,8 +83,9 @@ fn top_down_dom<N, C>(unsafe_nodes: UnsafeNodeList,
 
         // Perform the appropriate traversal.
         let mut children_to_process = 0isize;
-        if let RestyleResult::Continue = context.process_preorder(node) {
+        context.process_preorder(node);
-            C::traverse_children(node.as_element().unwrap(), |kid| {
+        if let Some(el) = node.as_element() {
+            C::traverse_children(el, |kid| {
                 children_to_process += 1;
                 discovered_child_nodes.push(kid.to_unsafe())
             });

components/style/sequential.rs

@@ -5,7 +5,7 @@
 //! Implements sequential traversal over the DOM tree.
 
 use dom::{StylingMode, TElement, TNode};
-use traversal::{RestyleResult, DomTraversalContext};
+use traversal::DomTraversalContext;
 
 pub fn traverse_dom<N, C>(root: N,
                           shared: &C::SharedContext)
@@ -16,9 +16,9 @@ pub fn traverse_dom<N, C>(root: N,
         where N: TNode,
               C: DomTraversalContext<N>
     {
-        if let RestyleResult::Continue = context.process_preorder(node) {
+        context.process_preorder(node);
-            C::traverse_children(node.as_element().unwrap(),
+        if let Some(el) = node.as_element() {
-                                 |kid| doit::<N, C>(context, kid));
+            C::traverse_children(el, |kid| doit::<N, C>(context, kid));
         }
 
         if context.needs_postorder_traversal() {

components/style/traversal.rs

@@ -4,7 +4,7 @@
 
 //! Traversing the DOM tree; the bloom filter.
 
-use atomic_refcell::AtomicRefCell;
+use atomic_refcell::{AtomicRefCell, AtomicRefMut};
 use context::{LocalStyleContext, SharedStyleContext, StyleContext};
 use data::ElementData;
 use dom::{OpaqueNode, StylingMode, TElement, TNode, UnsafeNode};
@@ -12,6 +12,7 @@ use matching::{ApplicableDeclarations, MatchMethods, StyleSharingResult};
 use selectors::bloom::BloomFilter;
 use selectors::matching::StyleRelations;
 use std::cell::RefCell;
+use std::mem;
 use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};
 use tid::tid;
 use util::opts;
@@ -20,16 +21,6 @@ use util::opts;
 /// detected by ticking a generation number every layout.
 pub type Generation = u32;
 
-/// This enum tells us about whether we can stop restyling or not after styling
-/// an element.
-///
-/// So far this only happens where a display: none node is found.
-#[derive(Clone, Copy, PartialEq)]
-pub enum RestyleResult {
-    Continue,
-    Stop,
-}
-
 /// Style sharing candidate cache stats. These are only used when
 /// `-Z style-sharing-stats` is given.
 pub static STYLE_SHARING_CACHE_HITS: AtomicUsize = ATOMIC_USIZE_INIT;
@@ -156,13 +147,25 @@ pub fn remove_from_bloom_filter<'a, N, C>(context: &C, root: OpaqueNode, node: N
     };
 }
 
+pub fn prepare_for_styling<E: TElement>(element: E,
+                                        data: &AtomicRefCell<ElementData>)
+                                        -> AtomicRefMut<ElementData> {
+    let mut d = data.borrow_mut();
+    d.gather_previous_styles(|| element.get_styles_from_frame());
+    if d.previous_styles().is_some() {
+        d.ensure_restyle_data();
+    }
+
+    d
+}
+
 pub trait DomTraversalContext<N: TNode> {
     type SharedContext: Sync + 'static;
 
     fn new<'a>(&'a Self::SharedContext, OpaqueNode) -> Self;
 
     /// Process `node` on the way down, before its children have been processed.
-    fn process_preorder(&self, node: N) -> RestyleResult;
+    fn process_preorder(&self, node: N);
 
     /// Process `node` on the way up, after its children have been processed.
     ///
@@ -200,7 +203,23 @@ pub trait DomTraversalContext<N: TNode> {
     /// Ensures the existence of the ElementData, and returns it. This can't live
     /// on TNode because of the trait-based separation between Servo's script
     /// and layout crates.
-    fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData>;
+    ///
+    /// This is only safe to call in top-down traversal before processing the
+    /// children of |element|.
+    unsafe fn ensure_element_data(element: &N::ConcreteElement) -> &AtomicRefCell<ElementData>;
+
+    /// Sets up the appropriate data structures to style or restyle a node,
+    /// returing a mutable handle to the node data upon which further style
+    /// calculations can be performed.
+    unsafe fn prepare_for_styling(element: &N::ConcreteElement) -> AtomicRefMut<ElementData> {
+        prepare_for_styling(*element, Self::ensure_element_data(element))
+    }
+
+    /// Clears the ElementData attached to this element, if any.
+    ///
+    /// This is only safe to call in top-down traversal before processing the
+    /// children of |element|.
+    unsafe fn clear_element_data(element: &N::ConcreteElement);
 
     fn local_context(&self) -> &LocalStyleContext;
 }
@@ -267,6 +286,7 @@ fn ensure_element_styled_internal<'a, E, C>(element: E,
     // probably not necessary since we're likely to be matching only a few
     // nodes, at best.
     let mut applicable_declarations = ApplicableDeclarations::new();
+    let data = prepare_for_styling(element, element.get_data().unwrap());
     let stylist = &context.shared_context().stylist;
 
     element.match_element(&**stylist,
@@ -274,7 +294,7 @@ fn ensure_element_styled_internal<'a, E, C>(element: E,
                           &mut applicable_declarations);
 
     unsafe {
-        element.cascade_node(context, parent, &applicable_declarations);
+        element.cascade_node(context, data, parent, &applicable_declarations);
     }
 }
 
@@ -283,7 +303,7 @@ fn ensure_element_styled_internal<'a, E, C>(element: E,
 #[allow(unsafe_code)]
 pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
                                     root: OpaqueNode,
-                                    element: E) -> RestyleResult
+                                    element: E)
     where E: TElement,
           C: StyleContext<'a>,
           D: DomTraversalContext<E::ConcreteNode>
@@ -291,10 +311,11 @@ pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
     // Get the style bloom filter.
     let mut bf = take_thread_local_bloom_filter(element.parent_element(), root, context.shared_context());
 
-    let mut restyle_result = RestyleResult::Continue;
     let mode = element.styling_mode();
     debug_assert!(mode != StylingMode::Stop, "Parent should not have enqueued us");
     if mode != StylingMode::Traverse {
+        let mut data = unsafe { D::prepare_for_styling(&element) };
+
         // Check to see whether we can share a style with someone.
         let style_sharing_candidate_cache =
             &mut context.local_context().style_sharing_candidate_cache.borrow_mut();
@@ -302,7 +323,8 @@ pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
         let sharing_result = if element.parent_element().is_none() {
             StyleSharingResult::CannotShare
         } else {
-            unsafe { element.share_style_if_possible(style_sharing_candidate_cache, context.shared_context()) }
+            unsafe { element.share_style_if_possible(style_sharing_candidate_cache,
+                                                     context.shared_context(), &mut data) }
         };
 
         // Otherwise, match and cascade selectors.
@@ -334,38 +356,57 @@ pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
 
                 // Perform the CSS cascade.
                 unsafe {
-                    restyle_result = element.cascade_node(context,
+                    element.cascade_node(context, data, element.parent_element(),
-                                                          element.parent_element(),
+                                         &applicable_declarations);
-                                                          &applicable_declarations);
                 }
 
                 // Add ourselves to the LRU cache.
                 if let Some(element) = shareable_element {
-                    style_sharing_candidate_cache.insert_if_possible(&element, relations);
+                    style_sharing_candidate_cache.insert_if_possible(&element,
+                                                                     &element.borrow_data()
+                                                                             .unwrap()
+                                                                             .current_styles()
+                                                                             .primary,
+                                                                     relations);
                 }
             }
-            StyleSharingResult::StyleWasShared(index, damage, cached_restyle_result) => {
+            StyleSharingResult::StyleWasShared(index, damage) => {
-                restyle_result = cached_restyle_result;
                 if opts::get().style_sharing_stats {
                     STYLE_SHARING_CACHE_HITS.fetch_add(1, Ordering::Relaxed);
                 }
                 style_sharing_candidate_cache.touch(index);
+
+                // Drop the mutable borrow early, since Servo's set_restyle_damage also borrows.
+                mem::drop(data);
+
                 element.set_restyle_damage(damage);
             }
         }
     }
 
-    // If we restyled this node, conservatively mark all our children as needing
+    if element.is_display_none() {
-    // processing. The eventual algorithm we're designing does this in a more granular
+        // If this element is display:none, throw away all style data in the subtree.
-    // fashion.
+        fn clear_descendant_data<E: TElement, D: DomTraversalContext<E::ConcreteNode>>(el: E) {
-    if mode == StylingMode::Restyle && restyle_result == RestyleResult::Continue {
+            for kid in el.as_node().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. By consequence, any element without data has no
+                    // descendants with data.
+                    if kid.get_data().is_some() {
+                        unsafe { D::clear_element_data(&kid) };
+                        clear_descendant_data::<_, D>(kid);
+                    }
+                }
+            }
+        };
+        clear_descendant_data::<_, D>(element);
+    } else if mode == StylingMode::Restyle {
+        // If we restyled this node, conservatively mark all our children as needing
+        // processing. The eventual algorithm we're designing does this in a more granular
+        // fashion.
         for kid in element.as_node().children() {
             if let Some(kid) = kid.as_element() {
-                let mut data = D::ensure_element_data(&kid).borrow_mut();
+                unsafe { let _ = D::prepare_for_styling(&kid); }
-                data.gather_previous_styles(|| kid.get_styles_from_frame());
-                if data.previous_styles().is_some() {
-                    data.ensure_restyle_data();
-                }
             }
         }
     }
@@ -379,6 +420,4 @@ pub fn recalc_style_at<'a, E, C, D>(context: &'a C,
 
     // NB: flow construction updates the bloom filter on the way up.
     put_thread_local_bloom_filter(bf, &unsafe_layout_node, context.shared_context());
-
-    restyle_result
 }