selectors: Simplify :visited by only using the "is inside link" information.

Right now we go through a lot of hoops to see if we ever see a relevant link.

However, that information is not needed: if the element is a link, we'll always
need to compute its visited style because its its own relevant link.

If the element inherits from a link, we need to also compute the visited style
anyway.

So the "has a relevant link been found" is pretty useless when we know what are
we inheriting from.

The branches at the beginning of matches_complex_selector_internal were
affecting performance, and there are no good reasons to keep them.

I've verified that this passes all the visited tests in mozilla central, and
that the test-cases too-flaky to be landed still pass.

components/layout_thread/dom_wrapper.rs

@@ -49,7 +49,7 @@ use script_layout_interface::{OpaqueStyleAndLayoutData, StyleData};
 use script_layout_interface::wrapper_traits::{DangerousThreadSafeLayoutNode, GetLayoutData, LayoutNode};
 use script_layout_interface::wrapper_traits::{PseudoElementType, ThreadSafeLayoutElement, ThreadSafeLayoutNode};
 use selectors::attr::{AttrSelectorOperation, NamespaceConstraint, CaseSensitivity};
-use selectors::matching::{ElementSelectorFlags, MatchingContext, QuirksMode, RelevantLinkStatus};
+use selectors::matching::{ElementSelectorFlags, MatchingContext, QuirksMode};
 use selectors::matching::VisitedHandlingMode;
 use selectors::sink::Push;
 use servo_arc::{Arc, ArcBorrow};
@@ -718,7 +718,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
         &self,
         pseudo_class: &NonTSPseudoClass,
         _: &mut MatchingContext<Self::Impl>,
-        _: &RelevantLinkStatus,
+        _: VisitedHandlingMode,
         _: &mut F,
     ) -> bool
     where
@@ -1233,7 +1233,7 @@ impl<'le> ::selectors::Element for ServoThreadSafeLayoutElement<'le> {
         &self,
         _: &NonTSPseudoClass,
         _: &mut MatchingContext<Self::Impl>,
-        _: &RelevantLinkStatus,
+        _: VisitedHandlingMode,
         _: &mut F,
     ) -> bool
     where

components/script/dom/element.rs

@@ -89,7 +89,8 @@ use script_layout_interface::message::ReflowGoal;
 use script_thread::ScriptThread;
 use selectors::Element as SelectorsElement;
 use selectors::attr::{AttrSelectorOperation, NamespaceConstraint, CaseSensitivity};
-use selectors::matching::{ElementSelectorFlags, MatchingContext, RelevantLinkStatus};
+use selectors::context::VisitedHandlingMode;
+use selectors::matching::{ElementSelectorFlags, MatchingContext};
 use selectors::sink::Push;
 use servo_arc::Arc;
 use servo_atoms::Atom;
@@ -2635,7 +2636,7 @@ impl<'a> SelectorsElement for DomRoot<Element> {
         &self,
         pseudo_class: &NonTSPseudoClass,
         _: &mut MatchingContext<Self::Impl>,
-        _: &RelevantLinkStatus,
+        _: VisitedHandlingMode,
         _: &mut F,
     ) -> bool
     where

components/selectors/context.rs

@@ -48,6 +48,26 @@ pub enum VisitedHandlingMode {
     RelevantLinkVisited,
 }
 
+impl VisitedHandlingMode {
+    #[inline]
+    pub fn matches_visited(&self) -> bool {
+        matches!(
+            *self,
+            VisitedHandlingMode::RelevantLinkVisited |
+            VisitedHandlingMode::AllLinksVisitedAndUnvisited
+        )
+    }
+
+    #[inline]
+    pub fn matches_unvisited(&self) -> bool {
+        matches!(
+            *self,
+            VisitedHandlingMode::AllLinksUnvisited |
+            VisitedHandlingMode::AllLinksVisitedAndUnvisited
+        )
+    }
+}
+
 /// Which quirks mode is this document in.
 ///
 /// See: https://quirks.spec.whatwg.org/
@@ -87,12 +107,6 @@ where
     pub nth_index_cache: Option<&'a mut NthIndexCache>,
     /// Input that controls how matching for links is handled.
     pub visited_handling: VisitedHandlingMode,
