script: Don't avoid all the mutation notification methods when the st…

…yle 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

@@ -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::<Node>().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::<Node>().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) => {

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<DOMString> {
         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<Ref<'b, AttrValue>> {
         match *self {
             AttributeMutation::Set(_) => Some(attr.value()),

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::<Node>().is_in_doc() || mutation == AttributeMutation::Removed {
+        if !self.upcast::<Node>().is_in_doc() || mutation.is_removal() {
             return;
         }
 

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<RGBA>),
     Dimension(String, LengthOrPercentageOrAuto),
     Url(String, Option<ServoUrl>),
+
+    /// 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<RwLock<PropertyDeclarationBlock>>)
 }
 
 /// 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,
         }