Bug 1364412: Properly handle state restyle hints for pseudo-elements. r?bholley draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 13 May 2017 22:27:10 +0200
changeset 577939 77091a2cd1cd79019fb1cfd68c501f7efeb06547
parent 577938 034d850fdbdd59085844db28a7aceeb79b7a5252
child 577940 b6cf1e0743e99ffbeb50c735610cd712e7096f79
push id58837
push userbmo:emilio+bugs@crisal.io
push dateMon, 15 May 2017 17:46:39 +0000
reviewersbholley
bugs1364412
milestone55.0a1
Bug 1364412: Properly handle state restyle hints for pseudo-elements. r?bholley I'd really love to get rid of the SUBTREE restyle, but that just isn't going to happen on the foreseeable future... Still all the abstractions are there to make it possible seamlessly if we ever can. MozReview-Commit-ID: 3ZqC9lSgM8c
servo/components/style/data.rs
servo/components/style/restyle_hints.rs
servo/components/style/traversal.rs
--- a/servo/components/style/data.rs
+++ b/servo/components/style/data.rs
@@ -379,16 +379,23 @@ impl ElementData {
                element,
                context.traversal_flags);
 
         let mut hint = match self.get_restyle() {
             Some(r) => r.hint.0,
             None => RestyleHint::empty(),
         };
 
+        debug!("compute_final_hint: {:?}, has_snapshot: {}, handled_snapshot: {}, \
+                pseudo: {:?}",
+                element,
+                element.has_snapshot(),
+                element.handled_snapshot(),
+                element.implemented_pseudo_element());
+
         if element.has_snapshot() && !element.handled_snapshot() {
             hint |= context.stylist.compute_restyle_hint(&element, context.snapshot_map);
             unsafe { element.set_handled_snapshot() }
             debug_assert!(element.handled_snapshot());
         }
 
         let empty_hint = hint.is_empty();
 
--- a/servo/components/style/restyle_hints.rs
+++ b/servo/components/style/restyle_hints.rs
@@ -4,21 +4,22 @@
 
 //! Restyle hints: an optimization to avoid unnecessarily matching selectors.
 
 #![deny(missing_docs)]
 
 use Atom;
 use dom::TElement;
 use element_state::*;
+use fnv::FnvHashMap;
 #[cfg(feature = "gecko")]
 use gecko_bindings::structs::nsRestyleHint;
 #[cfg(feature = "servo")]
 use heapsize::HeapSizeOf;
-use selector_parser::{AttrValue, NonTSPseudoClass, SelectorImpl, Snapshot, SnapshotMap};
+use selector_parser::{AttrValue, NonTSPseudoClass, PseudoElement, SelectorImpl, Snapshot, SnapshotMap};
 use selectors::{Element, MatchAttr};
 use selectors::matching::{ElementSelectorFlags, StyleRelations};
 use selectors::matching::matches_selector;
 use selectors::parser::{AttrSelector, Combinator, Component, Selector};
 use selectors::parser::{SelectorInner, SelectorMethods};
 use selectors::visitor::SelectorVisitor;
 use smallvec::SmallVec;
 use std::borrow::Borrow;
@@ -223,16 +224,28 @@ impl<'a, E> ElementWrapper<'a, E>
 
         let snapshot = self.snapshot_map.get(&self.element);
         debug_assert!(snapshot.is_some(), "has_snapshot lied!");
 
         self.cached_snapshot.set(snapshot);
 
         snapshot
     }
