Bug 1368236: Remove damage_handled, and use a reconstructed_ancestor bit instead. r?bholley draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 13 Jun 2017 14:13:24 +0200
changeset 593575 8c622cfa9d8b75e444be10f5ce692018a9a120d3
parent 593574 6025fdb27f8a381d0c30c87118977304b648b006
child 593576 2b965c93d8587e6d40eaa292d445fb23b5eef58b
push id63741
push userbmo:emilio+bugs@crisal.io
push dateTue, 13 Jun 2017 21:27:09 +0000
reviewersbholley
bugs1368236
milestone56.0a1
Bug 1368236: Remove damage_handled, and use a reconstructed_ancestor bit instead. r?bholley MozReview-Commit-ID: 8KK0YfhiS2
servo/components/style/data.rs
servo/components/style/matching.rs
servo/components/style/traversal.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -352,58 +352,29 @@ impl ElementStyles {
 /// either before or during restyle traversal, and is cleared at the end of node
 /// processing.
 #[derive(Debug, Default)]
 pub struct RestyleData {
     /// The restyle hint, which indicates whether selectors need to be rematched
     /// for this element, its children, and its descendants.
     pub hint: RestyleHint,
 
+    /// Whether we reframed/reconstructed any ancestor or self.
+    pub reconstructed_ancestor: bool,
+
     /// The restyle damage, indicating what kind of layout changes are required
     /// afte restyling.
     pub damage: RestyleDamage,
-
-    /// The restyle damage that has already been handled by our ancestors, and does
-    /// not need to be applied again at this element. Only non-empty during the
-    /// traversal, once ancestor damage has been calculated.
-    ///
-    /// Note that this optimization mostly makes sense in terms of Gecko's top-down
-    /// frame constructor and change list processing model. We don't bother with it
-    /// for Servo for now.
-    #[cfg(feature = "gecko")]
-    pub damage_handled: RestyleDamage,
 }
 
 impl RestyleData {
     /// Returns true if this RestyleData might invalidate the current style.
     pub fn has_invalidations(&self) -> bool {
         self.hint.has_self_invalidations()
     }
-
-    /// Returns damage handled.
-    #[cfg(feature = "gecko")]
-    pub fn damage_handled(&self) -> RestyleDamage {
-        self.damage_handled
-    }
-
-    /// Returns damage handled (always empty for servo).
-    #[cfg(feature = "servo")]
-    pub fn damage_handled(&self) -> RestyleDamage {
-        RestyleDamage::empty()
-    }
-
-    /// Sets damage handled.
-    #[cfg(feature = "gecko")]
-    pub fn set_damage_handled(&mut self, d: RestyleDamage) {
-        self.damage_handled = d;
-    }
-
-    /// Sets damage handled. No-op for Servo.
-    #[cfg(feature = "servo")]
-    pub fn set_damage_handled(&mut self, _: RestyleDamage) {}
 }
 
 /// Style system data associated with an Element.
 ///
 /// In Gecko, this hangs directly off the Element. Servo, this is embedded
 /// inside of layout data, which itself hangs directly off the Element. In
 /// both cases, it is wrapped inside an AtomicRefCell to ensure thread safety.
 #[derive(Debug)]
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -692,18 +692,18 @@ trait PrivateMatchMethods: TElement {
         // If an ancestor is already getting reconstructed by Gecko's top-down
         // frame constructor, no need to apply damage.  Similarly if we already
         // have an explicitly stored ReconstructFrame hint.
         //
         // See https://bugzilla.mozilla.org/show_bug.cgi?id=1301258#c12
         // for followup work to make the optimization here more optimal by considering
         // each bit individually.
         let skip_applying_damage =
-            restyle.damage_handled.contains(RestyleDamage::reconstruct()) ||
-            restyle.damage.contains(RestyleDamage::reconstruct());
+            restyle.damage.contains(RestyleDamage::reconstruct()) ||
+            restyle.reconstructed_ancestor;
 
         let difference = self.compute_style_difference(&old_values,
                                                        &new_values,
                                                        pseudo);
         if !skip_applying_damage {
             restyle.damage |= difference.damage;
         }
         difference.change.into()
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -292,18 +292,18 @@ pub trait DomTraversal<E: TElement> : Sy
         // transitions that we won't see otherwise.
         //
         // But it may be that we no longer match, so detect that case
         // and act appropriately here.
         if el.is_native_anonymous() {
             if let Some(parent) = el.traversal_parent() {
                 let parent_data = parent.borrow_data().unwrap();
                 let going_to_reframe = parent_data.get_restyle().map_or(false, |r| {
-                    (r.damage | r.damage_handled())
-                        .contains(RestyleDamage::reconstruct())
+                    r.reconstructed_ancestor ||
+                    r.damage.contains(RestyleDamage::reconstruct())
                 });
 
                 let mut is_before_or_after_pseudo = false;
                 if let Some(pseudo) = el.implemented_pseudo_element() {
                     if pseudo.is_before_or_after() {
                         is_before_or_after_pseudo = true;
                         let still_match =
                             parent_data.styles().pseudos.get(&pseudo).is_some();
@@ -725,24 +725,26 @@ pub fn recalc_style_at<E, D>(traversal: 
     // Preprocess children, propagating restyle hints and handling sibling
     // relationships.
     if traversal.should_traverse_children(&mut context.thread_local,
                                           element,
                                           &data,
                                           DontLog) &&
         (has_dirty_descendants_for_this_restyle ||
          !propagated_hint.is_empty()) {
-        let damage_handled = data.get_restyle().map_or(RestyleDamage::empty(), |r| {
-            r.damage_handled() | r.damage.handled_for_descendants()
+        let reconstructed_ancestor = data.get_restyle().map_or(false, |r| {
+            r.reconstructed_ancestor ||
+            r.damage.contains(RestyleDamage::reconstruct())
         });
-
-        preprocess_children::<E, D>(context,
-                                    element,
-                                    propagated_hint,
-                                    damage_handled);
+        preprocess_children::<E, D>(
+            context,
+            element,
+            propagated_hint,
+            reconstructed_ancestor,
+        )
     }
 
     // If we are in a restyle for reconstruction, drop the existing restyle
     // data here, since we won't need to perform a post-traversal to pick up
     // any change hints.
     if context.shared.traversal_flags.for_reconstruct() {
         data.clear_restyle();
     }
@@ -830,22 +832,25 @@ fn compute_style<E, D>(_traversal: &D,
                 context,
                 data,
                 /* important_rules_changed = */ false
             )
         }
     }
 }
 
-fn preprocess_children<E, D>(context: &mut StyleContext<E>,
-                             element: E,
-                             propagated_hint: RestyleHint,
-                             damage_handled: RestyleDamage)
-    where E: TElement,
-          D: DomTraversal<E>,
+fn preprocess_children<E, D>(
+    context: &mut StyleContext<E>,
+    element: E,
+    propagated_hint: RestyleHint,
+    reconstructed_ancestor: bool,
+)
+where
+    E: TElement,
+    D: DomTraversal<E>,
 {
     trace!("preprocess_children: {:?}", element);
 
     // Loop over all the traversal children.
     for child in element.as_node().traversal_children() {
         // FIXME(bholley): Add TElement::element_children instead of this.
         let child = match child.as_element() {
             Some(el) => el,
@@ -870,30 +875,29 @@ fn preprocess_children<E, D>(context: &m
                child,
                child_data.get_restyle().map(|r| &r.hint),
                propagated_hint,
                child.implemented_pseudo_element());
 
         // If the child doesn't have pre-existing RestyleData and we don't have
         // any reason to create one, avoid the useless allocation and move on to
         // the next child.
-        if propagated_hint.is_empty() &&
-            damage_handled.is_empty() &&
-            !child_data.has_restyle() {
+        if !reconstructed_ancestor && propagated_hint.is_empty() && !child_data.has_restyle() {
             continue;
         }
 
         let mut restyle_data = child_data.ensure_restyle();
 
+        debug_assert!(!restyle_data.reconstructed_ancestor,
+                      "How did we know this already?");
+
         // Propagate the parent restyle hint, that may make us restyle the whole
         // subtree.
+        restyle_data.reconstructed_ancestor = reconstructed_ancestor;
         restyle_data.hint.insert(propagated_hint);
-
-        // Store the damage already handled by ancestors.
-        restyle_data.set_damage_handled(damage_handled);
     }
 }
 
 /// Clear style data for all the subtree under `el`.
 pub fn clear_descendant_data<E: TElement, F: Fn(E)>(el: E, clear_data: &F) {
     for kid in el.as_node().traversal_children() {
         if let Some(kid) = kid.as_element() {
             // We maintain an invariant that, if an element has data, all its ancestors