servo: Merge #16909 - style: Refactor the cascade function (from emilio:simplify-cascade); r=bholley
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 17 May 2017 11:19:56 -0500
changeset 358797 580ca1d982e0103fd0e3be9159f841b09d6ba1e3
parent 358796 d4cb588ab2cc5a9edb75d0896f043c1c44b94b6d
child 358798 43bc4f4903cb3b52b6107bd3636b50aabad7d7b5
push id31838
push userkwierso@gmail.com
push dateWed, 17 May 2017 20:32:10 +0000
treeherdermozilla-central@b133ec74e3d0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #16909 - style: Refactor the cascade function (from emilio:simplify-cascade); r=bholley The `cascade_primary_or_pseudo` function was nice when we shared more code, but right now I think it just makes it harder to understand what's going on. Source-Repo: https://github.com/servo/servo Source-Revision: c6c960a661aa841a9915f5a816148c6275b98dbd
servo/components/script/layout_wrapper.rs
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/matching.rs
servo/components/style/selector_parser.rs
servo/components/style/servo/restyle_damage.rs
--- a/servo/components/script/layout_wrapper.rs
+++ b/servo/components/script/layout_wrapper.rs
@@ -399,19 +399,19 @@ impl<'le> TElement for ServoLayoutElemen
 
     #[inline]
     fn attr_equals(&self, namespace: &Namespace, attr: &LocalName, val: &Atom) -> bool {
         self.get_attr(namespace, attr).map_or(false, |x| x == val)
     }
 
     #[inline]
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_cv: &'a Arc<ComputedValues>,
+                                             current_cv: &'a ComputedValues,
                                              _pseudo_element: Option<&PseudoElement>)
-                                             -> Option<&'a Arc<ComputedValues>> {
+                                             -> Option<&'a ComputedValues> {
         Some(current_cv)
     }
 
     fn has_dirty_descendants(&self) -> bool {
         unsafe { self.as_node().node.get_flag(HAS_DIRTY_DESCENDANTS) }
     }
 
     fn has_snapshot(&self) -> bool {
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -384,17 +384,17 @@ pub trait TElement : Eq + PartialEq + De
     /// Get the pre-existing style to calculate restyle damage (change hints).
     ///
     /// This needs to be generic since it varies between Servo and Gecko.
     ///
     /// XXX(emilio): It's a bit unfortunate we need to pass the current computed
     /// values as an argument here, but otherwise Servo would crash due to
     /// double borrows to return it.
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_computed_values: &'a Arc<ComputedValues>,
+                                             current_computed_values: &'a ComputedValues,
                                              pseudo: Option<&PseudoElement>)
                                              -> Option<&'a PreExistingComputedValues>;
 
     /// Returns true if this element may have a descendant needing style processing.
     ///
     /// Note that we cannot guarantee the existence of such an element, because
     /// it may have been removed from the DOM between marking it for restyle and
     /// the actual restyle traversal.
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -619,17 +619,17 @@ impl<'le> TElement for GeckoElement<'le>
                                        namespace.0.as_ptr(),
                                        attr.as_ptr(),
                                        val.as_ptr(),
                                        /* ignoreCase = */ false)
         }
     }
 
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             _existing_values: &'a Arc<ComputedValues>,
+                                             _existing_values: &'a ComputedValues,
                                              pseudo: Option<&PseudoElement>)
                                              -> Option<&'a nsStyleContext> {
         // TODO(emilio): Migrate this to CSSPseudoElementType.
         let atom_ptr = pseudo.map_or(ptr::null_mut(), |p| p.atom().as_ptr());
         unsafe {
             let context_ptr = Gecko_GetStyleContext(self.0, atom_ptr);
             context_ptr.as_ref()
         }
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -491,16 +491,37 @@ trait PrivateMatchMethods: TElement {
         values
     }
 
     fn cascade_internal(&self,
                         context: &StyleContext<Self>,
                         primary_style: &ComputedStyle,
                         eager_pseudo_style: Option<&ComputedStyle>)
                         -> Arc<ComputedValues> {
+        if let Some(pseudo) = self.implemented_pseudo_element() {
+            debug_assert!(eager_pseudo_style.is_none());
+
+            // This is an element-backed pseudo, just grab the styles from the
+            // parent if it's eager, and recascade otherwise.
+            //
+            // We also recascade if the eager pseudo-style has any animation
+            // rules, because we don't cascade those during the eager traversal.
+            //
+            // We could make that a bit better if the complexity cost is not too
+            // big, but given further restyles are posted directly to
+            // pseudo-elements, it doesn't seem worth the effort at a glance.
+            if pseudo.is_eager() && self.get_animation_rules().is_empty() {
+                let parent = self.parent_element().unwrap();
+                let parent_data = parent.borrow_data().unwrap();
+                let pseudo_style =
+                    parent_data.styles().pseudos.get(&pseudo).unwrap();
+                return pseudo_style.values().clone()
+            }
+        }
+
         // Grab the rule node.
         let rule_node = &eager_pseudo_style.unwrap_or(primary_style).rules;
         let inherit_mode = if eager_pseudo_style.is_some() {
             InheritMode::FromPrimaryStyle
         } else {
             InheritMode::Normal
         };
 
@@ -508,107 +529,76 @@ trait PrivateMatchMethods: TElement {
                                 &context.thread_local.font_metrics_provider,
                                 rule_node,
                                 primary_style,
                                 inherit_mode)
     }
 
     /// Computes values and damage for the primary or pseudo style of an element,
     /// setting them on the ElementData.