-    /// Output that records whether we encountered a "relevant link" while
-    /// matching _any_ selector for this element. (This differs from
-    /// `RelevantLinkStatus` which tracks the status for the _current_ selector
-    /// only.)
-    pub relevant_link_found: bool,
-
     /// The element which is going to match :scope pseudo-class. It can be
     /// either one :scope element, or the scoping element.
     ///
@@ -107,6 +121,9 @@ where
     pub scope_element: Option<OpaqueElement>,
 
     /// The current nesting level of selectors that we're matching.
+    ///
+    /// FIXME(emilio): Move this somewhere else and make MatchingContext
+    /// immutable again.
     pub nesting_level: usize,
 
     /// An optional hook function for checking whether a pseudo-element
@@ -152,7 +169,6 @@ where
             visited_handling,
             nth_index_cache,
             quirks_mode,
-            relevant_link_found: false,
             classes_and_ids_case_sensitivity: quirks_mode.classes_and_ids_case_sensitivity(),
             scope_element: None,
             nesting_level: 0,

components/selectors/matching.rs

@@ -60,6 +60,7 @@ impl ElementSelectorFlags {
 struct LocalMatchingContext<'a, 'b: 'a, Impl: SelectorImpl> {
     shared: &'a mut MatchingContext<'b, Impl>,
     matches_hover_and_active_quirk: bool,
+    visited_handling: VisitedHandlingMode,
 }
 
 #[inline(always)]
@@ -109,124 +110,6 @@ fn may_match(hashes: &AncestorHashes, bf: &BloomFilter) -> bool {
     fourth == 0 || bf.might_contain_hash(fourth)
 }
 
-/// Tracks whether we are currently looking for relevant links for a given
-/// complex selector. A "relevant link" is the element being matched if it is a
-/// link or the nearest ancestor link.
-///
-/// `matches_complex_selector` creates a new instance of this for each complex
-/// selector we try to match for an element. This is done because `is_visited`
-/// and `is_unvisited` are based on relevant link state of only the current
-/// complex selector being matched (not the global relevant link status for all
-/// selectors in `MatchingContext`).
-#[derive(Clone, Copy, Debug, Eq, PartialEq)]
-pub enum RelevantLinkStatus {
-    /// Looking for a possible relevant link.  This is the initial mode when
-    /// matching a selector.
-    Looking,
-    /// Not looking for a relevant link.  We transition to this mode if we
-    /// encounter a sibiling combinator (since only ancestor combinators are
-    /// allowed for this purpose).
-    NotLooking,
-    /// Found a relevant link for the element being matched.
-    Found,
-}
-
-impl Default for RelevantLinkStatus {
-    fn default() -> Self {
-        RelevantLinkStatus::NotLooking
-    }
-}
-
-impl RelevantLinkStatus {
-    /// If we found the relevant link for this element, record that in the
-    /// overall matching context for the element as a whole and stop looking for
-    /// addtional links.
-    fn examine_potential_link<E>(
-        &self,
-        element: &E,
-        context: &mut MatchingContext<E::Impl>,
-    ) -> RelevantLinkStatus
-    where
-        E: Element,
-    {
-        // If a relevant link was previously found, we no longer want to look
-        // for links.  Only the nearest ancestor link is considered relevant.
-        if *self != RelevantLinkStatus::Looking {
-            return RelevantLinkStatus::NotLooking
-        }
-
-        if !element.is_link() {
-            return *self
-        }
-
-        // We found a relevant link. Record this in the `MatchingContext`,
-        // where we track whether one was found for _any_ selector (meaning
-        // this field might already be true from a previous selector).
-        context.relevant_link_found = true;
-        // Also return `Found` to update the relevant link status for _this_
-        // specific selector's matching process.
-        RelevantLinkStatus::Found
-    }
-
-    /// Returns whether an element is considered visited for the purposes of
-    /// matching.  This is true only if the element is a link, an relevant link
-    /// exists for the element, and the visited handling mode is set to accept
-    /// relevant links as visited.
-    pub fn is_visited<E>(
-        &self,
-        element: &E,
-        context: &MatchingContext<E::Impl>,
-    ) -> bool
-    where
-        E: Element,
-    {
-        if !element.is_link() {
-            return false
-        }
-
-        if context.visited_handling == VisitedHandlingMode::AllLinksVisitedAndUnvisited {
-            return true;
-        }
-
-        // Non-relevant links are always unvisited.
-        if *self != RelevantLinkStatus::Found {
-            return false
-        }
-
-        context.visited_handling == VisitedHandlingMode::RelevantLinkVisited
-    }
-
-    /// Returns whether an element is considered unvisited for the purposes of
-    /// matching.  Assuming the element is a link, this is always true for
-    /// non-relevant links, since only relevant links can potentially be treated
-    /// as visited.  If this is a relevant link, then is it unvisited if the
-    /// visited handling mode is set to treat all links as unvisted (including
-    /// relevant links).
-    pub fn is_unvisited<E>(
-        &self,
-        element: &E,
-        context: &MatchingContext<E::Impl>
-    ) -> bool
-    where
-        E: Element,
-    {
-        if !element.is_link() {
-            return false
-        }
-
-        if context.visited_handling == VisitedHandlingMode::AllLinksVisitedAndUnvisited {
-            return true;
-        }
-
-        // Non-relevant links are always unvisited.
-        if *self != RelevantLinkStatus::Found {
-            return true
-        }
-
-        context.visited_handling == VisitedHandlingMode::AllLinksUnvisited
-    }
-}
-
 /// A result of selector matching, includes 3 failure types,
 ///
 ///   NotMatchedAndRestartFromClosestLaterSibling
