From b9b3e592dd8687bb9a924113807d4c8c41c8e751 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 13:32:40 +0200
Subject: [PATCH 1/7] style: Split the invalidation collection from the
 invalidator step.

This is the first step in reusing the invalidation machinery for other stuff,
potentially including QuerySelector / QuerySelectorAll.
---
 components/style/data.rs                      |   4 +-
 .../style/invalidation/element/collector.rs   | 418 +++++++++++++++++
 .../style/invalidation/element/invalidator.rs | 423 ++----------------
 components/style/invalidation/element/mod.rs  |   1 +
 4 files changed, 462 insertions(+), 384 deletions(-)
 create mode 100644 components/style/invalidation/element/collector.rs

diff --git a/components/style/data.rs b/components/style/data.rs
index bc3f0b17497..c1ca6324110 100644
--- a/components/style/data.rs
+++ b/components/style/data.rs
@@ -244,6 +244,7 @@ impl ElementData {
             return InvalidationResult::empty();
         }
 
+        use invalidation::element::collector::StateAndAttrInvalidationCollector;
         use invalidation::element::invalidator::TreeStyleInvalidator;
 
         debug!("invalidate_style_if_needed: {:?}, flags: {:?}, has_snapshot: {}, \
@@ -266,7 +267,8 @@ impl ElementData {
             nth_index_cache,
         );
 
-        let result = invalidator.invalidate();
+        let result =
+            invalidator.invalidate::<StateAndAttrInvalidationCollector>();
         unsafe { element.set_handled_snapshot() }
         debug_assert!(element.handled_snapshot());
         result
diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
new file mode 100644
index 00000000000..a496e4220d0
--- /dev/null
+++ b/components/style/invalidation/element/collector.rs
@@ -0,0 +1,418 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+//! A collector for invalidations due to state and attribute changes.
+
+use Atom;
+use context::{QuirksMode, SharedStyleContext};
+use data::ElementData;
+use dom::TElement;
+use element_state::{ElementState, IN_VISITED_OR_UNVISITED_STATE};
+use invalidation::element::element_wrapper::{ElementSnapshot, ElementWrapper};
+use invalidation::element::invalidation_map::*;
+use invalidation::element::invalidator::{InvalidationVector, Invalidation, InvalidationCollector};
+use invalidation::element::restyle_hints::*;
+use selector_map::SelectorMap;
+use selector_parser::Snapshot;
+use selectors::NthIndexCache;
+use selectors::attr::CaseSensitivity;
+use selectors::matching::{MatchingContext, MatchingMode, VisitedHandlingMode};
+use selectors::matching::matches_selector;
+use smallvec::SmallVec;
+
+#[derive(Debug, PartialEq)]
+enum VisitedDependent {
+    Yes,
+    No,
+}
+
+/// The collector implementation.
+struct Collector<'a, 'b: 'a, E>
+where
+    E: TElement,
+{
+    element: E,
+    wrapper: ElementWrapper<'b, E>,
+    nth_index_cache: Option<&'a mut NthIndexCache>,
+    snapshot: &'a Snapshot,
+    quirks_mode: QuirksMode,
+    lookup_element: E,
+    removed_id: Option<&'a Atom>,
+    added_id: Option<&'a Atom>,
+    classes_removed: &'a SmallVec<[Atom; 8]>,
+    classes_added: &'a SmallVec<[Atom; 8]>,
+    state_changes: ElementState,
+    descendant_invalidations: &'a mut InvalidationVector,
+    sibling_invalidations: &'a mut InvalidationVector,
+    invalidates_self: bool,
+}
+
+///  A collector for state and attribute invalidations.
+pub struct StateAndAttrInvalidationCollector;
+
+impl InvalidationCollector for StateAndAttrInvalidationCollector {
+    fn collect_invalidations<E>(
+        element: E,
+        mut data: Option<&mut ElementData>,
+        nth_index_cache: Option<&mut NthIndexCache>,
+        shared_context: &SharedStyleContext,
+        descendant_invalidations: &mut InvalidationVector,
+        sibling_invalidations: &mut InvalidationVector,
+    ) -> bool
+    where
+        E: TElement,
+    {
+        debug_assert!(element.has_snapshot(), "Why bothering?");
+        debug_assert!(data.is_some(), "How exactly?");
+
+        let wrapper =
+            ElementWrapper::new(element, &*shared_context.snapshot_map);
+
+        let state_changes = wrapper.state_changes();
+        let snapshot = wrapper.snapshot().expect("has_snapshot lied");
+
+        if !snapshot.has_attrs() && state_changes.is_empty() {
+            return false;
+        }
+
+        // If we are sensitive to visitedness and the visited state changed, we
+        // force a restyle here. Matching doesn't depend on the actual visited
+        // state at all, so we can't look at matching results to decide what to
+        // do for this case.
+        if state_changes.intersects(IN_VISITED_OR_UNVISITED_STATE) {
+            trace!(" > visitedness change, force subtree restyle");
+            // We can't just return here because there may also be attribute
+            // changes as well that imply additional hints.
+            let data = data.as_mut().unwrap();
+            data.hint.insert(RestyleHint::restyle_subtree());
+        }
+
+        let mut classes_removed = SmallVec::<[Atom; 8]>::new();
+        let mut classes_added = SmallVec::<[Atom; 8]>::new();
+        if snapshot.class_changed() {
+            // TODO(emilio): Do this more efficiently!
+            snapshot.each_class(|c| {
+                if !element.has_class(c, CaseSensitivity::CaseSensitive) {
+                    classes_removed.push(c.clone())
+                }
+            });
+
+            element.each_class(|c| {
+                if !snapshot.has_class(c, CaseSensitivity::CaseSensitive) {
+                    classes_added.push(c.clone())
+                }
+            })
+        }
+
+        let mut id_removed = None;
+        let mut id_added = None;
+        if snapshot.id_changed() {
+            let old_id = snapshot.id_attr();
+            let current_id = element.get_id();
+
+            if old_id != current_id {
+                id_removed = old_id;
+                id_added = current_id;
+            }
+        }
+
+        let lookup_element =
+            if element.implemented_pseudo_element().is_some() {
+                element.pseudo_element_originating_element().unwrap()
+            } else {
+                element
+            };
+
+        let invalidated_self = {
+            let mut collector = Collector {
+                wrapper,
+                lookup_element,
+                nth_index_cache,
+                state_changes,
+                element,
+                snapshot: &snapshot,
+                quirks_mode: shared_context.quirks_mode(),
+                removed_id: id_removed.as_ref(),
+                added_id: id_added.as_ref(),
+                classes_removed: &classes_removed,
+                classes_added: &classes_added,
+                descendant_invalidations,
+                sibling_invalidations,
+                invalidates_self: false,
+            };
+
+            shared_context.stylist.each_invalidation_map(|invalidation_map| {
+                collector.collect_dependencies_in_invalidation_map(invalidation_map);
+            });
+
+            // TODO(emilio): Consider storing dependencies from the UA sheet in
+            // a different map. If we do that, we can skip the stuff on the
+            // shared stylist iff cut_off_inheritance is true, and we can look
+            // just at that map.
+            let _cut_off_inheritance =
+                element.each_xbl_stylist(|stylist| {
+                    // FIXME(emilio): Replace with assert / remove when we
+                    // figure out what to do with the quirks mode mismatches
+                    // (that is, when bug 1406875 is properly fixed).
+                    collector.quirks_mode = stylist.quirks_mode();
+                    stylist.each_invalidation_map(|invalidation_map| {
+                        collector.collect_dependencies_in_invalidation_map(invalidation_map);
+                    });
+                });
+
+            collector.invalidates_self
+        };
+
+        if invalidated_self {
+            if let Some(ref mut data) = data {
+                data.hint.insert(RESTYLE_SELF);
+            }
+        }
+
+        invalidated_self
+    }
+}
+
+impl<'a, 'b, E> Collector<'a, 'b, E>
+where
+    E: TElement,
+{
+    fn collect_dependencies_in_invalidation_map(
+        &mut self,
+        map: &InvalidationMap,
+    ) {
+        let quirks_mode = self.quirks_mode;
+        let removed_id = self.removed_id;
+        if let Some(ref id) = removed_id {
+            if let Some(deps) = map.id_to_selector.get(id, quirks_mode) {
+                for dep in deps {
+                    self.scan_dependency(dep, VisitedDependent::No);
+                }
+            }
+        }
+
+        let added_id = self.added_id;
+        if let Some(ref id) = added_id {
+            if let Some(deps) = map.id_to_selector.get(id, quirks_mode) {
+                for dep in deps {
+                    self.scan_dependency(dep, VisitedDependent::No);
+                }
+            }
+        }
+
+        for class in self.classes_added.iter().chain(self.classes_removed.iter()) {
+            if let Some(deps) = map.class_to_selector.get(class, quirks_mode) {
+                for dep in deps {
+                    self.scan_dependency(dep, VisitedDependent::No);
+                }
+            }
+        }
+
+        let should_examine_attribute_selector_map =
+            self.snapshot.other_attr_changed() ||
+            (self.snapshot.class_changed() && map.has_class_attribute_selectors) ||
+            (self.snapshot.id_changed() && map.has_id_attribute_selectors);
+
+        if should_examine_attribute_selector_map {
+            self.collect_dependencies_in_map(
+                &map.other_attribute_affecting_selectors
+            )
+        }
+
+        let state_changes = self.state_changes;
+        if !state_changes.is_empty() {
+            self.collect_state_dependencies(
+                &map.state_affecting_selectors,
+                state_changes,
+            )
+        }
+    }
+
+    fn collect_dependencies_in_map(
+        &mut self,
+        map: &SelectorMap<Dependency>,
+    ) {
+        map.lookup_with_additional(
+            self.lookup_element,
+            self.quirks_mode,
+            self.removed_id,
+            self.classes_removed,
+            &mut |dependency| {
+                self.scan_dependency(dependency, VisitedDependent::No);
+                true
+            },
+        );
+    }
+
+    fn collect_state_dependencies(
+        &mut self,
+        map: &SelectorMap<StateDependency>,
+        state_changes: ElementState,
+    ) {
+        map.lookup_with_additional(
+            self.lookup_element,
+            self.quirks_mode,
+            self.removed_id,
+            self.classes_removed,
+            &mut |dependency| {
+                if !dependency.state.intersects(state_changes) {
+                    return true;
+                }
+                let visited_dependent =
+                    if dependency.state.intersects(IN_VISITED_OR_UNVISITED_STATE) {
+                        VisitedDependent::Yes
+                    } else {
+                        VisitedDependent::No
+                    };
+                self.scan_dependency(&dependency.dep, visited_dependent);
+                true
+            },
+        );
+    }
+
+    /// Check whether a dependency should be taken into account, using a given
+    /// visited handling mode.
+    fn check_dependency(
+        &mut self,
+        visited_handling_mode: VisitedHandlingMode,
+        dependency: &Dependency,
+        relevant_link_found: &mut bool,
+    ) -> bool {
+        let (matches_now, relevant_link_found_now) = {
+            let mut context = MatchingContext::new_for_visited(
+                MatchingMode::Normal,
+                None,
+                self.nth_index_cache.as_mut().map(|c| &mut **c),
+                visited_handling_mode,
+                self.quirks_mode,
+            );
+
+            let matches_now = matches_selector(
+                &dependency.selector,
+                dependency.selector_offset,
+                None,
+                &self.element,
+                &mut context,
+                &mut |_, _| {},
+            );
+
+            (matches_now, context.relevant_link_found)
+        };
+
+        let (matched_then, relevant_link_found_then) = {
+            let mut context = MatchingContext::new_for_visited(
+                MatchingMode::Normal,
+                None,
+                self.nth_index_cache.as_mut().map(|c| &mut **c),
+                visited_handling_mode,
+                self.quirks_mode,
+            );
+
+            let matched_then = matches_selector(
+                &dependency.selector,
+                dependency.selector_offset,
+                None,
+                &self.wrapper,
+                &mut context,
+                &mut |_, _| {},
+            );
+
+            (matched_then, context.relevant_link_found)
+        };
+
+        *relevant_link_found = relevant_link_found_now;
+
+        // Check for mismatches in both the match result and also the status
+        // of whether a relevant link was found.
+        matched_then != matches_now ||
+            relevant_link_found_now != relevant_link_found_then
+    }
+
+    fn scan_dependency(
+        &mut self,
+        dependency: &Dependency,
+        is_visited_dependent: VisitedDependent,
+    ) {
+        debug!("TreeStyleInvalidator::scan_dependency({:?}, {:?}, {:?})",
+               self.element,
+               dependency,
+               is_visited_dependent);
+
+        if !self.dependency_may_be_relevant(dependency) {
+            return;
+        }
+
+        let mut relevant_link_found = false;
+
+        let should_account_for_dependency = self.check_dependency(
+            VisitedHandlingMode::AllLinksUnvisited,
+            dependency,
+            &mut relevant_link_found,
+        );
+
+        if should_account_for_dependency {
+            return self.note_dependency(dependency);
+        }
+
+        // If there is a relevant link, then we also matched in visited
+        // mode.
+        //
+        // Match again in this mode to ensure this also matches.
+        //
+        // Note that we never actually match directly against the element's true
+        // visited state at all, since that would expose us to timing attacks.
+        //
+        // The matching process only considers the relevant link state and
+        // visited handling mode when deciding if visited matches.  Instead, we
+        // are rematching here in case there is some :visited selector whose
+        // matching result changed for some other state or attribute change of
+        // this element (for example, for things like [foo]:visited).
+        //
+        // NOTE: This thing is actually untested because testing it is flaky,
+        // see the tests that were added and then backed out in bug 1328509.
+        if is_visited_dependent == VisitedDependent::Yes && relevant_link_found {
+            let should_account_for_dependency = self.check_dependency(
+                VisitedHandlingMode::RelevantLinkVisited,
+                dependency,
+                &mut false,
+            );
+
+            if should_account_for_dependency {
+                return self.note_dependency(dependency);
+            }
+        }
+    }
+
+    fn note_dependency(&mut self, dependency: &Dependency) {
+        if dependency.affects_self() {
+            self.invalidates_self = true;
+        }
+
+        if dependency.affects_descendants() {
+            debug_assert_ne!(dependency.selector_offset, 0);
+            debug_assert!(!dependency.affects_later_siblings());
+            self.descendant_invalidations.push(Invalidation::new(
+                dependency.selector.clone(),
+                dependency.selector_offset,
+            ));
+        } else if dependency.affects_later_siblings() {
+            debug_assert_ne!(dependency.selector_offset, 0);
+            self.sibling_invalidations.push(Invalidation::new(
+                dependency.selector.clone(),
+                dependency.selector_offset,
+            ));
+        }
+    }
+
+    /// Returns whether `dependency` may cause us to invalidate the style of
+    /// more elements than what we've already invalidated.
+    fn dependency_may_be_relevant(&self, dependency: &Dependency) -> bool {
+        if dependency.affects_descendants() || dependency.affects_later_siblings() {
+            return true;
+        }
+
+        debug_assert!(dependency.affects_self());
+        !self.invalidates_self
+    }
+}
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index 540408db2f0..012b9feedaa 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -5,29 +5,34 @@
 //! The struct that takes care of encapsulating all the logic on where and how
 //! element styles need to be invalidated.
 
-use Atom;
-use context::{QuirksMode, SharedStyleContext, StackLimitChecker};
+use context::{SharedStyleContext, StackLimitChecker};
 use data::ElementData;
 use dom::{TElement, TNode};
-use element_state::{ElementState, IN_VISITED_OR_UNVISITED_STATE};
-use invalidation::element::element_wrapper::{ElementSnapshot, ElementWrapper};
-use invalidation::element::invalidation_map::*;
 use invalidation::element::restyle_hints::*;
-use selector_map::SelectorMap;
-use selector_parser::{SelectorImpl, Snapshot};
+use selector_parser::SelectorImpl;
 use selectors::NthIndexCache;