-    fn cascade_primary_or_pseudo(&self,
-                                 context: &mut StyleContext<Self>,
-                                 data: &mut ElementData,
-                                 pseudo: Option<&PseudoElement>) {
-        debug_assert!(pseudo.is_none() || self.implemented_pseudo_element().is_none(),
-                      "Pseudo-element-implementing elements can't have pseudos!");
+    fn cascade_primary(&self,
+                       context: &mut StyleContext<Self>,
+                       data: &mut ElementData) {
         // Collect some values.
         let (mut styles, restyle) = data.styles_and_restyle_mut();
         let mut primary_style = &mut styles.primary;
-        let pseudos = &mut styles.pseudos;
-        let mut pseudo_style = match pseudo {
-            Some(p) => {
-                let style = pseudos.get_mut(p);
-                debug_assert!(style.is_some());
-                style
-            }
-            None => None,
-        };
-
-        let mut old_values = match pseudo_style {
-            Some(ref mut s) => s.values.take(),
-            None => primary_style.values.take(),
-        };
+        let mut old_values = primary_style.values.take();
 
         // Compute the new values.
-        let mut new_values = match self.implemented_pseudo_element() {
-            Some(ref pseudo) => {
-                // This is an element-backed pseudo, just grab the styles from
-                // the parent if it's eager, and recascade otherwise.
-                //
-                // We also recascade if the eager pseudo-style has any animation
-                // rules, because we don't cascade those during the eager
-                // traversal. We could make that a bit better if the complexity
-                // cost is not too big, but given further restyles are posted
-                // directly to pseudo-elements, it doesn't seem worth the effort
-                // at a glance.
-                if pseudo.is_eager() &&
-                   self.get_animation_rules().is_empty() {
-                    let parent = self.parent_element().unwrap();
-
-                    let parent_data = parent.borrow_data().unwrap();
-                    let pseudo_style =
-                        parent_data.styles().pseudos.get(pseudo).unwrap();
-                    pseudo_style.values().clone()
-                } else {
-                    self.cascade_internal(context,
-                                          primary_style,
-                                          None)
-                }
-            }
-            None => {
-                // Else it's an eager pseudo or a normal element, do the cascade
-                // work.
-                self.cascade_internal(context,
-                                      primary_style,
-                                      pseudo_style.as_ref().map(|s| &**s))
-            }
-        };
+        let mut new_values = self.cascade_internal(context, primary_style, None);
 
         // NB: Animations for pseudo-elements in Gecko are handled while
         // traversing the pseudo-elements themselves.
-        if pseudo.is_none() &&
-           !context.shared.traversal_flags.for_animation_only() {
+        if !context.shared.traversal_flags.for_animation_only() {
             self.process_animations(context,
                                     &mut old_values,
                                     &mut new_values,
                                     primary_style);
         }
 
-        // Accumulate restyle damage.
+        if let Some(old) = old_values {
+            self.accumulate_damage(&context.shared,
+                                   restyle.unwrap(),
+                                   &old,
+                                   &new_values,
+                                   None);
+        }
+
+        // Set the new computed values.
+        primary_style.values = Some(new_values);
+    }
+
+    fn cascade_eager_pseudo(&self,
+                            context: &mut StyleContext<Self>,
+                            data: &mut ElementData,
+                            pseudo: &PseudoElement) {
+        debug_assert!(pseudo.is_eager());
+        let (mut styles, restyle) = data.styles_and_restyle_mut();
+        let mut pseudo_style = styles.pseudos.get_mut(pseudo).unwrap();
+        let old_values = pseudo_style.values.take();
+
+        let new_values =
+            self.cascade_internal(context, &styles.primary, Some(pseudo_style));
+
         if let Some(old) = old_values {
             // ::before and ::after are element-backed in Gecko, so they do
             // the damage calculation for themselves.
-            //
-            // FIXME(emilio): We have more element-backed stuff, and this is
-            // redundant for them right now.
-            if cfg!(feature = "servo") ||
-               pseudo.map_or(true, |p| !p.is_before_or_after()) {
+            if cfg!(feature = "servo") || !pseudo.is_before_or_after() {
                 self.accumulate_damage(&context.shared,
                                        restyle.unwrap(),
                                        &old,
                                        &new_values,
-                                       pseudo);
+                                       Some(pseudo));
             }
         }
 
-        // Set the new computed values.
-        let mut relevant_style = pseudo_style.unwrap_or(primary_style);
-        relevant_style.values = Some(new_values);
+        pseudo_style.values = Some(new_values)
     }
 
+
     /// get_after_change_style removes the transition rules from the ComputedValues.
     /// If there is no transition rule in the ComputedValues, it returns None.
     #[cfg(feature = "gecko")]
     fn get_after_change_style(&self,
                               context: &mut StyleContext<Self>,
                               primary_style: &ComputedStyle)
                               -> Option<Arc<ComputedValues>> {
         let rule_node = &primary_style.rules;
@@ -748,25 +738,21 @@ trait PrivateMatchMethods: TElement {
                 &**values,
                 new_values,
                 &shared_context.timer,
                 &possibly_expired_animations);
         }
     }
 
     /// Computes and applies non-redundant damage.
-    ///
-    /// FIXME(emilio): Damage for non-::before and non-::after element-backed
-    /// pseudo-elements should be refactored to go on themselves (right now they
-    /// do, but we apply this twice).
     #[cfg(feature = "gecko")]
     fn accumulate_damage(&self,
                          shared_context: &SharedStyleContext,
                          restyle: &mut RestyleData,
-                         old_values: &Arc<ComputedValues>,
+                         old_values: &ComputedValues,
                          new_values: &Arc<ComputedValues>,
                          pseudo: Option<&PseudoElement>) {
         // Don't accumulate damage if we're in a restyle for reconstruction.
         if shared_context.traversal_flags.for_reconstruct() {
             return;
         }
 
         // If an ancestor is already getting reconstructed by Gecko's top-down
@@ -792,22 +778,22 @@ trait PrivateMatchMethods: TElement {
         }
     }
 
     /// Computes and applies restyle damage unless we've already maxed it out.
     #[cfg(feature = "servo")]
     fn accumulate_damage(&self,
                          _shared_context: &SharedStyleContext,
                          restyle: &mut RestyleData,
-                         old_values: &Arc<ComputedValues>,
+                         old_values: &ComputedValues,
                          new_values: &Arc<ComputedValues>,
                          pseudo: Option<&PseudoElement>) {
         if restyle.damage != RestyleDamage::rebuild_and_reflow() {
-            let d = self.compute_restyle_damage(&old_values, &new_values, pseudo);
-            restyle.damage |= d;
+            restyle.damage |=
+                self.compute_restyle_damage(&old_values, &new_values, pseudo);
         }
     }
 
     #[cfg(feature = "servo")]
     fn update_animations_for_cascade(&self,
                                      context: &SharedStyleContext,
                                      style: &mut Arc<ComputedValues>,
                                      possibly_expired_animations: &mut Vec<::animation::PropertyAnimation>,
@@ -1389,17 +1375,17 @@ pub trait MatchMethods : TElement {
             bf.remove_hash(class.get_hash())
         });
     }
 
     /// Given the old and new style of this element, and whether it's a
     /// pseudo-element, compute the restyle damage used to determine which
     /// kind of layout or painting operations we'll need.
     fn compute_restyle_damage(&self,
-                              old_values: &Arc<ComputedValues>,
+                              old_values: &ComputedValues,
                               new_values: &Arc<ComputedValues>,
                               pseudo: Option<&PseudoElement>)
                               -> RestyleDamage
     {
         match self.existing_style_for_restyle_damage(old_values, pseudo) {
             Some(ref source) => RestyleDamage::compute(source, new_values),
             None => {
                 // If there's no style source, that likely means that Gecko
@@ -1413,37 +1399,29 @@ pub trait MatchMethods : TElement {
                 } else {
                     // Something else. Be conservative for now.
                     RestyleDamage::reconstruct()
                 }
             }
         }
     }
 
