Auto merge of #16813 - bholley:importance_handling, r=emilio

Handle importance when inserting into the rule tree

https://bugzilla.mozilla.org/show_bug.cgi?id=1358635

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16813)
<!-- Reviewable:end -->

components/style/matching.rs

@@ -26,6 +26,7 @@ use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl};
 use selectors::bloom::BloomFilter;
 use selectors::matching::{ElementSelectorFlags, StyleRelations};
 use selectors::matching::AFFECTED_BY_PSEUDO_ELEMENTS;
+use shared_lock::StylesheetGuards;
 use sink::ForgetfulSink;
 use stylearc::Arc;
 use stylist::ApplicableDeclarationBlock;
@@ -858,11 +859,12 @@ trait PrivateMatchMethods: TElement {
 }
 
 fn compute_rule_node<E: TElement>(rule_tree: &RuleTree,
-                                  applicable_declarations: &mut Vec<ApplicableDeclarationBlock>)
+                                  applicable_declarations: &mut Vec<ApplicableDeclarationBlock>,
+                                  guards: &StylesheetGuards)
                                   -> StrongRuleNode
 {
     let rules = applicable_declarations.drain(..).map(|d| (d.source, d.level));
-    let rule_node = rule_tree.insert_ordered_rules(rules);
+    let rule_node = rule_tree.insert_ordered_rules_with_important(rules, guards);
     rule_node
 }
 
@@ -1007,12 +1009,13 @@ pub trait MatchMethods : TElement {
                                                           smil_override,
                                                           animation_rules,
                                                           implemented_pseudo.as_ref(),
-                                                          &context.shared.guards,
                                                           &mut applicable_declarations,
                                                           &mut set_selector_flags);
 
         let primary_rule_node =
-            compute_rule_node::<Self>(&stylist.rule_tree, &mut applicable_declarations);
+            compute_rule_node::<Self>(&stylist.rule_tree,
+                                      &mut applicable_declarations,
+                                      &context.shared.guards);
 
         return data.set_primary_rules(primary_rule_node);
     }
