servo: Merge #17806 - selectors: Don't track class and id ancestor hashes in quirks mode (from emilio:quirks-mode-bloom); r=bholley
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 20 Jul 2017 22:40:15 -0700
changeset 418845 6abb711bd0428505b0ef2d5ab109222201564b91
parent 418844 a1cdfee7cb078b53bbdb06b28039522a7e3da7cb
child 418846 ac12978e5346e24defd9042b721d590d11e31d44
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
milestone56.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 #17806 - selectors: Don't track class and id ancestor hashes in quirks mode (from emilio:quirks-mode-bloom); r=bholley It's incorrect to track classes and id selectors in a quirks-mode document, since they should match case-insensitively. Bug: 1382812 Reviewed-by: bholley MozReview-Commit-ID: 4uvrfYsWb1v Source-Repo: https://github.com/servo/servo Source-Revision: 1e6999b02b3568d29e8397c84852a4a916644cf7
servo/components/selectors/parser.rs
servo/components/style/stylist.rs
servo/tests/unit/style/stylist.rs
--- a/servo/components/selectors/parser.rs
+++ b/servo/components/selectors/parser.rs
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use attr::{AttrSelectorWithNamespace, ParsedAttrSelectorOperation, AttrSelectorOperator};
 use attr::{ParsedCaseSensitivity, SELECTOR_WHITESPACE, NamespaceConstraint};
 use bloom::BLOOM_HASH_MASK;
 use builder::{SelectorBuilder, SpecificityAndFlags};
+use context::QuirksMode;
 use cssparser::{ParseError, BasicParseError, CompactCowStr};
 use cssparser::{Token, Parser as CssParser, parse_nth, ToCss, serialize_identifier, CssStringWriter};
 use precomputed_hash::PrecomputedHash;
 use servo_arc::ThinArc;
 use sink::Push;
 use smallvec::SmallVec;
 use std::ascii::AsciiExt;
 use std::borrow::{Borrow, Cow};
