Make RestyleDamage::compute take no optional argument, delegate the handling to the caller.

components/layout/animation.rs

@@ -135,7 +135,7 @@ pub fn recalc_style_for_animations(context: &SharedLayoutContext,
                 update_style_for_animation(&context.style_context,
                                            animation,
                                            &mut fragment.style);
-                damage |= RestyleDamage::compute(Some(&old_style), &fragment.style);
+                damage |= RestyleDamage::compute(&old_style, &fragment.style);
             }
         }
     });

components/script_layout_interface/restyle_damage.rs

@@ -51,7 +51,7 @@ impl TRestyleDamage for RestyleDamage {
         RestyleDamage::empty()
     }
 
-    fn compute(old: Option<&Arc<ServoComputedValues>>,
+    fn compute(old: &Arc<ServoComputedValues>,
                new: &Arc<ServoComputedValues>) -> RestyleDamage {
         compute_damage(old, new)
     }
@@ -154,13 +154,7 @@ macro_rules! add_if_not_equal(
     })
 );
 
-fn compute_damage(old: Option<&Arc<ServoComputedValues>>, new: &Arc<ServoComputedValues>) -> RestyleDamage {
+fn compute_damage(old: &ServoComputedValues, new: &ServoComputedValues) -> RestyleDamage {
-    let new = &**new;
-    let old: &ServoComputedValues = match old {
-        None => return RestyleDamage::rebuild_and_reflow(),
-        Some(cv) => &**cv,
-    };
-
     let mut damage = RestyleDamage::empty();
 
     // This should check every CSS property, as enumerated in the fields of

components/style/dom.rs

@@ -45,7 +45,7 @@ impl OpaqueNode {
     }
 }
 
-pub trait TRestyleDamage : Debug + BitOr<Output=Self> + Copy {
+pub trait TRestyleDamage : Debug + PartialEq + 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.
     ///
@@ -56,7 +56,7 @@ pub trait TRestyleDamage : Debug + BitOr<Output=Self> + Copy {
     /// This should be obtained via TNode::existing_style_for_restyle_damage
     type PreExistingComputedValues;
 
-    fn compute(old: Option<&Self::PreExistingComputedValues>,
+    fn compute(old: &Self::PreExistingComputedValues,
                new: &Arc<ComputedValues>) -> Self;
 
     fn empty() -> Self;

components/style/matching.rs

@@ -638,13 +638,17 @@ pub trait ElementMatchMethods : TElement {
 
                 let style = &mut node.mutate_data().unwrap().style;
 
-                let damage = {
+                let damage =
-                    let source =
+                    match node.existing_style_for_restyle_damage((*style).as_ref()) {
-                        node.existing_style_for_restyle_damage((*style).as_ref());
+                        Some(ref source) => {
-                    let damage = <<Self as TElement>::ConcreteNode as TNode>
+                            <<Self as TElement>::ConcreteNode as TNode>
-                                     ::ConcreteRestyleDamage::compute(source, &shared_style);
+                            ::ConcreteRestyleDamage::compute(source, &shared_style)
-                    damage
+                        }
-                };
+                        None => {
+                            <<Self as TElement>::ConcreteNode as TNode>
+                            ::ConcreteRestyleDamage::rebuild_and_reflow()
+                        }
+                    };
 
                 let restyle_result = if shared_style.get_box().clone_display() == display::T::none {
                     RestyleResult::Stop
@@ -713,6 +717,43 @@ pub trait MatchMethods : TNode {
         }
     }
 
+    fn compute_restyle_damage(&self,
+                              old_style: Option<&Arc<ComputedValues>>,
+                              new_style: &Arc<ComputedValues>) -> Self::ConcreteRestyleDamage {
+        match self.existing_style_for_restyle_damage(old_style) {
+            Some(ref source) => {
+                Self::ConcreteRestyleDamage::compute(source,
+                                                     new_style)
+            }
+            None => {
+                // If there's no style source, two things can happen:
+                //
+                //  1. This is not an incremental restyle (old_style is none).
+                //     In this case we can't do too much than sending
+                //     rebuild_and_reflow.
+                //
+                //  2. This is an incremental restyle, but the old display value
+                //     is none, so there's no effective way for Gecko to get the
+                //     style source. In this case, we could return either
+                //     RestyleDamage::empty(), in the case both displays are
+                //     none, or rebuild_and_reflow, otherwise. The first case
+                //     should be already handled when calling this function, so
+                //     we can assert that the new display value is not none.
+                //
+                //     Also, this can be a text node (in which case we don't
+                //     care of watching the new display value).
+                //
+                // Unfortunately we can't strongly assert part of this, since
+                // we style some nodes that in Gecko never generate a frame,
+                // like children of replaced content. Arguably, we shouldn't be
+                // styling those here, but until we implement that we'll have to
+                // stick without the assertions.
+                debug_assert!(new_style.get_box().clone_display() != display::T::none);
+                Self::ConcreteRestyleDamage::rebuild_and_reflow()
+            }
+        }
+    }
+
     unsafe fn cascade_node<'a, Ctx>(&self,
                                     context: &Ctx,
                                     parent: Option<Self>,
@@ -741,12 +782,8 @@ pub trait MatchMethods : TNode {
             let mut data = &mut *data_ref;
             let cloned_parent_style = ComputedValues::style_for_child_text_node(parent_style.unwrap());
 
-            let damage = {
+            let damage =
-                let existing_style =
+                self.compute_restyle_damage(data.style.as_ref(), &cloned_parent_style);
-                    self.existing_style_for_restyle_damage(data.style.as_ref());
-                Self::ConcreteRestyleDamage::compute(existing_style,
-                                                     &cloned_parent_style)
-            };
 
             data.style = Some(cloned_parent_style);
 
@@ -810,9 +847,7 @@ pub trait MatchMethods : TNode {
                 Some(display) if display == this_display => {
                     Self::ConcreteRestyleDamage::empty()
                 }
-                _ => {
+                _ => Self::ConcreteRestyleDamage::rebuild_and_reflow()
-                    Self::ConcreteRestyleDamage::rebuild_and_reflow()
-                }
             };
 
             debug!("Short-circuiting traversal: {:?} {:?} {:?}",
@@ -824,12 +859,8 @@ pub trait MatchMethods : TNode {
 
         // Otherwise, we just compute the damage normally, and sum up the damage
         // related to pseudo-elements.
-        let mut damage = {
+        let mut damage =
-            let existing_style =
+            self.compute_restyle_damage(data.style.as_ref(), &final_style);
-                self.existing_style_for_restyle_damage(data.style.as_ref());
-
-            Self::ConcreteRestyleDamage::compute(existing_style, &final_style)
-        };
 
         data.style = Some(final_style);
 
@@ -837,18 +868,23 @@ pub trait MatchMethods : TNode {
         // 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
+        // We should be diffing the old pseudo-element styles with the new ones,
-        // element restyle damage vs the new one, given that we're only summing
+        // see https://bugzilla.mozilla.org/show_bug.cgi?id=1293399.
-        // 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
+        // For now we do the following, which preserves the old Servo behavior.
-        // 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());
 
+        let rebuild_and_reflow =
+            Self::ConcreteRestyleDamage::rebuild_and_reflow();
+
+        debug_assert!(existing_style.is_some() || damage == rebuild_and_reflow);
+
         <Self::ConcreteElement as MatchAttr>::Impl::each_eagerly_cascaded_pseudo_element(|pseudo| {
             let applicable_declarations_for_this_pseudo =
                 applicable_declarations.per_pseudo.get(&pseudo).unwrap();
@@ -868,9 +904,14 @@ pub trait MatchMethods : TNode {
                                                      /* shareable = */ false,
                                                      should_animate_properties);
 
-                let new_damage =
+                // No point in computing more damage if we're already doing all
-                    Self::ConcreteRestyleDamage::compute(existing_style, &new_pseudo_style);
+                // the work.
-                damage = damage | new_damage;
+                if damage != rebuild_and_reflow {
+                    let new_damage =
+                        Self::ConcreteRestyleDamage::compute(existing_style.unwrap(),
+                                                             &new_pseudo_style);
+                    damage = damage | new_damage;
+                }
                 data_per_pseudo.insert(pseudo, new_pseudo_style);
             }
         });

ports/geckolib/wrapper.rs

@@ -95,7 +95,7 @@ impl<'ln> GeckoNode<'ln> {
     }
 }
 
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 pub struct GeckoRestyleDamage(nsChangeHint);
 
 impl TRestyleDamage for GeckoRestyleDamage {
@@ -106,31 +106,10 @@ impl TRestyleDamage for GeckoRestyleDamage {
         GeckoRestyleDamage(unsafe { mem::transmute(0u32) })
     }
 
-    fn compute(source: Option<&nsStyleContext>,
+    fn compute(source: &nsStyleContext,
                new_style: &Arc<ComputedValues>) -> Self {
         type Helpers = ArcHelpers<ServoComputedValues, ComputedValues>;
-        let context = match source {
+        let context = source as *const nsStyleContext as *mut nsStyleContext;
-            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(),
-        };
 
         Helpers::borrow(new_style, |new_style| {
             let hint = unsafe { Gecko_CalcStyleDifference(context, new_style) };