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, }