-use selectors::attr::CaseSensitivity;
 use selectors::matching::{MatchingContext, MatchingMode, VisitedHandlingMode};
-use selectors::matching::{matches_selector, matches_compound_selector};
 use selectors::matching::CompoundSelectorMatchingResult;
+use selectors::matching::matches_compound_selector;
 use selectors::parser::{Combinator, Component, Selector};
 use smallvec::SmallVec;
 use std::fmt;
 
-#[derive(Debug, PartialEq)]
-enum VisitedDependent {
-    Yes,
-    No,
+/// A trait to abstract the collection of invalidations for a given pass.
+pub trait InvalidationCollector {
+    /// Collect invalidations for a given element's descendants and siblings.
+    ///
+    /// Returns whether the element itself was invalidated.
+    fn collect_invalidations<E>(
+        element: E,
+        data: Option<&mut ElementData>,
+        nth_index_cache: Option<&mut NthIndexCache>,
+        shared_context: &SharedStyleContext,
+        descendant_invalidations: &mut InvalidationVector,
+        sibling_invalidations: &mut InvalidationVector,
+    ) -> bool
+    where
+        E: TElement;
 }
 
 /// The struct that takes care of encapsulating all the logic on where and how
@@ -51,7 +56,8 @@ pub struct TreeStyleInvalidator<'a, 'b: 'a, E>
     nth_index_cache: Option<&'a mut NthIndexCache>,
 }
 
-type InvalidationVector = SmallVec<[Invalidation; 10]>;
+/// A vector of invalidations, optimized for small invalidation sets.
+pub type InvalidationVector = SmallVec<[Invalidation; 10]>;
 
 /// The kind of invalidation we're processing.
 ///
