From 7a9cdbb85726b64edb99384557b69a4b066c18cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 26 Jan 2017 23:01:06 +0100 Subject: [PATCH 1/7] style: Add a style attribute restyle hint. --- components/script/dom/document.rs | 2 +- components/style/restyle_hints.rs | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index be44ae0a91c..9ae34f52fcf 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -179,7 +179,7 @@ impl PendingRestyle { } } -// https://dom.spec.whatwg.org/#document +/// https://dom.spec.whatwg.org/#document #[dom_struct] pub struct Document { node: Node, diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index 89a8d273595..8528ef116b6 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -30,14 +30,23 @@ bitflags! { /// expensive, and so we use RestyleHints to short-circuit work we know is /// unnecessary. pub flags RestyleHint: u32 { - #[doc = "Rerun selector matching on the element."] + /// Rerun selector matching on the element. const RESTYLE_SELF = 0x01, - #[doc = "Rerun selector matching on all of the element's descendants."] - // NB: In Gecko, we have RESTYLE_SUBTREE which is inclusive of self, but heycam isn't aware - // of a good reason for that. + + /// Rerun selector matching on all of the element's descendants. + /// + /// NB: In Gecko, we have RESTYLE_SUBTREE which is inclusive of self, + /// but heycam isn't aware of a good reason for that. const RESTYLE_DESCENDANTS = 0x02, - #[doc = "Rerun selector matching on all later siblings of the element and all of their descendants."] + + /// Rerun selector matching on all later siblings of the element and all + /// of their descendants. const RESTYLE_LATER_SIBLINGS = 0x08, + + /// Don't re-run selector-matching on the element, only the style + /// attribute has changed, and this change didn't have any other + /// dependencies. + const RESTYLE_STYLE_ATTRIBUTE = 0x10, } } From d37e6f8c66cd35aef8f5dd7b322649701db93e5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 26 Jan 2017 23:05:29 +0100 Subject: [PATCH 2/7] script: Propagate the attribute into the document's element_attr_will_change method. --- components/script/dom/attr.rs | 2 +- components/script/dom/document.rs | 2 +- components/script/dom/element.rs | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/components/script/dom/attr.rs b/components/script/dom/attr.rs index c44cfd4367d..29c3279ad08 100644 --- a/components/script/dom/attr.rs +++ b/components/script/dom/attr.rs @@ -170,7 +170,7 @@ impl AttrMethods for Attr { impl Attr { pub fn set_value(&self, mut value: AttrValue, owner: &Element) { assert!(Some(owner) == self.owner().r()); - owner.will_mutate_attr(); + owner.will_mutate_attr(self); self.swap_value(&mut value); if self.identifier.namespace == ns!() { vtable_for(owner.upcast()) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 9ae34f52fcf..0af5bdfc83a 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -2157,7 +2157,7 @@ impl Document { } } - pub fn element_attr_will_change(&self, el: &Element) { + pub fn element_attr_will_change(&self, el: &Element, _attr: &Attr) { let mut snapshot = self.ensure_snapshot(el); if snapshot.attrs.is_none() { let attrs = el.attrs() diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index f85baf3811e..100d1ea18d0 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -962,7 +962,7 @@ impl Element { pub fn push_attribute(&self, attr: &Attr) { assert!(attr.GetOwnerElement().r() == Some(self)); - self.will_mutate_attr(); + self.will_mutate_attr(attr); self.attrs.borrow_mut().push(JS::from_ref(attr)); if attr.namespace() == &ns!() { vtable_for(self.upcast()).attribute_mutated(attr, AttributeMutation::Set(None)); @@ -1088,8 +1088,8 @@ impl Element { let idx = self.attrs.borrow().iter().position(|attr| find(&attr)); idx.map(|idx| { - self.will_mutate_attr(); let attr = Root::from_ref(&*(*self.attrs.borrow())[idx]); + self.will_mutate_attr(&attr); self.attrs.borrow_mut().remove(idx); attr.set_owner(None); if attr.namespace() == &ns!() { @@ -1227,9 +1227,9 @@ impl Element { self.set_attribute(local_name, AttrValue::UInt(value.to_string(), value)); } - pub fn will_mutate_attr(&self) { + pub fn will_mutate_attr(&self, attr: &Attr) { let node = self.upcast::(); - node.owner_doc().element_attr_will_change(self); + node.owner_doc().element_attr_will_change(self, attr); } // https://dom.spec.whatwg.org/#insert-adjacent @@ -1502,7 +1502,7 @@ impl ElementMethods for Element { } // Step 4. - self.will_mutate_attr(); + self.will_mutate_attr(attr); attr.set_owner(Some(self)); self.attrs.borrow_mut()[position] = JS::from_ref(attr); old_attr.set_owner(None); From cb304fd42ca6e3ca3b6216feec48d37d844f5f42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 28 Jan 2017 23:45:21 +0100 Subject: [PATCH 3/7] script: insert the restyle attribute restyle hint when a style attribute is modified. --- components/script/dom/document.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 0af5bdfc83a..b2e93325236 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -129,7 +129,7 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use style::attr::AttrValue; use style::context::{QuirksMode, ReflowGoal}; -use style::restyle_hints::RestyleHint; +use style::restyle_hints::{RestyleHint, RESTYLE_STYLE_ATTRIBUTE}; use style::selector_parser::{RestyleDamage, Snapshot}; use style::str::{split_html_space_chars, str_join}; use style::stylesheets::Stylesheet; @@ -2142,23 +2142,32 @@ impl Document { RefMut::map(map, |m| m.entry(JS::from_ref(el)).or_insert_with(PendingRestyle::new)) } - pub fn ensure_snapshot(&self, el: &Element) -> RefMut { + pub fn element_state_will_change(&self, el: &Element) { let mut entry = self.ensure_pending_restyle(el); if entry.snapshot.is_none() { entry.snapshot = Some(Snapshot::new(el.html_element_in_html_document())); } - RefMut::map(entry, |e| e.snapshot.as_mut().unwrap()) - } - - pub fn element_state_will_change(&self, el: &Element) { - let mut snapshot = self.ensure_snapshot(el); + let mut snapshot = entry.snapshot.as_mut().unwrap(); if snapshot.state.is_none() { snapshot.state = Some(el.state()); } } - pub fn element_attr_will_change(&self, el: &Element, _attr: &Attr) { - let mut snapshot = self.ensure_snapshot(el); + pub fn element_attr_will_change(&self, el: &Element, attr: &Attr) { + // FIXME(emilio): Kind of a shame we have to duplicate this. + // + // I'm getting rid of the whole hashtable soon anyway, since all it does + // right now is populate the element restyle data in layout, and we + // could in theory do it in the DOM I think. + let mut entry = self.ensure_pending_restyle(el); + if entry.snapshot.is_none() { + entry.snapshot = Some(Snapshot::new(el.html_element_in_html_document())); + } + if attr.local_name() == &local_name!("style") { + entry.hint |= RESTYLE_STYLE_ATTRIBUTE; + } + + let mut snapshot = entry.snapshot.as_mut().unwrap(); if snapshot.attrs.is_none() { let attrs = el.attrs() .iter() From 31ecc9b692f5108d479546af1e455f646e5439bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 29 Jan 2017 01:25:56 +0100 Subject: [PATCH 4/7] script: Don't avoid all the mutation notification methods when the style attribute changes. Styling was correct because of the explicit dirtiness, but still not fun. Some things, like dynamic updates to with things like [style~="color"] ~ foo selectors, were pretty broken, because we didn't take snapshots of those attributes. --- components/script/dom/cssstyledeclaration.rs | 41 ++++++---- components/script/dom/element.rs | 83 +++++++++++--------- components/script/dom/htmllinkelement.rs | 2 +- components/style/attr.rs | 22 +++++- 4 files changed, 93 insertions(+), 55 deletions(-) diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs index f04bd4017ee..12bf4386433 100644 --- a/components/script/dom/cssstyledeclaration.rs +++ b/components/script/dom/cssstyledeclaration.rs @@ -11,12 +11,13 @@ use dom::bindings::reflector::{DomObject, Reflector, reflect_dom_object}; use dom::bindings::str::DOMString; use dom::cssrule::CSSRule; use dom::element::Element; -use dom::node::{Node, NodeDamage, window_from_node}; +use dom::node::{Node, window_from_node}; use dom::window::Window; use parking_lot::RwLock; use servo_url::ServoUrl; use std::ascii::AsciiExt; use std::sync::Arc; +use style::attr::AttrValue; use style::parser::ParserContextExtraData; use style::properties::{Importance, PropertyDeclarationBlock, PropertyId, LonghandId, ShorthandId}; use style::properties::{parse_one_declaration, parse_style_attribute}; @@ -53,16 +54,12 @@ impl CSSStyleOwner { let mut changed = true; match *self { CSSStyleOwner::Element(ref el) => { - let mut attr = el.style_attribute().borrow_mut(); - let (result, needs_clear) = if attr.is_some() { + let mut attr = el.style_attribute().borrow_mut().take(); + let result = if attr.is_some() { let lock = attr.as_ref().unwrap(); let mut pdb = lock.write(); let result = f(&mut pdb, &mut changed); - if changed { - el.set_style_attr(pdb.to_css_string()); - el.upcast::().dirty(NodeDamage::NodeStyleDamaged); - } - (result, pdb.declarations.is_empty()) + result } else { let mut pdb = PropertyDeclarationBlock { important_count: 0, @@ -72,18 +69,32 @@ impl CSSStyleOwner { // Here `changed` is somewhat silly, because we know the // exact conditions under it changes. - if !pdb.declarations.is_empty() { - el.set_style_attr(pdb.to_css_string()); - el.upcast::().dirty(NodeDamage::NodeStyleDamaged); - *attr = Some(Arc::new(RwLock::new(pdb))); + changed = !pdb.declarations.is_empty(); + if changed { + attr = Some(Arc::new(RwLock::new(pdb))); } - (result, false) + result }; - if needs_clear { - *attr = None; + if changed { + // Note that there's no need to remove the attribute here if + // the declaration block is empty[1], and if `attr` is + // `None` it means that it necessarily didn't change, so no + // need to go through all the set_attribute machinery. + // + // [1]: https://github.com/whatwg/html/issues/2306 + if let Some(pdb) = attr { + let serialization = pdb.read().to_css_string(); + el.set_attribute(&local_name!("style"), + AttrValue::Declaration(serialization, + pdb)); + } + } else { + // Remember to put it back. + *el.style_attribute().borrow_mut() = attr; } + result } CSSStyleOwner::CSSRule(ref rule, ref pdb) => { diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 100d1ea18d0..34f9aa19742 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -824,31 +824,6 @@ impl Element { } } - // this sync method is called upon modification of the style_attribute property, - // therefore, it should not trigger subsequent mutation events - pub fn set_style_attr(&self, new_value: String) { - let mut new_style = AttrValue::String(new_value); - - if let Some(style_attr) = self.attrs.borrow().iter().find(|a| a.name() == &local_name!("style")) { - style_attr.swap_value(&mut new_style); - return; - } - - // explicitly not calling the push_new_attribute convenience method - // in order to avoid triggering mutation events - let window = window_from_node(self); - let attr = Attr::new(&window, - local_name!("style"), - new_style, - local_name!("style"), - ns!(), - None, - Some(self)); - - assert!(attr.GetOwnerElement().r() == Some(self)); - self.attrs.borrow_mut().push(JS::from_ref(&attr)); - } - pub fn serialize(&self, traversal_scope: TraversalScope) -> Fallible { let mut writer = vec![]; match serialize(&mut writer, @@ -2121,18 +2096,43 @@ impl VirtualMethods for Element { match attr.local_name() { &local_name!("style") => { // Modifying the `style` attribute might change style. - *self.style_attribute.borrow_mut() = - mutation.new_value(attr).map(|value| { - let win = window_from_node(self); - Arc::new(RwLock::new(parse_style_attribute( - &value, - &doc.base_url(), - win.css_error_reporter(), - ParserContextExtraData::default()))) - }); - if node.is_in_doc() { - node.dirty(NodeDamage::NodeStyleDamaged); - } + *self.style_attribute.borrow_mut() = match mutation { + AttributeMutation::Set(..) => { + // This is the fast path we use from + // CSSStyleDeclaration. + // + // Juggle a bit to keep the borrow checker happy + // while avoiding the extra clone. + let is_declaration = match *attr.value() { + AttrValue::Declaration(..) => true, + _ => false, + }; + + let block = if is_declaration { + let mut value = AttrValue::String(String::new()); + attr.swap_value(&mut value); + let (serialization, block) = match value { + AttrValue::Declaration(s, b) => (s, b), + _ => unreachable!(), + }; + let mut value = AttrValue::String(serialization); + attr.swap_value(&mut value); + block + } else { + let win = window_from_node(self); + Arc::new(RwLock::new(parse_style_attribute( + &attr.value(), + &doc.base_url(), + win.css_error_reporter(), + ParserContextExtraData::default()))) + }; + + Some(block) + } + AttributeMutation::Removed => { + None + } + }; }, &local_name!("id") => { *self.id_attribute.borrow_mut() = @@ -2716,7 +2716,7 @@ impl Element { } } -#[derive(Clone, Copy, PartialEq)] +#[derive(Clone, Copy)] pub enum AttributeMutation<'a> { /// The attribute is set, keep track of old value. /// https://dom.spec.whatwg.org/#attribute-is-set @@ -2728,6 +2728,13 @@ pub enum AttributeMutation<'a> { } impl<'a> AttributeMutation<'a> { + pub fn is_removal(&self) -> bool { + match *self { + AttributeMutation::Removed => true, + AttributeMutation::Set(..) => false, + } + } + pub fn new_value<'b>(&self, attr: &'b Attr) -> Option> { match *self { AttributeMutation::Set(_) => Some(attr.value()), diff --git a/components/script/dom/htmllinkelement.rs b/components/script/dom/htmllinkelement.rs index 4db3949d4f9..20e560e93ad 100644 --- a/components/script/dom/htmllinkelement.rs +++ b/components/script/dom/htmllinkelement.rs @@ -166,7 +166,7 @@ impl VirtualMethods for HTMLLinkElement { fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) { self.super_type().unwrap().attribute_mutated(attr, mutation); - if !self.upcast::().is_in_doc() || mutation == AttributeMutation::Removed { + if !self.upcast::().is_in_doc() || mutation.is_removal() { return; } diff --git a/components/style/attr.rs b/components/style/attr.rs index 17517da2dd9..ab3097d5ae7 100644 --- a/components/style/attr.rs +++ b/components/style/attr.rs @@ -11,9 +11,12 @@ use app_units::Au; use cssparser::{self, Color, RGBA}; use euclid::num::Zero; use num_traits::ToPrimitive; +use parking_lot::RwLock; +use properties::PropertyDeclarationBlock; use servo_url::ServoUrl; use std::ascii::AsciiExt; use std::str::FromStr; +use std::sync::Arc; use str::{HTML_SPACE_CHARACTERS, read_exponent, read_fraction}; use str::{read_numbers, split_commas, split_html_space_chars}; #[cfg(not(feature = "gecko"))] use str::str_join; @@ -30,7 +33,7 @@ pub enum LengthOrPercentageOrAuto { Length(Au), } -#[derive(PartialEq, Clone, Debug)] +#[derive(Clone, Debug)] #[cfg_attr(feature = "servo", derive(HeapSizeOf))] pub enum AttrValue { String(String), @@ -43,6 +46,22 @@ pub enum AttrValue { Color(String, Option), Dimension(String, LengthOrPercentageOrAuto), Url(String, Option), + + /// Note that this variant is only used transitively as a fast path to set + /// the property declaration block relevant to the style of an element when + /// set from the inline declaration of that element (that is, + /// `element.style`). + /// + /// This can, as of this writing, only correspond to the value of the + /// `style` element, and is set from its relevant CSSInlineStyleDeclaration, + /// and then converted to a string in Element::attribute_mutated. + /// + /// Note that we don't necessarily need to do that (we could just clone the + /// declaration block), but that avoids keeping a refcounted + /// declarationblock for longer than needed. + Declaration(String, + #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] + Arc>) } /// Shared implementation to parse an integer according to @@ -330,6 +349,7 @@ impl ::std::ops::Deref for AttrValue { AttrValue::Color(ref value, _) | AttrValue::Int(ref value, _) | AttrValue::Url(ref value, _) | + AttrValue::Declaration(ref value, _) | AttrValue::Dimension(ref value, _) => &value, AttrValue::Atom(ref value) => &value, } From 314f2ce714e819bfd14da69a44b117cae7222fbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 29 Jan 2017 02:59:54 +0100 Subject: [PATCH 5/7] style: Define a CascadeLevel enum, and make the rule tree operate on it. We'll use this level to know where to stop replacing nodes in the tree. --- components/script/dom/element.rs | 3 +- components/style/matching.rs | 12 ++-- components/style/rule_tree/mod.rs | 85 ++++++++++++++++++++++----- components/style/stylist.rs | 91 ++++++++++++++++------------- tests/unit/style/rule_tree/bench.rs | 12 ++-- tests/unit/style/stylist.rs | 5 +- 6 files changed, 144 insertions(+), 64 deletions(-) diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 34f9aa19742..e93becfffdd 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -105,6 +105,7 @@ use style::properties::{DeclaredValue, Importance}; use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, parse_style_attribute}; use style::properties::longhands::{background_image, border_spacing, font_family, font_size, overflow_x}; use style::restyle_hints::RESTYLE_SELF; +use style::rule_tree::CascadeLevel; use style::selector_parser::{NonTSPseudoClass, RestyleDamage, SelectorImpl, SelectorParser}; use style::sink::Push; use style::stylist::ApplicableDeclarationBlock; @@ -380,7 +381,7 @@ impl LayoutElementHelpers for LayoutJS { declarations: vec![(declaration, Importance::Normal)], important_count: 0, })), - Importance::Normal) + CascadeLevel::PresHints) } let bgcolor = if let Some(this) = self.downcast::() { diff --git a/components/style/matching.rs b/components/style/matching.rs index 11c1a85b382..51f05b8bdec 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -573,7 +573,7 @@ fn compute_rule_node(context: &StyleContext, applicable_declarations: &mut Vec) -> StrongRuleNode { - let rules = applicable_declarations.drain(..).map(|d| (d.source, d.importance)); + let rules = applicable_declarations.drain(..).map(|d| (d.source, d.level)); let rule_node = context.shared.stylist.rule_tree.insert_ordered_rules(rules); rule_node } @@ -583,10 +583,14 @@ impl PrivateMatchMethods for E {} /// The public API that elements expose for selector matching. pub trait MatchMethods : TElement { /// Runs selector matching of this element, and returns the result. - fn match_element(&self, context: &StyleContext, parent_bf: Option<&BloomFilter>) + fn match_element(&self, + context: &StyleContext, + parent_bf: Option<&BloomFilter>) -> MatchResults { - let mut applicable_declarations: Vec = Vec::with_capacity(16); + let mut applicable_declarations = + Vec::::with_capacity(16); + let stylist = &context.shared.stylist; let style_attribute = self.style_attribute(); let animation_rules = self.get_animation_rules(None); @@ -609,7 +613,7 @@ pub trait MatchMethods : TElement { let pseudo_animation_rules = self.get_animation_rules(Some(&pseudo)); stylist.push_applicable_declarations(self, parent_bf, None, pseudo_animation_rules, - Some(&pseudo.clone()), + Some(&pseudo), &mut applicable_declarations, MatchingReason::ForStyling); diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 932b30a8125..ee5ac080b49 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -151,11 +151,11 @@ impl RuleTree { /// 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 - where I: Iterator, + where I: Iterator, { let mut current = self.root.clone(); - for (source, importance) in iter { - current = current.ensure_child(self.root.downgrade(), source, importance); + for (source, level) in iter { + current = current.ensure_child(self.root.downgrade(), source, level); } current } @@ -176,6 +176,66 @@ impl RuleTree { /// where it likely did not result from a rigorous performance analysis.) const RULE_TREE_GC_INTERVAL: usize = 300; +/// The cascade level these rules are relevant at, as per[1]. +/// +/// The order of variants declared here is significant, and must be in +/// _ascending_ order of precedence. +/// +/// [1]: https://drafts.csswg.org/css-cascade/#cascade-origin +#[repr(u8)] +#[derive(Eq, PartialEq, Copy, Clone, Debug)] +#[cfg_attr(feature = "servo", derive(HeapSizeOf))] +pub enum CascadeLevel { + /// Normal User-Agent rules. + UANormal = 0, + /// Presentational hints. + PresHints, + /// User normal rules. + UserNormal, + /// Author normal rules. + AuthorNormal, + /// Style attribute normal rules. + StyleAttributeNormal, + /// CSS animations and script-generated animations. + Animations, + /// Author-supplied important rules. + AuthorImportant, + /// Style attribute important rules. + StyleAttributeImportant, + /// User important rules. + UserImportant, + /// User-agent important rules. + UAImportant, + /// Transitions + Transitions, +} + +impl CascadeLevel { + /// Returns whether this cascade level represents important rules of some + /// sort. + #[inline] + pub fn is_important(&self) -> bool { + match *self { + CascadeLevel::AuthorImportant | + CascadeLevel::StyleAttributeImportant | + CascadeLevel::UserImportant | + CascadeLevel::UAImportant => true, + _ => false, + } + } + + /// Returns the importance relevant for this rule. Pretty similar to + /// `is_important`. + #[inline] + pub fn importance(&self) -> Importance { + if self.is_important() { + Importance::Important + } else { + Importance::Normal + } + } +} + struct RuleNode { /// The root node. Only the root has no root pointer, for obvious reasons. root: Option, @@ -187,9 +247,8 @@ struct RuleNode { /// or a raw property declaration block (like the style attribute). source: Option, - /// The importance of the declarations relevant in the style rule, - /// meaningless in the root node. - importance: Importance, + /// The cascade level this rule is positioned at. + cascade_level: CascadeLevel, refcount: AtomicUsize, first_child: AtomicPtr, @@ -215,13 +274,13 @@ impl RuleNode { fn new(root: WeakRuleNode, parent: StrongRuleNode, source: StyleSource, - importance: Importance) -> Self { + cascade_level: CascadeLevel) -> Self { debug_assert!(root.upgrade().parent().is_none()); RuleNode { root: Some(root), parent: Some(parent), source: Some(source), - importance: importance, + cascade_level: cascade_level, refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), @@ -236,7 +295,7 @@ impl RuleNode { root: None, parent: None, source: None, - importance: Importance::Normal, + cascade_level: CascadeLevel::UANormal, refcount: AtomicUsize::new(1), first_child: AtomicPtr::new(ptr::null_mut()), next_sibling: AtomicPtr::new(ptr::null_mut()), @@ -385,10 +444,10 @@ impl StrongRuleNode { fn ensure_child(&self, root: WeakRuleNode, source: StyleSource, - importance: Importance) -> StrongRuleNode { + cascade_level: CascadeLevel) -> StrongRuleNode { let mut last = None; for child in self.get().iter_children() { - if child .get().importance == importance && + if child .get().cascade_level == cascade_level && child.get().source.as_ref().unwrap().ptr_equals(&source) { return child; } @@ -398,7 +457,7 @@ impl StrongRuleNode { let mut node = Box::new(RuleNode::new(root, self.clone(), source.clone(), - importance)); + cascade_level)); let new_ptr: *mut RuleNode = &mut *node; loop { @@ -462,7 +521,7 @@ impl StrongRuleNode { /// Get the importance that this rule node represents. pub fn importance(&self) -> Importance { - self.get().importance + self.get().cascade_level.importance() } /// Get an iterator for this rule node and its ancestors. diff --git a/components/style/stylist.rs b/components/style/stylist.rs index ab2a9fc14fe..5530bd98554 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -14,10 +14,10 @@ use keyframes::KeyframesAnimation; use media_queries::Device; use parking_lot::RwLock; use pdqsort::sort_by; -use properties::{self, CascadeFlags, ComputedValues, INHERIT_ALL, Importance}; -use properties::{PropertyDeclaration, PropertyDeclarationBlock}; +use properties::{self, CascadeFlags, ComputedValues, INHERIT_ALL}; +use properties::PropertyDeclarationBlock; use restyle_hints::{RestyleHint, DependencySet}; -use rule_tree::{RuleTree, StrongRuleNode, StyleSource}; +use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource}; use selector_parser::{ElementExt, SelectorImpl, PseudoElement, Snapshot}; use selectors::Element; use selectors::bloom::BloomFilter; @@ -31,7 +31,6 @@ use std::borrow::Borrow; use std::collections::HashMap; use std::fmt; use std::hash::Hash; -use std::slice; use std::sync::Arc; use style_traits::viewport::ViewportConstraints; use stylesheets::{CssRule, Origin, StyleRule, Stylesheet, UserAgentStylesheets}; @@ -215,7 +214,9 @@ impl Stylist { // extra field, and avoid this precomputation entirely. if let Some(map) = self.pseudos_map.remove(&pseudo) { let mut declarations = vec![]; - map.user_agent.get_universal_rules(&mut declarations); + map.user_agent.get_universal_rules(&mut declarations, + CascadeLevel::UANormal, + CascadeLevel::UAImportant); self.precomputed_pseudo_element_decls.insert(pseudo, declarations); } }); @@ -311,7 +312,7 @@ impl Stylist { // FIXME(emilio): When we've taken rid of the cascade we can just // use into_iter. self.rule_tree.insert_ordered_rules( - declarations.into_iter().map(|a| (a.source.clone(), a.importance))) + declarations.into_iter().map(|a| (a.source.clone(), a.level))) } None => self.rule_tree.root(), }; @@ -394,7 +395,7 @@ impl Stylist { let rule_node = self.rule_tree.insert_ordered_rules( - declarations.into_iter().map(|a| (a.source, a.importance))); + declarations.into_iter().map(|a| (a.source, a.level))); let computed = properties::cascade(self.device.au_viewport_size(), @@ -527,13 +528,18 @@ impl Stylist { applicable_declarations, &mut relations, reason, - Importance::Normal); + CascadeLevel::UANormal); debug!("UA normal: {:?}", relations); // Step 2: Presentational hints. - let length = applicable_declarations.len(); + let length_before_preshints = applicable_declarations.len(); element.synthesize_presentational_hints_for_legacy_attributes(applicable_declarations); - if applicable_declarations.len() != length { + if applicable_declarations.len() != length_before_preshints { + if cfg!(debug_assertions) { + for declaration in &applicable_declarations[length_before_preshints..] { + assert_eq!(declaration.level, CascadeLevel::PresHints); + } + } // Never share style for elements with preshints relations |= AFFECTED_BY_PRESENTATIONAL_HINTS; } @@ -546,14 +552,14 @@ impl Stylist { applicable_declarations, &mut relations, reason, - Importance::Normal); + CascadeLevel::UserNormal); debug!("user normal: {:?}", relations); map.author.get_all_matching_rules(element, parent_bf, applicable_declarations, &mut relations, reason, - Importance::Normal); + CascadeLevel::AuthorNormal); debug!("author normal: {:?}", relations); // Step 4: Normal style attributes. @@ -562,7 +568,8 @@ impl Stylist { relations |= AFFECTED_BY_STYLE_ATTRIBUTE; Push::push( applicable_declarations, - ApplicableDeclarationBlock::from_declarations(sa.clone(), Importance::Normal)); + ApplicableDeclarationBlock::from_declarations(sa.clone(), + CascadeLevel::StyleAttributeNormal)); } } @@ -575,7 +582,8 @@ impl Stylist { relations |= AFFECTED_BY_ANIMATIONS; Push::push( applicable_declarations, - ApplicableDeclarationBlock::from_declarations(anim.clone(), Importance::Normal)); + ApplicableDeclarationBlock::from_declarations(anim.clone(), + CascadeLevel::Animations)); } debug!("animation: {:?}", relations); @@ -585,7 +593,7 @@ impl Stylist { applicable_declarations, &mut relations, reason, - Importance::Important); + CascadeLevel::AuthorImportant); debug!("author important: {:?}", relations); @@ -595,7 +603,8 @@ impl Stylist { relations |= AFFECTED_BY_STYLE_ATTRIBUTE; Push::push( applicable_declarations, - ApplicableDeclarationBlock::from_declarations(sa.clone(), Importance::Important)); + ApplicableDeclarationBlock::from_declarations(sa.clone(), + CascadeLevel::StyleAttributeImportant)); } } @@ -607,7 +616,7 @@ impl Stylist { applicable_declarations, &mut relations, reason, - Importance::Important); + CascadeLevel::UserImportant); debug!("user important: {:?}", relations); } else { @@ -620,7 +629,7 @@ impl Stylist { applicable_declarations, &mut relations, reason, - Importance::Important); + CascadeLevel::UAImportant); debug!("UA important: {:?}", relations); @@ -630,7 +639,7 @@ impl Stylist { relations |= AFFECTED_BY_TRANSITIONS; Push::push( applicable_declarations, - ApplicableDeclarationBlock::from_declarations(anim.clone(), Importance::Normal)); + ApplicableDeclarationBlock::from_declarations(anim.clone(), CascadeLevel::Transitions)); } debug!("transition: {:?}", relations); @@ -858,7 +867,7 @@ impl SelectorMap { matching_rules_list: &mut V, relations: &mut StyleRelations, reason: MatchingReason, - importance: Importance) + cascade_level: CascadeLevel) where E: Element, V: VecLike { @@ -876,7 +885,7 @@ impl SelectorMap { matching_rules_list, relations, reason, - importance) + cascade_level) } element.each_class(|class| { @@ -887,7 +896,7 @@ impl SelectorMap { matching_rules_list, relations, reason, - importance); + cascade_level); }); let local_name_hash = if element.is_html_element_in_html_document() { @@ -902,7 +911,7 @@ impl SelectorMap { matching_rules_list, relations, reason, - importance); + cascade_level); SelectorMap::get_matching_rules(element, parent_bf, @@ -910,7 +919,7 @@ impl SelectorMap { matching_rules_list, relations, reason, - importance); + cascade_level); // Sort only the rules we just added. sort_by_key(&mut matching_rules_list[init_len..], @@ -918,11 +927,15 @@ impl SelectorMap { } /// Append to `rule_list` all universal Rules (rules with selector `*|*`) in - /// `self` sorted by specifity and source order. + /// `self` sorted by specificity and source order. pub fn get_universal_rules(&self, - matching_rules_list: &mut V) + matching_rules_list: &mut V, + cascade_level: CascadeLevel, + important_cascade_level: CascadeLevel) where V: VecLike { + debug_assert!(!cascade_level.is_important()); + debug_assert!(important_cascade_level.is_important()); if self.empty { return } @@ -936,11 +949,11 @@ impl SelectorMap { let block = guard.block.read(); if block.any_normal() { matching_rules_list.push( - rule.to_applicable_declaration_block(Importance::Normal)); + rule.to_applicable_declaration_block(cascade_level)); } if block.any_important() { matching_rules_list.push( - rule.to_applicable_declaration_block(Importance::Important)); + rule.to_applicable_declaration_block(important_cascade_level)); } } } @@ -957,7 +970,7 @@ impl SelectorMap { matching_rules: &mut Vector, relations: &mut StyleRelations, reason: MatchingReason, - importance: Importance) + cascade_level: CascadeLevel) where E: Element, Str: Borrow + Eq + Hash, BorrowedStr: Eq + Hash, @@ -970,7 +983,7 @@ impl SelectorMap { matching_rules, relations, reason, - importance) + cascade_level) } } @@ -981,14 +994,14 @@ impl SelectorMap { matching_rules: &mut V, relations: &mut StyleRelations, reason: MatchingReason, - importance: Importance) + cascade_level: CascadeLevel) where E: Element, V: VecLike { for rule in rules.iter() { let guard = rule.style_rule.read(); let block = guard.block.read(); - let any_declaration_for_importance = if importance.important() { + let any_declaration_for_importance = if cascade_level.is_important() { block.any_important() } else { block.any_normal() @@ -997,7 +1010,7 @@ impl SelectorMap { matches_complex_selector(&*rule.selector, element, parent_bf, relations, reason) { matching_rules.push( - rule.to_applicable_declaration_block(importance)); + rule.to_applicable_declaration_block(cascade_level)); } } } @@ -1093,10 +1106,10 @@ pub struct Rule { } impl Rule { - fn to_applicable_declaration_block(&self, importance: Importance) -> ApplicableDeclarationBlock { + fn to_applicable_declaration_block(&self, level: CascadeLevel) -> ApplicableDeclarationBlock { ApplicableDeclarationBlock { source: StyleSource::Style(self.style_rule.clone()), - importance: importance, + level: level, source_order: self.source_order, specificity: self.specificity, } @@ -1114,8 +1127,8 @@ pub struct ApplicableDeclarationBlock { /// The style source, either a style rule, or a property declaration block. #[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")] pub source: StyleSource, - /// The importance of this declaration block. - pub importance: Importance, + /// The cascade level this applicable declaration block is in. + pub level: CascadeLevel, /// The source order of this block. pub source_order: usize, /// The specificity of the selector this block is represented by. @@ -1127,13 +1140,13 @@ impl ApplicableDeclarationBlock { /// declaration block and importance. #[inline] pub fn from_declarations(declarations: Arc>, - importance: Importance) + level: CascadeLevel) -> Self { ApplicableDeclarationBlock { source: StyleSource::Declarations(declarations), + level: level, source_order: 0, specificity: 0, - importance: importance, } } } diff --git a/tests/unit/style/rule_tree/bench.rs b/tests/unit/style/rule_tree/bench.rs index 2504c6f2893..de68d99b71d 100644 --- a/tests/unit/style/rule_tree/bench.rs +++ b/tests/unit/style/rule_tree/bench.rs @@ -11,7 +11,7 @@ use style::error_reporting::ParseErrorReporter; use style::media_queries::MediaList; use style::parser::ParserContextExtraData; use style::properties::{longhands, DeclaredValue, Importance, PropertyDeclaration, PropertyDeclarationBlock}; -use style::rule_tree::{RuleTree, StrongRuleNode, StyleSource}; +use style::rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource}; use style::stylesheets::{Origin, Stylesheet, CssRule}; use test::{self, Bencher}; @@ -40,7 +40,7 @@ impl<'a> Drop for AutoGCRuleTree<'a> { } } -fn parse_rules(css: &str) -> Vec<(StyleSource, Importance)> { +fn parse_rules(css: &str) -> Vec<(StyleSource, CascadeLevel)> { let s = Stylesheet::from_str(css, ServoUrl::parse("http://localhost").unwrap(), Origin::Author, @@ -57,15 +57,15 @@ fn parse_rules(css: &str) -> Vec<(StyleSource, Importance)> { _ => None, } }).cloned().map(StyleSource::Style).map(|s| { - (s, Importance::Normal) + (s, CascadeLevel::UserNormal) }).collect() } -fn test_insertion(rule_tree: &RuleTree, rules: Vec<(StyleSource, Importance)>) -> StrongRuleNode { +fn test_insertion(rule_tree: &RuleTree, rules: Vec<(StyleSource, CascadeLevel)>) -> StrongRuleNode { rule_tree.insert_ordered_rules(rules.into_iter()) } -fn test_insertion_style_attribute(rule_tree: &RuleTree, rules: &[(StyleSource, Importance)]) -> StrongRuleNode { +fn test_insertion_style_attribute(rule_tree: &RuleTree, rules: &[(StyleSource, CascadeLevel)]) -> StrongRuleNode { let mut rules = rules.to_vec(); rules.push((StyleSource::Declarations(Arc::new(RwLock::new(PropertyDeclarationBlock { declarations: vec![ @@ -74,7 +74,7 @@ fn test_insertion_style_attribute(rule_tree: &RuleTree, rules: &[(StyleSource, I Importance::Normal), ], important_count: 0, - }))), Importance::Normal)); + }))), CascadeLevel::UserNormal)); test_insertion(rule_tree, rules) } diff --git a/tests/unit/style/stylist.rs b/tests/unit/style/stylist.rs index 98c5b68a2ef..fe330d865c7 100644 --- a/tests/unit/style/stylist.rs +++ b/tests/unit/style/stylist.rs @@ -9,6 +9,7 @@ use servo_atoms::Atom; use std::sync::Arc; use style::properties::{PropertyDeclarationBlock, PropertyDeclaration, DeclaredValue}; use style::properties::{longhands, Importance}; +use style::rule_tree::CascadeLevel; use style::selector_parser::SelectorParser; use style::stylesheets::StyleRule; use style::stylist::{Rule, SelectorMap}; @@ -113,7 +114,9 @@ fn test_get_universal_rules() { let map = get_mock_map(&["*|*", "#foo > *|*", ".klass", "#id"]); let mut decls = vec![]; - map.get_universal_rules(&mut decls); + map.get_universal_rules(&mut decls, + CascadeLevel::UserNormal, + CascadeLevel::UserImportant); assert_eq!(decls.len(), 1); } From f48328edfe06003d86c79f331a7fa6541b78d819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 29 Jan 2017 03:02:39 +0100 Subject: [PATCH 6/7] style: Remove a bit of unused code. --- components/style/stylist.rs | 33 --------------------------------- 1 file changed, 33 deletions(-) diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 5530bd98554..3d81e3897bc 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1151,39 +1151,6 @@ impl ApplicableDeclarationBlock { } } -/// An iterator over the declarations that a given block represent, which is -/// effectively a filter by importance. -pub struct ApplicableDeclarationBlockIter<'a> { - iter: slice::Iter<'a, (PropertyDeclaration, Importance)>, - importance: Importance, -} - -impl<'a> Iterator for ApplicableDeclarationBlockIter<'a> { - type Item = &'a PropertyDeclaration; - - #[inline] - fn next(&mut self) -> Option { - while let Some(&(ref declaration, importance)) = self.iter.next() { - if importance == self.importance { - return Some(declaration) - } - } - None - } -} - -impl<'a> DoubleEndedIterator for ApplicableDeclarationBlockIter<'a> { - #[inline] - fn next_back(&mut self) -> Option { - while let Some(&(ref declaration, importance)) = self.iter.next_back() { - if importance == self.importance { - return Some(declaration) - } - } - None - } -} - #[inline] fn find_push(map: &mut FnvHashMap>, key: Str, value: Rule) { From 0b2c32a45a182388029594d0efbb2d93688d6f7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 29 Jan 2017 14:20:42 +0100 Subject: [PATCH 7/7] style: Temporarily force selector matching for style attribute restyles. The test failures on try were because of this. --- components/style/data.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/style/data.rs b/components/style/data.rs index 475a04b7023..0c097b103a1 100644 --- a/components/style/data.rs +++ b/components/style/data.rs @@ -203,7 +203,7 @@ impl From for StoredRestyleHint { use self::DescendantRestyleHint::*; debug_assert!(!hint.contains(RESTYLE_LATER_SIBLINGS), "Caller should apply sibling hints"); StoredRestyleHint { - restyle_self: hint.contains(RESTYLE_SELF), + restyle_self: hint.contains(RESTYLE_SELF) || hint.contains(RESTYLE_STYLE_ATTRIBUTE), descendants: if hint.contains(RESTYLE_DESCENDANTS) { Descendants } else { Empty }, } }