Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-08-11 08:25:32 +01:00
Code Issues Projects Releases Packages Wiki Activity

Bug 1360399: Don't deduplicate revalidation selectors. r=bholley

Browse source 
It's unfortunate, but it's a correctness issue. I was looking at the
expectations update here:

 * https://hg.mozilla.org/integration/autoland/rev/659cddddd434

And investigating it I realised that it's wrong to coalesce selectors like that,
because we keep the bloom filter flags.

So in the test cases disabled, we have a selector that looks like this:

msub > :not(:first-child),
msup > :not(:first-child),
msubsup > :not(:first-child),
mmultiscripts > :not(:first-child) {
    -moz-script-level: +1;
    -moz-math-display: inline;
}

And an element that looks like this:

<msubsup><mi></mi><mi></mi></msubsup>

We're only inserting the first selector msub > :not(:first-child) into the set,
so when we're going to match the <mi> elements we fast-reject it in both cases
due to the bloom filter, so they share style.

I can't see an easy way to fix this keeping the deduplication. If we keep it, we
need to remove the bloom filter optimization, which means that we'd trash the
cache for every first-child in the document (the :not(:first-child) effectively
becomes a global rule).

MozReview-Commit-ID: 9VPkmdj9zDg
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
This commit is contained in:
Emilio Cobos Álvarez 2017-04-28 02:15:19 +02:00
parent 04aac0247a
commit 7a556a7f03
No known key found for this signature in database
GPG key ID: 056B727BB9C1027C
3 changed files with 17 additions and 23 deletions
Download patch file Download diff file Expand all files Collapse all files

2
components/style/matching.rs
View file

@ -199,6 +199,8 @@ fn element_matches_candidate<E: TElement>(element: &E,
debug_assert!(data.has_current_styles());
let current_styles = data.styles();
debug!("Sharing style between {:?} and {:?}", element, candidate_element);
Ok(current_styles.primary.clone())
}

28
components/style/stylist.rs
View file

@ -336,20 +336,12 @@ impl Stylist {
self.dependencies.note_selector(selector);
if needs_revalidation(selector) {
// For revalidation, we can skip everything left of the first ancestor
// combinator.
let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator();
// For revalidation, we can skip everything left of
// the first ancestor combinator.
let revalidation_sel =
selector.inner.slice_to_first_ancestor_combinator();
// Because of the slicing we do above, we can often end up with
// adjacent duplicate selectors when we have selectors like
// body > foo, td > foo, th > foo, etc. Doing a check for
// adjacent duplicates here reduces the number of revalidation
// selectors for Gecko's UA sheet by 30%.
let duplicate = self.selectors_for_cache_revalidation.last()
.map_or(false, |x| x.complex == revalidation_sel.complex);
if !duplicate {
self.selectors_for_cache_revalidation.push(revalidation_sel);
}
self.selectors_for_cache_revalidation.push(revalidation_sel);
}
}
}
@ -844,7 +836,7 @@ impl Stylist {
flags_setter: &mut F)
-> BitVec
where E: TElement,
F: FnMut(&E, ElementSelectorFlags)
F: FnMut(&E, ElementSelectorFlags),
{
use selectors::matching::StyleRelations;
use selectors::matching::matches_selector;
@ -957,8 +949,8 @@ impl SelectorVisitor for RevalidationVisitor {
Component::OnlyOfType => {
false
},
Component::NonTSPseudoClass(ref p) if p.needs_cache_revalidation() => {
false
Component::NonTSPseudoClass(ref p) => {
!p.needs_cache_revalidation()
},
_ => {
true
@ -984,8 +976,8 @@ pub fn needs_revalidation(selector: &Selector<SelectorImpl>) -> bool {
}
// If none of the simple selectors in the rightmost sequence required
// revalidaiton, we need revalidation if and only if the combinator is
// a sibling combinator.
// revalidation, we need revalidation if and only if the combinator is a
// sibling combinator.
iter.next_sequence().map_or(false, |c| c.is_sibling())
}