From 74eaf2ad6f9e74419bbaf58603e31696b4bab5d3 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Mon, 5 Sep 2016 13:53:49 +0800 Subject: [PATCH] Remove one level of nesting in `Stylist` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since #13134, the "normal" and "important" parts of `Stylist` are identical, so we don’t need to store them twice. --- components/style/selector_matching.rs | 188 ++++++++++++-------------- tests/unit/style/selector_matching.rs | 32 +++-- 2 files changed, 104 insertions(+), 116 deletions(-) diff --git a/components/style/selector_matching.rs b/components/style/selector_matching.rs index 2542226892b5..292b6cc7d9ca 100644 --- a/components/style/selector_matching.rs +++ b/components/style/selector_matching.rs @@ -38,8 +38,7 @@ pub type FnvHashMap = HashMap>; /// for a given document. The selectors are converted into `Rule`s /// (defined in rust-selectors), and introduced in a `SelectorMap` /// depending on the pseudo-element (see `PerPseudoElementSelectorMap`), -/// stylesheet origin (see `PerOriginSelectorMap`), and priority -/// (see the `normal` and `important` fields in `PerOriginSelectorMap`). +/// and stylesheet origin (see the fields of `PerPseudoElementSelectorMap`). /// /// This structure is effectively created once per pipeline, in the /// LayoutThread corresponding to that pipeline. @@ -162,38 +161,26 @@ impl Stylist { } let mut rules_source_order = self.rules_source_order; - // Take apart the StyleRule into individual Rules and insert - // them into the SelectorMap of that priority. - macro_rules! append( - ($style_rule: ident, $priority: ident, $importance: expr) => { - for selector in &$style_rule.selectors { - let map = if let Some(ref pseudo) = selector.pseudo_element { - self.pseudos_map - .entry(pseudo.clone()) - .or_insert_with(PerPseudoElementSelectorMap::new) - .borrow_for_origin(&stylesheet.origin) - } else { - self.element_map.borrow_for_origin(&stylesheet.origin) - }; - - map.$priority.insert(Rule { - selector: selector.complex_selector.clone(), - declarations: ApplicableDeclarationBlock { - specificity: selector.specificity, - mixed_declarations: $style_rule.declarations.clone(), - importance: $importance, - source_order: rules_source_order, - }, - }); - } - }; - ); - for rule in stylesheet.effective_rules(&self.device) { match *rule { CSSRule::Style(ref style_rule) => { - append!(style_rule, normal, Importance::Normal); - append!(style_rule, important, Importance::Important); + for selector in &style_rule.selectors { + let map = if let Some(ref pseudo) = selector.pseudo_element { + self.pseudos_map + .entry(pseudo.clone()) + .or_insert_with(PerPseudoElementSelectorMap::new) + .borrow_for_origin(&stylesheet.origin) + } else { + self.element_map.borrow_for_origin(&stylesheet.origin) + }; + + map.insert(Rule { + selector: selector.complex_selector.clone(), + declarations: style_rule.declarations.clone(), + specificity: selector.specificity, + source_order: rules_source_order, + }); + } rules_source_order += 1; for selector in &style_rule.selectors { @@ -242,8 +229,7 @@ impl Stylist { if let Some(map) = self.pseudos_map.remove(&pseudo) { let mut declarations = vec![]; - map.user_agent.normal.get_universal_rules(&mut declarations); - map.user_agent.important.get_universal_rules(&mut declarations); + map.user_agent.get_universal_rules(&mut declarations); self.precomputed_pseudo_element_decls.insert(pseudo, declarations); } @@ -368,12 +354,12 @@ impl Stylist { debug!("Determining if style is shareable: pseudo: {}", pseudo_element.is_some()); // Step 1: Normal user-agent rules. - map.user_agent.normal.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Normal); + map.user_agent.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Normal); debug!("UA normal: {:?}", relations); // Step 2: Presentational hints. @@ -386,19 +372,19 @@ impl Stylist { debug!("preshints: {:?}", relations); // Step 3: User and author normal rules. - map.user.normal.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Normal); + map.user.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Normal); debug!("user normal: {:?}", relations); - map.author.normal.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Normal); + map.author.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Normal); debug!("author normal: {:?}", relations); // Step 4: Normal style attributes. @@ -414,12 +400,12 @@ impl Stylist { debug!("style attr: {:?}", relations); // Step 5: Author-supplied `!important` rules. - map.author.important.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Important); + map.author.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Important); debug!("author important: {:?}", relations); @@ -436,21 +422,21 @@ impl Stylist { debug!("style attr important: {:?}", relations); // Step 7: User and UA `!important` rules. - map.user.important.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Important); + map.user.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Important); debug!("user important: {:?}", relations); - map.user_agent.important.get_all_matching_rules(element, - parent_bf, - applicable_declarations, - &mut relations, - reason, - Importance::Important); + map.user_agent.get_all_matching_rules(element, + parent_bf, + applicable_declarations, + &mut relations, + reason, + Importance::Important); debug!("UA important: {:?}", relations); @@ -549,51 +535,30 @@ impl Stylist { } -/// Map that contains the CSS rules for a given origin. -#[cfg_attr(feature = "servo", derive(HeapSizeOf))] -struct PerOriginSelectorMap { - /// Rules that contains at least one property declaration with - /// normal importance. - normal: SelectorMap, - /// Rules that contains at least one property declaration with - /// !important. - important: SelectorMap, -} - -impl PerOriginSelectorMap { - #[inline] - fn new() -> Self { - PerOriginSelectorMap { - normal: SelectorMap::new(), - important: SelectorMap::new(), - } - } -} - /// Map that contains the CSS rules for a specific PseudoElement /// (or lack of PseudoElement). #[cfg_attr(feature = "servo", derive(HeapSizeOf))] struct PerPseudoElementSelectorMap { /// Rules from user agent stylesheets - user_agent: PerOriginSelectorMap, + user_agent: SelectorMap, /// Rules from author stylesheets - author: PerOriginSelectorMap, + author: SelectorMap, /// Rules from user stylesheets - user: PerOriginSelectorMap, + user: SelectorMap, } impl PerPseudoElementSelectorMap { #[inline] fn new() -> Self { PerPseudoElementSelectorMap { - user_agent: PerOriginSelectorMap::new(), - author: PerOriginSelectorMap::new(), - user: PerOriginSelectorMap::new(), + user_agent: SelectorMap::new(), + author: SelectorMap::new(), + user: SelectorMap::new(), } } #[inline] - fn borrow_for_origin(&mut self, origin: &Origin) -> &mut PerOriginSelectorMap { + fn borrow_for_origin(&mut self, origin: &Origin) -> &mut SelectorMap { match *origin { Origin::UserAgent => &mut self.user_agent, Origin::Author => &mut self.author, @@ -737,7 +702,14 @@ impl SelectorMap { for rule in self.other_rules.iter() { if rule.selector.compound_selector.is_empty() && rule.selector.next.is_none() { - matching_rules_list.push(rule.declarations.clone()); + if rule.declarations.any_normal() { + matching_rules_list.push( + rule.to_applicable_declaration_block(Importance::Normal)); + } + if rule.declarations.any_important() { + matching_rules_list.push( + rule.to_applicable_declaration_block(Importance::Important)); + } } } @@ -782,7 +754,7 @@ impl SelectorMap { V: VecLike { for rule in rules.iter() { - let block = &rule.declarations.mixed_declarations; + let block = &rule.declarations; let any_declaration_for_importance = if importance.important() { block.any_important() } else { @@ -791,7 +763,7 @@ impl SelectorMap { if any_declaration_for_importance && matches_complex_selector(&*rule.selector, element, parent_bf, relations, reason) { - matching_rules.push(rule.declarations.clone()); + matching_rules.push(rule.to_applicable_declaration_block(importance)); } } } @@ -868,7 +840,21 @@ pub struct Rule { // that it matches. Selector contains an owned vector (through // ComplexSelector) and we want to avoid the allocation. pub selector: Arc>, - pub declarations: ApplicableDeclarationBlock, + pub declarations: Arc, + pub source_order: usize, + pub specificity: u32, +} + +impl Rule { + fn to_applicable_declaration_block(&self, importance: Importance) + -> ApplicableDeclarationBlock { + ApplicableDeclarationBlock { + mixed_declarations: self.declarations.clone(), + importance: importance, + source_order: self.source_order, + specificity: self.specificity, + } + } } /// A property declaration together with its precedence among rules of equal specificity so that diff --git a/tests/unit/style/selector_matching.rs b/tests/unit/style/selector_matching.rs index 23515209989d..b6422a75c094 100644 --- a/tests/unit/style/selector_matching.rs +++ b/tests/unit/style/selector_matching.rs @@ -6,8 +6,9 @@ use cssparser::Parser; use selectors::parser::{LocalName, ParserContext, parse_selector_list}; use std::sync::Arc; use string_cache::Atom; -use style::properties::{Importance, PropertyDeclarationBlock}; -use style::selector_matching::{ApplicableDeclarationBlock, Rule, SelectorMap}; +use style::properties::{PropertyDeclarationBlock, PropertyDeclaration, DeclaredValue}; +use style::properties::{longhands, Importance}; +use style::selector_matching::{Rule, SelectorMap}; /// Helper method to get some Rules from selector strings. /// Each sublist of the result contains the Rules for one StyleRule. @@ -18,15 +19,16 @@ fn get_mock_rules(css_selectors: &[&str]) -> Vec> { .unwrap().into_iter().map(|s| { Rule { selector: s.complex_selector.clone(), - declarations: ApplicableDeclarationBlock { - mixed_declarations: Arc::new(PropertyDeclarationBlock { - declarations: Vec::new(), - important_count: 0, - }), - importance: Importance::Normal, - specificity: s.specificity, - source_order: i, - } + declarations: Arc::new(PropertyDeclarationBlock { + declarations: vec![ + (PropertyDeclaration::Display(DeclaredValue::Value( + longhands::display::SpecifiedValue::block)), + Importance::Normal), + ], + important_count: 0, + }), + specificity: s.specificity, + source_order: i, } }).collect() }).collect() @@ -48,8 +50,8 @@ fn get_mock_map(selectors: &[&str]) -> SelectorMap { #[test] fn test_rule_ordering_same_specificity() { let rules_list = get_mock_rules(&["a.intro", "img.sidebar"]); - let a = &rules_list[0][0].declarations; - let b = &rules_list[1][0].declarations; + let a = &rules_list[0][0]; + let b = &rules_list[1][0]; assert!((a.specificity, a.source_order) < ((b.specificity, b.source_order)), "The rule that comes later should win."); } @@ -89,9 +91,9 @@ fn test_insert() { let rules_list = get_mock_rules(&[".intro.foo", "#top"]); let mut selector_map = SelectorMap::new(); selector_map.insert(rules_list[1][0].clone()); - assert_eq!(1, selector_map.id_hash.get(&atom!("top")).unwrap()[0].declarations.source_order); + assert_eq!(1, selector_map.id_hash.get(&atom!("top")).unwrap()[0].source_order); selector_map.insert(rules_list[0][0].clone()); - assert_eq!(0, selector_map.class_hash.get(&Atom::from("intro")).unwrap()[0].declarations.source_order); + assert_eq!(0, selector_map.class_hash.get(&Atom::from("intro")).unwrap()[0].source_order); assert!(selector_map.class_hash.get(&Atom::from("foo")).is_none()); }