Bug 1340334: Allow sibling hints in StoredRestyleHint, and handle them correctly. r=bholley

MozReview-Commit-ID: H6NuKsfjEdj
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>

components/style/data.rs

@@ -9,7 +9,7 @@
 use dom::TElement;
 use properties::ComputedValues;
 use properties::longhands::display::computed_value as display;
-use restyle_hints::{RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RestyleHint};
+use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RestyleHint};
 use rule_tree::StrongRuleNode;
 use selector_parser::{PseudoElement, RestyleDamage, Snapshot};
 use std::collections::HashMap;
@@ -126,99 +126,53 @@ impl ElementStyles {
     }
 }
 
-/// Enum to describe the different requirements that a restyle hint may impose
-/// on its descendants.
-#[derive(Clone, Copy, Debug, PartialEq)]
-pub enum DescendantRestyleHint {
-    /// This hint does not require any descendants to be restyled.
-    Empty,
-    /// This hint requires direct children to be restyled.
-    Children,
-    /// This hint requires all descendants to be restyled.
-    Descendants,
-}
-
-impl DescendantRestyleHint {
-    /// Propagates this descendant behavior to a child element.
-    fn propagate(self) -> Self {
-        use self::DescendantRestyleHint::*;
-        if self == Descendants {
-            Descendants
-        } else {
-            Empty
-        }
-    }
-
-    fn union(self, other: Self) -> Self {
-        use self::DescendantRestyleHint::*;
-        if self == Descendants || other == Descendants {
-            Descendants
-        } else if self == Children || other == Children {
-            Children
-        } else {
-            Empty
-        }
-    }
-}
-
-/// Restyle hint for storing on ElementData. We use a separate representation
-/// to provide more type safety while propagating restyle hints down the tree.
-#[derive(Clone, Debug)]
-pub struct StoredRestyleHint {
-    /// Whether this element should be restyled during the traversal, and how.
+/// Restyle hint for storing on ElementData.
 ///
-    /// This hint is stripped down, and only contains hints that are a subset of
-    /// RestyleHint::for_single_element().
-    pub self_: RestyleHint,
-    /// Whether the descendants of this element need to be restyled.
-    pub descendants: DescendantRestyleHint,
-}
+/// We wrap it in a newtype to force the encapsulation of the complexity of
+/// handling the correct invalidations in this file.
+#[derive(Clone, Debug)]
+pub struct StoredRestyleHint(RestyleHint);
 
 impl StoredRestyleHint {
     /// Propagates this restyle hint to a child element.
     pub fn propagate(&self) -> Self {
-        StoredRestyleHint {
-            self_: if self.descendants == DescendantRestyleHint::Empty {
-                RestyleHint::empty()
+        StoredRestyleHint(if self.0.contains(RESTYLE_DESCENDANTS) {
+            RESTYLE_SELF | RESTYLE_DESCENDANTS
         } else {
-                RESTYLE_SELF
-            },
-            descendants: self.descendants.propagate(),
-        }
+            RestyleHint::empty()
+        })
     }
 
     /// Creates an empty `StoredRestyleHint`.
     pub fn empty() -> Self {
-        StoredRestyleHint {
-            self_: RestyleHint::empty(),
-            descendants: DescendantRestyleHint::Empty,
-        }
+        StoredRestyleHint(RestyleHint::empty())
     }
 
     /// Creates a restyle hint that forces the whole subtree to be restyled,
     /// including the element.
     pub fn subtree() -> Self {
-        StoredRestyleHint {
-            self_: RESTYLE_SELF,
-            descendants: DescendantRestyleHint::Descendants,
-        }
+        StoredRestyleHint(RESTYLE_SELF | RESTYLE_DESCENDANTS)
     }
 
     /// Returns true if the hint indicates that our style may be invalidated.
     pub fn has_self_invalidations(&self) -> bool {
-        !self.self_.is_empty()
+        self.0.intersects(RestyleHint::for_self())
+    }
+
+    /// Returns true if the hint indicates that our sibling's style may be
+    /// invalidated.
+    pub fn has_sibling_invalidations(&self) -> bool {
+        self.0.intersects(RESTYLE_LATER_SIBLINGS)
     }
 
     /// Whether the restyle hint is empty (nothing requires to be restyled).
     pub fn is_empty(&self) -> bool {
-        !self.has_self_invalidations() &&
-            self.descendants == DescendantRestyleHint::Empty
+        self.0.is_empty()
     }
 
     /// Insert another restyle hint, effectively resulting in the union of both.
     pub fn insert(&mut self, other: &Self) {
-        self.self_ |= other.self_;
-        self.descendants = self.descendants.union(other.descendants);
+        self.0 |= other.0
     }
 }
 