-    /// Performs the cascade for the element's primary style.
-    fn cascade_primary(&self,
-                       context: &mut StyleContext<Self>,
-                       mut data: &mut ElementData)
-    {
-        self.cascade_primary_or_pseudo(context, &mut data, None);
-    }
-
-    /// Performs the cascade for the element's eager pseudos.
+    /// Cascade the eager pseudo-elements of this element.
     fn cascade_pseudos(&self,
                        context: &mut StyleContext<Self>,
                        mut data: &mut ElementData)
     {
         // Note that we've already set up the map of matching pseudo-elements
         // in match_pseudos (and handled the damage implications of changing
         // which pseudos match), so now we can just iterate what we have. This
         // does mean collecting owned pseudos, so that the borrow checker will
         // let us pass the mutable |data| to the cascade function.
         let matched_pseudos = data.styles().pseudos.keys();
         for pseudo in matched_pseudos {
-            self.cascade_primary_or_pseudo(context, data, Some(&pseudo));
+            self.cascade_eager_pseudo(context, data, &pseudo);
         }
     }
 
     /// Returns computed values without animation and transition rules.
     fn get_base_style(&self,
                       shared_context: &SharedStyleContext,
                       font_metrics_provider: &FontMetricsProvider,
                       primary_style: &ComputedStyle,
--- a/servo/components/style/selector_parser.rs
+++ b/servo/components/style/selector_parser.rs
@@ -32,17 +32,17 @@ pub use gecko::snapshot::GeckoElementSna
 pub use servo::restyle_damage::ServoRestyleDamage as RestyleDamage;
 
 #[cfg(feature = "gecko")]
 pub use gecko::restyle_damage::GeckoRestyleDamage as RestyleDamage;
 
 /// A type that represents the previous computed values needed for restyle
 /// damage calculation.
 #[cfg(feature = "servo")]
-pub type PreExistingComputedValues = ::stylearc::Arc<::properties::ServoComputedValues>;
+pub type PreExistingComputedValues = ::properties::ServoComputedValues;
 
 /// A type that represents the previous computed values needed for restyle
 /// damage calculation.
 #[cfg(feature = "gecko")]
 pub type PreExistingComputedValues = ::gecko_bindings::structs::nsStyleContext;
 
 /// Servo's selector parser.
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
--- a/servo/components/style/servo/restyle_damage.rs
+++ b/servo/components/style/servo/restyle_damage.rs
@@ -6,17 +6,16 @@
 //! be needed in presence of incremental style changes.
 
 #![deny(missing_docs)]
 
 use computed_values::display;
 use heapsize::HeapSizeOf;
 use properties::ServoComputedValues;
 use std::fmt;
-use stylearc::Arc;
 
 bitflags! {
     #[doc = "Individual layout actions that may be necessary after restyling."]
     pub flags ServoRestyleDamage: u8 {
         #[doc = "Repaint the node itself."]
         #[doc = "Currently unused; need to decide how this propagates."]
         const REPAINT = 0x01,
 
@@ -55,26 +54,27 @@ bitflags! {
 
 impl HeapSizeOf for ServoRestyleDamage {
     fn heap_size_of_children(&self) -> usize { 0 }
 }
 
 impl ServoRestyleDamage {
     /// Compute the appropriate restyle damage for a given style change between
     /// `old` and `new`.
-    pub fn compute(old: &Arc<ServoComputedValues>,
-                   new: &Arc<ServoComputedValues>) -> ServoRestyleDamage {
+    pub fn compute(old: &ServoComputedValues,
+                   new: &ServoComputedValues)
+                   -> ServoRestyleDamage {
         compute_damage(old, new)
     }
 
     /// Returns a bitmask that represents a flow that needs to be rebuilt and
     /// reflowed.
     ///
-    /// FIXME(bholley): Do we ever actually need this? Shouldn't RECONSTRUCT_FLOW
-    /// imply everything else?
+    /// FIXME(bholley): Do we ever actually need this? Shouldn't
+    /// RECONSTRUCT_FLOW imply everything else?
     pub fn rebuild_and_reflow() -> ServoRestyleDamage {
         REPAINT | REPOSITION | STORE_OVERFLOW | BUBBLE_ISIZES | REFLOW_OUT_OF_FLOW | REFLOW |
             RECONSTRUCT_FLOW
     }
 
     /// Returns a bitmask indicating that the frame needs to be reconstructed.
     pub fn reconstruct() -> ServoRestyleDamage {
         RECONSTRUCT_FLOW