@@ -71,7 +77,7 @@ enum InvalidationKind {
 /// must be restyled if the compound selector matches.  Otherwise, if
 /// describes which descendants (or later siblings) must be restyled.
 #[derive(Clone)]
-struct Invalidation {
+pub struct Invalidation {
     selector: Selector<SelectorImpl>,
     offset: usize,
     /// Whether the invalidation was already matched by any previous sibling or
@@ -84,6 +90,15 @@ struct Invalidation {
 }
 
 impl Invalidation {
+    /// Create a new invalidation for a given selector and offset.
+    pub fn new(selector: Selector<SelectorImpl>, offset: usize) -> Self {
+        Self {
+            selector,
+            offset,
+            matched_by_any_previous: false,
+        }
+    }
+
     /// Whether this invalidation is effective for the next sibling or
     /// descendant after us.
     fn effective_for_next(&self) -> bool {
@@ -188,121 +203,25 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
     }
 
     /// Perform the invalidation pass.
-    pub fn invalidate(mut self) -> InvalidationResult {
+    pub fn invalidate<C: InvalidationCollector>(mut self) -> InvalidationResult {
         debug!("StyleTreeInvalidator::invalidate({:?})", self.element);
-        debug_assert!(self.element.has_snapshot(), "Why bothering?");
-        debug_assert!(self.data.is_some(), "How exactly?");
-
-        let shared_context = self.shared_context;
-
-        let wrapper =
-            ElementWrapper::new(self.element, shared_context.snapshot_map);
-        let state_changes = wrapper.state_changes();
-        let snapshot = wrapper.snapshot().expect("has_snapshot lied");
-
-        if !snapshot.has_attrs() && state_changes.is_empty() {
-            return InvalidationResult::empty();
-        }
-
-        // If we are sensitive to visitedness and the visited state changed, we
-        // force a restyle here. Matching doesn't depend on the actual visited
-        // state at all, so we can't look at matching results to decide what to
-        // do for this case.
-        if state_changes.intersects(IN_VISITED_OR_UNVISITED_STATE) {
-            trace!(" > visitedness change, force subtree restyle");
-            // We can't just return here because there may also be attribute
-            // changes as well that imply additional hints.
-            let data = self.data.as_mut().unwrap();
-            data.hint.insert(RestyleHint::restyle_subtree());
-        }
-
-        let mut classes_removed = SmallVec::<[Atom; 8]>::new();
-        let mut classes_added = SmallVec::<[Atom; 8]>::new();
-        if snapshot.class_changed() {
-            // TODO(emilio): Do this more efficiently!
-            snapshot.each_class(|c| {
-                if !self.element.has_class(c, CaseSensitivity::CaseSensitive) {
-                    classes_removed.push(c.clone())
-                }
-            });
-
-            self.element.each_class(|c| {
-                if !snapshot.has_class(c, CaseSensitivity::CaseSensitive) {
-                    classes_added.push(c.clone())
-                }
-            })
-        }
-
-        let mut id_removed = None;
-        let mut id_added = None;
-        if snapshot.id_changed() {
-            let old_id = snapshot.id_attr();
-            let current_id = self.element.get_id();
-
-            if old_id != current_id {
-                id_removed = old_id;
-                id_added = current_id;
-            }
-        }
-
-        let lookup_element =
-            if self.element.implemented_pseudo_element().is_some() {
-                self.element.pseudo_element_originating_element().unwrap()
-            } else {
-                self.element
-            };
 
         let mut descendant_invalidations = InvalidationVector::new();
         let mut sibling_invalidations = InvalidationVector::new();
-        let invalidated_self = {
-            let mut collector = InvalidationCollector {
-                wrapper,
-                lookup_element,
-                nth_index_cache: self.nth_index_cache.as_mut().map(|c| &mut **c),
-                state_changes,
-                element: self.element,
-                snapshot: &snapshot,
-                quirks_mode: self.shared_context.quirks_mode(),
-                removed_id: id_removed.as_ref(),
-                added_id: id_added.as_ref(),
-                classes_removed: &classes_removed,
-                classes_added: &classes_added,
-                descendant_invalidations: &mut descendant_invalidations,
-                sibling_invalidations: &mut sibling_invalidations,
-                invalidates_self: false,
-            };
 
-            shared_context.stylist.each_invalidation_map(|invalidation_map| {
-                collector.collect_dependencies_in_invalidation_map(invalidation_map);
-            });
-
-            // TODO(emilio): Consider storing dependencies from the UA sheet in
-            // a different map. If we do that, we can skip the stuff on the
-            // shared stylist iff cut_off_inheritance is true, and we can look
-            // just at that map.
-            let _cut_off_inheritance =
-                self.element.each_xbl_stylist(|stylist| {
-                    // FIXME(emilio): Replace with assert / remove when we
-                    // figure out what to do with the quirks mode mismatches
-                    // (that is, when bug 1406875 is properly fixed).
-                    collector.quirks_mode = stylist.quirks_mode();
-                    stylist.each_invalidation_map(|invalidation_map| {
-                        collector.collect_dependencies_in_invalidation_map(invalidation_map);
-                    });
-                });
-
-            collector.invalidates_self
-        };
-
-        if invalidated_self {
-            if let Some(ref mut data) = self.data {
-                data.hint.insert(RESTYLE_SELF);
-            }
-        }
+        let invalidated_self = C::collect_invalidations(
+            self.element,
+            self.data.as_mut().map(|d| &mut **d),
+            self.nth_index_cache.as_mut().map(|c| &mut **c),
+            self.shared_context,
+            &mut descendant_invalidations,
+            &mut sibling_invalidations,
+        );
 
         debug!("Collected invalidations (self: {}): ", invalidated_self);
         debug!(" > descendants: {:?}", descendant_invalidations);
         debug!(" > siblings: {:?}", sibling_invalidations);
+
         let invalidated_descendants = self.invalidate_descendants(&descendant_invalidations);
         let invalidated_siblings = self.invalidate_siblings(&mut sibling_invalidations);
 
@@ -820,265 +739,3 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
     }
 }
 
-struct InvalidationCollector<'a, 'b: 'a, E>
-    where E: TElement,
-{
-    element: E,
-    wrapper: ElementWrapper<'b, E>,
-    nth_index_cache: Option<&'a mut NthIndexCache>,
-    snapshot: &'a Snapshot,
-    quirks_mode: QuirksMode,
-    lookup_element: E,
-    removed_id: Option<&'a Atom>,
-    added_id: Option<&'a Atom>,
-    classes_removed: &'a SmallVec<[Atom; 8]>,
-    classes_added: &'a SmallVec<[Atom; 8]>,
-    state_changes: ElementState,
-    descendant_invalidations: &'a mut InvalidationVector,
-    sibling_invalidations: &'a mut InvalidationVector,
-    invalidates_self: bool,
-}
-
-impl<'a, 'b: 'a, E> InvalidationCollector<'a, 'b, E>
-    where E: TElement,
-{
-    fn collect_dependencies_in_invalidation_map(
-        &mut self,
-        map: &InvalidationMap,
-    ) {
-        let quirks_mode = self.quirks_mode;
-        let removed_id = self.removed_id;
-        if let Some(ref id) = removed_id {
-            if let Some(deps) = map.id_to_selector.get(id, quirks_mode) {
-                for dep in deps {
-                    self.scan_dependency(dep, VisitedDependent::No);
-                }
-            }
-        }
-
-        let added_id = self.added_id;
-        if let Some(ref id) = added_id {
-            if let Some(deps) = map.id_to_selector.get(id, quirks_mode) {
-                for dep in deps {
-                    self.scan_dependency(dep, VisitedDependent::No);
-                }
-            }
-        }
-
-        for class in self.classes_added.iter().chain(self.classes_removed.iter()) {
-            if let Some(deps) = map.class_to_selector.get(class, quirks_mode) {
-                for dep in deps {
-                    self.scan_dependency(dep, VisitedDependent::No);
-                }
-            }
-        }
-
-        let should_examine_attribute_selector_map =
-            self.snapshot.other_attr_changed() ||
-            (self.snapshot.class_changed() && map.has_class_attribute_selectors) ||
-            (self.snapshot.id_changed() && map.has_id_attribute_selectors);
-
-        if should_examine_attribute_selector_map {
-            self.collect_dependencies_in_map(
-                &map.other_attribute_affecting_selectors
-            )
-        }
-
-        let state_changes = self.state_changes;
-        if !state_changes.is_empty() {
-            self.collect_state_dependencies(
-                &map.state_affecting_selectors,
-                state_changes,
-            )
-        }
-    }
-
-    fn collect_dependencies_in_map(
-        &mut self,
-        map: &SelectorMap<Dependency>,
-    ) {
-        map.lookup_with_additional(
-            self.lookup_element,
-            self.quirks_mode,
-            self.removed_id,
-            self.classes_removed,
-            &mut |dependency| {
-                self.scan_dependency(dependency, VisitedDependent::No);
-                true
-            },
-        );
-    }
-
-    fn collect_state_dependencies(
-        &mut self,
-        map: &SelectorMap<StateDependency>,
-        state_changes: ElementState,
-    ) {
-        map.lookup_with_additional(
-            self.lookup_element,
-            self.quirks_mode,
-            self.removed_id,
-            self.classes_removed,
-            &mut |dependency| {
-                if !dependency.state.intersects(state_changes) {
-                    return true;
-                }
-                let visited_dependent =
-                    if dependency.state.intersects(IN_VISITED_OR_UNVISITED_STATE) {
-                        VisitedDependent::Yes
-                    } else {
-                        VisitedDependent::No
-                    };
-                self.scan_dependency(&dependency.dep, visited_dependent);
-                true
-            },
-        );
-    }
-
-    /// Check whether a dependency should be taken into account, using a given
-    /// visited handling mode.
-    fn check_dependency(
-        &mut self,
-        visited_handling_mode: VisitedHandlingMode,
-        dependency: &Dependency,
-        relevant_link_found: &mut bool,
-    ) -> bool {
-        let (matches_now, relevant_link_found_now) = {
-            let mut context = MatchingContext::new_for_visited(
-                MatchingMode::Normal,
-                None,
-                self.nth_index_cache.as_mut().map(|c| &mut **c),
-                visited_handling_mode,
-                self.quirks_mode,
-            );
-
-            let matches_now = matches_selector(
-                &dependency.selector,
-                dependency.selector_offset,
-                None,
-                &self.element,
-                &mut context,
-                &mut |_, _| {},
-            );
-
-            (matches_now, context.relevant_link_found)
-        };
-
-        let (matched_then, relevant_link_found_then) = {
-            let mut context = MatchingContext::new_for_visited(
-                MatchingMode::Normal,
-                None,
-                self.nth_index_cache.as_mut().map(|c| &mut **c),
-                visited_handling_mode,
-                self.quirks_mode,
-            );
-
-            let matched_then = matches_selector(
-                &dependency.selector,
-                dependency.selector_offset,
-                None,
-                &self.wrapper,
-                &mut context,
-                &mut |_, _| {},
-            );
-
-            (matched_then, context.relevant_link_found)
-        };
-
-        *relevant_link_found = relevant_link_found_now;
-
-        // Check for mismatches in both the match result and also the status
-        // of whether a relevant link was found.
-        matched_then != matches_now ||
-            relevant_link_found_now != relevant_link_found_then
-    }
-
-    fn scan_dependency(
-        &mut self,
-        dependency: &Dependency,
-        is_visited_dependent: VisitedDependent,
-    ) {
-        debug!("TreeStyleInvalidator::scan_dependency({:?}, {:?}, {:?})",
-               self.element,
-               dependency,
-               is_visited_dependent);
-
-        if !self.dependency_may_be_relevant(dependency) {
-            return;
-        }
-
-        let mut relevant_link_found = false;
-
-        let should_account_for_dependency = self.check_dependency(
-            VisitedHandlingMode::AllLinksUnvisited,
-            dependency,
-            &mut relevant_link_found,
-        );
-
-        if should_account_for_dependency {
-            return self.note_dependency(dependency);
-        }
-
-        // If there is a relevant link, then we also matched in visited
-        // mode.
-        //
-        // Match again in this mode to ensure this also matches.
-        //
-        // Note that we never actually match directly against the element's true
-        // visited state at all, since that would expose us to timing attacks.
-        //
-        // The matching process only considers the relevant link state and
-        // visited handling mode when deciding if visited matches.  Instead, we
-        // are rematching here in case there is some :visited selector whose
-        // matching result changed for some other state or attribute change of
-        // this element (for example, for things like [foo]:visited).
-        //
-        // NOTE: This thing is actually untested because testing it is flaky,
-        // see the tests that were added and then backed out in bug 1328509.
-        if is_visited_dependent == VisitedDependent::Yes && relevant_link_found {
-            let should_account_for_dependency = self.check_dependency(
-                VisitedHandlingMode::RelevantLinkVisited,
-                dependency,
-                &mut false,
-            );
-
-            if should_account_for_dependency {
-                return self.note_dependency(dependency);
-            }
-        }
-    }
-
-    fn note_dependency(&mut self, dependency: &Dependency) {
-        if dependency.affects_self() {
-            self.invalidates_self = true;
-        }
-
-        if dependency.affects_descendants() {
-            debug_assert_ne!(dependency.selector_offset, 0);
-            debug_assert!(!dependency.affects_later_siblings());
-            self.descendant_invalidations.push(Invalidation {
-                selector: dependency.selector.clone(),
-                offset: dependency.selector_offset,
-                matched_by_any_previous: false,
-            });
-        } else if dependency.affects_later_siblings() {
-            debug_assert_ne!(dependency.selector_offset, 0);
-            self.sibling_invalidations.push(Invalidation {
-                selector: dependency.selector.clone(),
-                offset: dependency.selector_offset,
-                matched_by_any_previous: false,
-            });
-        }
-    }
-
-    /// Returns whether `dependency` may cause us to invalidate the style of
-    /// more elements than what we've already invalidated.
-    fn dependency_may_be_relevant(&self, dependency: &Dependency) -> bool {
-        if dependency.affects_descendants() || dependency.affects_later_siblings() {
-            return true;
-        }
-
-        debug_assert!(dependency.affects_self());
-        !self.invalidates_self
-    }
-}
diff --git a/components/style/invalidation/element/mod.rs b/components/style/invalidation/element/mod.rs
index 61aae2ffcd2..3947a2d057c 100644
--- a/components/style/invalidation/element/mod.rs
+++ b/components/style/invalidation/element/mod.rs
@@ -4,6 +4,7 @@
 
 //! Invalidation of element styles due to attribute or style changes.
 
+pub mod collector;
 pub mod element_wrapper;
 pub mod invalidation_map;
 pub mod invalidator;

From a5e2f2c76c698fddf822d94f2ae0e0895a9e4bdb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 17:17:58 +0200
Subject: [PATCH 2/7] style: Isolate all the restyling related logic in the
 invalidator in an InvalidationProcessor trait.

Ditto, no change in behavior.
---
 components/style/data.rs                      |   9 +-
 .../style/invalidation/element/collector.rs   | 103 ++++++++++++-
 .../style/invalidation/element/invalidator.rs | 137 +++++++++++++-----
 3 files changed, 204 insertions(+), 45 deletions(-)

diff --git a/components/style/data.rs b/components/style/data.rs
index c1ca6324110..f57418628b2 100644
--- a/components/style/data.rs
+++ b/components/style/data.rs
@@ -244,7 +244,7 @@ impl ElementData {
             return InvalidationResult::empty();
         }
 
-        use invalidation::element::collector::StateAndAttrInvalidationCollector;
+        use invalidation::element::collector::StateAndAttrInvalidationProcessor;
         use invalidation::element::invalidator::TreeStyleInvalidator;
 
         debug!("invalidate_style_if_needed: {:?}, flags: {:?}, has_snapshot: {}, \
@@ -259,18 +259,21 @@ impl ElementData {
             return InvalidationResult::empty();
         }
 
+        let processor = StateAndAttrInvalidationProcessor;
         let invalidator = TreeStyleInvalidator::new(
             element,
             Some(self),
             shared_context,
             stack_limit_checker,
             nth_index_cache,
+            &processor,
         );
 
-        let result =
-            invalidator.invalidate::<StateAndAttrInvalidationCollector>();
+        let result = invalidator.invalidate();
+
         unsafe { element.set_handled_snapshot() }
         debug_assert!(element.handled_snapshot());
+
         result
     }
 
diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index a496e4220d0..b80dbb3db5b 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -2,7 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-//! A collector for invalidations due to state and attribute changes.
+//! An invalidation processor for style changes due to state and attribute
+//! changes.
 
 use Atom;
 use context::{QuirksMode, SharedStyleContext};
@@ -11,7 +12,7 @@ use dom::TElement;
 use element_state::{ElementState, IN_VISITED_OR_UNVISITED_STATE};
 use invalidation::element::element_wrapper::{ElementSnapshot, ElementWrapper};
 use invalidation::element::invalidation_map::*;
-use invalidation::element::invalidator::{InvalidationVector, Invalidation, InvalidationCollector};
+use invalidation::element::invalidator::{InvalidationVector, Invalidation, InvalidationProcessor};
 use invalidation::element::restyle_hints::*;
 use selector_map::SelectorMap;
 use selector_parser::Snapshot;
@@ -48,11 +49,13 @@ where
     invalidates_self: bool,
 }
 
-///  A collector for state and attribute invalidations.
-pub struct StateAndAttrInvalidationCollector;
+/// An invalidation processor for style changes due to state and attribute
+/// changes.
+pub struct StateAndAttrInvalidationProcessor;
 
-impl InvalidationCollector for StateAndAttrInvalidationCollector {
+impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
     fn collect_invalidations<E>(
+        &self,
         element: E,
         mut data: Option<&mut ElementData>,
         nth_index_cache: Option<&mut NthIndexCache>,
@@ -172,6 +175,96 @@ impl InvalidationCollector for StateAndAttrInvalidationCollector {
 
         invalidated_self
     }
+
+    fn should_process_descendants<E>(
+        &self,
+        _element: E,
+        data: Option<&mut ElementData>,
+    ) -> bool
+    where
+        E: TElement,
+    {
+        let data = match data {
+            None => return false,
+            Some(ref data) => data,
+        };
+
+        // FIXME(emilio): should check only RESTYLE_DESCENDANTS.
+        //
+        // Also, could probably return false if data.styles.is_display_none()
+        // returns true.
+        !data.hint.contains_subtree()
+    }
+
+    fn recursion_limit_exceeded<E>(
+        &self,
+        _element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement,
+    {
+        if let Some(data) = data {
+            data.hint.insert(RESTYLE_DESCENDANTS);
+        }
+    }
+
+    fn invalidated_child<E>(
+        &self,
+        element: E,
+        _data: Option<&mut ElementData>,
+        child: E,
+    )
+    where
+        E: TElement,
+    {
+        if child.get_data().is_none() {
+            return;
+        }
+
+        // The child may not be a flattened tree child of the current element,
+        // but may be arbitrarily deep.
+        //
+        // Since we keep the traversal flags in terms of the flattened tree,
+        // we need to propagate it as appropriate.
+        let mut current = child.traversal_parent();
+        while let Some(parent) = current.take() {
+            if parent == element {
+                break;
+            }
+
+            unsafe { parent.set_dirty_descendants() };
+            current = parent.traversal_parent();
+        }
+    }
+
+    fn invalidated_descendants<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement,
+    {
+        // FIXME(emilio): We probably want to walk the flattened tree here too,
+        // and remove invalidated_child instead, or something like that.
+        if data.as_ref().map_or(false, |d| !d.styles.is_display_none()) {
+            unsafe { element.set_dirty_descendants() };
+        }
+    }
+
+    fn invalidated_self<E>(
+        &self,
+        _element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement,
+    {
+        if let Some(data) = data {
+            data.hint.insert(RESTYLE_SELF);
+        }
+    }
 }
 
 impl<'a, 'b, E> Collector<'a, 'b, E>
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index 012b9feedaa..0c7636cc1c3 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -8,7 +8,6 @@
 use context::{SharedStyleContext, StackLimitChecker};
 use data::ElementData;
 use dom::{TElement, TNode};
-use invalidation::element::restyle_hints::*;
 use selector_parser::SelectorImpl;
 use selectors::NthIndexCache;
 use selectors::matching::{MatchingContext, MatchingMode, VisitedHandlingMode};
@@ -19,11 +18,15 @@ use smallvec::SmallVec;
 use std::fmt;
 
 /// A trait to abstract the collection of invalidations for a given pass.
-pub trait InvalidationCollector {
+///
+/// The `data` argument is a mutable reference to the element's style data, if
+/// any.
+pub trait InvalidationProcessor {
     /// Collect invalidations for a given element's descendants and siblings.
     ///
     /// Returns whether the element itself was invalidated.
     fn collect_invalidations<E>(
+        &self,
         element: E,
         data: Option<&mut ElementData>,
         nth_index_cache: Option<&mut NthIndexCache>,
@@ -33,12 +36,62 @@ pub trait InvalidationCollector {
     ) -> bool
     where
         E: TElement;
+
+    /// Returns whether the invalidation process should process the descendants
+    /// of the given element.
+    fn should_process_descendants<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+    ) -> bool
+    where
+        E: TElement;
+
+    /// Executes an arbitrary action when the recursion limit is exceded (if
+    /// any).
+    fn recursion_limit_exceeded<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement;
+
+    /// Executes an arbitrary action when a direct child is invalidated.
+    fn invalidated_child<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+        child: E,
+    )
+    where
+        E: TElement;
+
+    /// Executes an action when `Self` is invalidated.
+    fn invalidated_self<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement;
+
+    /// Executes an action when any descendant of `Self` is invalidated.
+    fn invalidated_descendants<E>(
+        &self,
+        element: E,
+        data: Option<&mut ElementData>,
+    )
+    where
+        E: TElement;
 }
 
 /// The struct that takes care of encapsulating all the logic on where and how
 /// element styles need to be invalidated.
-pub struct TreeStyleInvalidator<'a, 'b: 'a, E>
-    where E: TElement,
+pub struct TreeStyleInvalidator<'a, 'b: 'a, E, P: 'a>
+where
+    E: TElement,
+    P: InvalidationProcessor
 {
     element: E,
     // TODO(emilio): It's tempting enough to just avoid running invalidation for
@@ -54,6 +107,10 @@ pub struct TreeStyleInvalidator<'a, 'b: 'a, E>
     shared_context: &'a SharedStyleContext<'b>,
     stack_limit_checker: Option<&'a StackLimitChecker>,
     nth_index_cache: Option<&'a mut NthIndexCache>,
+
+    // TODO(emilio): Make a mutable reference, we're going to need that for
+    // QS/QSA.
+    processor: &'a P,
 }
 
 /// A vector of invalidations, optimized for small invalidation sets.
@@ -182,8 +239,10 @@ impl InvalidationResult {
     }
 }
 
-impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
-    where E: TElement,
+impl<'a, 'b: 'a, E, P: 'a> TreeStyleInvalidator<'a, 'b, E, P>
+where
+    E: TElement,
+    P: InvalidationProcessor,
 {
     /// Trivially constructs a new `TreeStyleInvalidator`.
     pub fn new(
@@ -192,6 +251,7 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         shared_context: &'a SharedStyleContext<'b>,
         stack_limit_checker: Option<&'a StackLimitChecker>,
         nth_index_cache: Option<&'a mut NthIndexCache>,
+        processor: &'a P,
     ) -> Self {
         Self {
             element,
@@ -199,17 +259,18 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
             shared_context,
             stack_limit_checker,
             nth_index_cache,
+            processor,
         }
     }
 
     /// Perform the invalidation pass.
-    pub fn invalidate<C: InvalidationCollector>(mut self) -> InvalidationResult {
+    pub fn invalidate(mut self) -> InvalidationResult {
         debug!("StyleTreeInvalidator::invalidate({:?})", self.element);
 
         let mut descendant_invalidations = InvalidationVector::new();
         let mut sibling_invalidations = InvalidationVector::new();
 
-        let invalidated_self = C::collect_invalidations(
+        let invalidated_self = self.processor.collect_invalidations(
             self.element,
             self.data.as_mut().map(|d| &mut **d),
             self.nth_index_cache.as_mut().map(|c| &mut **c),
@@ -246,14 +307,14 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
 
         while let Some(sibling) = current {
             let mut sibling_data = sibling.mutate_data();
-            let sibling_data = sibling_data.as_mut().map(|d| &mut **d);
 
             let mut sibling_invalidator = TreeStyleInvalidator::new(
                 sibling,
-                sibling_data,
+                sibling_data.as_mut().map(|d| &mut **d),
                 self.shared_context,
                 self.stack_limit_checker,
                 self.nth_index_cache.as_mut().map(|c| &mut **c),
+                self.processor,
             );
 
             let mut invalidations_for_descendants = InvalidationVector::new();
@@ -310,14 +371,14 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         sibling_invalidations: &mut InvalidationVector,
     ) -> bool {
         let mut child_data = child.mutate_data();
-        let child_data = child_data.as_mut().map(|d| &mut **d);
 
         let mut child_invalidator = TreeStyleInvalidator::new(
             child,
-            child_data,
+            child_data.as_mut().map(|d| &mut **d),
             self.shared_context,
             self.stack_limit_checker,
             self.nth_index_cache.as_mut().map(|c| &mut **c),
+            self.processor,
         );
 
         let mut invalidations_for_descendants = InvalidationVector::new();
@@ -341,16 +402,12 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         //
         // Since we keep the traversal flags in terms of the flattened tree,
         // we need to propagate it as appropriate.
-        if invalidated_child && child.get_data().is_some() {
-            let mut current = child.traversal_parent();
-            while let Some(parent) = current.take() {
-                if parent == self.element {
-                    break;
-                }
-
-                unsafe { parent.set_dirty_descendants() };
-                current = parent.traversal_parent();
-            }
+        if invalidated_child {
+            self.processor.invalidated_child(
+                self.element,
+                self.data.as_mut().map(|d| &mut **d),
+                child,
+            );
         }
 
         let invalidated_descendants = child_invalidator.invalidate_descendants(
@@ -424,20 +481,22 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
                self.element);
         debug!(" > {:?}", invalidations);
 
-        match self.data {
-            None => return false,
-            Some(ref data) => {
-                // FIXME(emilio): Only needs to check RESTYLE_DESCENDANTS,
-                // really.
-                if data.hint.contains_subtree() {
-                    return false;
-                }
-            }
+        let should_process =
+            self.processor.should_process_descendants(
+                self.element,
+                self.data.as_mut().map(|d| &mut **d),
+            );
+
+        if !should_process {
+            return false;
         }
 
         if let Some(checker) = self.stack_limit_checker {
             if checker.limit_exceeded() {
-                self.data.as_mut().unwrap().hint.insert(RESTYLE_DESCENDANTS);
+                self.processor.recursion_limit_exceeded(
+                    self.element,
+                    self.data.as_mut().map(|d| &mut **d)
+                );
                 return true;
             }
         }
@@ -467,8 +526,11 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
 
         any_descendant |= self.invalidate_nac(invalidations);
 
-        if any_descendant && self.data.as_ref().map_or(false, |d| !d.styles.is_display_none()) {
-            unsafe { self.element.set_dirty_descendants() };
+        if any_descendant {
+            self.processor.invalidated_descendants(
+                self.element,
+                self.data.as_mut().map(|d| &mut **d)
+            );
         }
 
         any_descendant
@@ -730,9 +792,10 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         }
 
         if invalidated_self {
-            if let Some(ref mut data) = self.data {
-                data.hint.insert(RESTYLE_SELF);
-            }
+            self.processor.invalidated_self(
+                self.element,
+                self.data.as_mut().map(|d| &mut **d),
+            );
         }
 
         SingleInvalidationResult { invalidated_self, matched, }

From 557353c1f62b93b98736fdfaa64423887066f0fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 17:22:34 +0200
Subject: [PATCH 3/7] style: Move the bounds up in InvalidationProcessor.

---
 .../style/invalidation/element/collector.rs   | 47 +++++++------------
 .../style/invalidation/element/invalidator.rs | 45 +++++++-----------
 2 files changed, 34 insertions(+), 58 deletions(-)

diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index b80dbb3db5b..f6fbff96707 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -53,8 +53,11 @@ where
 /// changes.
 pub struct StateAndAttrInvalidationProcessor;
 
-impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
-    fn collect_invalidations<E>(
+impl<E> InvalidationProcessor<E> for StateAndAttrInvalidationProcessor
+where
+    E: TElement,
+{
+    fn collect_invalidations(
         &self,
         element: E,
         mut data: Option<&mut ElementData>,
@@ -62,10 +65,7 @@ impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
         shared_context: &SharedStyleContext,
         descendant_invalidations: &mut InvalidationVector,
         sibling_invalidations: &mut InvalidationVector,
-    ) -> bool
-    where
-        E: TElement,
-    {
+    ) -> bool {
         debug_assert!(element.has_snapshot(), "Why bothering?");
         debug_assert!(data.is_some(), "How exactly?");
 
@@ -176,14 +176,11 @@ impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
         invalidated_self
     }
 
-    fn should_process_descendants<E>(
+    fn should_process_descendants(
         &self,
         _element: E,
         data: Option<&mut ElementData>,
-    ) -> bool
-    where
-        E: TElement,
-    {
+    ) -> bool {
         let data = match data {
             None => return false,
             Some(ref data) => data,
@@ -196,28 +193,22 @@ impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
         !data.hint.contains_subtree()
     }
 
-    fn recursion_limit_exceeded<E>(
+    fn recursion_limit_exceeded(
         &self,
         _element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement,
-    {
+    ) {
         if let Some(data) = data {
             data.hint.insert(RESTYLE_DESCENDANTS);
         }
     }
 
-    fn invalidated_child<E>(
+    fn invalidated_child(
         &self,
         element: E,
         _data: Option<&mut ElementData>,
         child: E,
-    )
-    where
-        E: TElement,
-    {
+    ) {
         if child.get_data().is_none() {
             return;
         }
@@ -238,14 +229,11 @@ impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
         }
     }
 
-    fn invalidated_descendants<E>(
+    fn invalidated_descendants(
         &self,
         element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement,
-    {
+    ) {
         // FIXME(emilio): We probably want to walk the flattened tree here too,
         // and remove invalidated_child instead, or something like that.
         if data.as_ref().map_or(false, |d| !d.styles.is_display_none()) {
@@ -253,14 +241,11 @@ impl InvalidationProcessor for StateAndAttrInvalidationProcessor {
         }
     }
 
-    fn invalidated_self<E>(
+    fn invalidated_self(
         &self,
         _element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement,
-    {
+    ) {
         if let Some(data) = data {
             data.hint.insert(RESTYLE_SELF);
         }
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index 0c7636cc1c3..682e7b23c6b 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -21,11 +21,14 @@ use std::fmt;
 ///
 /// The `data` argument is a mutable reference to the element's style data, if
 /// any.
-pub trait InvalidationProcessor {
+pub trait InvalidationProcessor<E>
+where
+    E: TElement,
+{
     /// Collect invalidations for a given element's descendants and siblings.
     ///
     /// Returns whether the element itself was invalidated.
-    fn collect_invalidations<E>(
+    fn collect_invalidations(
         &self,
         element: E,
         data: Option<&mut ElementData>,
@@ -33,57 +36,45 @@ pub trait InvalidationProcessor {
         shared_context: &SharedStyleContext,
         descendant_invalidations: &mut InvalidationVector,
         sibling_invalidations: &mut InvalidationVector,
-    ) -> bool
-    where
-        E: TElement;
+    ) -> bool;
 
     /// Returns whether the invalidation process should process the descendants
     /// of the given element.
-    fn should_process_descendants<E>(
+    fn should_process_descendants(
         &self,
         element: E,
         data: Option<&mut ElementData>,
-    ) -> bool
-    where
-        E: TElement;
+    ) -> bool;
 
     /// Executes an arbitrary action when the recursion limit is exceded (if
     /// any).
-    fn recursion_limit_exceeded<E>(
+    fn recursion_limit_exceeded(
         &self,
         element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement;
+    );
 
     /// Executes an arbitrary action when a direct child is invalidated.
-    fn invalidated_child<E>(
+    fn invalidated_child(
         &self,
         element: E,
         data: Option<&mut ElementData>,
         child: E,
-    )
-    where
-        E: TElement;
+    );
 
     /// Executes an action when `Self` is invalidated.
-    fn invalidated_self<E>(
+    fn invalidated_self(
         &self,
         element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement;
+    );
 
     /// Executes an action when any descendant of `Self` is invalidated.
-    fn invalidated_descendants<E>(
+    fn invalidated_descendants(
         &self,
         element: E,
         data: Option<&mut ElementData>,
-    )
-    where
-        E: TElement;
+    );
 }
 
 /// The struct that takes care of encapsulating all the logic on where and how
@@ -91,7 +82,7 @@ pub trait InvalidationProcessor {
 pub struct TreeStyleInvalidator<'a, 'b: 'a, E, P: 'a>
 where
     E: TElement,
-    P: InvalidationProcessor
+    P: InvalidationProcessor<E>
 {
     element: E,
     // TODO(emilio): It's tempting enough to just avoid running invalidation for
@@ -242,7 +233,7 @@ impl InvalidationResult {
 impl<'a, 'b: 'a, E, P: 'a> TreeStyleInvalidator<'a, 'b, E, P>
 where
     E: TElement,
-    P: InvalidationProcessor,
+    P: InvalidationProcessor<E>,
 {
     /// Trivially constructs a new `TreeStyleInvalidator`.
     pub fn new(

From 9e61c1962b3fc410395351085e33d46d9d631f01 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 17:30:41 +0200
Subject: [PATCH 4/7] style: Unify invalidated_child with
 invalidated_descendants.

I think invalidated_descendants was buggy, and this fixes it.
---
 .../style/invalidation/element/collector.rs   | 26 ++++++-----------
 .../style/invalidation/element/invalidator.rs | 28 +++++--------------
 2 files changed, 16 insertions(+), 38 deletions(-)

diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index f6fbff96707..271e16f323c 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -203,16 +203,20 @@ where
         }
     }
 
-    fn invalidated_child(
+    fn invalidated_descendants(
         &self,
         element: E,
-        _data: Option<&mut ElementData>,
+        data: Option<&mut ElementData>,
         child: E,
     ) {
         if child.get_data().is_none() {
             return;
         }
 
+        if data.as_ref().map_or(true, |d| d.styles.is_display_none()) {
+            return;
+        }
+
         // The child may not be a flattened tree child of the current element,
         // but may be arbitrarily deep.
         //
@@ -220,24 +224,12 @@ where
         // we need to propagate it as appropriate.
         let mut current = child.traversal_parent();
         while let Some(parent) = current.take() {
+            unsafe { parent.set_dirty_descendants() };
+            current = parent.traversal_parent();
+
             if parent == element {
                 break;
             }
-
-            unsafe { parent.set_dirty_descendants() };
-            current = parent.traversal_parent();
-        }
-    }
-
-    fn invalidated_descendants(
-        &self,
-        element: E,
-        data: Option<&mut ElementData>,
-    ) {
-        // FIXME(emilio): We probably want to walk the flattened tree here too,
-        // and remove invalidated_child instead, or something like that.
-        if data.as_ref().map_or(false, |d| !d.styles.is_display_none()) {
-            unsafe { element.set_dirty_descendants() };
         }
     }
 
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index 682e7b23c6b..072577d6f44 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -54,14 +54,6 @@ where
         data: Option<&mut ElementData>,
     );
 
-    /// Executes an arbitrary action when a direct child is invalidated.
-    fn invalidated_child(
-        &self,
-        element: E,
-        data: Option<&mut ElementData>,
-        child: E,
-    );
-
     /// Executes an action when `Self` is invalidated.
     fn invalidated_self(
         &self,
@@ -74,6 +66,7 @@ where
         &self,
         element: E,
         data: Option<&mut ElementData>,
+        child: E,
     );
 }
 
@@ -388,23 +381,23 @@ where
                 sibling_invalidations,
             );
 
+        let invalidated_descendants = child_invalidator.invalidate_descendants(
+            &invalidations_for_descendants
+        );
+
         // The child may not be a flattened tree child of the current element,
         // but may be arbitrarily deep.
         //
         // Since we keep the traversal flags in terms of the flattened tree,
         // we need to propagate it as appropriate.
-        if invalidated_child {
-            self.processor.invalidated_child(
+        if invalidated_child || invalidated_descendants {
+            self.processor.invalidated_descendants(
                 self.element,
                 self.data.as_mut().map(|d| &mut **d),
                 child,
             );
         }
 
-        let invalidated_descendants = child_invalidator.invalidate_descendants(
-            &invalidations_for_descendants
-        );
-
         invalidated_child || invalidated_descendants
     }
 
@@ -517,13 +510,6 @@ where
 
         any_descendant |= self.invalidate_nac(invalidations);
 
-        if any_descendant {
-            self.processor.invalidated_descendants(
-                self.element,
-                self.data.as_mut().map(|d| &mut **d)
-            );
-        }
-
         any_descendant
     }
 

From ecdb10ef5c07aa41797d5c30fc42e1d942825504 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 17:35:14 +0200
Subject: [PATCH 5/7] Be more precise in should_process_descendants.

---
 components/style/invalidation/element/collector.rs | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index 271e16f323c..08456e032e7 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -186,11 +186,8 @@ where
             Some(ref data) => data,
         };
 
-        // FIXME(emilio): should check only RESTYLE_DESCENDANTS.
-        //
-        // Also, could probably return false if data.styles.is_display_none()
-        // returns true.
-        !data.hint.contains_subtree()
+        !data.styles.is_display_none() &&
+            !data.hint.contains(RESTYLE_DESCENDANTS)
     }
 
     fn recursion_limit_exceeded(

From 9034e6a73251fa8841a245ce60b1150ba7ced58a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 17:55:38 +0200
Subject: [PATCH 6/7] style: Add a way to skip the "invalidation on eager
 pseudo invalidates self" bit.

---
 components/style/invalidation/element/collector.rs   | 5 +++++
 components/style/invalidation/element/invalidator.rs | 8 +++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index 08456e032e7..9b8f6aca747 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -57,6 +57,11 @@ impl<E> InvalidationProcessor<E> for StateAndAttrInvalidationProcessor
 where
     E: TElement,
 {
+    /// We need to invalidate style on an eager pseudo-element, in order to
+    /// process changes that could otherwise end up in ::before or ::after
+    /// content being generated.
+    fn invalidates_on_eager_pseudo_element(&self) -> bool { true }
+
     fn collect_invalidations(
         &self,
         element: E,
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index 072577d6f44..d0c77cc48df 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -25,6 +25,11 @@ pub trait InvalidationProcessor<E>
 where
     E: TElement,
 {
+    /// Whether an invalidation that contains only an eager pseudo-element
+    /// selector like ::before or ::after triggers invalidation of the element
+    /// that would originate it.
+    fn invalidates_on_eager_pseudo_element(&self) -> bool { false }
+
     /// Collect invalidations for a given element's descendants and siblings.
     ///
     /// Returns whether the element itself was invalidated.
@@ -670,7 +675,8 @@ where
                     //
                     // Note that we'll also restyle the pseudo-element because
                     // it would match this invalidation.
-                    if pseudo.is_eager() {
+                    if self.processor.invalidates_on_eager_pseudo_element() &&
+                        pseudo.is_eager() {
                         invalidated_self = true;
                     }
                 }

From e447f819a22e71d586dc25af8e03107a169d5cb1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= <emilio@crisal.io>
Date: Thu, 12 Oct 2017 18:08:44 +0200
Subject: [PATCH 7/7] style: Make InvalidationProcessor methods be &mut self.

This would allow querySelector / querySelectorAll to mutate the list of matched
nodes as it sees fit.
---
 components/style/data.rs                      |  4 +-
 .../style/invalidation/element/collector.rs   | 10 +--
 .../style/invalidation/element/invalidator.rs | 65 +++++++++----------
 3 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/components/style/data.rs b/components/style/data.rs
index f57418628b2..7e50a67dbb3 100644
--- a/components/style/data.rs
+++ b/components/style/data.rs
@@ -259,14 +259,14 @@ impl ElementData {
             return InvalidationResult::empty();
         }
 
-        let processor = StateAndAttrInvalidationProcessor;
+        let mut processor = StateAndAttrInvalidationProcessor;
         let invalidator = TreeStyleInvalidator::new(
             element,
             Some(self),
             shared_context,
             stack_limit_checker,
             nth_index_cache,
-            &processor,
+            &mut processor,
         );
 
         let result = invalidator.invalidate();
diff --git a/components/style/invalidation/element/collector.rs b/components/style/invalidation/element/collector.rs
index 9b8f6aca747..82d99c025dc 100644
--- a/components/style/invalidation/element/collector.rs
+++ b/components/style/invalidation/element/collector.rs
@@ -63,7 +63,7 @@ where
     fn invalidates_on_eager_pseudo_element(&self) -> bool { true }
 
     fn collect_invalidations(
-        &self,
+        &mut self,
         element: E,
         mut data: Option<&mut ElementData>,
         nth_index_cache: Option<&mut NthIndexCache>,
@@ -182,7 +182,7 @@ where
     }
 
     fn should_process_descendants(
-        &self,
+        &mut self,
         _element: E,
         data: Option<&mut ElementData>,
     ) -> bool {
@@ -196,7 +196,7 @@ where
     }
 
     fn recursion_limit_exceeded(
-        &self,
+        &mut self,
         _element: E,
         data: Option<&mut ElementData>,
     ) {
@@ -206,7 +206,7 @@ where
     }
 
     fn invalidated_descendants(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
         child: E,
@@ -236,7 +236,7 @@ where
     }
 
     fn invalidated_self(
-        &self,
+        &mut self,
         _element: E,
         data: Option<&mut ElementData>,
     ) {
diff --git a/components/style/invalidation/element/invalidator.rs b/components/style/invalidation/element/invalidator.rs
index d0c77cc48df..8b269569af9 100644
--- a/components/style/invalidation/element/invalidator.rs
+++ b/components/style/invalidation/element/invalidator.rs
@@ -34,7 +34,7 @@ where
     ///
     /// Returns whether the element itself was invalidated.
     fn collect_invalidations(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
         nth_index_cache: Option<&mut NthIndexCache>,
@@ -46,7 +46,7 @@ where
     /// Returns whether the invalidation process should process the descendants
     /// of the given element.
     fn should_process_descendants(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
     ) -> bool;
@@ -54,21 +54,21 @@ where
     /// Executes an arbitrary action when the recursion limit is exceded (if
     /// any).
     fn recursion_limit_exceeded(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
     );
 
     /// Executes an action when `Self` is invalidated.
     fn invalidated_self(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
     );
 
     /// Executes an action when any descendant of `Self` is invalidated.
     fn invalidated_descendants(
-        &self,
+        &mut self,
         element: E,
         data: Option<&mut ElementData>,
         child: E,
@@ -96,10 +96,7 @@ where
     shared_context: &'a SharedStyleContext<'b>,
     stack_limit_checker: Option<&'a StackLimitChecker>,
     nth_index_cache: Option<&'a mut NthIndexCache>,
-
-    // TODO(emilio): Make a mutable reference, we're going to need that for
-    // QS/QSA.
-    processor: &'a P,
+    processor: &'a mut P,
 }
 
 /// A vector of invalidations, optimized for small invalidation sets.
@@ -240,7 +237,7 @@ where
         shared_context: &'a SharedStyleContext<'b>,
         stack_limit_checker: Option<&'a StackLimitChecker>,
         nth_index_cache: Option<&'a mut NthIndexCache>,
-        processor: &'a P,
+        processor: &'a mut P,
     ) -> Self {
         Self {
             element,
@@ -359,36 +356,36 @@ where
         invalidations: &InvalidationVector,
         sibling_invalidations: &mut InvalidationVector,
     ) -> bool {
-        let mut child_data = child.mutate_data();
-
-        let mut child_invalidator = TreeStyleInvalidator::new(
-            child,
-            child_data.as_mut().map(|d| &mut **d),
-            self.shared_context,
-            self.stack_limit_checker,
-            self.nth_index_cache.as_mut().map(|c| &mut **c),
-            self.processor,
-        );
-
         let mut invalidations_for_descendants = InvalidationVector::new();
+
         let mut invalidated_child = false;
+        let invalidated_descendants = {
+            let mut child_data = child.mutate_data();
 
-        invalidated_child |=
-            child_invalidator.process_sibling_invalidations(
-                &mut invalidations_for_descendants,
-                sibling_invalidations,
+            let mut child_invalidator = TreeStyleInvalidator::new(
+                child,
+                child_data.as_mut().map(|d| &mut **d),
+                self.shared_context,
+                self.stack_limit_checker,
+                self.nth_index_cache.as_mut().map(|c| &mut **c),
+                self.processor,
             );
 
-        invalidated_child |=
-            child_invalidator.process_descendant_invalidations(
-                invalidations,
-                &mut invalidations_for_descendants,
-                sibling_invalidations,
-            );
+            invalidated_child |=
+                child_invalidator.process_sibling_invalidations(
+                    &mut invalidations_for_descendants,
+                    sibling_invalidations,
+                );
 
-        let invalidated_descendants = child_invalidator.invalidate_descendants(
-            &invalidations_for_descendants
-        );
+            invalidated_child |=
+                child_invalidator.process_descendant_invalidations(
+                    invalidations,
+                    &mut invalidations_for_descendants,
+                    sibling_invalidations,
+                );
+
+            child_invalidator.invalidate_descendants(&invalidations_for_descendants)
+        };
 
         // The child may not be a flattened tree child of the current element,
         // but may be arbitrarily deep.