servo: Merge #17225 - Fix revalidation selectors when pseudo-elements are involved (from bzbarsky:pseudo-sharing-fixage); r=emilio
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 08 Jun 2017 07:21:01 -0700
changeset 413499 ede89b73c7dbc61a8a34a2b5cfd3a60024f84f42
parent 413498 b86fec2a2ed8f2f5ca5f5f71f794decba6e15b74
child 413500 69a0d95d19bbee728d6e255bb63706da1fd36ce8
push id1490
push usermtabara@mozilla.com
push dateMon, 31 Jul 2017 14:08:16 +0000
treeherdermozilla-release@70e32e6bf15e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #17225 - Fix revalidation selectors when pseudo-elements are involved (from bzbarsky:pseudo-sharing-fixage); r=emilio <!-- Please describe your changes on the following line: --> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix https://bugzilla.mozilla.org/show_bug.cgi?id=1371112 <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because I am adding a Gecko test for them <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 3616b8f0c3c7c1beba4d0977ea3a4fb2a5f9c835
servo/components/selectors/parser.rs
servo/components/style/sharing/mod.rs
servo/components/style/stylist.rs
--- a/servo/components/selectors/parser.rs
+++ b/servo/components/selectors/parser.rs
@@ -474,16 +474,19 @@ impl<'a, Impl: 'a + SelectorImpl> Ancest
         result
     }
 
     /// Skips a sequence of simple selectors and all subsequent sequences until
     /// a non-pseudo-element ancestor combinator is reached.
     fn skip_until_ancestor(&mut self) {
         loop {
             while self.0.next().is_some() {}
+            // If this is ever changed to stop at the "pseudo-element"
+            // combinator, we will need to fix the way we compute hashes for
+            // revalidation selectors.
             if self.0.next_sequence().map_or(true, |x| matches!(x, Combinator::Child | Combinator::Descendant)) {
                 break;
             }
         }
     }
 }
 
 impl<'a, Impl: SelectorImpl> Iterator for AncestorIter<'a, Impl> {
@@ -510,17 +513,19 @@ impl<'a, Impl: SelectorImpl> Iterator fo
 pub enum Combinator {
     Child,  //  >
     Descendant,  // space
     NextSibling,  // +
     LaterSibling,  // ~
     /// A dummy combinator we use to the left of pseudo-elements.
     ///
     /// It serializes as the empty string, and acts effectively as a child
-    /// combinator.
+    /// combinator in most cases.  If we ever actually start using a child
+    /// combinator for this, we will need to fix up the way hashes are computed
+    /// for revalidation selectors.
     PseudoElement,
 }
 
 impl Combinator {
     /// Returns true if this combinator is a child or descendant combinator.
     pub fn is_ancestor(&self) -> bool {
         matches!(*self, Combinator::Child |
                         Combinator::Descendant |
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -56,20 +56,20 @@
 //!
 //! It's very important that a selector be added to the set of revalidation
 //! selectors any time there are two elements that could pass all the up-front
 //! checks but match differently against some ComplexSelector in the selector.
 //! If that happens, then they can have descendants that might themselves pass
 //! the up-front checks but would have different matching results for the
 //! selector in question.  In this case, "descendants" includes pseudo-elements,
 //! so there is a single selector map of revalidation selectors that includes
-//! both selectors targeting element and selectors targeting pseudo-elements.
-//! This relies on matching an element against a pseudo-element-targeting
-//! selector being a sensible operation that will effectively check whether that
-//! element is a matching originating element for the selector.
+//! both selectors targeting elements and selectors targeting pseudo-element
+//! originating elements.  We ensure that the pseudo-element parts of all these
+//! selectors are effectively stripped off, so that matching them all against
+//! elements makes sense.
 
 use Atom;
 use bit_vec::BitVec;
 use bloom::StyleBloom;
 use cache::{LRUCache, LRUCacheMutIterator};
 use context::{SelectorFlagsMap, SharedStyleContext, StyleContext};
 use data::{ComputedStyle, ElementData, ElementStyles};
 use dom::{TElement, SendElement};
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -155,17 +155,17 @@ pub struct Stylist {
     /// FIXME(bz): This doesn't really need to be a counting Blooom filter.
     #[cfg_attr(feature = "servo", ignore_heap_size_of = "just an array")]
     mapped_ids: BloomFilter,
 
     /// Selectors that require explicit cache revalidation (i.e. which depend
     /// on state that is not otherwise visible to the cache, like attributes or
     /// tree-structural state like child index and pseudos).
     #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
-    selectors_for_cache_revalidation: SelectorMap<SelectorAndHashes<SelectorImpl>>,
+    selectors_for_cache_revalidation: SelectorMap<RevalidationSelectorAndHashes>,
 
     /// The total number of selectors.
     num_selectors: usize,
 
     /// The total number of declarations.
     num_declarations: usize,
 
     /// The total number of times the stylist has been rebuilt.
@@ -469,17 +469,18 @@ impl Stylist {
 
                         map.insert(Rule::new(selector_and_hashes.selector.clone(),
                                              selector_and_hashes.hashes.clone(),
                                              locked.clone(),
                                              self.rules_source_order));
 
                         self.dependencies.note_selector(selector_and_hashes);
                         if needs_revalidation(&selector_and_hashes.selector) {
-                            self.selectors_for_cache_revalidation.insert(selector_and_hashes.clone());
+                            self.selectors_for_cache_revalidation.insert(
+                                RevalidationSelectorAndHashes::new(&selector_and_hashes));
                         }
                         selector_and_hashes.selector.visit(&mut AttributeAndStateDependencyVisitor {
                             attribute_dependencies: &mut self.attribute_dependencies,
                             style_attribute_dependency: &mut self.style_attribute_dependency,
                             state_dependencies: &mut self.state_dependencies,
                         });
                         selector_and_hashes.selector.visit(&mut MappedIdVisitor {
                             mapped_ids: &mut self.mapped_ids,
@@ -1160,17 +1161,17 @@ impl Stylist {
         // Note that, by the time we're revalidating, we're guaranteed that the
         // candidate and the entry have the same id, classes, and local name.
         // This means we're guaranteed to get the same rulehash buckets for all
         // the lookups, which means that the bitvecs are comparable. We verify
         // this in the caller by asserting that the bitvecs are same-length.
         let mut results = BitVec::new();
         self.selectors_for_cache_revalidation.lookup(*element, &mut |selector_and_hashes| {
             results.push(matches_selector(&selector_and_hashes.selector,
-                                          0,
+                                          selector_and_hashes.selector_offset,
                                           &selector_and_hashes.hashes,
                                           element,
                                           &mut matching_context,
                                           flags_setter));
             true
         });
 
         results
@@ -1314,17 +1315,62 @@ impl<'a> SelectorVisitor for MappedIdVis
                               combinator: Option<Combinator>) -> bool {
         match combinator {
             None | Some(Combinator::PseudoElement) => true,
             _ => false,
         }
     }
 }
 
-/// Visitor determine whether a selector requires cache revalidation.
+/// SelectorMapEntry implementation for use in our revalidation selector map.
+#[derive(Clone, Debug)]
+struct RevalidationSelectorAndHashes {
+    selector: Selector<SelectorImpl>,
+    selector_offset: usize,
+    hashes: AncestorHashes,
+}
+
+impl RevalidationSelectorAndHashes {
+    fn new(selector_and_hashes: &SelectorAndHashes<SelectorImpl>) -> Self {
+        // We basically want to check whether the first combinator is a
+        // pseudo-element combinator.  If it is, we want to use the offset one
+        // past it.  Otherwise, our offset is 0.
+        let mut index = 0;
+        let mut iter = selector_and_hashes.selector.iter();
+
+        // First skip over the first ComplexSelector.  We can't check what sort
+        // of combinator we have until we do that.
+        for _ in &mut iter {
+            index += 1; // Simple selector
+        }
+
+        let offset = match iter.next_sequence() {
+            Some(Combinator::PseudoElement) => index + 1, // +1 for the combinator
+            _ => 0
+        };
+
+        RevalidationSelectorAndHashes {
+            selector: selector_and_hashes.selector.clone(),
+            selector_offset: offset,
+            hashes: selector_and_hashes.hashes.clone(),
+        }
+    }
+}
+
+impl SelectorMapEntry for RevalidationSelectorAndHashes {
+    fn selector(&self) -> SelectorIter<SelectorImpl> {
+        self.selector.iter_from(self.selector_offset)
+    }
+
+    fn hashes(&self) -> &AncestorHashes {
+        &self.hashes
+    }
+}
+
+/// Visitor to 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.
 ///
 /// 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).