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

components/script/dom/cssstyledeclaration.rs

@@ -40,17 +40,78 @@ pub enum CSSStyleOwner {
 }
 
 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 win, ref pdb) => {
+                let result = f(&mut *pdb.write(), &mut changed);
+                if changed {
+                    win.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())
             }
         }
     }
@@ -61,21 +122,6 @@ impl CSSStyleOwner {
             CSSStyleOwner::CSSRule(ref window, _) => Root::from_ref(&**window),
         }
     }
-
-    fn flush_style(&self, pdb: &PropertyDeclarationBlock) {
-        if let CSSStyleOwner::Element(ref el) = *self {
-            el.set_style_attr(pdb.to_css_string());
-        }
-    }
-
-    fn dirty(&self) {
-        match *self {
-            CSSStyleOwner::Element(ref el) =>
-                el.upcast::<Node>().dirty(NodeDamage::NodeStyleDamaged),
-            CSSStyleOwner::CSSRule(ref window, _) =>
-                window.Document().invalidate_stylesheets(),
-        }
-    }
 }
 
 #[derive(PartialEq, HeapSizeOf)]
@@ -147,14 +193,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 +208,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, &window.get_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 +288,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 +322,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 +334,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 +362,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 +383,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 +397,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 +411,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 {
+                                         &window.get_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/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>