@@ -342,8 +225,10 @@ where
         selector.combinator_at_parse_order(from_offset - 1); // This asserts.
     }
 
+    let visited_handling = context.visited_handling;
     let mut local_context = LocalMatchingContext {
         shared: context,
+        visited_handling,
         matches_hover_and_active_quirk: false,
     };
 
@@ -359,7 +244,6 @@ where
             component,
             element,
             &mut local_context,
-            &RelevantLinkStatus::NotLooking,
             &mut |_, _| {}) {
             return CompoundSelectorMatchingResult::NotMatched;
         }
@@ -417,11 +301,12 @@ where
         }
     }
 
+    let visited_handling = context.visited_handling;
     let result = matches_complex_selector_internal(
         iter,
         element,
         context,
-        &mut RelevantLinkStatus::Looking,
+        visited_handling,
         flags_setter,
         Rightmost::Yes,
     );
@@ -505,6 +390,7 @@ where
             if element.blocks_ancestor_combinators() {
                 return None;
             }
+
             element.parent_element()
         }
         Combinator::PseudoElement => {
@@ -517,7 +403,7 @@ fn matches_complex_selector_internal<E, F>(
     mut selector_iter: SelectorIter<E::Impl>,
     element: &E,
     context: &mut MatchingContext<E::Impl>,
-    relevant_link: &mut RelevantLinkStatus,
+    visited_handling: VisitedHandlingMode,
     flags_setter: &mut F,
     rightmost: Rightmost,
 ) -> SelectorMatchingResult
@@ -525,11 +411,7 @@ where
     E: Element,
     F: FnMut(&E, ElementSelectorFlags),
 {
-    *relevant_link = relevant_link.examine_potential_link(element, context);
+    debug!("Matching complex selector {:?} for {:?}", selector_iter, element);
-
-    debug!("Matching complex selector {:?} for {:?}, relevant link {:?}",
-           selector_iter, element, relevant_link);
-
 
     let matches_all_simple_selectors = {
         let matches_hover_and_active_quirk =
@@ -537,6 +419,7 @@ where
         let mut local_context =
             LocalMatchingContext {
                 shared: context,
+                visited_handling,
                 matches_hover_and_active_quirk,
             };
         selector_iter.all(|simple| {
@@ -544,7 +427,6 @@ where
                 simple,
                 element,
                 &mut local_context,
-                &relevant_link,
                 flags_setter,
             )
         })
@@ -567,9 +449,6 @@ where
     let candidate_not_found = match combinator {
         Combinator::NextSibling |
         Combinator::LaterSibling => {
-            // Only ancestor combinators are allowed while looking for
-            // relevant links, so switch to not looking.
-            *relevant_link = RelevantLinkStatus::NotLooking;
             SelectorMatchingResult::NotMatchedAndRestartFromClosestDescendant
         }
         Combinator::Child |
@@ -581,16 +460,26 @@ where
 
     let mut next_element = next_element_for_combinator(element, combinator);
 
+    // Stop matching :visited as soon as we find a link, or a combinator for
+    // something that isn't an ancestor.
+    let mut visited_handling =
+        if element.is_link() || combinator.is_sibling() {
+            VisitedHandlingMode::AllLinksUnvisited
+        } else {
+            visited_handling
+        };
+
     loop {
         let element = match next_element {
             None => return candidate_not_found,
             Some(next_element) => next_element,
         };
+
         let result = matches_complex_selector_internal(
             selector_iter.clone(),
             &element,
             context,
-            relevant_link,
+            visited_handling,
             flags_setter,
             Rightmost::No,
         );
@@ -627,6 +516,13 @@ where
             _ => {},
         }
 
+        visited_handling =
+            if element.is_link() || combinator.is_sibling() {
+                VisitedHandlingMode::AllLinksUnvisited
+            } else {
+                visited_handling
+            };
+
         next_element = next_element_for_combinator(&element, combinator);
     }
 }
@@ -637,7 +533,6 @@ fn matches_simple_selector<E, F>(
     selector: &Component<E::Impl>,
     element: &E,
     context: &mut LocalMatchingContext<E::Impl>,
-    relevant_link: &RelevantLinkStatus,
     flags_setter: &mut F,
 ) -> bool
 where
@@ -733,7 +628,12 @@ where
                 return false;
             }
 
