From a0cb37d29d211f549a5afb7d00626d6df96b8f9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 14 Aug 2018 10:49:21 +0200 Subject: [PATCH] style: Simplify visited-related code in invalidation. 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 --- .../element/state_and_attributes.rs | 105 +++++------------- 1 file changed, 27 insertions(+), 78 deletions(-) diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index 0be44c1fc42..1eb022b298a 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -24,12 +24,6 @@ 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 @@ -352,7 +346,7 @@ where 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); } } } @@ -361,7 +355,7 @@ where 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); } } } @@ -369,7 +363,7 @@ where 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); } } } @@ -395,7 +389,7 @@ where self.removed_id, self.classes_removed, |dependency| { - self.scan_dependency(dependency, VisitedDependent::No); + self.scan_dependency(dependency); true }, ); @@ -415,98 +409,53 @@ where 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) {