style: Account for cousin sharing in the revalidation logic.

Fixes bug 1361013.

components/selectors/parser.rs

@@ -169,27 +169,6 @@ impl<Impl: SelectorImpl> SelectorInner<Impl> {
         }
     }
 
-    /// Creates a clone of this selector with everything to the left of
-    /// (and including) the rightmost ancestor combinator removed. So
-    /// the selector |span foo > bar + baz| will become |bar + baz|.
-    /// This is used for revalidation selectors in servo.
-    ///
-    /// The bloom filter hashes are copied, even though they correspond to
-    /// parts of the selector that have been stripped out, because they are
-    /// still useful for fast-rejecting the reduced selectors.
-    pub fn slice_to_first_ancestor_combinator(&self) -> Self {
-        let maybe_pos = self.complex.iter_raw()
-                            .position(|s| s.as_combinator()
-                                           .map_or(false, |c| c.is_ancestor()));
-        match maybe_pos {
-            None => self.clone(),
-            Some(index) => SelectorInner {
-                complex: self.complex.slice_to(index),
-                ancestor_hashes: self.ancestor_hashes.clone(),
-            },
-        }
-    }
-
     /// Creates a SelectorInner from a Vec of Components. Used in tests.
     pub fn from_vec(vec: Vec<Component<Impl>>) -> Self {
         let complex = ComplexSelector::from_vec(vec);

components/style/stylist.rs

@@ -27,7 +27,8 @@ use selectors::Element;
 use selectors::bloom::BloomFilter;
 use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS};
 use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector};
-use selectors::parser::{Component, Selector, SelectorInner, SelectorMethods, LocalName as LocalNameSelector};
+use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorIter};
+use selectors::parser::{SelectorMethods, LocalName as LocalNameSelector};
 use selectors::visitor::SelectorVisitor;
 use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards};
 use sink::Push;
@@ -375,10 +376,7 @@ impl Stylist {
     #[inline]
     fn note_for_revalidation(&mut self, selector: &Selector<SelectorImpl>) {
         if needs_revalidation(selector) {
-            // For revalidation, we can skip everything left of the first ancestor
+            self.selectors_for_cache_revalidation.push(selector.inner.clone());
-            // combinator.
-            let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator();
-            self.selectors_for_cache_revalidation.push(revalidation_sel);
         }
     }
 
@@ -923,18 +921,47 @@ impl Drop for Stylist {
 /// Visitor determine whether a selector requires cache revalidation.
 ///
 /// Note that we just check simple selectors and eagerly return when the first
-/// need for revalidation is found, so we don't need to store state on the visitor.
+/// need for revalidation is found, so we don't need to store state on the
+/// visitor.
+///
+/// Also, note that it's important to check the whole selector, due to cousins
+/// sharing arbitrarily deep in the DOM, not just the rightmost part of it
+/// (unfortunately, though).
+///
+/// With cousin sharing, we not only need to care about selectors in stuff like
+/// foo:first-child, but also about selectors like p:first-child foo, since the
+/// two parents may have shared style, and in that case we can test cousins
+/// whose matching depends on the selector up in the chain.
+///
+/// TODO(emilio): We can optimize when matching only siblings to only match the
+/// rightmost selector until a descendant combinator is found, I guess, and in
+/// general when we're sharing at depth `n`, to the `n + 1` sequences of
+/// descendant combinators.
+///
+/// I don't think that in presence of the bloom filter it's worth it, though.
 struct RevalidationVisitor;
 
 impl SelectorVisitor for RevalidationVisitor {
     type Impl = SelectorImpl;
 
-    /// Check whether a rightmost sequence of simple selectors containing this
+
-    /// simple selector to be explicitly matched against both the style sharing
+    fn visit_complex_selector(&mut self,
-    /// cache entry and the candidate.
+                              _: SelectorIter<SelectorImpl>,
+                              combinator: Option<Combinator>) -> bool {
+        let is_sibling_combinator =
+            combinator.map_or(false, |c| c.is_sibling());
+
+        !is_sibling_combinator
+    }
+
+
+    /// Check whether sequence of simple selectors containing this simple
+    /// selector to be explicitly matched against both the style sharing cache
+    /// entry and the candidate.
     ///
-    /// We use this for selectors that can have different matching behavior between
+    /// We use this for selectors that can have different matching behavior
-    /// siblings that are otherwise identical as far as the cache is concerned.
+    /// between siblings that are otherwise identical as far as the cache is
+    /// concerned.
     fn visit_simple_selector(&mut self, s: &Component<SelectorImpl>) -> bool {
         match *s {
             Component::AttrExists(_) |
@@ -970,23 +997,7 @@ impl SelectorVisitor for RevalidationVisitor {
 /// Returns true if the given selector needs cache revalidation.
 pub fn needs_revalidation(selector: &Selector<SelectorImpl>) -> bool {
     let mut visitor = RevalidationVisitor;
-
+    !selector.visit(&mut visitor)
-    // We only need to consider the rightmost sequence of simple selectors, so
-    // we can stop at the first combinator. This is because:
-    // * If it's an ancestor combinator, we can ignore everything to the left
-    //   because matching won't differ between siblings.
-    // * If it's a sibling combinator, then we know we need revalidation.
-    let mut iter = selector.inner.complex.iter();
-    for ss in &mut iter {
-        if !ss.visit(&mut visitor) {
-            return true;
-        }
-    }
-
-    // If none of the simple selectors in the rightmost sequence required
-    // revalidation, we need revalidation if and only if the combinator is a
-    // sibling combinator.
-    iter.next_sequence().map_or(false, |c| c.is_sibling())
 }
 
 /// Map that contains the CSS rules for a specific PseudoElement

tests/unit/style/stylist.rs

@@ -109,10 +109,8 @@ fn test_revalidation_selectors() {
         "span + div",
         "span ~ div",
 
-        // Revalidation selectors that will get sliced.
+        // Selectors in the ancestor chain (needed for cousin sharing).
-        "td > h1[dir]",
+        "p:first-child span",
-        "td > span + h1[dir]",
-        "table td > span + div ~ h1[dir]",
     ]).into_iter()
       .filter(|s| needs_revalidation(&s))
       .map(|s| s.inner.slice_to_first_ancestor_combinator().complex)
@@ -147,10 +145,8 @@ fn test_revalidation_selectors() {
         "span + div",
         "span ~ div",
 
-        // Revalidation selectors that got sliced.
+        // Selectors in the ancestor chain (needed for cousin sharing).
-        "h1[dir]",
+        "p:first-child span",
-        "span + h1[dir]",
-        "span + div ~ h1[dir]",
     ]).into_iter()
       .map(|s| s.inner.complex)
       .collect::<Vec<_>>();