Auto merge of #15256 - emilio:cssstyledeclaration, r=SimonSapin

script: Refactor CSSStyleDeclaration and fix some bugs in the way.

<!-- Please describe your changes on the following line: -->

This is preliminar work for the style attribute restyle hint.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15256)
<!-- Reviewable:end -->

components/script/dom/csskeyframerule.rs

@@ -3,6 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use dom::bindings::codegen::Bindings::CSSKeyframeRuleBinding::{self, CSSKeyframeRuleMethods};
+use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, MutNullableJS, Root};
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
 use dom::bindings::str::DOMString;
@@ -47,8 +48,8 @@ impl CSSKeyframeRuleMethods for CSSKeyframeRule {
     fn Style(&self) -> Root<CSSStyleDeclaration> {
         self.style_decl.or_init(|| {
             CSSStyleDeclaration::new(self.global().as_window(),
-                                     CSSStyleOwner::CSSRule(JS::from_ref(self.global().as_window()),
+                                     CSSStyleOwner::CSSRule(JS::from_ref(self.upcast()),
-                                                                 self.keyframerule.read().block.clone()),
+                                                            self.keyframerule.read().block.clone()),
                                      None,
                                      CSSModificationAccess::ReadWrite)
         })

components/script/dom/cssstyledeclaration.rs

@@ -7,12 +7,14 @@ use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use dom::bindings::error::{Error, ErrorResult, Fallible};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, Root};
-use dom::bindings::reflector::{Reflector, reflect_dom_object};
+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::window::Window;
 use parking_lot::RwLock;
+use servo_url::ServoUrl;
 use std::ascii::AsciiExt;
 use std::sync::Arc;
 use style::parser::ParserContextExtraData;
@@ -34,23 +36,84 @@ pub struct CSSStyleDeclaration {
 #[must_root]
 pub enum CSSStyleOwner {
     Element(JS<Element>),
-    CSSRule(JS<Window>,
+    CSSRule(JS<CSSRule>,
             #[ignore_heap_size_of = "Arc"]
             Arc<RwLock<PropertyDeclarationBlock>>),
 }
 
 impl CSSStyleOwner {
-    fn style_attribute(&self) -> Option<Arc<RwLock<PropertyDeclarationBlock>>> {
+    // Mutate the declaration block associated to this style owner, and
+    // optionally indicate if it has changed (assumed to be true).
+    fn mutate_associated_block<F, R>(&self, f: F) -> R
+        where F: FnOnce(&mut PropertyDeclarationBlock, &mut bool) -> R,
+    {
+        // TODO(emilio): This has some duplication just to avoid dummy clones.
+        //
+        // This is somewhat complex but the complexity is encapsulated.
+        let mut changed = true;
         match *self {
             CSSStyleOwner::Element(ref el) => {
-                if let Some(ref pdb) = *el.style_attribute().borrow() {
+                let mut attr = el.style_attribute().borrow_mut();
-                    Some(pdb.clone())
+                let (result, needs_clear) = 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())
                 } else {
-                    None
+                    let mut pdb = PropertyDeclarationBlock {
+                        important_count: 0,
+                        declarations: vec![],
+                    };
+                    let result = f(&mut pdb, &mut changed);
+
+                    // 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)));
+                    }
+
+                    (result, false)
+                };
+
+                if needs_clear {
+                    *attr = None;
+                }
+                result
+            }
+            CSSStyleOwner::CSSRule(ref rule, ref pdb) => {
+                let result = f(&mut *pdb.write(), &mut changed);
+                if changed {
+                    rule.global().as_window().Document().invalidate_stylesheets();
+                }
+                result
+            }
+        }
+    }
+
+    fn with_block<F, R>(&self, f: F) -> R
+        where F: FnOnce(&PropertyDeclarationBlock) -> R,
+    {
+        match *self {
+            CSSStyleOwner::Element(ref el) => {
+                match *el.style_attribute().borrow() {
+                    Some(ref pdb) => f(&pdb.read()),
+                    None => {
+                        let pdb = PropertyDeclarationBlock {
+                            important_count: 0,
+                            declarations: vec![],
+                        };
+                        f(&pdb)
+                    }
                 }
             }
             CSSStyleOwner::CSSRule(_, ref pdb) => {
-                Some(pdb.clone())
+                f(&pdb.read())
             }
         }
     }
@@ -58,22 +121,16 @@ impl CSSStyleOwner {
     fn window(&self) -> Root<Window> {
         match *self {
             CSSStyleOwner::Element(ref el) => window_from_node(&**el),
-            CSSStyleOwner::CSSRule(ref window, _) => Root::from_ref(&**window),
+            CSSStyleOwner::CSSRule(ref rule, _) => Root::from_ref(rule.global().as_window()),
         }
     }
 
-    fn flush_style(&self, pdb: &PropertyDeclarationBlock) {
+    fn base_url(&self) -> ServoUrl {
-        if let CSSStyleOwner::Element(ref el) = *self {
-            el.set_style_attr(pdb.to_css_string());
-        }
-    }
-
-    fn dirty(&self) {
         match *self {
-            CSSStyleOwner::Element(ref el) =>
+            CSSStyleOwner::Element(ref el) => window_from_node(&**el).Document().base_url(),
-                el.upcast::<Node>().dirty(NodeDamage::NodeStyleDamaged),
+            CSSStyleOwner::CSSRule(ref rule, _) => {
-            CSSStyleOwner::CSSRule(ref window, _) =>
+                rule.parent_stylesheet().style_stylesheet().base_url.clone()
-                window.Document().invalidate_stylesheets(),
+            }
         }
     }
 }
@@ -147,14 +204,13 @@ impl CSSStyleDeclaration {
             return self.get_computed_style(id);
         }
 
-        if let Some(ref lock) = self.owner.style_attribute() {
+        let mut string = String::new();
-            let mut string = String::new();
+
-            lock.read().property_value_to_css(&id, &mut string).unwrap();
+        self.owner.with_block(|ref pdb| {
-            DOMString::from(string)
+            pdb.property_value_to_css(&id, &mut string).unwrap();
-        } else {
+        });
-            // No style attribute is like an empty style attribute: no matching declaration.
+
-            DOMString::new()
+        DOMString::from(string)
-        }
     }
 
     fn set_property(&self, id: PropertyId, value: DOMString, priority: DOMString) -> ErrorResult {
@@ -163,83 +219,59 @@ impl CSSStyleDeclaration {
             return Err(Error::NoModificationAllowed);
         }
 
-        if value.is_empty() {
+        self.owner.mutate_associated_block(|ref mut pdb, mut changed| {
-            // Step 4
+            if value.is_empty() {
-            let empty = {
+                // Step 4
-                if let Some(ref lock) = self.owner.style_attribute() {
+                *changed = pdb.remove_property(&id);
-                    let mut style_attribute = lock.write();
+                return Ok(());
-                    style_attribute.remove_property(&id);
-                    style_attribute.declarations.is_empty()
-                } else {
-                    // No style attribute is like an empty style attribute: nothing to remove.
-                    return Ok(())
-                }
-            };
-            if let (&CSSStyleOwner::Element(ref el), true) = (&self.owner, empty) {
-                *el.style_attribute().borrow_mut() = None;
             }
-        } else {
+
             // Step 5
             let importance = match &*priority {
                 "" => Importance::Normal,
                 p if p.eq_ignore_ascii_case("important") => Importance::Important,
-                _ => return Ok(()),
+                _ => {
+                    *changed = false;
+                    return Ok(());
+                }
             };
 
             // Step 6
             let window = self.owner.window();
             let declarations =
-                parse_one_declaration(id, &value, &window.get_url(), window.css_error_reporter(),
+                parse_one_declaration(id, &value, &self.owner.base_url(),
+                                      window.css_error_reporter(),
                                       ParserContextExtraData::default());
 
             // Step 7
-            let mut declarations = match declarations {
+            let declarations = match declarations {
                 Ok(declarations) => declarations,
-                Err(_) => return Ok(())
+                Err(_) => {
+                    *changed = false;
+                    return Ok(());
+                }
             };
 
             // Step 8
             // Step 9
-            match self.owner.style_attribute() {
+            // We could try to be better I guess?
-                Some(ref lock) => {
+            *changed = !declarations.is_empty();
-                    let mut style_attribute = lock.write();
+            for declaration in declarations {
-                    for declaration in declarations {
+                // TODO(emilio): We could check it changed
-                        style_attribute.set_parsed_declaration(declaration.0, importance);
+                pdb.set_parsed_declaration(declaration.0, importance);
-                    }
-                    self.owner.flush_style(&style_attribute);
-                }
-                None => {
-                    let important_count = if importance.important() {
-                        declarations.len() as u32
-                    } else {
-                        0
-                    };
-                    for decl in &mut declarations {
-                        decl.1 = importance
-                    }
-                    let block = PropertyDeclarationBlock {
-                        declarations: declarations,
-                        important_count: important_count,
-                    };
-                    if let CSSStyleOwner::Element(ref el) = self.owner {
-                        el.set_style_attr(block.to_css_string());
-                        *el.style_attribute().borrow_mut() = Some(Arc::new(RwLock::new(block)));
-                    } else {
-                        panic!("set_property called on a CSSStyleDeclaration with a non-Element owner");
-                    }
-                }
             }
-        }
 
-        self.owner.dirty();
+            Ok(())
-        Ok(())
+        })
     }
 }
 
 impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-length
     fn Length(&self) -> u32 {
-        self.owner.style_attribute().as_ref().map_or(0, |lock| lock.read().declarations.len() as u32)
+        self.owner.with_block(|ref pdb| {
+            pdb.declarations.len() as u32
+        })
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-item
@@ -267,17 +299,14 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
             return DOMString::new()
         };
 
-        if let Some(ref lock) = self.owner.style_attribute() {
+        self.owner.with_block(|ref pdb| {
-            if lock.read().property_priority(&id).important() {
+            if pdb.property_priority(&id).important() {
                 DOMString::from("important")
             } else {
                 // Step 4
                 DOMString::new()
             }
-        } else {
+        })
-            // No style attribute is like an empty style attribute: no matching declaration.
-            DOMString::new()
-        }
     }
 
     // https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setproperty
@@ -304,11 +333,9 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
         }
 
         // Step 2 & 3
-        let id = if let Ok(id) = PropertyId::parse(property.into()) {
+        let id = match PropertyId::parse(property.into()) {
-            id
+            Ok(id) => id,
-        } else {
+            Err(..) => return Ok(()), // Unkwown property
-            // Unkwown property
-            return Ok(())
         };
 
         // Step 4
@@ -318,15 +345,11 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
             _ => return Ok(()),
         };
 
-        if let Some(ref lock) = self.owner.style_attribute() {
+        self.owner.mutate_associated_block(|ref mut pdb, mut changed| {
-            let mut style_attribute = lock.write();
-
             // Step 5 & 6
-            style_attribute.set_importance(&id, importance);
+            *changed = pdb.set_importance(&id, importance);
+        });
 
-            self.owner.flush_style(&style_attribute);
-            self.owner.dirty();
-        }
         Ok(())
     }
 
@@ -350,25 +373,10 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
         };
 
         let mut string = String::new();
-        let empty = {
+        self.owner.mutate_associated_block(|mut pdb, mut changed| {
-            if let Some(ref lock) = self.owner.style_attribute() {
+            pdb.property_value_to_css(&id, &mut string).unwrap();
-                let mut style_attribute = lock.write();
+            *changed = pdb.remove_property(&id);
-                // Step 3
+        });
-                style_attribute.property_value_to_css(&id, &mut string).unwrap();
-
-                // Step 4 & 5
-                style_attribute.remove_property(&id);
-                self.owner.flush_style(&style_attribute);
-                style_attribute.declarations.is_empty()
-            } else {
-                // No style attribute is like an empty style attribute: nothing to remove.
-                return Ok(DOMString::new())
-            }
-        };
-        if let (&CSSStyleOwner::Element(ref el), true) = (&self.owner, empty) {
-            *el.style_attribute().borrow_mut() = None;
-        }
-        self.owner.dirty();
 
         // Step 6
         Ok(DOMString::from(string))
@@ -386,8 +394,8 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
 
     // https://dev.w3.org/csswg/cssom/#the-cssstyledeclaration-interface
     fn IndexedGetter(&self, index: u32) -> Option<DOMString> {
-        self.owner.style_attribute().as_ref().and_then(|lock| {
+        self.owner.with_block(|ref pdb| {
-            lock.read().declarations.get(index as usize).map(|entry| {
+            pdb.declarations.get(index as usize).map(|entry| {
                 let (ref declaration, importance) = *entry;
                 let mut css = declaration.to_css_string();
                 if importance.important() {
@@ -400,8 +408,9 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
 
     // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-csstext
     fn CssText(&self) -> DOMString {
-        self.owner.style_attribute().as_ref().map_or(DOMString::new(), |lock|
+        self.owner.with_block(|ref pdb| {
-            DOMString::from(lock.read().to_css_string()))
+            DOMString::from(pdb.to_css_string())
+        })
     }
 
     // https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-csstext
@@ -413,19 +422,14 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
             return Err(Error::NoModificationAllowed);
         }
 
-        // Step 3
+        self.owner.mutate_associated_block(|mut pdb, mut _changed| {
-        let decl_block = parse_style_attribute(&value, &window.get_url(), window.css_error_reporter(),
+            // Step 3
-                                               ParserContextExtraData::default());
+            *pdb = parse_style_attribute(&value,
-        if let CSSStyleOwner::Element(ref el) = self.owner {
+                                         &self.owner.base_url(),
-            *el.style_attribute().borrow_mut() = if decl_block.declarations.is_empty() {
+                                         window.css_error_reporter(),
-                el.set_style_attr(String::new());
+                                         ParserContextExtraData::default());
-                None // Step 2
+        });
-            } else {
+
-                el.set_style_attr(decl_block.to_css_string());
-                Some(Arc::new(RwLock::new(decl_block)))
-            };
-        }
-        self.owner.dirty();
         Ok(())
     }
 

components/script/dom/cssstylerule.rs

@@ -3,6 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use dom::bindings::codegen::Bindings::CSSStyleRuleBinding::{self, CSSStyleRuleMethods};
+use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{JS, MutNullableJS, Root};
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
 use dom::bindings::str::DOMString;
@@ -58,8 +59,8 @@ impl CSSStyleRuleMethods for CSSStyleRule {
     fn Style(&self) -> Root<CSSStyleDeclaration> {
         self.style_decl.or_init(|| {
             CSSStyleDeclaration::new(self.global().as_window(),
-                                     CSSStyleOwner::CSSRule(JS::from_ref(self.global().as_window()),
+                                     CSSStyleOwner::CSSRule(JS::from_ref(self.upcast()),
-                                                                 self.stylerule.read().block.clone()),
+                                                            self.stylerule.read().block.clone()),
                                      None,
                                      CSSModificationAccess::ReadWrite)
         })

components/style/properties/declaration_block.rs

@@ -192,7 +192,10 @@ impl PropertyDeclarationBlock {
     }
 
     /// Set the declaration importance for a given property, if found.
-    pub fn set_importance(&mut self, property: &PropertyId, new_importance: Importance) {
+    ///
+    /// Returns whether any declaration was updated.
+    pub fn set_importance(&mut self, property: &PropertyId, new_importance: Importance) -> bool {
+        let mut updated_at_least_one = false;
         for &mut (ref declaration, ref mut importance) in &mut self.declarations {
             if declaration.id().is_or_is_longhand_of(property) {
                 match (*importance, new_importance) {
@@ -202,23 +205,35 @@ impl PropertyDeclarationBlock {
                     (Importance::Important, Importance::Normal) => {
                         self.important_count -= 1;
                     }
-                    _ => {}
+                    _ => {
+                        continue;
+                    }
                 }
+                updated_at_least_one = true;
                 *importance = new_importance;
             }
         }
+        updated_at_least_one
     }
 
     /// https://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-removeproperty
-    pub fn remove_property(&mut self, property: &PropertyId) {
+    ///
+    /// Returns whether any declaration was actually removed.
+    pub fn remove_property(&mut self, property: &PropertyId) -> bool {
         let important_count = &mut self.important_count;
+        let mut removed_at_least_one = false;
         self.declarations.retain(|&(ref declaration, importance)| {
             let remove = declaration.id().is_or_is_longhand_of(property);
-            if remove && importance.important() {
+            if remove {
-                *important_count -= 1
+                removed_at_least_one = true;
+                if importance.important() {
+                    *important_count -= 1
+                }
             }
             !remove
-        })
+        });
+
+        removed_at_least_one
     }
 
     /// Take a declaration block known to contain a single property and serialize it.

tests/wpt/mozilla/meta/MANIFEST.json

@@ -15284,6 +15284,12 @@
             "url": "/_mozilla/mozilla/style_no_trailing_space.html"
           }
         ],
+        "mozilla/style_remove_prop.html": [
+          {
+            "path": "mozilla/style_remove_prop.html",
+            "url": "/_mozilla/mozilla/style_remove_prop.html"
+          }
+        ],
         "mozilla/textcontent.html": [
           {
             "path": "mozilla/textcontent.html",

tests/wpt/mozilla/tests/mozilla/style_remove_prop.html

@@ -0,0 +1,13 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Removing a property from the style object updates the attributeRemove u</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div style="color: red"></div>
+<script>
+test(function() {
+  var div = document.querySelector('div');
+  div.style.color = "";
+  assert_equals(div.getAttribute('style'), "");
+}, "Removing a property from the style object updates the attribute");
+</script>