@@ -213,27 +214,31 @@ impl<Impl: SelectorImpl> SelectorList<Im
 /// complicated to assemble, because we often bail out before checking all the
 /// hashes.
 #[derive(Eq, PartialEq, Clone, Debug)]
 pub struct AncestorHashes {
     pub packed_hashes: [u32; 3],
 }
 
 impl AncestorHashes {
-    pub fn new<Impl: SelectorImpl>(s: &Selector<Impl>) -> Self {
-        Self::from_iter(s.iter())
+    pub fn new<Impl: SelectorImpl>(
+        selector: &Selector<Impl>,
+        quirks_mode: QuirksMode,
+    ) -> Self {
+        Self::from_iter(selector.iter(), quirks_mode)
     }
 
-    pub fn from_iter<Impl: SelectorImpl>(iter: SelectorIter<Impl>) -> Self {
+    fn from_iter<Impl: SelectorImpl>(
+        iter: SelectorIter<Impl>,
+        quirks_mode: QuirksMode,
+    ) -> Self {
         // Compute ancestor hashes for the bloom filter.
         let mut hashes = [0u32; 4];
         let mut hash_iter = AncestorIter::new(iter)
-                             .map(|x| x.ancestor_hash())
-                             .filter(|x| x.is_some())
-                             .map(|x| x.unwrap());
+                             .filter_map(|x| x.ancestor_hash(quirks_mode));
         for i in 0..4 {
             hashes[i] = match hash_iter.next() {
                 Some(x) => x & BLOOM_HASH_MASK,
                 None => break,
             }
         }
 
         // Now, pack the fourth hash (if it exists) into the upper byte of each of
@@ -527,17 +532,17 @@ impl<'a, Impl: SelectorImpl> fmt::Debug 
         for component in iter {
             component.to_css(f)?
         }
         Ok(())
     }
 }
 
 /// An iterator over all simple selectors belonging to ancestors.
-pub struct AncestorIter<'a, Impl: 'a + SelectorImpl>(SelectorIter<'a, Impl>);
+struct AncestorIter<'a, Impl: 'a + SelectorImpl>(SelectorIter<'a, Impl>);
 impl<'a, Impl: 'a + SelectorImpl> AncestorIter<'a, Impl> {
     /// Creates an AncestorIter. The passed-in iterator is assumed to point to
     /// the beginning of the child sequence, which will be skipped.
     fn new(inner: SelectorIter<'a, Impl>) -> Self {
         let mut result = AncestorIter(inner);
         result.skip_until_ancestor();
         result
     }
@@ -667,36 +672,38 @@ pub enum Component<Impl: SelectorImpl> {
     LastOfType,
     OnlyOfType,
     NonTSPseudoClass(Impl::NonTSPseudoClass),
     PseudoElement(Impl::PseudoElement),
 }
 
 impl<Impl: SelectorImpl> Component<Impl> {
     /// Compute the ancestor hash to check against the bloom filter.
-    pub fn ancestor_hash(&self) -> Option<u32> {
+    fn ancestor_hash(&self, quirks_mode: QuirksMode) -> Option<u32> {
         match *self {
             Component::LocalName(LocalName { ref name, ref lower_name }) => {
-                // Only insert the local-name into the filter if it's all lowercase.
-                // Otherwise we would need to test both hashes, and our data structures
-                // aren't really set up for that.
+                // Only insert the local-name into the filter if it's all
+                // lowercase.  Otherwise we would need to test both hashes, and
+                // our data structures aren't really set up for that.
                 if name == lower_name {
                     Some(name.precomputed_hash())
                 } else {
                     None
                 }
             },
             Component::DefaultNamespace(ref url) |
             Component::Namespace(_, ref url) => {
                 Some(url.precomputed_hash())
             },
-            Component::ID(ref id) => {
+            // In quirks mode, class and id selectors should match
+            // case-insensitively, so just avoid inserting them into the filter.
+            Component::ID(ref id) if quirks_mode != QuirksMode::Quirks => {
                 Some(id.precomputed_hash())
             },
-            Component::Class(ref class) => {
+            Component::Class(ref class) if quirks_mode != QuirksMode::Quirks => {
                 Some(class.precomputed_hash())
             },
             _ => None,
         }
     }
 
     /// Returns true if this is a combinator.
     pub fn is_combinator(&self) -> bool {
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -485,17 +485,19 @@ impl Stylist {
                             self.pseudos_map
                                 .entry(pseudo.canonical())
                                 .or_insert_with(PerPseudoElementSelectorMap::new)
                                 .borrow_for_origin(&origin)
                         } else {
                             self.element_map.borrow_for_origin(&origin)
                         };
 
-                        let hashes = AncestorHashes::new(&selector);
+                        let hashes =
+                            AncestorHashes::new(&selector, self.quirks_mode);
+
                         map.insert(
                             Rule::new(selector.clone(),
                                       hashes.clone(),
                                       locked.clone(),
                                       self.rules_source_order),
                             self.quirks_mode);
 
                         self.invalidation_map.note_selector(selector, self.quirks_mode);
--- a/servo/tests/unit/style/stylist.rs
+++ b/servo/tests/unit/style/stylist.rs
@@ -41,17 +41,17 @@ fn get_mock_rules(css_selectors: &[&str]
                 line: 0,
                 column: 0,
             },
         }));
 
         let guard = shared_lock.read();
         let rule = locked.read_with(&guard);
         rule.selectors.0.iter().map(|s| {
-            Rule::new(s.clone(), AncestorHashes::new(s), locked.clone(), i as u32)
+            Rule::new(s.clone(), AncestorHashes::new(s, QuirksMode::NoQuirks), locked.clone(), i as u32)
         }).collect()
     }).collect(), shared_lock)
 }
 
 fn get_mock_map(selectors: &[&str]) -> (SelectorMap<Rule>, SharedRwLock) {
     let mut map = SelectorMap::<Rule>::new();
     let (selector_rules, shared_lock) = get_mock_rules(selectors);