servo: Merge #19520 - selectors: Simplify :visited by only using the "is inside link" information (from emilio:simplify-visited); r=jryans

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.

Source-Repo: https://github.com/servo/servo
Source-Revision: 7a8733723551201d2c06acde9b0915b4c03938b4

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 00a655d0bcdf9cec70330c34eea0756b73c38cc3
This commit is contained in:
Emilio Cobos Álvarez 2017-12-08 17:37:32 -06:00
Родитель 8e218cb777
Коммит 63378542f1
10 изменённых файлов: 128 добавлений и 208 удалений

Просмотреть файл

@ -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

Просмотреть файл

@ -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

Просмотреть файл

@ -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,

Просмотреть файл

@ -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 {:?}, 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 =
@ -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,
)
});

Просмотреть файл

@ -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

Просмотреть файл

@ -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::Visited => relevant_link.is_visited(self, context),
NonTSPseudoClass::Link => {
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();

Просмотреть файл

@ -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 ||
relevant_link_found_now != relevant_link_found_then
matched_then != matches_now
}
fn scan_dependency(
@ -464,20 +460,18 @@ where
is_visited_dependent: VisitedDependent,
) {
debug!("TreeStyleInvalidator::scan_dependency({:?}, {:?}, {:?})",
self.element,
dependency,
is_visited_dependent);
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,
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 {

Просмотреть файл

@ -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
// state directly. Instead, we use the `relevant_link` to determine if
// they match.
// For :link and :visited, we don't actually want to test the
// element state directly.
//
// 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 |_, _| {},
)
}

Просмотреть файл

@ -44,7 +44,6 @@ where
struct MatchingResults {
rule_node: StrongRuleNode,
relevant_link_found: bool,
}
/// A style returned from the resolver machinery.
@ -158,15 +157,17 @@ 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_matching_results =
self.match_primary(VisitedHandlingMode::RelevantLinkVisited);
Some(visited_matching_results.rule_node)
} else {
None
};
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)
} else {
None
};
self.cascade_primary_style(
CascadeInputs {
@ -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(

Просмотреть файл

@ -825,15 +825,16 @@ impl Stylist {
where
E: TElement,
{
let cascade_inputs =
self.lazy_pseudo_rules(
guards,
element,
pseudo,
is_probe,
rule_inclusion,
matching_fn
);
let cascade_inputs = 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 =
MatchingContext::new(
MatchingMode::ForStatelessPseudoElement,
None,
None,
self.quirks_mode,
);
let mut matching_context = 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(