@@ -230,13 +184,7 @@ impl Default for StoredRestyleHint {
 
 impl From<RestyleHint> for StoredRestyleHint {
     fn from(hint: RestyleHint) -> Self {
-        use restyle_hints::*;
-        use self::DescendantRestyleHint::*;
-        debug_assert!(!hint.contains(RESTYLE_LATER_SIBLINGS), "Caller should apply sibling hints");
-        StoredRestyleHint {
-            self_: hint & RestyleHint::for_self(),
-            descendants: if hint.contains(RESTYLE_DESCENDANTS) { Descendants } else { Empty },
-        }
+        StoredRestyleHint(hint)
     }
 }
 
@@ -322,24 +270,30 @@ pub struct RestyleData {
 }
 
 impl RestyleData {
-    /// Expands the snapshot (if any) into a restyle hint. Returns true if later
-    /// siblings must be restyled.
-    pub fn expand_snapshot<E: TElement>(&mut self, element: E, stylist: &Stylist) -> bool {
-        if self.snapshot.is_none() {
-            return false;
-        }
+    /// Computes the final restyle hint for this element.
+    ///
+    /// This expands the snapshot (if any) into a restyle hint, and handles
+    /// explicit sibling restyle hints from the stored restyle hint.
+    ///
+    /// Returns true if later siblings must be restyled.
+    pub fn compute_final_hint<E: TElement>(&mut self,
+                                           element: E,
+                                           stylist: &Stylist)
+                                           -> bool {
+        let mut hint = self.hint.0;
 
-        // Compute the hint.
-        let mut hint = stylist.compute_restyle_hint(&element,
-                                                    self.snapshot.as_ref().unwrap());
+        if let Some(snapshot) = self.snapshot.as_ref() {
+            hint |= stylist.compute_restyle_hint(&element, snapshot);
+        }
 
         // If the hint includes a directive for later siblings, strip it out and
         // notify the caller to modify the base hint for future siblings.
         let later_siblings = hint.contains(RESTYLE_LATER_SIBLINGS);
         hint.remove(RESTYLE_LATER_SIBLINGS);
 
-        // Insert the hint.
-        self.hint.insert(&hint.into());
+        // Insert the hint, overriding the previous hint. This effectively takes
+        // care of removing the later siblings restyle hint.
+        self.hint = hint.into();
 
         // Destroy the snapshot.
         self.snapshot.destroy();
@@ -354,6 +308,11 @@ impl RestyleData {
             self.snapshot.is_some()
     }
 
+    /// Returns true if this RestyleData might invalidate sibling styles.
+    pub fn has_sibling_invalidations(&self) -> bool {
+        self.hint.has_sibling_invalidations() || self.snapshot.is_some()
+    }
+
     /// Returns damage handled.
     #[cfg(feature = "gecko")]
     pub fn damage_handled(&self) -> RestyleDamage {
@@ -436,7 +395,8 @@ impl ElementData {
 
         debug_assert!(self.restyle.is_some());
         let restyle_data = self.restyle.as_ref().unwrap();
-        let hint = restyle_data.hint.self_;
+
+        let hint = restyle_data.hint.0;
         if hint.contains(RESTYLE_SELF) {
             return RestyleKind::MatchAndCascade;
         }

components/style/traversal.rs

@@ -130,7 +130,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         if let Some(mut data) = root.mutate_data() {
             if let Some(r) = data.get_restyle_mut() {
                 debug_assert!(root.next_sibling_element().is_none());
-                let _later_siblings = r.expand_snapshot(root, stylist);
+                let _later_siblings = r.compute_final_hint(root, stylist);
             }
         }
 
@@ -414,8 +414,9 @@ pub fn recalc_style_at<E, D>(traversal: &D,
 {
     context.thread_local.begin_element(element, &data);
     context.thread_local.statistics.elements_traversed += 1;
-    debug_assert!(data.get_restyle().map_or(true, |r| r.snapshot.is_none()),
-                  "Snapshots should be expanded by the caller");
+    debug_assert!(data.get_restyle().map_or(true, |r| {
+        r.snapshot.is_none() && !r.has_sibling_invalidations()
+    }), "Should've computed the final hint and handled later_siblings already");
 
     let compute_self = !data.has_current_styles();
     let mut inherited_style_changed = false;
@@ -594,9 +595,9 @@ fn preprocess_children<E, D>(traversal: &D,
             restyle_data.hint.insert(&propagated_hint);
         }
 
-        // Handle element snapshots.
+        // Handle element snapshots and sibling restyle hints.
         let stylist = &traversal.shared_context().stylist;
-        let later_siblings = restyle_data.expand_snapshot(child, stylist);
+        let later_siblings = restyle_data.compute_final_hint(child, stylist);
         if later_siblings {
             propagated_hint.insert(&(RESTYLE_SELF | RESTYLE_DESCENDANTS).into());
         }