From 7a9b917c4fd587b5da6d522f631773795c252e05 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 13:43:30 +0000 Subject: [PATCH 1/9] Remove some trailing whitespace. --- src/components/style/selector_matching.rs | 10 +++++----- src/components/style/selectors.rs | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index f949b2b8221..7fc6c72eccc 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -120,7 +120,7 @@ impl SelectorMap { N:TNode>( node: &N, pseudo_element: Option, - hash: &HashMap<~str,~[Rule]>, + hash: &HashMap<~str,~[Rule]>, key: &str, matching_rules: &mut ~[Rule]) { match hash.find(&key.to_str()) { @@ -130,7 +130,7 @@ impl SelectorMap { None => {} } } - + /// Adds rules in `rules` that match `node` to the `matching_rules` list. fn get_matching_rules>( @@ -310,7 +310,7 @@ impl Stylist { assert!(element.is_element()); assert!(style_attribute.is_none() || self.pseudo_element.is_none(), "Style attributes do not apply to pseudo-elements"); - + // In cascading order: let rule_map_list = [ &self.ua_rule_map.normal, @@ -332,7 +332,7 @@ impl Stylist { rule_map_indices[i] = matching_rules_list.len(); rule_map.get_all_matching_rules(element, self.pseudo_element, &mut matching_rules_list); } - + let count = matching_rules_list.len(); let mut declaration_iter = matching_rules_list.move_iter().map(|rule| { @@ -373,7 +373,7 @@ impl Stylist { applicable_declarations } - + pub fn get_pseudo_element(&self) -> Option { self.pseudo_element } diff --git a/src/components/style/selectors.rs b/src/components/style/selectors.rs index 3935121fadc..b54d17967f3 100644 --- a/src/components/style/selectors.rs +++ b/src/components/style/selectors.rs @@ -310,7 +310,7 @@ fn parse_one_simple_selector(iter: &mut Iter, namespaces: &NamespaceMap, inside_ Some(Ident(name)) => match parse_simple_pseudo_class(name) { None => { // FIXME: Workaround for https://github.com/mozilla/rust/issues/10683 - let name_lower = name.to_ascii_lower(); + let name_lower = name.to_ascii_lower(); match name_lower.as_slice() { // Supported CSS 2.1 pseudo-elements only. // ** Do not add to this list! ** @@ -470,7 +470,7 @@ fn parse_functional_pseudo_class(name: ~str, arguments: ~[ComponentValue], namespaces: &NamespaceMap, inside_negation: bool) -> Option { // FIXME: Workaround for https://github.com/mozilla/rust/issues/10683 - let name_lower = name.to_ascii_lower(); + let name_lower = name.to_ascii_lower(); match name_lower.as_slice() { // "lang" => parse_lang(arguments), "nth-child" => parse_nth(arguments).map(|(a, b)| NthChild(a, b)), @@ -485,7 +485,7 @@ fn parse_functional_pseudo_class(name: ~str, arguments: ~[ComponentValue], fn parse_pseudo_element(name: ~str) -> Option { // FIXME: Workaround for https://github.com/mozilla/rust/issues/10683 - let name_lower = name.to_ascii_lower(); + let name_lower = name.to_ascii_lower(); match name_lower.as_slice() { // All supported pseudo-elements "before" => Some(Before), From 1cdcaa940cd7a0ddc51af3db98e04f30fc42dc49 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 13:43:47 +0000 Subject: [PATCH 2/9] =?UTF-8?q?Comment=20out=20parsing=20of=20::firt-line?= =?UTF-8?q?=20and=20::first-letter,=20they=E2=80=99re=20not=20supported=20?= =?UTF-8?q?yet.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/components/main/css/matching.rs | 1 - src/components/style/selectors.rs | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/components/main/css/matching.rs b/src/components/main/css/matching.rs index e739a4c0c2e..5fc36489c00 100644 --- a/src/components/main/css/matching.rs +++ b/src/components/main/css/matching.rs @@ -46,7 +46,6 @@ impl<'self> MatchMethods for LayoutNode<'self> { Some(Before) => layout_data.before_applicable_declarations = applicable_declarations, Some(After) => layout_data.after_applicable_declarations = applicable_declarations, None => layout_data.applicable_declarations = applicable_declarations, - _ => {} } } None => fail!("no layout data") diff --git a/src/components/style/selectors.rs b/src/components/style/selectors.rs index b54d17967f3..3346c0cd897 100644 --- a/src/components/style/selectors.rs +++ b/src/components/style/selectors.rs @@ -23,8 +23,8 @@ pub static STYLE_ATTRIBUTE_SPECIFICITY: u32 = 1 << 31; pub enum PseudoElement { Before, After, - FirstLine, - FirstLetter, +// FirstLine, +// FirstLetter, } @@ -316,8 +316,8 @@ fn parse_one_simple_selector(iter: &mut Iter, namespaces: &NamespaceMap, inside_ // ** Do not add to this list! ** "before" => PseudoElementResult(Before), "after" => PseudoElementResult(After), - "first-line" => PseudoElementResult(FirstLine), - "first-letter" => PseudoElementResult(FirstLetter), +// "first-line" => PseudoElementResult(FirstLine), +// "first-letter" => PseudoElementResult(FirstLetter), _ => InvalidSimpleSelector } }, @@ -490,8 +490,8 @@ fn parse_pseudo_element(name: ~str) -> Option { // All supported pseudo-elements "before" => Some(Before), "after" => Some(After), - "first-line" => Some(FirstLine), - "first-letter" => Some(FirstLetter), +// "first-line" => Some(FirstLine), +// "first-letter" => Some(FirstLetter), _ => None } } From 806882394883a5f87cc5e4f56cd635a19f03baca Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 13:48:43 +0000 Subject: [PATCH 3/9] Make SelectorMap private. --- src/components/style/selector_matching.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 7fc6c72eccc..4cfabfba0b1 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -42,14 +42,14 @@ static SELECTOR_WHITESPACE: &'static [char] = &'static [' ', '\t', '\n', '\r', ' /// Hence, the union of the rules keyed on each of node's classes, ID, /// element name, etc. will contain the Rules that actually match that /// node. -pub struct SelectorMap { +struct SelectorMap { // TODO: Tune the initial capacity of the HashMap // FIXME: Use interned strings - priv id_hash: HashMap<~str, ~[Rule]>, - priv class_hash: HashMap<~str, ~[Rule]>, - priv element_hash: HashMap<~str, ~[Rule]>, + id_hash: HashMap<~str, ~[Rule]>, + class_hash: HashMap<~str, ~[Rule]>, + element_hash: HashMap<~str, ~[Rule]>, // For Rules that don't have ID, class, or element selectors. - priv universal_rules: ~[Rule], + universal_rules: ~[Rule], } impl SelectorMap { From 146029bc06187afb4811c5dde2b87fdfa0c8525f Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 13:49:35 +0000 Subject: [PATCH 4/9] Remove unused PerOriginRules struct. --- src/components/style/selector_matching.rs | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 4cfabfba0b1..fae04dd14f5 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -379,21 +379,6 @@ impl Stylist { } } -struct PerOriginRules { - normal: ~[Rule], - important: ~[Rule], -} - -impl PerOriginRules { - #[inline] - fn new() -> PerOriginRules { - PerOriginRules { - normal: ~[], - important: ~[], - } - } -} - struct PerOriginSelectorMap { normal: SelectorMap, important: SelectorMap, From 1d6f94fd96294fabe5868bb64b434bcfe33a1f8a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 13:50:23 +0000 Subject: [PATCH 5/9] Move selector matching tests and testing helpers into a #[cfg(test)] module. --- src/components/style/selector_matching.rs | 112 +++++++++++----------- 1 file changed, 57 insertions(+), 55 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index fae04dd14f5..9f778dcea36 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -710,66 +710,68 @@ fn match_attribute ~[~[Rule]] { - use namespaces::NamespaceMap; - use selectors::parse_selector_list; - use cssparser::tokenize; +#[cfg(test)] +mod tests { + /// Helper method to get some Rules from selector strings. + /// Each sublist of the result contains the Rules for one StyleRule. + fn get_mock_rules(css_selectors: &[&str]) -> ~[~[Rule]] { + use namespaces::NamespaceMap; + use selectors::parse_selector_list; + use cssparser::tokenize; - let namespaces = NamespaceMap::new(); - css_selectors.iter().enumerate().map(|(i, selectors)| { - parse_selector_list(tokenize(*selectors).map(|(c, _)| c).to_owned_vec(), &namespaces) - .unwrap().move_iter().map(|s| { - Rule { - selector: Arc::new(s), - declarations: Arc::new(~[]), - index: i, - stylesheet_index: 0u, - } + let namespaces = NamespaceMap::new(); + css_selectors.iter().enumerate().map(|(i, selectors)| { + parse_selector_list(tokenize(*selectors).map(|(c, _)| c).to_owned_vec(), &namespaces) + .unwrap().move_iter().map(|s| { + Rule { + selector: Arc::new(s), + declarations: Arc::new(~[]), + index: i, + stylesheet_index: 0u, + } + }).to_owned_vec() }).to_owned_vec() - }).to_owned_vec() -} + } -#[test] -fn test_rule_ordering_same_specificity(){ - let rules_list = get_mock_rules(["a.intro", "img.sidebar"]); - let rule1 = rules_list[0][0].clone(); - let rule2 = rules_list[1][0].clone(); - assert!(rule1 < rule2, "The rule that comes later should win."); -} + #[test] + fn test_rule_ordering_same_specificity(){ + let rules_list = get_mock_rules(["a.intro", "img.sidebar"]); + let rule1 = rules_list[0][0].clone(); + let rule2 = rules_list[1][0].clone(); + assert!(rule1 < rule2, "The rule that comes later should win."); + } -#[test] -fn test_get_id_name(){ - let rules_list = get_mock_rules([".intro", "#top"]); - assert_eq!(SelectorMap::get_id_name(&rules_list[0][0]), None); - assert_eq!(SelectorMap::get_id_name(&rules_list[1][0]), Some(~"top")); -} + #[test] + fn test_get_id_name(){ + let rules_list = get_mock_rules([".intro", "#top"]); + assert_eq!(SelectorMap::get_id_name(&rules_list[0][0]), None); + assert_eq!(SelectorMap::get_id_name(&rules_list[1][0]), Some(~"top")); + } -#[test] -fn test_get_class_name(){ - let rules_list = get_mock_rules([".intro.foo", "#top"]); - assert_eq!(SelectorMap::get_class_name(&rules_list[0][0]), Some(~"intro")); - assert_eq!(SelectorMap::get_class_name(&rules_list[1][0]), None); -} + #[test] + fn test_get_class_name(){ + let rules_list = get_mock_rules([".intro.foo", "#top"]); + assert_eq!(SelectorMap::get_class_name(&rules_list[0][0]), Some(~"intro")); + assert_eq!(SelectorMap::get_class_name(&rules_list[1][0]), None); + } -#[test] -fn test_get_element_name(){ - let rules_list = get_mock_rules(["img.foo", "#top", "IMG", "ImG"]); - assert_eq!(SelectorMap::get_element_name(&rules_list[0][0]), Some(~"img")); - assert_eq!(SelectorMap::get_element_name(&rules_list[1][0]), None); - assert_eq!(SelectorMap::get_element_name(&rules_list[2][0]), Some(~"img")); - assert_eq!(SelectorMap::get_element_name(&rules_list[3][0]), Some(~"img")); -} + #[test] + fn test_get_element_name(){ + let rules_list = get_mock_rules(["img.foo", "#top", "IMG", "ImG"]); + assert_eq!(SelectorMap::get_element_name(&rules_list[0][0]), Some(~"img")); + assert_eq!(SelectorMap::get_element_name(&rules_list[1][0]), None); + assert_eq!(SelectorMap::get_element_name(&rules_list[2][0]), Some(~"img")); + assert_eq!(SelectorMap::get_element_name(&rules_list[3][0]), Some(~"img")); + } -#[test] -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.find(&~"top").unwrap()[0].index); - selector_map.insert(rules_list[0][0].clone()); - assert_eq!(0, selector_map.class_hash.find(&~"intro").unwrap()[0].index); - assert!(selector_map.class_hash.find(&~"foo").is_none()); + #[test] + 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.find(&~"top").unwrap()[0].index); + selector_map.insert(rules_list[0][0].clone()); + assert_eq!(0, selector_map.class_hash.find(&~"intro").unwrap()[0].index); + assert!(selector_map.class_hash.find(&~"foo").is_none()); + } } - From a16087da5f8371b1f7dbc530c3780e448e431d4a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 14:54:16 +0000 Subject: [PATCH 6/9] Selector mathing: Move the pseudo-elements buckets inside Stylist. Refactor to have only one Stylist per document. --- src/components/main/css/matching.rs | 39 +++--- src/components/main/css/select.rs | 7 +- src/components/main/layout/layout_task.rs | 23 +--- src/components/style/selector_matching.rs | 155 ++++++++++++---------- 4 files changed, 112 insertions(+), 112 deletions(-) diff --git a/src/components/main/css/matching.rs b/src/components/main/css/matching.rs index 5fc36489c00..87d91006f20 100644 --- a/src/components/main/css/matching.rs +++ b/src/components/main/css/matching.rs @@ -22,7 +22,7 @@ use style::{Before, After}; pub trait MatchMethods { fn match_node(&self, stylist: &Stylist); - fn match_subtree(&self, stylist: ~[RWArc]); + fn match_subtree(&self, stylist: RWArc); fn cascade_before_node(&self, parent: Option); fn cascade_node(&self, parent: Option); @@ -32,26 +32,26 @@ pub trait MatchMethods { impl<'self> MatchMethods for LayoutNode<'self> { fn match_node(&self, stylist: &Stylist) { - let applicable_declarations = do self.with_element |element| { - let style_attribute = match *element.style_attribute() { + let style_attribute = do self.with_element |element| { + match *element.style_attribute() { None => None, Some(ref style_attribute) => Some(style_attribute) - }; - stylist.get_applicable_declarations(self, style_attribute) + } }; match *self.mutate_layout_data().ptr { Some(ref mut layout_data) => { - match stylist.get_pseudo_element() { - Some(Before) => layout_data.before_applicable_declarations = applicable_declarations, - Some(After) => layout_data.after_applicable_declarations = applicable_declarations, - None => layout_data.applicable_declarations = applicable_declarations, - } + layout_data.applicable_declarations = stylist.get_applicable_declarations( + self, style_attribute, None); + layout_data.before_applicable_declarations = stylist.get_applicable_declarations( + self, None, Some(Before)); + layout_data.after_applicable_declarations = stylist.get_applicable_declarations( + self, None, Some(After)); } None => fail!("no layout data") } } - fn match_subtree(&self, stylists: ~[RWArc]) { + fn match_subtree(&self, stylist: RWArc) { let num_tasks = rt::default_sched_threads() * 2; let mut node_count = 0; let mut nodes_per_task = vec::from_elem(num_tasks, ~[]); @@ -70,8 +70,8 @@ impl<'self> MatchMethods for LayoutNode<'self> { for nodes in nodes_per_task.move_iter() { if nodes.len() > 0 { let chan = chan.clone(); - let stylists = stylists.clone(); - + let stylist = stylist.clone(); + // FIXME(pcwalton): This transmute is to work around the fact that we have no // mechanism for safe fork/join parallelism. If we had such a thing, then we could // close over the lifetime-bounded `LayoutNode`. But we can't, so we force it with @@ -80,19 +80,16 @@ impl<'self> MatchMethods for LayoutNode<'self> { cast::transmute(nodes) }; - do task::spawn_with((evil, stylists)) |(evil, stylists)| { + do task::spawn_with((evil, stylist)) |(evil, stylist)| { let nodes: ~[LayoutNode] = unsafe { cast::transmute(evil) }; let nodes = Cell::new(nodes); - for stylist in stylists.iter() { - do stylist.read |stylist| { - nodes.with_ref(|nodes|{ - for node in nodes.iter() { - node.match_node(stylist); - } - }); + do stylist.read |stylist| { + let nodes = nodes.take(); + for node in nodes.iter() { + node.match_node(stylist); } } chan.send(()); diff --git a/src/components/main/css/select.rs b/src/components/main/css/select.rs index 5292c963dc3..10ac49875d5 100644 --- a/src/components/main/css/select.rs +++ b/src/components/main/css/select.rs @@ -3,17 +3,16 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use style::{Stylesheet, Stylist, UserAgentOrigin, with_errors_silenced}; -use style::PseudoElement; use extra::url; -pub fn new_stylist(pseudo_element: Option) -> Stylist { - let mut stylist = Stylist::new(pseudo_element); +pub fn new_stylist() -> Stylist { + let mut stylist = Stylist::new(); let ua_stylesheet = with_errors_silenced(|| Stylesheet::from_bytes( include_bin!("user-agent.css"), url::from_str("chrome:///user-agent.css").unwrap(), None, None)); - stylist.add_stylesheet(&ua_stylesheet, UserAgentOrigin); + stylist.add_stylesheet(ua_stylesheet, UserAgentOrigin); stylist } diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index 9ce50a0bf4c..2224401188a 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -51,7 +51,6 @@ use std::comm::Port; use std::task; use std::util; use style::{AuthorOrigin, Stylesheet, Stylist}; -use style::{Before, After}; /// Information needed by the layout task. struct LayoutTask { @@ -82,7 +81,7 @@ struct LayoutTask { /// A cached display list. display_list: Option>>, - stylists: ~[RWArc], + stylist: RWArc, /// The channel on which messages can be sent to the profiler. profiler_chan: ProfilerChan, @@ -237,14 +236,6 @@ impl LayoutTask { profiler_chan: ProfilerChan) -> LayoutTask { - let mut stylists = ~[]; - // We implemented parsing/selector-matching only for Before and After. - // FirstLine and FirstLetter have to be added later. - let stylist_owners = ~[Some(Before), Some(After), None]; - for pseudo_element in stylist_owners.iter() { - stylists.push(RWArc::new(new_stylist(*pseudo_element))); - } - LayoutTask { id: id, port: port, @@ -257,7 +248,7 @@ impl LayoutTask { display_list: None, - stylists: stylists, + stylist: RWArc::new(new_stylist()), profiler_chan: profiler_chan, opts: opts.clone() } @@ -356,12 +347,8 @@ impl LayoutTask { fn handle_add_stylesheet(&mut self, sheet: Stylesheet) { let sheet = Cell::new(sheet); - for stylist in self.stylists.iter() { - do stylist.write |stylist| { - sheet.with_ref(|sheet|{ - stylist.add_stylesheet(sheet, AuthorOrigin); - }); - } + do self.stylist.write |stylist| { + stylist.add_stylesheet(sheet.take(), AuthorOrigin); } } @@ -458,7 +445,7 @@ impl LayoutTask { ReflowDocumentDamage => {} _ => { do profile(time::LayoutSelectorMatchCategory, self.profiler_chan.clone()) { - node.match_subtree(self.stylists.clone()); + node.match_subtree(self.stylist.clone()); node.cascade_subtree(None); } } diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 9f778dcea36..fdd060ada07 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -70,7 +70,6 @@ impl SelectorMap { N:TNode>( &self, node: &N, - pseudo_element: Option, matching_rules_list: &mut ~[Rule]) { // At the end, we're going to sort the rules that we added, so remember where we began. let init_len = matching_rules_list.len(); @@ -78,7 +77,6 @@ impl SelectorMap { match element.get_attr(None, "id") { Some(id) => { SelectorMap::get_matching_rules_from_hash(node, - pseudo_element, &self.id_hash, id, matching_rules_list) @@ -90,7 +88,6 @@ impl SelectorMap { Some(ref class_attr) => { for class in class_attr.split_iter(SELECTOR_WHITESPACE) { SelectorMap::get_matching_rules_from_hash(node, - pseudo_element, &self.class_hash, class, matching_rules_list) @@ -102,12 +99,10 @@ impl SelectorMap { // HTML elements in HTML documents must be matched case-insensitively. // TODO(pradeep): Case-sensitivity depends on the document type. SelectorMap::get_matching_rules_from_hash(node, - pseudo_element, &self.element_hash, element.get_local_name().to_ascii_lower(), matching_rules_list); SelectorMap::get_matching_rules(node, - pseudo_element, self.universal_rules, matching_rules_list); }); @@ -119,13 +114,12 @@ impl SelectorMap { fn get_matching_rules_from_hash>( node: &N, - pseudo_element: Option, hash: &HashMap<~str,~[Rule]>, key: &str, matching_rules: &mut ~[Rule]) { match hash.find(&key.to_str()) { Some(rules) => { - SelectorMap::get_matching_rules(node, pseudo_element, *rules, matching_rules) + SelectorMap::get_matching_rules(node, *rules, matching_rules) } None => {} } @@ -135,11 +129,10 @@ impl SelectorMap { fn get_matching_rules>( node: &N, - pseudo_element: Option, rules: &[Rule], matching_rules: &mut ~[Rule]) { for rule in rules.iter() { - if matches_selector(rule.selector.get(), node, pseudo_element) { + if matches_compound_selector(rule.selector.get(), node) { // TODO(pradeep): Is the cloning inefficient? matching_rules.push(rule.clone()); } @@ -198,7 +191,7 @@ impl SelectorMap { /// Retrieve the first ID name in Rule, or None otherwise. fn get_id_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // TODO(pradeep): Implement case-sensitivity based on the document type and quirks @@ -212,7 +205,7 @@ impl SelectorMap { /// Retrieve the FIRST class name in Rule, or None otherwise. fn get_class_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // TODO(pradeep): Implement case-sensitivity based on the document type and quirks @@ -226,7 +219,7 @@ impl SelectorMap { /// Retrieve the name if it is a type selector, or None otherwise. fn get_element_name(rule: &Rule) -> Option<~str> { - let simple_selector_sequence = &rule.selector.get().compound_selectors.simple_selectors; + let simple_selector_sequence = &rule.selector.get().simple_selectors; for ss in simple_selector_sequence.iter() { match *ss { // HTML elements in HTML documents must be matched case-insensitively @@ -240,51 +233,62 @@ impl SelectorMap { } pub struct Stylist { - priv ua_rule_map: PerOriginSelectorMap, - priv author_rule_map: PerOriginSelectorMap, - priv user_rule_map: PerOriginSelectorMap, + priv element_map: PerPseudoElementSelectorMap, + priv before_map: PerPseudoElementSelectorMap, + priv after_map: PerPseudoElementSelectorMap, priv stylesheet_index: uint, - priv pseudo_element: Option, } impl Stylist { #[inline] - pub fn new(pseudo_element: Option) -> Stylist { + pub fn new() -> Stylist { Stylist { - ua_rule_map: PerOriginSelectorMap::new(), - author_rule_map: PerOriginSelectorMap::new(), - user_rule_map: PerOriginSelectorMap::new(), + element_map: PerPseudoElementSelectorMap::new(), + before_map: PerPseudoElementSelectorMap::new(), + after_map: PerPseudoElementSelectorMap::new(), stylesheet_index: 0u, - pseudo_element: pseudo_element, } } - pub fn add_stylesheet(&mut self, stylesheet: &Stylesheet, origin: StylesheetOrigin) { - let rule_map = match origin { - UserAgentOrigin => &mut self.ua_rule_map, - AuthorOrigin => &mut self.author_rule_map, - UserOrigin => &mut self.user_rule_map, + pub fn add_stylesheet(&mut self, stylesheet: Stylesheet, origin: StylesheetOrigin) { + let (mut element_map, mut before_map, mut after_map) = match origin { + UserAgentOrigin => ( + &mut self.element_map.user_agent, + &mut self.before_map.user_agent, + &mut self.after_map.user_agent, + ), + AuthorOrigin => ( + &mut self.element_map.author, + &mut self.before_map.author, + &mut self.after_map.author, + ), + UserOrigin => ( + &mut self.element_map.user, + &mut self.before_map.user, + &mut self.after_map.user, + ), }; - let mut added_normal_declarations = false; - let mut added_important_declarations = false; let mut style_rule_index = 0u; // Take apart the StyleRule into individual Rules and insert // them into the SelectorMap of that priority. macro_rules! append( - ($priority: ident, $flag: ident) => { + ($priority: ident) => { if style_rule.declarations.$priority.get().len() > 0 { - $flag = true; for selector in style_rule.selectors.iter() { - // TODO: avoid copying? - if selector.pseudo_element == self.pseudo_element { - rule_map.$priority.insert(Rule { - selector: Arc::new(selector.clone()), - declarations: style_rule.declarations.$priority.clone(), - index: style_rule_index, - stylesheet_index: self.stylesheet_index, - }); - } + let map = match selector.pseudo_element { + None => &mut element_map, + Some(Before) => &mut before_map, + Some(After) => &mut after_map, + }; + map.$priority.insert(Rule { + // TODO: avoid copying? + selector: Arc::new(selector.compound_selectors.clone()), + specificity: selector.specificity, + declarations: style_rule.declarations.$priority.clone(), + index: style_rule_index, + stylesheet_index: self.stylesheet_index, + }); } } }; @@ -292,8 +296,8 @@ impl Stylist { let device = &Device { media_type: Screen }; // TODO, use Print when printing do iter_style_rules(stylesheet.rules.as_slice(), device) |style_rule| { - append!(normal, added_normal_declarations); - append!(important, added_important_declarations); + append!(normal); + append!(important); style_rule_index += 1u; } self.stylesheet_index += 1; @@ -305,20 +309,26 @@ impl Stylist { N:TNode>( &self, element: &N, - style_attribute: Option<&PropertyDeclarationBlock>) + style_attribute: Option<&PropertyDeclarationBlock>, + pseudo_element: Option) -> ~[Arc<~[PropertyDeclaration]>] { assert!(element.is_element()); - assert!(style_attribute.is_none() || self.pseudo_element.is_none(), + assert!(style_attribute.is_none() || pseudo_element.is_none(), "Style attributes do not apply to pseudo-elements"); + let map = match pseudo_element { + None => &self.element_map, + Some(Before) => &self.before_map, + Some(After) => &self.after_map, + }; // In cascading order: let rule_map_list = [ - &self.ua_rule_map.normal, - &self.user_rule_map.normal, - &self.author_rule_map.normal, - &self.author_rule_map.important, - &self.user_rule_map.important, - &self.ua_rule_map.important + &map.user_agent.normal, + &map.user.normal, + &map.author.normal, + &map.author.important, + &map.user.important, + &map.user_agent.important ]; // We keep track of the indices of each of the rule maps in the list we're building so that @@ -330,7 +340,7 @@ impl Stylist { for (i, rule_map) in rule_map_list.iter().enumerate() { rule_map_indices[i] = matching_rules_list.len(); - rule_map.get_all_matching_rules(element, self.pseudo_element, &mut matching_rules_list); + rule_map.get_all_matching_rules(element, &mut matching_rules_list); } let count = matching_rules_list.len(); @@ -373,10 +383,6 @@ impl Stylist { applicable_declarations } - - pub fn get_pseudo_element(&self) -> Option { - self.pseudo_element - } } struct PerOriginSelectorMap { @@ -394,40 +400,47 @@ impl PerOriginSelectorMap { } } +struct PerPseudoElementSelectorMap { + user_agent: PerOriginSelectorMap, + author: PerOriginSelectorMap, + user: PerOriginSelectorMap, +} + +impl PerPseudoElementSelectorMap { + #[inline] + fn new() -> PerPseudoElementSelectorMap { + PerPseudoElementSelectorMap { + user_agent: PerOriginSelectorMap::new(), + author: PerOriginSelectorMap::new(), + user: PerOriginSelectorMap::new(), + } + } +} + #[deriving(Clone)] struct Rule { // This is an Arc because Rule will essentially be cloned for every node // that it matches. Selector contains an owned vector (through // CompoundSelector) and we want to avoid the allocation. - selector: Arc, + selector: Arc, declarations: Arc<~[PropertyDeclaration]>, // Index of the parent StyleRule in the parent Stylesheet (useful for // breaking ties while cascading). index: uint, // Index of the parent stylesheet among all the stylesheets stylesheet_index: uint, + specificity: u32, } impl Ord for Rule { #[inline] fn lt(&self, other: &Rule) -> bool { - let this_rank = (self.selector.get().specificity, self.stylesheet_index, self.index); - let other_rank = (other.selector.get().specificity, other.stylesheet_index, other.index); + let this_rank = (self.specificity, self.stylesheet_index, self.index); + let other_rank = (other.specificity, other.stylesheet_index, other.index); this_rank < other_rank } } -#[inline] -fn matches_selector>( - selector: &Selector, - element: &N, - pseudo_element: Option) - -> bool { - selector.pseudo_element == pseudo_element && - matches_compound_selector::(&selector.compound_selectors, element) -} - fn matches_compound_selector>(selector: &CompoundSelector, element: &N) -> bool { if !do selector.simple_selectors.iter().all |simple_selector| { @@ -712,6 +725,9 @@ fn match_attribute ~[~[Rule]] { @@ -724,7 +740,8 @@ mod tests { parse_selector_list(tokenize(*selectors).map(|(c, _)| c).to_owned_vec(), &namespaces) .unwrap().move_iter().map(|s| { Rule { - selector: Arc::new(s), + specificity: s.specificity, + selector: Arc::new(s.compound_selectors), declarations: Arc::new(~[]), index: i, stylesheet_index: 0u, From faeab3773e7e716d65f1d5b877ae1a3fa1f2cc90 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 14:59:10 +0000 Subject: [PATCH 7/9] Simplify tracking of CSS rules source order in selector matching. --- src/components/style/selector_matching.rs | 29 +++++++++-------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index fdd060ada07..375e00e1b6b 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -236,7 +236,7 @@ pub struct Stylist { priv element_map: PerPseudoElementSelectorMap, priv before_map: PerPseudoElementSelectorMap, priv after_map: PerPseudoElementSelectorMap, - priv stylesheet_index: uint, + priv rules_source_order: uint, } impl Stylist { @@ -246,7 +246,7 @@ impl Stylist { element_map: PerPseudoElementSelectorMap::new(), before_map: PerPseudoElementSelectorMap::new(), after_map: PerPseudoElementSelectorMap::new(), - stylesheet_index: 0u, + rules_source_order: 0u, } } @@ -268,7 +268,6 @@ impl Stylist { &mut self.after_map.user, ), }; - let mut style_rule_index = 0u; // Take apart the StyleRule into individual Rules and insert // them into the SelectorMap of that priority. @@ -286,8 +285,7 @@ impl Stylist { selector: Arc::new(selector.compound_selectors.clone()), specificity: selector.specificity, declarations: style_rule.declarations.$priority.clone(), - index: style_rule_index, - stylesheet_index: self.stylesheet_index, + source_order: self.rules_source_order, }); } } @@ -298,9 +296,8 @@ impl Stylist { do iter_style_rules(stylesheet.rules.as_slice(), device) |style_rule| { append!(normal); append!(important); - style_rule_index += 1u; + self.rules_source_order += 1; } - self.stylesheet_index += 1; } /// Returns the applicable CSS declarations for the given element. This corresponds to @@ -424,19 +421,16 @@ struct Rule { // CompoundSelector) and we want to avoid the allocation. selector: Arc, declarations: Arc<~[PropertyDeclaration]>, - // Index of the parent StyleRule in the parent Stylesheet (useful for - // breaking ties while cascading). - index: uint, - // Index of the parent stylesheet among all the stylesheets - stylesheet_index: uint, + // Precedence among rules of equal specificity + source_order: uint, specificity: u32, } impl Ord for Rule { #[inline] fn lt(&self, other: &Rule) -> bool { - let this_rank = (self.specificity, self.stylesheet_index, self.index); - let other_rank = (other.specificity, other.stylesheet_index, other.index); + let this_rank = (self.specificity, self.source_order); + let other_rank = (other.specificity, other.source_order); this_rank < other_rank } } @@ -743,8 +737,7 @@ mod tests { specificity: s.specificity, selector: Arc::new(s.compound_selectors), declarations: Arc::new(~[]), - index: i, - stylesheet_index: 0u, + source_order: i, } }).to_owned_vec() }).to_owned_vec() @@ -786,9 +779,9 @@ mod tests { 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.find(&~"top").unwrap()[0].index); + assert_eq!(1, selector_map.id_hash.find(&~"top").unwrap()[0].source_order); selector_map.insert(rules_list[0][0].clone()); - assert_eq!(0, selector_map.class_hash.find(&~"intro").unwrap()[0].index); + assert_eq!(0, selector_map.class_hash.find(&~"intro").unwrap()[0].source_order); assert!(selector_map.class_hash.find(&~"foo").is_none()); } } From 724fd7caaf87048b6188c7bb252c5b18ddf11d8c Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 15:08:27 +0000 Subject: [PATCH 8/9] Store selectors in Arc early to avoid some copying. --- src/components/style/selector_matching.rs | 5 ++-- src/components/style/selectors.rs | 36 +++++++++++++++-------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs index 375e00e1b6b..3d48fd66a57 100644 --- a/src/components/style/selector_matching.rs +++ b/src/components/style/selector_matching.rs @@ -281,8 +281,7 @@ impl Stylist { Some(After) => &mut after_map, }; map.$priority.insert(Rule { - // TODO: avoid copying? - selector: Arc::new(selector.compound_selectors.clone()), + selector: selector.compound_selectors.clone(), specificity: selector.specificity, declarations: style_rule.declarations.$priority.clone(), source_order: self.rules_source_order, @@ -735,7 +734,7 @@ mod tests { .unwrap().move_iter().map(|s| { Rule { specificity: s.specificity, - selector: Arc::new(s.compound_selectors), + selector: s.compound_selectors, declarations: Arc::new(~[]), source_order: i, } diff --git a/src/components/style/selectors.rs b/src/components/style/selectors.rs index 3346c0cd897..2e401e1a407 100644 --- a/src/components/style/selectors.rs +++ b/src/components/style/selectors.rs @@ -4,14 +4,25 @@ use std::{vec, iter}; use std::ascii::StrAsciiExt; +use extra::arc::Arc; + use cssparser::ast::*; use cssparser::parse_nth; + use namespaces::NamespaceMap; +// Only used in tests +impl Eq for Arc { + fn eq(&self, other: &Arc) -> bool { + self.get() == other.get() + } +} + + #[deriving(Eq, Clone)] pub struct Selector { - compound_selectors: CompoundSelector, + compound_selectors: Arc, pseudo_element: Option, specificity: u32, } @@ -157,7 +168,7 @@ fn parse_selector(iter: &mut Iter, namespaces: &NamespaceMap) } Some(Selector { specificity: compute_specificity(&compound, &pseudo_element), - compound_selectors: compound, + compound_selectors: Arc::new(compound), pseudo_element: pseudo_element, }) } @@ -549,6 +560,7 @@ fn skip_whitespace(iter: &mut Iter) -> bool { #[cfg(test)] mod tests { + use extra::arc::Arc; use cssparser; use namespaces::NamespaceMap; use super::*; @@ -567,48 +579,48 @@ mod tests { fn test_parsing() { assert_eq!(parse(""), None) assert_eq!(parse("e"), Some(~[Selector{ - compound_selectors: CompoundSelector { + compound_selectors: Arc::new(CompoundSelector { simple_selectors: ~[LocalNameSelector(~"e")], next: None, - }, + }), pseudo_element: None, specificity: specificity(0, 0, 1), }])) assert_eq!(parse(".foo"), Some(~[Selector{ - compound_selectors: CompoundSelector { + compound_selectors: Arc::new(CompoundSelector { simple_selectors: ~[ClassSelector(~"foo")], next: None, - }, + }), pseudo_element: None, specificity: specificity(0, 1, 0), }])) assert_eq!(parse("#bar"), Some(~[Selector{ - compound_selectors: CompoundSelector { + compound_selectors: Arc::new(CompoundSelector { simple_selectors: ~[IDSelector(~"bar")], next: None, - }, + }), pseudo_element: None, specificity: specificity(1, 0, 0), }])) assert_eq!(parse("e.foo#bar"), Some(~[Selector{ - compound_selectors: CompoundSelector { + compound_selectors: Arc::new(CompoundSelector { simple_selectors: ~[LocalNameSelector(~"e"), ClassSelector(~"foo"), IDSelector(~"bar")], next: None, - }, + }), pseudo_element: None, specificity: specificity(1, 1, 1), }])) assert_eq!(parse("e.foo #bar"), Some(~[Selector{ - compound_selectors: CompoundSelector { + compound_selectors: Arc::new(CompoundSelector { simple_selectors: ~[IDSelector(~"bar")], next: Some((~CompoundSelector { simple_selectors: ~[LocalNameSelector(~"e"), ClassSelector(~"foo")], next: None, }, Descendant)), - }, + }), pseudo_element: None, specificity: specificity(1, 1, 1), }])) From 8d8a8ad6e7cc20389eebbf25069ccfa629617c0c Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 8 Jan 2014 16:08:42 +0000 Subject: [PATCH 9/9] De-duplicate some redundant code with a macro. --- src/components/main/css/matching.rs | 133 +++++++--------------------- 1 file changed, 34 insertions(+), 99 deletions(-) diff --git a/src/components/main/css/matching.rs b/src/components/main/css/matching.rs index 87d91006f20..cace6ffd5f1 100644 --- a/src/components/main/css/matching.rs +++ b/src/components/main/css/matching.rs @@ -24,9 +24,6 @@ pub trait MatchMethods { fn match_node(&self, stylist: &Stylist); fn match_subtree(&self, stylist: RWArc); - fn cascade_before_node(&self, parent: Option); - fn cascade_node(&self, parent: Option); - fn cascade_after_node(&self, parent: Option); fn cascade_subtree(&self, parent: Option); } @@ -102,109 +99,47 @@ impl<'self> MatchMethods for LayoutNode<'self> { } } - fn cascade_before_node(&self, parent: Option) { - let parent_style = match parent { - Some(ref parent) => Some(parent.style()), - None => None - }; - - let computed_values = unsafe { - Arc::new(cascade(self.borrow_layout_data_unchecked() - .as_ref() - .unwrap() - .before_applicable_declarations, - parent_style.map(|parent_style| parent_style.get()))) - }; - - match *self.mutate_layout_data().ptr { - None => fail!("no layout data"), - Some(ref mut layout_data) => { - let style = &mut layout_data.before_style; - match *style { - None => (), - Some(ref previous_style) => { - layout_data.restyle_damage = - Some(incremental::compute_damage(previous_style.get(), - computed_values.get()).to_int()) - } - } - *style = Some(computed_values) - } - } - } - - fn cascade_node(&self, parent: Option) { - let parent_style = match parent { - Some(ref parent) => Some(parent.style()), - None => None - }; - - let computed_values = unsafe { - Arc::new(cascade(self.borrow_layout_data_unchecked() - .as_ref() - .unwrap() - .applicable_declarations, - parent_style.map(|parent_style| parent_style.get()))) - }; - - match *self.mutate_layout_data().ptr { - None => fail!("no layout data"), - Some(ref mut layout_data) => { - let style = &mut layout_data.style; - match *style { - None => (), - Some(ref previous_style) => { - layout_data.restyle_damage = - Some(incremental::compute_damage(previous_style.get(), - computed_values.get()).to_int()) - } - } - *style = Some(computed_values) - } - } - } - - fn cascade_after_node(&self, parent: Option) { - let parent_style = match parent { - Some(ref parent) => Some(parent.style()), - None => None - }; - - let computed_values = unsafe { - Arc::new(cascade(self.borrow_layout_data_unchecked() - .as_ref() - .unwrap() - .after_applicable_declarations, - parent_style.map(|parent_style| parent_style.get()))) - }; - - match *self.mutate_layout_data().ptr { - None => fail!("no layout data"), - Some(ref mut layout_data) => { - let style = &mut layout_data.after_style; - match *style { - None => (), - Some(ref previous_style) => { - layout_data.restyle_damage = - Some(incremental::compute_damage(previous_style.get(), - computed_values.get()).to_int()) - } - } - *style = Some(computed_values) - } - } - } - fn cascade_subtree(&self, parent: Option) { + let layout_data = unsafe { + self.borrow_layout_data_unchecked().as_ref().unwrap() + }; + macro_rules! cascade_node( + ($applicable_declarations: ident, $style: ident) => {{ + let parent_style = match parent { + Some(ref parent) => Some(parent.style()), + None => None + }; + + let computed_values = Arc::new(cascade( + layout_data.$applicable_declarations, + parent_style.map(|parent_style| parent_style.get()))); + + match *self.mutate_layout_data().ptr { + None => fail!("no layout data"), + Some(ref mut layout_data) => { + let style = &mut layout_data.$style; + match *style { + None => (), + Some(ref previous_style) => { + layout_data.restyle_damage = Some(incremental::compute_damage( + previous_style.get(), computed_values.get()).to_int()) + } + } + *style = Some(computed_values) + } + } + }} + ); + unsafe { if self.borrow_layout_data_unchecked().as_ref().unwrap().before_applicable_declarations.len() > 0 { - self.cascade_before_node(parent); + cascade_node!(before_applicable_declarations, before_style); } } - self.cascade_node(parent); + cascade_node!(applicable_declarations, style); unsafe { if self.borrow_layout_data_unchecked().as_ref().unwrap().after_applicable_declarations.len() > 0 { - self.cascade_after_node(parent); + cascade_node!(after_applicable_declarations, after_style); } }