+
+    fn state_changes(&self) -> ElementState {
+        let snapshot = match self.snapshot() {
+            Some(s) => s,
+            None => return ElementState::empty(),
+        };
+
+        match snapshot.state() {
+            Some(state) => state ^ self.element.get_state(),
+            None => ElementState::empty(),
+        }
+    }
 }
 
 impl<'a, E> MatchAttr for ElementWrapper<'a, E>
     where E: TElement,
 {
     type Impl = SelectorImpl;
 
     fn match_attr_has(&self, attr: &AttrSelector<SelectorImpl>) -> bool {
@@ -499,31 +512,62 @@ impl Sensitivities {
 ///
 /// We generate a Dependency for both |a _ b:X _| and |a _ b:X _ c _ d:Y _|,
 /// even though those selectors may not appear on their own in any stylesheet.
 /// This allows us to quickly scan through the dependency sites of all style
 /// rules and determine the maximum effect that a given state or attribute
 /// change may have on the style of elements in the document.
 #[derive(Clone, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-pub struct Dependency {
+pub struct ElementDependency {
     #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
     selector: SelectorInner<SelectorImpl>,
     /// The hint associated with this dependency.
     pub hint: RestyleHint,
     /// The sensitivities associated with this dependency.
     pub sensitivities: Sensitivities,
 }
 
-impl Borrow<SelectorInner<SelectorImpl>> for Dependency {
+impl Borrow<SelectorInner<SelectorImpl>> for ElementDependency {
     fn borrow(&self) -> &SelectorInner<SelectorImpl> {
         &self.selector
     }
 }
 
+/// A similar version of the above, but for pseudo-elements, which only care
+/// about the full selector, and need it in order to properly track
+/// pseudo-element selector state.
+///
+/// NOTE(emilio): We could add a `hint` and `sensitivities` field to the
+/// `PseudoElementDependency` and stop posting `RESTYLE_DESCENDANTS`s hints if
+/// we visited all the pseudo-elements of an element unconditionally as part of
+/// the traversal.
+///
+/// That would allow us to stop posting `RESTYLE_DESCENDANTS` hints for dumb
+/// selectors, and storing pseudo dependencies in the element dependency map.
+///
+/// That would allow us to avoid restyling the element itself when a selector
+/// has only changed a pseudo-element's style, too.
+///
+/// There's no good way to do that right now though, and I think for the
+/// foreseeable future we may just want to optimize that `RESTYLE_DESCENDANTS`
+/// to become a `RESTYLE_PSEUDO_ELEMENTS` or something like that, in order to at
+/// least not restyle the whole subtree.
+#[derive(Clone, Debug)]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+struct PseudoElementDependency {
+    selector: Selector<SelectorImpl>,
+}
+
+impl Borrow<SelectorInner<SelectorImpl>> for PseudoElementDependency {
+    fn borrow(&self) -> &SelectorInner<SelectorImpl> {
+        &self.selector.inner
+    }
+}
+
 /// The following visitor visits all the simple selectors for a given complex
 /// selector, taking care of :not and :any combinators, collecting whether any
 /// of them is sensitive to attribute or state changes.
 struct SensitivitiesVisitor {
     sensitivities: Sensitivities,
 }
 
 impl SelectorVisitor for SensitivitiesVisitor {
@@ -538,23 +582,29 @@ impl SelectorVisitor for SensitivitiesVi
 /// A set of dependencies for a given stylist.
 ///
 /// Note that we can have many dependencies, often more than the total number
 /// of selectors given that we can get multiple partial selectors for a given
 /// selector. As such, we want all the usual optimizations, including the
 /// SelectorMap and the bloom filter.
 #[derive(Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-pub struct DependencySet(pub SelectorMap<Dependency>);
+pub struct DependencySet {
+    /// A map used for pseudo-element's dependencies.
+    ///
+    /// Note that pseudo-elements are somewhat special, because some of them in
+    /// Gecko track state, and also because they don't do selector-matching as
+    /// normal, but against their parent element.
+    pseudo_dependencies: FnvHashMap<PseudoElement, SelectorMap<PseudoElementDependency>>,
+
+    /// This is for all other normal element's selectors/selector parts.
+    element_dependencies: SelectorMap<ElementDependency>,
+}
 
 impl DependencySet {
-    fn add_dependency(&mut self, dep: Dependency) {
-        self.0.insert(dep);
-    }
-
     /// Adds a selector to this `DependencySet`.
     pub fn note_selector(&mut self, selector: &Selector<SelectorImpl>) {
         let mut combinator = None;
         let mut iter = selector.inner.complex.iter();
         let mut index = 0;
 
         loop {
             let sequence_start = index;
@@ -567,41 +617,53 @@ impl DependencySet {
             // Note that this works because we can't have combinators nested
             // inside simple selectors (i.e. in :not() or :-moz-any()). If we
             // ever support that we'll need to visit complex selectors as well.
             for ss in &mut iter {
                 ss.visit(&mut visitor);
                 index += 1; // Account for the simple selector.
             }
 
+
+            let pseudo_selector_affects_state =
+                sequence_start == 0 &&
+                selector.pseudo_element.as_ref().map_or(false, |pseudo_selector| {
+                    !pseudo_selector.state().is_empty()
+                });
+
+            if pseudo_selector_affects_state {
+                let pseudo_selector = selector.pseudo_element.as_ref().unwrap();
+                self.pseudo_dependencies
+                    .entry(pseudo_selector.pseudo_element().clone())
+                    .or_insert_with(SelectorMap::new)
+                    .insert(PseudoElementDependency {
+                        selector: selector.clone(),
+                    });
+            }
+
             // If we found a sensitivity, add an entry in the dependency set.
             if !visitor.sensitivities.is_empty() {
                 let mut hint = combinator_to_restyle_hint(combinator);
-                let dep_selector;
-                if sequence_start == 0 {
-                    if selector.pseudo_element.is_some() {
-                        // TODO(emilio): use more fancy restyle hints to avoid
-                        // restyling the whole subtree when pseudos change.
-                        //
-                        // We currently need is_pseudo_element to handle eager
-                        // pseudos (so the style the parent stores doesn't
-                        // become stale), and restyle_descendants to handle all
-                        // of them (::before and ::after, because we find them
-                        // in the subtree, and other lazy pseudos for the same
-                        // reason).
-                        hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS;
-                    }
 
-                    // Reuse the bloom hashes if this is the base selector.
-                    dep_selector = selector.inner.clone();
-                } else {
-                    dep_selector = SelectorInner::new(selector.inner.complex.slice_from(sequence_start));
+                if sequence_start == 0 && selector.pseudo_element.is_some() {
+                    // FIXME(emilio): Be more granular about this. See the
+                    // comment in `PseudoElementDependency` about how could this
+                    // be modified in order to be more efficient and restyle
+                    // less.
+                    hint |= RESTYLE_SELF | RESTYLE_DESCENDANTS;
                 }
 
-                self.add_dependency(Dependency {
+                let dep_selector = if sequence_start == 0 {
+                    // Reuse the bloom hashes if this is the base selector.
+                    selector.inner.clone()
+                } else {
+                    SelectorInner::new(selector.inner.complex.slice_from(sequence_start))
+                };
+
+                self.element_dependencies.insert(ElementDependency {
                     sensitivities: visitor.sensitivities,
                     hint: hint,
                     selector: dep_selector,
                 });
             }
 
             combinator = iter.next_sequence();
             if combinator.is_none() {
@@ -609,93 +671,169 @@ impl DependencySet {
             }
 
             index += 1; // Account for the combinator.
         }
     }
 
     /// Create an empty `DependencySet`.
     pub fn new() -> Self {
-        DependencySet(SelectorMap::new())
+        DependencySet {
+            element_dependencies: SelectorMap::new(),
+            pseudo_dependencies: FnvHashMap::default(),
+        }
     }
 
     /// Return the total number of dependencies that this set contains.
+    ///
+    /// FIXME(emilio): Add also pseudo-element dependencies.
     pub fn len(&self) -> usize {
-        self.0.len()
+        self.element_dependencies.len()
     }
 
     /// Clear this dependency set.
     pub fn clear(&mut self) {
-        self.0 = SelectorMap::new();
+        self.element_dependencies = SelectorMap::new();
+        self.pseudo_dependencies.clear()
     }
 
-    /// Compute a restyle hint given an element and a snapshot, per the rules
-    /// explained in the rest of the documentation.
-    pub fn compute_hint<E>(&self,
-                           el: &E,
-                           snapshots: &SnapshotMap)
-                           -> RestyleHint
-        where E: TElement + Clone,
+    fn compute_pseudo_hint<E>(
+        &self,
+        pseudo: &E,
+        pseudo_element: PseudoElement,
+        snapshots: &SnapshotMap)
+        -> RestyleHint
+        where E: TElement,
+    {
+        debug!("compute_pseudo_hint: {:?}, {:?}", pseudo, pseudo_element);
+        debug_assert!(pseudo.has_snapshot());
+
+        let map = match self.pseudo_dependencies.get(&pseudo_element) {
+            Some(map) => map,
+            None => return RestyleHint::empty(),
+        };
+
+        // Only pseudo-element's state is relevant.
+        let pseudo_state_changes =
+            ElementWrapper::new(*pseudo, snapshots).state_changes();
+        debug!("pseudo_state_changes: {:?}", pseudo_state_changes);
+
+        if pseudo_state_changes.is_empty() {
+            return RestyleHint::empty();
+        }
+
+        let selector_matching_target =
+            pseudo.closest_non_native_anonymous_ancestor().unwrap();
+
+        // Note that we rely on that, if the originating element changes, it'll
+        // post a restyle hint that would make us redo selector matching, so we
+        // don't need to care about that.
+        //
+        // If that ever changes, we'd need to share more code with
+        // `compute_element_hint`.
+        let mut hint = RestyleHint::empty();
+        map.lookup(selector_matching_target, &mut |dep| {
+            let pseudo_selector = dep.selector.pseudo_element.as_ref().unwrap();
+            debug_assert!(!pseudo_selector.state().is_empty());
+            if pseudo_selector.state().intersects(pseudo_state_changes) {
+                hint = RESTYLE_SELF;
+                return false;
+            }
+
+            true
+        });
+
+        hint
+    }
+
+    fn compute_element_hint<E>(
+        &self,
+        el: &E,
+        map: &SelectorMap<ElementDependency>,
+        snapshots: &SnapshotMap)
+        -> RestyleHint
+        where E: TElement,
     {
         debug_assert!(el.has_snapshot(), "Shouldn't be here!");
-        let snapshot_el = ElementWrapper::new(el.clone(), snapshots);
 
-        let snapshot =
-            snapshot_el.snapshot().expect("has_snapshot lied so badly");
+        let wrapper = ElementWrapper::new(el.clone(), snapshots);
+        let element_snapshot = wrapper.snapshot().unwrap();
 
-        let current_state = el.get_state();
-        let state_changes =
-            snapshot.state()
-                .map_or_else(ElementState::empty,
-                             |old_state| current_state ^ old_state);
-        let attrs_changed = snapshot.has_attrs();
-
-        if state_changes.is_empty() && !attrs_changed {
+        let attributes_changed = element_snapshot.has_attrs();
+        let state_changes = wrapper.state_changes();
+        if state_changes.is_empty() && !attributes_changed {
             return RestyleHint::empty();
         }
 
         let mut hint = RestyleHint::empty();
 
         // Compute whether the snapshot has any different id or class attributes
         // from the element. If it does, we need to pass those to the lookup, so
         // that we get all the possible applicable selectors from the rulehash.
         let mut additional_id = None;
         let mut additional_classes = SmallVec::<[Atom; 8]>::new();
-        if snapshot.has_attrs() {
-            let id = snapshot.id_attr();
+        if attributes_changed {
+            let id = element_snapshot.id_attr();
             if id.is_some() && id != el.get_id() {
                 additional_id = id;
             }
 
-            snapshot.each_class(|c| if !el.has_class(c) { additional_classes.push(c.clone()) });
+            element_snapshot.each_class(|c| {
+                if !el.has_class(c) {
+                    additional_classes.push(c.clone())
+                }
+            });
         }
 
-        self.0.lookup_with_additional(*el, additional_id, &additional_classes, &mut |dep| {
-            if !dep.sensitivities.sensitive_to(attrs_changed, state_changes) ||
-               hint.contains(dep.hint) {
+        map.lookup_with_additional(*el, additional_id, &additional_classes, &mut |dep| {
+            trace!("scanning dependency: {:?}", dep);
+            if !dep.sensitivities.sensitive_to(attributes_changed,
+                                               state_changes) {
+                trace!(" > non-sensitive");
+                return true;
+            }
+
+            if hint.contains(dep.hint) {
+                trace!(" > hint was already there");
                 return true;
             }
 
             // We can ignore the selector flags, since they would have already
             // been set during original matching for any element that might
             // change its matching behavior here.
             let matched_then =
-                matches_selector(&dep.selector, &snapshot_el, None,
+                matches_selector(&dep.selector, &wrapper, None,
                                  &mut StyleRelations::empty(),
                                  &mut |_, _| {});
             let matches_now =
                 matches_selector(&dep.selector, el, None,
                                  &mut StyleRelations::empty(),
                                  &mut |_, _| {});
             if matched_then != matches_now {
                 hint.insert(dep.hint);
             }
 
             !hint.is_all()
         });
 
         debug!("Calculated restyle hint: {:?} for {:?}. (State={:?}, {} Deps)",
-               hint, el, current_state, self.len());
-        trace!("Deps: {:?}", self);
+               hint, el, el.get_state(), self.len());
 
         hint
     }
+
+
+    /// Compute a restyle hint given an element and a snapshot, per the rules
+    /// explained in the rest of the documentation.
+    pub fn compute_hint<E>(&self,
+                           el: &E,
+                           snapshots: &SnapshotMap)
+                           -> RestyleHint
+        where E: TElement + Clone,
+    {
+        debug!("DependencySet::compute_hint({:?})", el);
+        if let Some(pseudo) = el.implemented_pseudo_element() {
+            return self.compute_pseudo_hint(el, pseudo, snapshots);
+        }
+
+        self.compute_element_hint(el, &self.element_dependencies, snapshots)
+    }
 }
--- a/servo/components/style/traversal.rs
+++ b/servo/components/style/traversal.rs
@@ -782,20 +782,21 @@ fn preprocess_children<E, D>(traversal: 
         }
 
         // Handle element snapshots and sibling restyle hints.
         //
         // NB: This will be a no-op if there's no restyle data and no snapshot.
         let later_siblings =
             child_data.compute_final_hint(child, traversal.shared_context());
 
-        trace!(" > {:?} -> {:?} + {:?}, later_siblings: {:?}",
+        trace!(" > {:?} -> {:?} + {:?}, pseudo: {:?}, later_siblings: {:?}",
                child,
                child_data.get_restyle().map(|r| &r.hint),
                propagated_hint,
+               child.implemented_pseudo_element(),
                later_siblings);
 
         // 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() && !parent_inherited_style_changed &&
            damage_handled.is_empty() && !child_data.has_restyle() {
             continue;