Bug 1340334: Allow sibling hints in StoredRestyleHint, and handle them correctly. r?bholley draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 01 Mar 2017 11:55:18 +0100
changeset 490833 d7d7fed66cdddbe02e8dd44d96018cbcea0e3103
parent 490832 c595ed0d28da33dbfe07398394201269f9702030
child 547392 9994fa8683bbd36caf39cb46f51bc8d93142c033
push id47244
push userbmo:emilio+bugs@crisal.io
push dateWed, 01 Mar 2017 10:56:33 +0000
reviewersbholley
bugs1340334
milestone54.0a1
Bug 1340334: Allow sibling hints in StoredRestyleHint, and handle them correctly. r?bholley MozReview-Commit-ID: H6NuKsfjEdj
servo/components/style/data.rs
servo/components/style/traversal.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -4,17 +4,17 @@
 
 //! Per-node data used in style calculation.
 
 #![deny(missing_docs)]
 
 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;
 use std::fmt;
 use std::hash::BuildHasherDefault;
 use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
 use stylist::Stylist;
@@ -121,127 +121,75 @@ impl ElementStyles {
     }
 
     /// Whether this element `display` value is `none`.
     pub fn is_display_none(&self) -> bool {
         self.primary.values().get_box().clone_display() == display::T::none
     }
 }
 
-/// 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.
+/// Restyle hint for storing on ElementData.
+///
+/// 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 {
-    /// Whether this element should be restyled during the traversal, and how.
-    ///
-    /// 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,
-}
+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()
-            } else {
-                RESTYLE_SELF
-            },
-            descendants: self.descendants.propagate(),
-        }
+        StoredRestyleHint(if self.0.contains(RESTYLE_DESCENDANTS) {
+            RESTYLE_SELF | RESTYLE_DESCENDANTS
+        } else {
+            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
     }
 }
 
 impl Default for StoredRestyleHint {
     fn default() -> Self {
         StoredRestyleHint::empty()
     }
 }
 
 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)
     }
 }
 
 static NO_SNAPSHOT: Option<Snapshot> = None;
 
 /// We really want to store an Option<Snapshot> here, but we can't drop Gecko
 /// Snapshots off-main-thread. So we make a convenient little wrapper to provide
 /// the semantics of Option<Snapshot>, while deferring the actual drop.
@@ -317,48 +265,59 @@ pub struct RestyleData {
     pub damage_handled: RestyleDamage,
 
     /// An optional snapshot of the original state and attributes of the element,
     /// from which we may compute additional restyle hints at traversal time.
     pub snapshot: SnapshotOption,
 }
 
 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;
+
+        if let Some(snapshot) = self.snapshot.as_ref() {
+            hint |= stylist.compute_restyle_hint(&element, snapshot);
         }
 
-        // Compute the hint.
-        let mut hint = stylist.compute_restyle_hint(&element,
-                                                    self.snapshot.as_ref().unwrap());
-
         // 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();
 
         later_siblings
     }
 
     /// Returns true if this RestyleData might invalidate the current style.
     pub fn has_invalidations(&self) -> bool {
         self.hint.has_self_invalidations() ||
             self.recascade ||
             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 {
         self.damage_handled
     }
 
     /// Returns damage handled (always empty for servo).
     #[cfg(feature = "servo")]
@@ -431,17 +390,18 @@ impl ElementData {
     pub fn restyle_kind(&self) -> RestyleKind {
         debug_assert!(!self.has_current_styles(), "Should've stopped earlier");
         if !self.has_styles() {
             return RestyleKind::MatchAndCascade;
         }
 
         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;
         }
 
         if !hint.is_empty() {
             return RestyleKind::CascadeWithReplacements(hint);
         }
 
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -125,17 +125,17 @@ pub trait DomTraversal<E: TElement> : Sy
         // we need a special case for the root.
         //
         // Expanding snapshots here may create a LATER_SIBLINGS restyle hint, which
         // we will drop on the floor. To prevent missed restyles, we assert against
         // restyling a root with later siblings.
         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);
             }
         }
 
         PreTraverseToken {
             traverse: Self::node_needs_traversal(root.as_node()),
             unstyled_children_only: false,
         }
     }
@@ -409,18 +409,19 @@ pub fn recalc_style_at<E, D>(traversal: 
                              context: &mut StyleContext<E>,
                              element: E,
                              mut data: &mut AtomicRefMut<ElementData>)
     where E: TElement,
           D: DomTraversal<E>
 {
     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;
 
     debug!("recalc_style_at: {:?} (compute_self={:?}, dirty_descendants={:?}, data={:?})",
            element, compute_self, element.has_dirty_descendants(), data);
 
     // Compute style for this element if necessary.
@@ -589,19 +590,19 @@ fn preprocess_children<E, D>(traversal: 
         }
         let mut restyle_data = child_data.ensure_restyle();
 
         // Propagate the parent and sibling restyle hint.
         if !propagated_hint.is_empty() {
             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());
         }
 
         // Store the damage already handled by ancestors.
         restyle_data.set_damage_handled(damage_handled);
 
         // If properties that we inherited from the parent changed, we need to recascade.