Bug 1477773: Simplify visited-related code in invalidation. r=xidorn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 14 Aug 2018 10:49:21 +0200
changeset 431686 4c42d90e56a6e6364ca0ed9de11aaa79ea2f4706
parent 431685 dc9a17cc8a62e70fe20ac5ee5e0366500c22db70
child 431687 cf5e7017ffdc1ebc32f564f95de1ca91bf7a5795
push id34451
push userebalazs@mozilla.com
push dateThu, 16 Aug 2018 09:25:15 +0000
treeherdermozilla-central@161817e6d127 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersxidorn
bugs1477773, 1328509, 19520, 1406622
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1477773: Simplify visited-related code in invalidation. r=xidorn We match with AllLinksVisitedAndUnvisited for style invalidation, and we already do a subtree restyle because :visited matching doesn't depend on the actual element state. So all this stuff is just not needed. The comment points to the attribute tests in bug 1328509, but those still trivially pass with this change. I think this was unneeded since I introduced AllLinksVisitedAndUnvisited, or maybe since https://github.com/servo/servo/pull/19520. In any case it doesn't really matter, and I already had done this cleanup in my WIP patches for bug 1406622, but I guess this is a slightly more suitable place to land them :) Differential Revision: https://phabricator.services.mozilla.com/D3305
servo/components/style/invalidation/element/state_and_attributes.rs
--- a/servo/components/style/invalidation/element/state_and_attributes.rs
+++ b/servo/components/style/invalidation/element/state_and_attributes.rs
@@ -19,22 +19,16 @@ 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;
 use stylesheets::origin::{Origin, OriginSet};
 
-#[derive(Debug, PartialEq)]
-enum VisitedDependent {
-    Yes,
-    No,
-}
-
 /// The collector implementation.
 struct Collector<'a, 'b: 'a, 'selectors: 'a, E>
 where
     E: TElement,
 {
     element: E,
     wrapper: ElementWrapper<'b, E>,
     snapshot: &'a Snapshot,
@@ -347,34 +341,34 @@ where
     'selectors: 'a,
 {
     fn collect_dependencies_in_invalidation_map(&mut self, map: &'selectors InvalidationMap) {
         let quirks_mode = self.matching_context.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);
+                    self.scan_dependency(dep);
                 }
             }
         }
 
         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);
+                    self.scan_dependency(dep);
                 }
             }
         }
 
         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);
+                    self.scan_dependency(dep);
                 }
             }
         }
 
         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);
 
@@ -390,17 +384,17 @@ where
 
     fn collect_dependencies_in_map(&mut self, map: &'selectors SelectorMap<Dependency>) {
         map.lookup_with_additional(
             self.lookup_element,
             self.matching_context.quirks_mode(),
             self.removed_id,
             self.classes_removed,
             |dependency| {
-                self.scan_dependency(dependency, VisitedDependent::No);
+                self.scan_dependency(dependency);
                 true
             },
         );
     }
 
     fn collect_state_dependencies(
         &mut self,
         map: &'selectors SelectorMap<StateDependency>,
@@ -410,108 +404,63 @@ where
             self.lookup_element,
             self.matching_context.quirks_mode(),
             self.removed_id,
             self.classes_removed,
             |dependency| {
                 if !dependency.state.intersects(state_changes) {
                     return true;
                 }
-                let visited_dependent = if dependency
-                    .state
-                    .intersects(ElementState::IN_VISITED_OR_UNVISITED_STATE)
-                {
-                    VisitedDependent::Yes
-                } else {
-                    VisitedDependent::No
-                };
-                self.scan_dependency(&dependency.dep, visited_dependent);
+                self.scan_dependency(&dependency.dep);
                 true
             },
         );
     }
 
-    /// Check whether a dependency should be taken into account, using a given
-    /// visited handling mode.
+    /// Check whether a dependency should be taken into account.
     fn check_dependency(
         &mut self,
-        visited_handling_mode: VisitedHandlingMode,
         dependency: &Dependency,
     ) -> bool {
         let element = &self.element;
         let wrapper = &self.wrapper;
-        self.matching_context
-            .with_visited_handling_mode(visited_handling_mode, |mut context| {
-                let matches_now = matches_selector(
-                    &dependency.selector,
-                    dependency.selector_offset,
-                    None,
-                    element,
-                    &mut context,
-                    &mut |_, _| {},
-                );
+        let matches_now = matches_selector(
+            &dependency.selector,
+            dependency.selector_offset,
+            None,
+            element,
+            &mut self.matching_context,
+            &mut |_, _| {},
+        );
 
-                let matched_then = matches_selector(
-                    &dependency.selector,
-                    dependency.selector_offset,
-                    None,
-                    wrapper,
-                    &mut context,
-                    &mut |_, _| {},
-                );
+        let matched_then = matches_selector(
+            &dependency.selector,
+            dependency.selector_offset,
+            None,
+            wrapper,
+            &mut self.matching_context,
+            &mut |_, _| {},
+        );
 
-                matched_then != matches_now
-            })
+        matched_then != matches_now
     }
 
-    fn scan_dependency(
-        &mut self,
-        dependency: &'selectors Dependency,
-        is_visited_dependent: VisitedDependent,
-    ) {
+    fn scan_dependency(&mut self, dependency: &'selectors Dependency) {
         debug!(
-            "TreeStyleInvalidator::scan_dependency({:?}, {:?}, {:?})",
-            self.element, dependency, is_visited_dependent,
+            "TreeStyleInvalidator::scan_dependency({:?}, {:?})",
+            self.element, dependency
         );
 
         if !self.dependency_may_be_relevant(dependency) {
             return;
         }
 
-        let should_account_for_dependency =
-            self.check_dependency(VisitedHandlingMode::AllLinksVisitedAndUnvisited, dependency);
-
-        if should_account_for_dependency {
+        if self.check_dependency(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 && self.element.is_link() {
-            let should_account_for_dependency =
-                self.check_dependency(VisitedHandlingMode::RelevantLinkVisited, dependency);
-
-            if should_account_for_dependency {
-                return self.note_dependency(dependency);
-            }
-        }
     }
 
     fn note_dependency(&mut self, dependency: &'selectors Dependency) {
         debug_assert!(self.dependency_may_be_relevant(dependency));
 
         let invalidation_kind = dependency.invalidation_kind();
         if matches!(invalidation_kind, DependencyInvalidationKind::Element) {
             self.invalidates_self = true;