@@ -1061,13 +1064,14 @@ pub trait MatchMethods : TElement {
                                                  None,
                                                  AnimationRules(None, None),
                                                  Some(&pseudo),
-                                                 &guards,
                                                  &mut applicable_declarations,
                                                  &mut set_selector_flags);
 
             if !applicable_declarations.is_empty() {
                 let new_rules =
-                    compute_rule_node::<Self>(rule_tree, &mut applicable_declarations);
+                    compute_rule_node::<Self>(rule_tree,
+                                              &mut applicable_declarations,
+                                              &guards);
                 if pseudos.has(&pseudo) {
                     rule_nodes_changed = pseudos.set_rules(&pseudo, new_rules);
                 } else {

components/style/rule_tree/mod.rs

@@ -10,6 +10,7 @@
 use heapsize::HeapSizeOf;
 use properties::{Importance, PropertyDeclarationBlock};
 use shared_lock::{Locked, StylesheetGuards, SharedRwLockReadGuard};
+use smallvec::SmallVec;
 use std::io::{self, Write};
 use std::ptr;
 use std::sync::atomic::{AtomicPtr, AtomicUsize, Ordering};
@@ -124,6 +125,96 @@ impl RuleTree {
         self.dump(guards, &mut stdout);
     }
 
+    /// Inserts the given rules, that must be in proper order by specifity, and
+    /// returns the corresponding rule node representing the last inserted one.
+    ///
+    /// !important rules are detected and inserted into the appropriate position
+    /// in the rule tree. This allows selector matching to ignore importance,
+    /// while still maintaining the appropriate cascade order in the rule tree.
+    pub fn insert_ordered_rules_with_important<'a, I>(&self,
+                                                      iter: I,
+                                                      guards: &StylesheetGuards)
+                                                      -> StrongRuleNode
+        where I: Iterator<Item=(StyleSource, CascadeLevel)>,
+    {
+        use self::CascadeLevel::*;
+        let mut current = self.root.clone();
+        let mut last_level = current.get().level;
+
+        let mut found_important = false;
+        let mut important_style_attr = None;
+        let mut important_author = SmallVec::<[StyleSource; 4]>::new();
+        let mut important_user = SmallVec::<[StyleSource; 4]>::new();
+        let mut important_ua = SmallVec::<[StyleSource; 4]>::new();
+        let mut transition = None;
+
+        for (source, level) in iter {
+            debug_assert!(last_level <= level, "Not really ordered");
+            debug_assert!(!level.is_important(), "Important levels handled internally");
+            let (any_normal, any_important) = {
+                let pdb = source.read(level.guard(guards));
+                (pdb.any_normal(), pdb.any_important())
+            };
+            if any_important {
+                found_important = true;
+                match level {
+                    AuthorNormal => important_author.push(source.clone()),
+                    UANormal => important_ua.push(source.clone()),
+                    UserNormal => important_user.push(source.clone()),
+                    StyleAttributeNormal => {
+                        debug_assert!(important_style_attr.is_none());
+                        important_style_attr = Some(source.clone());
+                    },
+                    _ => {},
+                };
+            }
+            if any_normal {
+                if matches!(level, Transitions) && found_important {
+                    // There can be at most one transition, and it will come at
+                    // the end of the iterator. Stash it and apply it after
+                    // !important rules.
+                    debug_assert!(transition.is_none());
+                    transition = Some(source);
+                } else {
+                    current = current.ensure_child(self.root.downgrade(), source, level);
+                }
+            }
+            last_level = level;
+        }
+
+        // Early-return in the common case of no !important declarations.
+        if !found_important {
+            return current;
+        }
+
+        //
+        // Insert important declarations, in order of increasing importance,
+        // followed by any transition rule.
+        //
+
+        for source in important_author.into_iter() {
+            current = current.ensure_child(self.root.downgrade(), source, AuthorImportant);
+        }
+
+        if let Some(source) = important_style_attr {
+            current = current.ensure_child(self.root.downgrade(), source, StyleAttributeImportant);
+        }
+
+        for source in important_user.into_iter() {
+            current = current.ensure_child(self.root.downgrade(), source, UserImportant);
+        }
+
+        for source in important_ua.into_iter() {
+            current = current.ensure_child(self.root.downgrade(), source, UAImportant);
+        }
+
+        if let Some(source) = transition {
+            current = current.ensure_child(self.root.downgrade(), source, Transitions);
+        }
+
+        current
+    }
+
     /// Insert the given rules, that must be in proper order by specifity, and
     /// return the corresponding rule node representing the last inserted one.
     pub fn insert_ordered_rules<'a, I>(&self, iter: I) -> StrongRuleNode

components/style/stylist.rs

@@ -327,10 +327,7 @@ impl Stylist {
 
         SelectorImpl::each_precomputed_pseudo_element(|pseudo| {
             if let Some(map) = self.pseudos_map.remove(&pseudo) {
-                let declarations =
+                let declarations = map.user_agent.get_universal_rules(CascadeLevel::UANormal);
-                    map.user_agent.get_universal_rules(
-                        guards.ua_or_user, CascadeLevel::UANormal, CascadeLevel::UAImportant
-                    );
                 self.precomputed_pseudo_element_decls.insert(pseudo, declarations);
             }
         });
@@ -376,7 +373,7 @@ impl Stylist {
                     self.num_declarations += style_rule.block.read_with(&guard).len();
                     for selector in &style_rule.selectors.0 {
                         self.num_selectors += 1;
-                        self.add_rule_to_map(guard, selector, locked, stylesheet);
+                        self.add_rule_to_map(selector, locked, stylesheet);
                         self.dependencies.note_selector(selector);
                         self.note_for_revalidation(selector);
                     }
@@ -412,7 +409,6 @@ impl Stylist {
 
     #[inline]
     fn add_rule_to_map(&mut self,
-                       guard: &SharedRwLockReadGuard,
                        selector: &Selector<SelectorImpl>,
                        rule: &Arc<Locked<StyleRule>>,
                        stylesheet: &Stylesheet)
@@ -426,8 +422,7 @@ impl Stylist {
             self.element_map.borrow_for_origin(&stylesheet.origin)
         };
 
-        map.insert(Rule::new(guard,
+        map.insert(Rule::new(selector.inner.clone(),
-                             selector.inner.clone(),
                              rule.clone(),
                              self.rules_source_order,
                              selector.specificity));
@@ -460,8 +455,9 @@ impl Stylist {
             Some(declarations) => {
                 // FIXME(emilio): When we've taken rid of the cascade we can just
                 // use into_iter.
-                self.rule_tree.insert_ordered_rules(
+                self.rule_tree.insert_ordered_rules_with_important(
-                    declarations.into_iter().map(|a| (a.source.clone(), a.level)))
+                    declarations.into_iter().map(|a| (a.source.clone(), a.level)),
+                    guards)
             }
             None => self.rule_tree.root(),
         };
@@ -622,17 +618,21 @@ impl Stylist {
                                           None,
                                           AnimationRules(None, None),
                                           Some(pseudo),
-                                          guards,
                                           &mut declarations,
                                           &mut set_selector_flags);
         if declarations.is_empty() {
             return None
         }
 
-        let rule_node = self.rule_tree.insert_ordered_rules(declarations.into_iter().map(|a| {
+        let rule_node =
-            (a.source, a.level)
+            self.rule_tree.insert_ordered_rules_with_important(
-        }));
+                declarations.into_iter().map(|a| (a.source, a.level)),
-        Some(rule_node)
+                guards);
+        if rule_node == self.rule_tree.root() {
+            None
+        } else {
+            Some(rule_node)
+        }
     }
 
     /// Set a given device, which may change the styles that apply to the
@@ -740,7 +740,6 @@ impl Stylist {
                                         smil_override: Option<&Arc<Locked<PropertyDeclarationBlock>>>,
                                         animation_rules: AnimationRules,
                                         pseudo_element: Option<&PseudoElement>,
-                                        guards: &StylesheetGuards,
                                         applicable_declarations: &mut V,
                                         flags_setter: &mut F)
                                         -> StyleRelations
@@ -810,13 +809,11 @@ impl Stylist {
 
             // Step 4: Normal style attributes.
             if let Some(sa) = style_attribute {
-                if sa.read_with(guards.author).any_normal() {
+                relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
-                    relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
+                Push::push(
-                    Push::push(
+                    applicable_declarations,
-                        applicable_declarations,
+                    ApplicableDeclarationBlock::from_declarations(sa.clone(),
-                        ApplicableDeclarationBlock::from_declarations(sa.clone(),
+                                                                  CascadeLevel::StyleAttributeNormal));
-                                                                      CascadeLevel::StyleAttributeNormal));
-                }
             }
 
             debug!("style attr: {:?}", relations);
@@ -841,52 +838,14 @@ impl Stylist {
                                                                   CascadeLevel::Animations));
             }
             debug!("animation: {:?}", relations);
-
-            // Step 7: Author-supplied `!important` rules.
-            map.author.get_all_matching_rules(element,
-                                              parent_bf,
-                                              applicable_declarations,
-                                              &mut relations,
-                                              flags_setter,
-                                              CascadeLevel::AuthorImportant);
-
-            debug!("author important: {:?}", relations);
-
-            // Step 8: `!important` style attributes.
-            if let Some(sa) = style_attribute {
-                if sa.read_with(guards.author).any_important() {
-                    relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
-                    Push::push(
-                        applicable_declarations,
-                        ApplicableDeclarationBlock::from_declarations(sa.clone(),
-                                                                      CascadeLevel::StyleAttributeImportant));
-                }
-            }
-
-            debug!("style attr important: {:?}", relations);
-
-            // Step 9: User `!important` rules.
-            map.user.get_all_matching_rules(element,
-                                            parent_bf,
-                                            applicable_declarations,
-                                            &mut relations,
-                                            flags_setter,
-                                            CascadeLevel::UserImportant);
-
-            debug!("user important: {:?}", relations);
         } else {
             debug!("skipping non-agent rules");
         }
 
-        // Step 10: UA `!important` rules.
+        //
-        map.user_agent.get_all_matching_rules(element,
+        // Steps 7-10 correspond to !important rules, and are handled during
-                                              parent_bf,
+        // rule tree insertion.
-                                              applicable_declarations,
+        //
-                                              &mut relations,
-                                              flags_setter,
-                                              CascadeLevel::UAImportant);
-
-        debug!("UA important: {:?}", relations);
 
         // Step 11: Transitions.
         // The transitions sheet (CSS transitions that are tied to CSS markup)
@@ -1256,45 +1215,24 @@ impl SelectorMap<Rule> {
     /// Append to `rule_list` all universal Rules (rules with selector `*|*`) in
     /// `self` sorted by specificity and source order.
     pub fn get_universal_rules(&self,
-                               guard: &SharedRwLockReadGuard,
+                               cascade_level: CascadeLevel)
-                               cascade_level: CascadeLevel,
-                               important_cascade_level: CascadeLevel)
                                -> Vec<ApplicableDeclarationBlock> {
         debug_assert!(!cascade_level.is_important());
-        debug_assert!(important_cascade_level.is_important());
         if self.is_empty() {
             return vec![];
         }
 
-        let mut matching_rules_list = vec![];
+        let mut rules_list = vec![];
-
-        // We need to insert important rules _after_ normal rules for this to be
-        // correct, and also to not trigger rule tree assertions.
-        let mut important = vec![];
         for rule in self.other.iter() {
             if rule.selector.complex.iter_raw().next().is_none() {
-                let style_rule = rule.style_rule.read_with(guard);
+                rules_list.push(rule.to_applicable_declaration_block(cascade_level));
-                let block = style_rule.block.read_with(guard);
-                if block.any_normal() {
-                    matching_rules_list.push(
-                        rule.to_applicable_declaration_block(cascade_level));
-                }
-                if block.any_important() {
-                    important.push(
-                        rule.to_applicable_declaration_block(important_cascade_level));
-                }
             }
         }
 
-        let normal_len = matching_rules_list.len();
+        sort_by_key(&mut rules_list,
-        matching_rules_list.extend(important.into_iter());
-
-        sort_by_key(&mut matching_rules_list[0..normal_len],
-                    |block| (block.specificity, block.source_order));
-        sort_by_key(&mut matching_rules_list[normal_len..],
                     |block| (block.specificity, block.source_order));
 
-        matching_rules_list
+        rules_list
     }
 
     fn get_matching_rules_from_hash<E, Str, BorrowedStr: ?Sized, Vector, F>(
@@ -1336,13 +1274,7 @@ impl SelectorMap<Rule> {
               F: FnMut(&E, ElementSelectorFlags),
     {
         for rule in rules.iter() {
-            let any_declaration_for_importance = if cascade_level.is_important() {
+            if matches_selector(&rule.selector, element, parent_bf,
-                rule.any_important_declarations()
-            } else {
-                rule.any_normal_declarations()
-            };
-            if any_declaration_for_importance &&
-               matches_selector(&rule.selector, element, parent_bf,
                                 relations, flags_setter) {
                 matching_rules.push(
                     rule.to_applicable_declaration_block(cascade_level));
@@ -1556,10 +1488,11 @@ pub struct Rule {
     pub style_rule: Arc<Locked<StyleRule>>,
     /// The source order this style rule appears in.
     pub source_order: usize,
-    /// Bottom 30 bits: The specificity of the rule this selector represents.
+    /// The specificity of the rule this selector represents.
-    /// 31st bit: Whether the rule's declaration block has any important declarations.
+    ///
-    /// 32nd bit: Whether the rule's declaration block has any normal declarations.
+    /// Note: The top two bits of this are unused, and could be used to store
-    specificity_and_bits: u32,
+    /// flags.
+    specificity: u32,
 }
 
 impl Borrow<SelectorInner<SelectorImpl>> for Rule {
@@ -1568,23 +1501,10 @@ impl Borrow<SelectorInner<SelectorImpl>> for Rule {
     }
 }
 
-/// Masks for specificity_and_bits.
-const SPECIFICITY_MASK: u32 = 0x3fffffff;
-const ANY_IMPORTANT_DECLARATIONS_BIT: u32 = 1 << 30;
-const ANY_NORMAL_DECLARATIONS_BIT: u32 = 1 << 31;
-
 impl Rule {
     /// Returns the specificity of the rule.
     pub fn specificity(&self) -> u32 {
-        self.specificity_and_bits & SPECIFICITY_MASK
+        self.specificity
-    }
-
-    fn any_important_declarations(&self) -> bool {
-        (self.specificity_and_bits & ANY_IMPORTANT_DECLARATIONS_BIT) != 0
-    }
-
-    fn any_normal_declarations(&self) -> bool {
-        (self.specificity_and_bits & ANY_NORMAL_DECLARATIONS_BIT) != 0
     }
 
     fn to_applicable_declaration_block(&self, level: CascadeLevel) -> ApplicableDeclarationBlock {
@@ -1597,31 +1517,17 @@ impl Rule {
     }
 
     /// Creates a new Rule.
-    pub fn new(guard: &SharedRwLockReadGuard,
+    pub fn new(selector: SelectorInner<SelectorImpl>,
-               selector: SelectorInner<SelectorImpl>,
                style_rule: Arc<Locked<StyleRule>>,
                source_order: usize,
                specificity: u32)
                -> Self
     {
-        let (any_important, any_normal) = {
-            let block = style_rule.read_with(guard).block.read_with(guard);
-            (block.any_important(), block.any_normal())
-        };
-        debug_assert!(specificity & (ANY_IMPORTANT_DECLARATIONS_BIT | ANY_NORMAL_DECLARATIONS_BIT) == 0);
-        let mut specificity_and_bits = specificity;
-        if any_important {
-            specificity_and_bits |= ANY_IMPORTANT_DECLARATIONS_BIT;
-        }
-        if any_normal {
-            specificity_and_bits |= ANY_NORMAL_DECLARATIONS_BIT;
-        }
-
         Rule {
             selector: selector,
             style_rule: style_rule,
             source_order: source_order,
-            specificity_and_bits: specificity_and_bits,
+            specificity: specificity,
         }
     }
 }

tests/unit/style/stylist.rs

@@ -37,8 +37,7 @@ fn get_mock_rules(css_selectors: &[&str]) -> (Vec<Vec<Rule>>, SharedRwLock) {
         let guard = shared_lock.read();
         let rule = locked.read_with(&guard);
         rule.selectors.0.iter().map(|s| {
-            Rule::new(&guard,
+            Rule::new(s.inner.clone(),
-                      s.inner.clone(),
                       locked.clone(),
                       i,
                       s.specificity)
@@ -213,9 +212,7 @@ fn test_get_universal_rules() {
     thread_state::initialize(thread_state::LAYOUT);
     let (map, shared_lock) = get_mock_map(&["*|*", "#foo > *|*", ".klass", "#id"]);
 
-    let guard = shared_lock.read();
+    let decls = map.get_universal_rules(CascadeLevel::UserNormal);
-    let decls = map.get_universal_rules(
-        &guard, CascadeLevel::UserNormal, CascadeLevel::UserImportant);
 
     assert_eq!(decls.len(), 1);
 }