Clean up and simplify the accumulation of restyle damage.

components/script/layout_wrapper.rs

@@ -393,10 +393,10 @@ impl<'le> TElement for ServoLayoutElement<'le> {
 
     #[inline]
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_cv: Option<&'a Arc<ComputedValues>>,
+                                             current_cv: &'a Arc<ComputedValues>,
                                              _pseudo_element: Option<&PseudoElement>)
                                              -> Option<&'a Arc<ComputedValues>> {
-        current_cv
+        Some(current_cv)
     }
 
     fn has_dirty_descendants(&self) -> bool {

components/style/dom.rs

@@ -276,7 +276,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
     /// 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: Option<&'a Arc<ComputedValues>>,
+                                             current_computed_values: &'a Arc<ComputedValues>,
                                              pseudo: Option<&PseudoElement>)
                                              -> Option<&'a PreExistingComputedValues>;
 

components/style/gecko/wrapper.rs

@@ -451,14 +451,9 @@ impl<'le> TElement for GeckoElement<'le> {
     }
 
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_cv: Option<&'a Arc<ComputedValues>>,
+                                             _existing_values: &'a Arc<ComputedValues>,
                                              pseudo: Option<&PseudoElement>)
                                              -> Option<&'a nsStyleContext> {
-        if current_cv.is_none() {
-            // Don't bother in doing an ffi call to get null back.
-            return None;
-        }
-
         unsafe {
             let atom_ptr = pseudo.map(|p| p.as_atom().as_ptr())
                                  .unwrap_or(ptr::null_mut());

components/style/matching.rs

@@ -13,7 +13,7 @@ use atomic_refcell::AtomicRefMut;
 use cache::LRUCache;
 use cascade_info::CascadeInfo;
 use context::{SequentialTask, SharedStyleContext, StyleContext};
-use data::{ComputedStyle, ElementData, ElementStyles};
+use data::{ComputedStyle, ElementData, ElementStyles, RestyleData};
 use dom::{SendElement, TElement, TNode};
 use properties::{CascadeFlags, ComputedValues, SHAREABLE, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade};
 use properties::longhands::display::computed_value as display;
@@ -556,13 +556,9 @@ trait PrivateMatchMethods: TElement {
             }
         }
 
-        // Accumulate restyle damage unless we've already maxed it out.
+        // Accumulate restyle damage.
         if let Some(old) = old_values {
-            let restyle = restyle.expect("Old values but no restyle?");
+            self.accumulate_damage(restyle.unwrap(), &old, &new_values, pseudo);
-            if restyle.damage != RestyleDamage::rebuild_and_reflow() {
-                let d = self.compute_restyle_damage(Some(&old), &new_values, pseudo);
-                restyle.damage |= d;
-            }
         }
 
         // Set the new computed values.
@@ -573,6 +569,18 @@ trait PrivateMatchMethods: TElement {
         }
     }
 
+    /// Computes and applies restyle damage unless we've already maxed it out.
+    fn accumulate_damage(&self,
+                         restyle: &mut RestyleData,
+                         old_values: &Arc<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;
+        }
+    }
+
     fn update_animations_for_cascade(&self,
                                      context: &SharedStyleContext,
                                      style: &mut Arc<ComputedValues>,
@@ -804,21 +812,18 @@ pub trait MatchMethods : TElement {
                 Ok(shared_style) => {
                     // Yay, cache hit. Share the style.
 
-                    // TODO: add the display: none optimisation here too! Even
+                    // Accumulate restyle damage.
-                    // better, factor it out/make it a bit more generic so Gecko
+                    debug_assert_eq!(data.has_styles(), data.has_restyle());
-                    // can decide more easily if it knows that it's a child of
+                    let old_values = data.get_styles_mut()
-                    // replaced content, or similar stuff!
+                                         .and_then(|s| s.primary.values.take());
-                    let maybe_damage = {
+                    if let Some(old) = old_values {
-                        let previous = data.get_styles().map(|x| x.primary.values());
+                        self.accumulate_damage(data.restyle_mut(), &old,
-                        let existing = self.existing_style_for_restyle_damage(previous, None);
+                                               shared_style.values(), None);
-                        existing.map(|e| RestyleDamage::compute(e, &shared_style.values()))
-                    };
-                    if let Some(d) = maybe_damage {
-                        data.restyle_mut().damage |= d;
                     }
 
-                    // We never put elements with pseudo style into the style sharing cache,
+                    // We never put elements with pseudo style into the style
-                    // so we can just mint an ElementStyles directly here.
+                    // sharing cache, so we can just mint an ElementStyles
+                    // directly here.
                     //
                     // See https://bugzilla.mozilla.org/show_bug.cgi?id=1329361
                     let styles = ElementStyles::new(shared_style);
@@ -901,40 +906,26 @@ pub trait MatchMethods : TElement {
     /// 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_style: Option<&Arc<ComputedValues>>,
+                              old_values: &Arc<ComputedValues>,
-                              new_style: &Arc<ComputedValues>,
+                              new_values: &Arc<ComputedValues>,
                               pseudo: Option<&PseudoElement>)
                               -> RestyleDamage
     {
-        match self.existing_style_for_restyle_damage(old_style, pseudo) {
+        match self.existing_style_for_restyle_damage(old_values, pseudo) {
-            Some(ref source) => RestyleDamage::compute(source, new_style),
+            Some(ref source) => RestyleDamage::compute(source, new_values),
             None => {
-                // If there's no style source, two things can happen:
+                // If there's no style source, that likely means that Gecko
-                //
+                // couldn't find a style context. This happens with display:none
-                //  1. This is not an incremental restyle (old_style is none).
+                // elements, and probably a number of other edge cases that
-                //     In this case we can't do too much than sending
+                // we don't handle well yet (like display:contents).
-                //     rebuild_and_reflow.
+                if new_values.get_box().clone_display() == display::T::none &&
-                //
+                    old_values.get_box().clone_display() == display::T::none {
-                //  2. This is an incremental restyle, but the old display value
+                    // The style remains display:none. No need for damage.
-                //     is none, so there's no effective way for Gecko to get the
+                    RestyleDamage::empty()
-                //     style source (which is the style context).
+                } else {
-                //
+                    // Something else. Be conservative for now.
-                //     In this case, we could return either
+                    RestyleDamage::rebuild_and_reflow()
-                //     RestyleDamage::empty(), in the case both displays are
-                //     none, or rebuild_and_reflow, otherwise.
-                //
-                if let Some(old_style) = old_style {
-                    // FIXME(emilio): This should assert the old style is
-                    // display: none, but we still can't get an old style
-                    // context for other stuff that should give us a style
-                    // context source like display: contents, so we fall on the
-                    // safe side here.
-                    if new_style.get_box().clone_display() == display::T::none &&
-                        old_style.get_box().clone_display() == display::T::none {
-                        return RestyleDamage::empty();
-                    }
                 }
-                RestyleDamage::rebuild_and_reflow()
             }
         }
     }