style: Force to stop computing children if we find the parent has display: none.

components/layout/traversal.rs

@@ -16,6 +16,7 @@ use std::mem;
 use style::context::SharedStyleContext;
 use style::dom::TNode;
 use style::selector_impl::ServoSelectorImpl;
+use style::traversal::RestyleResult;
 use style::traversal::{DomTraversalContext, remove_from_bloom_filter, recalc_style_at};
 use util::opts;
 use wrapper::{LayoutNodeLayoutData, ThreadSafeLayoutNodeHelpers};
@@ -69,12 +70,12 @@ impl<'lc, N> DomTraversalContext<N> for RecalcStyleAndConstructFlows<'lc>
         }
     }
 
-    fn process_preorder(&self, node: N) {
+    fn process_preorder(&self, node: N) -> RestyleResult {
-        // FIXME(pcwalton): Stop allocating here. Ideally this should just be done by the HTML
+        // FIXME(pcwalton): Stop allocating here. Ideally this should just be
-        // parser.
+        // done by the HTML parser.
         node.initialize_data();
 
-        recalc_style_at(&self.context, self.root, node);
+        recalc_style_at(&self.context, self.root, node)
     }
 
     fn process_postorder(&self, node: N) {

components/script_layout_interface/restyle_damage.rs

@@ -47,6 +47,10 @@ impl TRestyleDamage for RestyleDamage {
     /// For Servo the style source is always the computed values.
     type PreExistingComputedValues = Arc<ServoComputedValues>;
 
+    fn empty() -> Self {
+        RestyleDamage::empty()
+    }
+
     fn compute(old: Option<&Arc<ServoComputedValues>>,
                new: &Arc<ServoComputedValues>) -> RestyleDamage {
         compute_damage(old, new)

components/style/dom.rs

@@ -15,6 +15,7 @@ use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, R
 use selector_impl::ElementExt;
 use selectors::matching::DeclarationBlock;
 use sink::Push;
+use std::fmt::Debug;
 use std::ops::BitOr;
 use std::sync::Arc;
 use string_cache::{Atom, Namespace};
@@ -44,7 +45,7 @@ impl OpaqueNode {
     }
 }
 
-pub trait TRestyleDamage : BitOr<Output=Self> + Copy {
+pub trait TRestyleDamage : Debug + BitOr<Output=Self> + Copy {
     /// The source for our current computed values in the cascade. This is a
     /// ComputedValues in Servo and a StyleContext in Gecko.
     ///
@@ -58,6 +59,8 @@ pub trait TRestyleDamage : BitOr<Output=Self> + Copy {
     fn compute(old: Option<&Self::PreExistingComputedValues>,
                new: &Arc<ComputedValues>) -> Self;
 
+    fn empty() -> Self;
+
     fn rebuild_and_reflow() -> Self;
 }
 

components/style/matching.rs

@@ -12,6 +12,7 @@ use cache::{LRUCache, SimpleHashCache};
 use context::{StyleContext, SharedStyleContext};
 use data::PrivateStyleData;
 use dom::{TElement, TNode, TRestyleDamage};
+use properties::longhands::display::computed_value as display;
 use properties::{ComputedValues, PropertyDeclaration, cascade};
 use selector_impl::{ElementExt, TheSelectorImpl, PseudoElement};
 use selector_matching::{DeclarationBlock, Stylist};
@@ -25,6 +26,7 @@ use std::hash::{BuildHasherDefault, Hash, Hasher};
 use std::slice::Iter;
 use std::sync::Arc;
 use string_cache::{Atom, Namespace};
+use traversal::RestyleResult;
 use util::opts;
 
 fn create_common_style_affecting_attributes_from_element<E: TElement>(element: &E)
@@ -410,9 +412,9 @@ impl StyleSharingCandidateCache {
 pub enum StyleSharingResult<ConcreteRestyleDamage: TRestyleDamage> {
     /// We didn't find anybody to share the style with.
     CannotShare,
-    /// The node's style can be shared. The integer specifies the index in the LRU cache that was
+    /// The node's style can be shared. The integer specifies the index in the
-    /// hit and the damage that was done.
+    /// LRU cache that was hit and the damage that was done.
-    StyleWasShared(usize, ConcreteRestyleDamage),
+    StyleWasShared(usize, ConcreteRestyleDamage, RestyleResult),
 }
 
 trait PrivateMatchMethods: TNode {
@@ -424,22 +426,22 @@ trait PrivateMatchMethods: TNode {
                                             context: &Ctx,
                                             parent_style: Option<&Arc<ComputedValues>>,
                                             applicable_declarations: &[DeclarationBlock],
-                                            mut style: Option<&mut Arc<ComputedValues>>,
+                                            mut old_style: Option<&mut Arc<ComputedValues>>,
                                             applicable_declarations_cache:
                                              &mut ApplicableDeclarationsCache,
                                             shareable: bool,
                                             animate_properties: bool)
-                                            -> (Self::ConcreteRestyleDamage, Arc<ComputedValues>)
+                                            -> Arc<ComputedValues>
-    where Ctx: StyleContext<'a> {
+        where Ctx: StyleContext<'a>
+    {
         let mut cacheable = true;
         let shared_context = context.shared_context();
         if animate_properties {
             cacheable = !self.update_animations_for_cascade(shared_context,
-                                                            &mut style) && cacheable;
+                                                            &mut old_style) && cacheable;
         }
 
-        let this_style;
+        let (this_style, is_cacheable) = match parent_style {
-        match parent_style {
             Some(ref parent_style) => {
                 let cache_entry = applicable_declarations_cache.find(applicable_declarations);
                 let cached_computed_values = match cache_entry {
@@ -447,27 +449,25 @@ trait PrivateMatchMethods: TNode {
                     None => None,
                 };
 
-                let (the_style, is_cacheable) = cascade(shared_context.viewport_size,
+                cascade(shared_context.viewport_size,
-                                                        applicable_declarations,
+                        applicable_declarations,
-                                                        shareable,
+                        shareable,
-                                                        Some(&***parent_style),
+                        Some(&***parent_style),
-                                                        cached_computed_values,
+                        cached_computed_values,
-                                                        shared_context.error_reporter.clone());
+                        shared_context.error_reporter.clone())
-                cacheable = cacheable && is_cacheable;
-                this_style = the_style
             }
             None => {
-                let (the_style, is_cacheable) = cascade(shared_context.viewport_size,
+                cascade(shared_context.viewport_size,
-                                                        applicable_declarations,
+                        applicable_declarations,
-                                                        shareable,
+                        shareable,
-                                                        None,
+                        None,
-                                                        None,
+                        None,
-                                                        shared_context.error_reporter.clone());
+                        shared_context.error_reporter.clone())
-                cacheable = cacheable && is_cacheable;
-                this_style = the_style
             }
         };
 
+        cacheable = cacheable && is_cacheable;
+
         let mut this_style = Arc::new(this_style);
 
         if animate_properties {
@@ -482,7 +482,7 @@ trait PrivateMatchMethods: TNode {
 
             // Trigger transitions if necessary. This will reset `this_style` back
             // to its old value if it did trigger a transition.
-            if let Some(ref style) = style {
+            if let Some(ref style) = old_style {
                 animations_started |=
                     animation::start_transitions_if_applicable(
                         new_animations_sender,
@@ -496,21 +496,13 @@ trait PrivateMatchMethods: TNode {
         }
 
 
-        let existing_style =
-            self.existing_style_for_restyle_damage(style.map(|s| &*s));
-
-        // Calculate style difference.
-        let damage =
-            Self::ConcreteRestyleDamage::compute(existing_style, &this_style);
-
         // Cache the resolved style if it was cacheable.
         if cacheable {
             applicable_declarations_cache.insert(applicable_declarations.to_vec(),
                                                  this_style.clone());
         }
 
-        // Return the final style and the damage done to our caller.
+        this_style
-        (damage, this_style)
     }
 
     fn update_animations_for_cascade(&self,
@@ -654,8 +646,15 @@ pub trait ElementMatchMethods : TElement {
                     damage
                 };
 
+                let restyle_result = if shared_style.get_box().clone_display() == display::T::none {
+                    RestyleResult::Stop
+                } else {
+                    RestyleResult::Continue
+                };
+
                 *style = Some(shared_style);
-                return StyleSharingResult::StyleWasShared(i, damage)
+
+                return StyleSharingResult::StyleWasShared(i, damage, restyle_result)
             }
         }
 
@@ -718,6 +717,7 @@ pub trait MatchMethods : TNode {
                                     context: &Ctx,
                                     parent: Option<Self>,
                                     applicable_declarations: &ApplicableDeclarations)
+                                    -> RestyleResult
         where Ctx: StyleContext<'a>
     {
         // Get our parent's style. This must be unsafe so that we don't touch the parent's
@@ -736,70 +736,146 @@ pub trait MatchMethods : TNode {
         let mut applicable_declarations_cache =
             context.local_context().applicable_declarations_cache.borrow_mut();
 
-        let damage;
+        let (damage, restyle_result) = if self.is_text_node() {
-        if self.is_text_node() {
             let mut data_ref = self.mutate_data().unwrap();
             let mut data = &mut *data_ref;
             let cloned_parent_style = ComputedValues::style_for_child_text_node(parent_style.unwrap());
 
-            {
+            let damage = {
                 let existing_style =
                     self.existing_style_for_restyle_damage(data.style.as_ref());
-                damage = Self::ConcreteRestyleDamage::compute(existing_style,
+                Self::ConcreteRestyleDamage::compute(existing_style,
-                                                              &cloned_parent_style);
+                                                     &cloned_parent_style)
-            }
+            };
 
             data.style = Some(cloned_parent_style);
+
+            (damage, RestyleResult::Continue)
         } else {
-            damage = {
+            let mut data_ref = self.mutate_data().unwrap();
-                let mut data_ref = self.mutate_data().unwrap();
+            let mut data = &mut *data_ref;
-                let mut data = &mut *data_ref;
+            let final_style =
-                let (mut damage, final_style) = self.cascade_node_pseudo_element(
+                self.cascade_node_pseudo_element(context, parent_style,
-                    context,
+                                                 &applicable_declarations.normal,
-                    parent_style,
+                                                 data.style.as_mut(),
-                    &applicable_declarations.normal,
+                                                 &mut applicable_declarations_cache,
-                    data.style.as_mut(),
+                                                 applicable_declarations.normal_shareable,
-                    &mut applicable_declarations_cache,
+                                                 /* should_animate = */ true);
-                    applicable_declarations.normal_shareable,
-                    true);
 
-                data.style = Some(final_style);
+            let (damage, restyle_result) =
-
+                self.compute_damage_and_cascade_pseudos(final_style,
-                <Self::ConcreteElement as MatchAttr>::Impl::each_eagerly_cascaded_pseudo_element(|pseudo| {
+                                                        data,
-                    let applicable_declarations_for_this_pseudo =
+                                                        context,
-                        applicable_declarations.per_pseudo.get(&pseudo).unwrap();
+                                                        applicable_declarations,
-
+                                                        &mut applicable_declarations_cache);
-                    if !applicable_declarations_for_this_pseudo.is_empty() {
-                        // NB: Transitions and animations should only work for
-                        // pseudo-elements ::before and ::after
-                        let should_animate_properties =
-                            <Self::ConcreteElement as MatchAttr>::Impl::pseudo_is_before_or_after(&pseudo);
-                        let (new_damage, style) = self.cascade_node_pseudo_element(
-                            context,
-                            Some(data.style.as_ref().unwrap()),
-                            &*applicable_declarations_for_this_pseudo,
-                            data.per_pseudo.get_mut(&pseudo),
-                            &mut applicable_declarations_cache,
-                            false,
-                            should_animate_properties);
-                        data.per_pseudo.insert(pseudo, style);
-
-                        damage = damage | new_damage;
-                    }
-                });
-
-                damage
-            };
 
             self.set_can_be_fragmented(parent.map_or(false, |p| {
                 p.can_be_fragmented() ||
                 parent_style.as_ref().unwrap().is_multicol()
             }));
+
+            (damage, restyle_result)
+        };
+
+
+        // This method needs to borrow the data as mutable, so make sure
+        // data_ref goes out of scope first.
+        self.set_restyle_damage(damage);
+
+        restyle_result
+    }
+
+    fn compute_damage_and_cascade_pseudos<'a, Ctx>(&self,
+                                                   final_style: Arc<ComputedValues>,
+                                                   data: &mut PrivateStyleData,
+                                                   context: &Ctx,
+                                                   applicable_declarations: &ApplicableDeclarations,
+                                                   mut applicable_declarations_cache: &mut ApplicableDeclarationsCache)
+                                                   -> (Self::ConcreteRestyleDamage, RestyleResult)
+        where Ctx: StyleContext<'a>
+    {
+        // Here we optimise the case of the style changing but both the
+        // previous and the new styles having display: none. In this
+        // case, we can always optimize the traversal, regardless of the
+        // restyle hint.
+        let this_display = final_style.get_box().clone_display();
+        if this_display == display::T::none {
+            let old_display = data.style.as_ref().map(|old_style| {
+                old_style.get_box().clone_display()
+            });
+
+            // If display passed from none to something, then we need to reflow,
+            // otherwise, we don't do anything.
+            let damage = match old_display {
+                Some(display) if display == this_display => {
+                    Self::ConcreteRestyleDamage::empty()
+                }
+                _ => {
+                    Self::ConcreteRestyleDamage::rebuild_and_reflow()
+                }
+            };
+
+            debug!("Short-circuiting traversal: {:?} {:?} {:?}",
+                   this_display, old_display, damage);
+
+            data.style = Some(final_style);
+            return (damage, RestyleResult::Stop);
         }
 
-        // This method needs to borrow the data as mutable, so make sure data_ref goes out of
+        // Otherwise, we just compute the damage normally, and sum up the damage
-        // scope first.
+        // related to pseudo-elements.
-        self.set_restyle_damage(damage);
+        let mut damage = {
+            let existing_style =
+                self.existing_style_for_restyle_damage(data.style.as_ref());
+
+            Self::ConcreteRestyleDamage::compute(existing_style, &final_style)
+        };
+
+        data.style = Some(final_style);
+
+        // FIXME(emilio): This is not pretty, and in the Gecko case means
+        // effectively comparing with the old computed values (given our style
+        // source is the old nsStyleContext).
+        //
+        // I... don't think any difference can arise from comparing with the old
+        // element restyle damage vs the new one, given that we're only summing
+        // the changes, and any change that we could miss would already have
+        // been caught by the parent's change. If for some reason I'm wrong on
+        // this, we'd have to compare computed values in Gecko too.
+        let existing_style =
+            self.existing_style_for_restyle_damage(data.style.as_ref());
+
+        let data_per_pseudo = &mut data.per_pseudo;
+        let new_style = data.style.as_ref();
+        debug_assert!(new_style.is_some());
+
+        <Self::ConcreteElement as MatchAttr>::Impl::each_eagerly_cascaded_pseudo_element(|pseudo| {
+            let applicable_declarations_for_this_pseudo =
+                applicable_declarations.per_pseudo.get(&pseudo).unwrap();
+
+            if !applicable_declarations_for_this_pseudo.is_empty() {
+                // NB: Transitions and animations should only work for
+                // pseudo-elements ::before and ::after
+                let should_animate_properties =
+                    <Self::ConcreteElement as MatchAttr>::Impl::pseudo_is_before_or_after(&pseudo);
+
+                let new_pseudo_style =
+                    self.cascade_node_pseudo_element(context,
+                                                     new_style,
+                                                     &*applicable_declarations_for_this_pseudo,
+                                                     data_per_pseudo.get_mut(&pseudo),
+                                                     &mut applicable_declarations_cache,
+                                                     /* shareable = */ false,
+                                                     should_animate_properties);
+
+                let new_damage =
+                    Self::ConcreteRestyleDamage::compute(existing_style, &new_pseudo_style);
+                damage = damage | new_damage;
+                data_per_pseudo.insert(pseudo, new_pseudo_style);
+            }
+        });
+
+        (damage, RestyleResult::Continue)
     }
 }
 

components/style/parallel.rs

@@ -11,7 +11,7 @@
 use dom::{OpaqueNode, TNode, UnsafeNode};
 use std::mem;
 use std::sync::atomic::Ordering;
-use traversal::DomTraversalContext;
+use traversal::{RestyleResult, DomTraversalContext};
 use workqueue::{WorkQueue, WorkUnit, WorkerProxy};
 
 #[allow(dead_code)]
@@ -68,21 +68,27 @@ fn top_down_dom<N, C>(unsafe_nodes: UnsafeNodeList,
         }
 
         // Perform the appropriate traversal.
-        context.process_preorder(node);
+        let should_stop = match context.process_preorder(node) {
+            RestyleResult::Stop => true,
+            RestyleResult::Continue => false,
+        };
 
         // Possibly enqueue the children.
         let mut children_to_process = 0isize;
-        for kid in node.children() {
+        if !should_stop {
-            // Trigger the hook pre-adding the kid to the list. This can (and in
+            for kid in node.children() {
-            // fact uses to) change the result of the should_process operation.
+                // Trigger the hook pre-adding the kid to the list. This can
-            //
+                // (and in fact uses to) change the result of the should_process
-            // As of right now, this hook takes care of propagating the restyle
+                // operation.
-            // flag down the tree. In the future, more accurate behavior is
+                //
-            // probably going to be needed.
+                // As of right now, this hook takes care of propagating the
-            context.pre_process_child_hook(node, kid);
+                // restyle flag down the tree. In the future, more accurate
-            if context.should_process(kid) {
+                // behavior is probably going to be needed.
-                children_to_process += 1;
+                context.pre_process_child_hook(node, kid);
-                discovered_child_nodes.push(kid.to_unsafe())
+                if context.should_process(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::TNode;
-use traversal::DomTraversalContext;
+use traversal::{RestyleResult, DomTraversalContext};
 
 pub fn traverse_dom<N, C>(root: N,
                           shared: &C::SharedContext)
@@ -17,12 +17,17 @@ pub fn traverse_dom<N, C>(root: N,
               C: DomTraversalContext<N>
     {
         debug_assert!(context.should_process(node));
-        context.process_preorder(node);
+        let should_stop = match context.process_preorder(node) {
+            RestyleResult::Stop => true,
+            RestyleResult::Continue => false,
+        };
 
-        for kid in node.children() {
+        if !should_stop {
-            context.pre_process_child_hook(node, kid);
+            for kid in node.children() {
-            if context.should_process(kid) {
+                context.pre_process_child_hook(node, kid);
-                doit::<N, C>(context, kid);
+                if context.should_process(kid) {
+                    doit::<N, C>(context, kid);
+                }
             }
         }
 

components/style/traversal.rs

@@ -18,6 +18,16 @@ use values::HasViewportPercentage;
 /// 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.
+pub enum RestyleResult {
+    Continue,
+    Stop,
+}
+
+
 /// A pair of the bloom filter used for css selector matching, and the node to
 /// which it applies. This is used to efficiently do `Descendant` selector
 /// matches. Thanks to the bloom filter, we can avoid walking up the tree
@@ -145,7 +155,7 @@ pub trait DomTraversalContext<N: TNode>  {
     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) -> ForceTraversalStop;
+    fn process_preorder(&self, node: N) -> RestyleResult;
 
     /// Process `node` on the way up, after its children have been processed.
     ///
@@ -188,7 +198,7 @@ pub trait DomTraversalContext<N: TNode>  {
 #[allow(unsafe_code)]
 pub fn recalc_style_at<'a, N, C>(context: &'a C,
                                  root: OpaqueNode,
-                                 node: N)
+                                 node: N) -> RestyleResult
     where N: TNode,
           C: StyleContext<'a> {
     // Get the parent node.
@@ -201,6 +211,7 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
     let mut bf = take_thread_local_bloom_filter(parent_opt, root, context.shared_context());
 
     let nonincremental_layout = opts::get().nonincremental_layout;
+    let mut restyle_result = RestyleResult::Continue;
     if nonincremental_layout || node.is_dirty() {
         // Remove existing CSS styles from nodes whose content has changed (e.g. text changed),
         // to force non-incremental reflow.
@@ -250,9 +261,9 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
 
                 // Perform the CSS cascade.
                 unsafe {
-                    node.cascade_node(context,
+                    restyle_result = node.cascade_node(context,
-                                      parent_opt,
+                                                    parent_opt,
-                                      &applicable_declarations);
+                                                    &applicable_declarations);
                 }
 
                 // Add ourselves to the LRU cache.
@@ -260,7 +271,8 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
                     style_sharing_candidate_cache.insert_if_possible::<'ln, N>(&element);
                 }
             }
-            StyleSharingResult::StyleWasShared(index, damage) => {
+            StyleSharingResult::StyleWasShared(index, damage, restyle_result_cascade) => {
+                restyle_result = restyle_result_cascade;
                 style_sharing_candidate_cache.touch(index);
                 node.set_restyle_damage(damage);
             }
@@ -297,4 +309,10 @@ pub fn recalc_style_at<'a, N, C>(context: &'a C,
             }
         }
     }
+
+    if nonincremental_layout {
+        RestyleResult::Continue
+    } else {
+        restyle_result
+    }
 }

ports/geckolib/traversal.rs

@@ -6,6 +6,7 @@ use context::StandaloneStyleContext;
 use std::mem;
 use style::context::SharedStyleContext;
 use style::dom::OpaqueNode;
+use style::traversal::RestyleResult;
 use style::traversal::{DomTraversalContext, recalc_style_at};
 use wrapper::GeckoNode;
 
@@ -27,12 +28,12 @@ impl<'lc, 'ln> DomTraversalContext<GeckoNode<'ln>> for RecalcStyleOnly<'lc> {
         }
     }
 
-    fn process_preorder(&self, node: GeckoNode<'ln>) {
+    fn process_preorder(&self, node: GeckoNode<'ln>) -> RestyleResult {
         // FIXME(pcwalton): Stop allocating here. Ideally this should just be done by the HTML
         // parser.
         node.initialize_data();
 
-        recalc_style_at(&self.context, self.root, node);
+        recalc_style_at(&self.context, self.root, node)
     }
 
     fn process_postorder(&self, _: GeckoNode<'ln>) {

ports/geckolib/wrapper.rs

@@ -95,16 +95,40 @@ impl<'ln> GeckoNode<'ln> {
     }
 }
 
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug)]
 pub struct GeckoRestyleDamage(nsChangeHint);
 
 impl TRestyleDamage for GeckoRestyleDamage {
     type PreExistingComputedValues = nsStyleContext;
+
+    fn empty() -> Self {
+        use std::mem;
+        GeckoRestyleDamage(unsafe { mem::transmute(0u32) })
+    }
+
     fn compute(source: Option<&nsStyleContext>,
                new_style: &Arc<ComputedValues>) -> Self {
         type Helpers = ArcHelpers<ServoComputedValues, ComputedValues>;
         let context = match source {
             Some(ctx) => ctx as *const nsStyleContext as *mut nsStyleContext,
+            // If there's no style source (that is, no style context), there can
+            // be two reasons for it.
+            //
+            // The first one, is that this is not an incremental restyle (so we
+            // also don't have the old computed values). In that case the best
+            // we can do is return rebuild_and_reflow.
+            //
+            // The second one is that this is an incremental restyle, but the
+            // node has display: none. In this case, the old computed values
+            // should still be present, and we should be able to compare the new
+            // to the old display to see if it effectively needs a reflow, or we
+            // can do nothing on it because the old and the new display values
+            // are none.
+            //
+            // This is done outside of this method in servo itself, so we
+            // effectively only need to check for the first case. Ideally, we
+            // should be able to assert that in this case the display values
+            // differ or are not none, but we can't reach the node from here.
             None => return Self::rebuild_and_reflow(),
         };