-            element.match_non_ts_pseudo_class(pc, &mut context.shared, relevant_link, flags_setter)
+            element.match_non_ts_pseudo_class(
+                pc,
+                &mut context.shared,
+                context.visited_handling,
+                flags_setter
+            )
         }
         Component::FirstChild => {
             matches_first_child(element, flags_setter)
@@ -787,7 +687,6 @@ where
                     ss,
                     element,
                     context,
-                    relevant_link,
                     flags_setter,
                 )
             });

components/selectors/tree.rs

@@ -6,7 +6,8 @@
 //! between layout and style.
 
 use attr::{AttrSelectorOperation, NamespaceConstraint, CaseSensitivity};
-use matching::{ElementSelectorFlags, MatchingContext, RelevantLinkStatus};
+use context::VisitedHandlingMode;
+use matching::{ElementSelectorFlags, MatchingContext};
 use parser::SelectorImpl;
 use servo_arc::NonZeroPtrMut;
 use std::fmt::Debug;
@@ -68,7 +69,7 @@ pub trait Element: Sized + Clone + Debug {
         &self,
         pc: &<Self::Impl as SelectorImpl>::NonTSPseudoClass,
         context: &mut MatchingContext<Self::Impl>,
-        relevant_link: &RelevantLinkStatus,
+        visited_handling: VisitedHandlingMode,
         flags_setter: &mut F,
     ) -> bool
     where

components/style/gecko/wrapper.rs

@@ -76,7 +76,7 @@ use selector_parser::{AttrValue, Direction, PseudoClassStringArg};
 use selectors::{Element, OpaqueElement};
 use selectors::attr::{AttrSelectorOperation, AttrSelectorOperator, CaseSensitivity, NamespaceConstraint};
 use selectors::matching::{ElementSelectorFlags, MatchingContext};
-use selectors::matching::{RelevantLinkStatus, VisitedHandlingMode};
+use selectors::matching::VisitedHandlingMode;
 use selectors::sink::Push;
 use servo_arc::{Arc, ArcBorrow, RawOffsetArc};
 use shared_lock::Locked;
@@ -1977,7 +1977,7 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
         &self,
         pseudo_class: &NonTSPseudoClass,
         context: &mut MatchingContext<Self::Impl>,
-        relevant_link: &RelevantLinkStatus,
+        visited_handling: VisitedHandlingMode,
         flags_setter: &mut F,
     ) -> bool
     where
