Skip to content

Commit

Permalink
Auto merge of #19520 - emilio:simplify-visited, r=jryans
Browse files Browse the repository at this point in the history 
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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19520)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Dec 8, 2017
2 parents fd785f2 + 3119db7 commit 7a87337
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 208 deletions.
6 changes: 3 additions & 3 deletions components/layout_thread/dom_wrapper.rs
View file
Expand Up @@ -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};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1233,7 +1233,7 @@ impl<'le> ::selectors::Element for ServoThreadSafeLayoutElement<'le> {
&self,
_: &NonTSPseudoClass,
_: &mut MatchingContext<Self::Impl>,
_: &RelevantLinkStatus,
_: VisitedHandlingMode,
_: &mut F,
) -> bool
where
Expand Down
5 changes: 3 additions & 2 deletions components/script/dom/element.rs
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -2635,7 +2636,7 @@ impl<'a> SelectorsElement for DomRoot<Element> {
&self,
pseudo_class: &NonTSPseudoClass,
_: &mut MatchingContext<Self::Impl>,
_: &RelevantLinkStatus,
_: VisitedHandlingMode,
_: &mut F,
) -> bool
where
Expand Down
30 changes: 23 additions & 7 deletions components/selectors/context.rs
View file
Expand Up @@ -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/
Expand Down Expand Up @@ -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.
///
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
167 changes: 33 additions & 134 deletions components/selectors/matching.rs
View file
Expand Up @@ -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)]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
};

Expand All @@ -359,7 +244,6 @@ where
component,
element,
&mut local_context,
&RelevantLinkStatus::NotLooking,
&mut |_, _| {}) {
return CompoundSelectorMatchingResult::NotMatched;
}
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -505,6 +390,7 @@ where
if element.blocks_ancestor_combinators() {
return None;
}

element.parent_element()
}
Combinator::PseudoElement => {
Expand All @@ -517,34 +403,30 @@ 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
where
E: Element,
F: FnMut(&E, ElementSelectorFlags),
{
*relevant_link = relevant_link.examine_potential_link(element, context);

debug!("Matching complex selector {:?} for {:?}, relevant link {:?}",
selector_iter, element, relevant_link);

debug!("Matching complex selector {:?} for {:?}", selector_iter, element);

let matches_all_simple_selectors = {
let matches_hover_and_active_quirk =
matches_hover_and_active_quirk(&selector_iter, context, rightmost);
let mut local_context =
LocalMatchingContext {
shared: context,
visited_handling,
matches_hover_and_active_quirk,
};
selector_iter.all(|simple| {
matches_simple_selector(
simple,
element,
&mut local_context,
&relevant_link,
flags_setter,
)
})
Expand All @@ -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 |
Expand All @@ -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,
);
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -787,7 +687,6 @@ where
ss,
element,
context,
relevant_link,
flags_setter,
)
});
Expand Down

0 comments on commit 7a87337

Please sign in to comment.