servo: Merge #15791 - Bug 1340334: Allow sibling hints in StoredRestyleHint, and handle them correctly (from emilio:hint); r=bholley
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 01 Mar 2017 18:08:43 -0800
changeset 392143 36bb16774744cba10efdfa1d33f4a5dd027f32ce
parent 392142 0f9b03faff358ed78b951b0f67401f8bd108bf34
child 392144 6fb5321bf52cd852ea49a6efb3c30c21205c03ef
push id7198
push userjlorenzo@mozilla.com
push dateTue, 18 Apr 2017 12:07:49 +0000
treeherdermozilla-beta@d57aa49c3948 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1340334
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #15791 - Bug 1340334: Allow sibling hints in StoredRestyleHint, and handle them correctly (from emilio:hint); r=bholley Reviewed upstream by Bobby. Source-Repo: https://github.com/servo/servo Source-Revision: d0856fd4cdc8dacd0f2c4034d32e495e76490d0a
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.