@@ -2037,8 +2037,12 @@ impl<'le> ::selectors::Element for GeckoElement<'le> {
                 self.get_state().intersects(pseudo_class.state_flag())
             },
             NonTSPseudoClass::AnyLink => self.is_link(),
-            NonTSPseudoClass::Link => relevant_link.is_unvisited(self, context),
+            NonTSPseudoClass::Link => {
-            NonTSPseudoClass::Visited => relevant_link.is_visited(self, context),
+                self.is_link() && visited_handling.matches_unvisited()
+            }
+            NonTSPseudoClass::Visited => {
+                self.is_link() && visited_handling.matches_visited()
+            }
             NonTSPseudoClass::MozFirstNode => {
                 flags_setter(self, ElementSelectorFlags::HAS_EDGE_CHILD_SELECTOR);
                 let mut elem = self.as_node();

components/style/invalidation/element/collector.rs

@@ -406,9 +406,8 @@ where
         &mut self,
         visited_handling_mode: VisitedHandlingMode,
         dependency: &Dependency,
-        relevant_link_found: &mut bool,
     ) -> bool {
-        let (matches_now, relevant_link_found_now) = {
+        let matches_now = {
             let mut context = MatchingContext::new_for_visited(
                 MatchingMode::Normal,
                 None,
@@ -426,10 +425,10 @@ where
                 &mut |_, _| {},
             );
 
-            (matches_now, context.relevant_link_found)
+            matches_now
         };
 
-        let (matched_then, relevant_link_found_then) = {
+        let matched_then = {
             let mut context = MatchingContext::new_for_visited(
                 MatchingMode::Normal,
                 None,
@@ -447,15 +446,12 @@ where
                 &mut |_, _| {},
             );
 
-            (matched_then, context.relevant_link_found)
+            matched_then
         };
 
-        *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 ||
+        matched_then != matches_now
-            relevant_link_found_now != relevant_link_found_then
     }
 
     fn scan_dependency(
@@ -466,18 +462,16 @@ where
         debug!("TreeStyleInvalidator::scan_dependency({:?}, {:?}, {:?})",
            self.element,
            dependency,
-               is_visited_dependent);
+           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,
+            VisitedHandlingMode::AllLinksVisitedAndUnvisited,
             dependency,
-            &mut relevant_link_found,
         );
 
         if should_account_for_dependency {
@@ -500,11 +494,12 @@ where
         //
         // 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 {
+        if is_visited_dependent == VisitedDependent::Yes &&
+            self.element.is_link()
+        {
             let should_account_for_dependency = self.check_dependency(
                 VisitedHandlingMode::RelevantLinkVisited,
                 dependency,
-                &mut false,
             );
 
             if should_account_for_dependency {

components/style/invalidation/element/element_wrapper.rs

@@ -11,8 +11,8 @@ use element_state::ElementState;
 use selector_parser::{NonTSPseudoClass, PseudoElement, SelectorImpl, Snapshot, SnapshotMap, AttrValue};
 use selectors::{Element, OpaqueElement};
 use selectors::attr::{AttrSelectorOperation, CaseSensitivity, NamespaceConstraint};
+use selectors::context::VisitedHandlingMode;
 use selectors::matching::{ElementSelectorFlags, MatchingContext};
-use selectors::matching::RelevantLinkStatus;
 use std::cell::Cell;
 use std::fmt;
 
@@ -153,7 +153,7 @@ impl<'a, E> Element for ElementWrapper<'a, E>
         &self,
         pseudo_class: &NonTSPseudoClass,
         context: &mut MatchingContext<Self::Impl>,
-        relevant_link: &RelevantLinkStatus,
+        visited_handling: VisitedHandlingMode,
         _setter: &mut F,
     ) -> bool
     where
@@ -196,14 +196,16 @@ impl<'a, E> Element for ElementWrapper<'a, E>
                 return state.contains(selector_flag);
             }
 
-            // For :link and :visited, we don't actually want to test the element
+            // For :link and :visited, we don't actually want to test the
-            // state directly.  Instead, we use the `relevant_link` to determine if
+            // element state directly.
-            // they match.
+            //
+            // Instead, we use the `visited_handling` to determine if they
+            // match.
             NonTSPseudoClass::Link => {
-                return relevant_link.is_unvisited(self, context);
+                return self.is_link() && visited_handling.matches_unvisited()
             }
             NonTSPseudoClass::Visited => {
-                return relevant_link.is_visited(self, context);
+                return self.is_link() && visited_handling.matches_visited()
             }
 
             #[cfg(feature = "gecko")]
@@ -238,7 +240,7 @@ impl<'a, E> Element for ElementWrapper<'a, E>
             return self.element.match_non_ts_pseudo_class(
                 pseudo_class,
                 context,
-                relevant_link,
+                visited_handling,
                 &mut |_, _| {},
             )
         }
@@ -248,7 +250,7 @@ impl<'a, E> Element for ElementWrapper<'a, E>
                 self.element.match_non_ts_pseudo_class(
                     pseudo_class,
                     context,
-                    relevant_link,
+                    visited_handling,
                     &mut |_, _| {},
                 )
             }

components/style/style_resolver.rs

@@ -44,7 +44,6 @@ where
 
 struct MatchingResults {
     rule_node: StrongRuleNode,
-    relevant_link_found: bool,
 }
 
 /// A style returned from the resolver machinery.
@@ -158,9 +157,11 @@ where
         let primary_results =
             self.match_primary(VisitedHandlingMode::AllLinksUnvisited);
 
-        let relevant_link_found = primary_results.relevant_link_found;
+        let inside_link =
+            parent_style.map_or(false, |s| s.visited_style().is_some());
 
-        let visited_rules = if relevant_link_found {
+        let visited_rules =
+            if inside_link || self.element.is_link() {
                 let visited_matching_results =
                     self.match_primary(VisitedHandlingMode::RelevantLinkVisited);
                 Some(visited_matching_results.rule_node)
@@ -446,7 +447,6 @@ where
         // FIXME(emilio): This is a hack for animations, and should go away.
         self.element.unset_dirty_style_attribute();
 
-        let relevant_link_found = matching_context.relevant_link_found;
         let rule_node = stylist.rule_tree().compute_rule_node(
             &mut applicable_declarations,
             &self.context.shared.guards
@@ -462,7 +462,7 @@ where
             }
         }
 
-        MatchingResults { rule_node, relevant_link_found }
+        MatchingResults { rule_node, }
     }
 
     fn match_pseudo(

components/style/stylist.rs

@@ -825,15 +825,16 @@ impl Stylist {
     where
         E: TElement,
     {
-        let cascade_inputs =
+        let cascade_inputs = self.lazy_pseudo_rules(
-            self.lazy_pseudo_rules(
             guards,
             element,
+            parent_style,
             pseudo,
             is_probe,
             rule_inclusion,
             matching_fn
         );
+
         self.compute_pseudo_element_style_with_inputs(
             &cascade_inputs,
             pseudo,
@@ -981,10 +982,11 @@ impl Stylist {
     ///
     /// See the documentation on lazy pseudo-elements in
     /// docs/components/style.md
-    pub fn lazy_pseudo_rules<E>(
+    fn lazy_pseudo_rules<E>(
         &self,
         guards: &StylesheetGuards,
         element: &E,
+        parent_style: &ComputedValues,
         pseudo: &PseudoElement,
         is_probe: bool,
         rule_inclusion: RuleInclusion,
@@ -1028,13 +1030,13 @@ impl Stylist {
 
         let mut inputs = CascadeInputs::default();
         let mut declarations = ApplicableDeclarationList::new();
-        let mut matching_context =
+        let mut matching_context = MatchingContext::new(
-            MatchingContext::new(
             MatchingMode::ForStatelessPseudoElement,
             None,
             None,
             self.quirks_mode,
         );
+
         matching_context.pseudo_element_matching_fn = matching_fn;
 
         self.push_applicable_declarations(
@@ -1062,7 +1064,7 @@ impl Stylist {
             return inputs;
         }
 
-        if matching_context.relevant_link_found {
+        if parent_style.visited_style().is_some() {
             let mut declarations = ApplicableDeclarationList::new();
             let mut matching_context =
                 MatchingContext::new_for_visited(