Address review comments.

6 files changed, 83 insertions, 109 deletions

diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs
index 871fe865f0b..7a69990cc21 100644
--- a/components/script/dom/cssstyledeclaration.rs
+++ b/components/script/dom/cssstyledeclaration.rs
@@ -63,103 +63,95 @@ impl<'a> CSSStyleDeclarationMethods for JSRef<'a, CSSStyleDeclaration> {
     fn Length(self) -> u32 {
         let owner = self.owner.root();
         let elem: JSRef<Element> = ElementCast::from_ref(*owner);
-        let style_attribute = elem.style_attribute().borrow();
-        style_attribute.as_ref().map(|declarations| {
-            declarations.normal.len() + declarations.important.len()
-        }).unwrap_or(0) as u32
+        let len = match *elem.style_attribute().borrow() {
+            Some(ref declarations) => declarations.normal.len() + declarations.important.len(),
+            None => 0
+        };
+        len as u32
     }
 
     fn Item(self, index: u32) -> DOMString {
         let owner = self.owner.root();
         let elem: JSRef<Element> = ElementCast::from_ref(*owner);
         let style_attribute = elem.style_attribute().borrow();
-        style_attribute.as_ref().and_then(|declarations| {
+        let result = style_attribute.as_ref().and_then(|declarations| {
             if index as uint > declarations.normal.len() {
                 declarations.important
-                            .iter()
-                            .nth(index as uint - declarations.normal.len())
+                            .as_slice()
+                            .get(index as uint - declarations.normal.len())
                             .map(|decl| format!("{} !important", decl))
             } else {
                 declarations.normal
-                            .iter()
-                            .nth(index as uint)
+                            .as_slice()
+                            .get(index as uint)
                             .map(|decl| format!("{}", decl))
             }
-        }).unwrap_or("".to_string())
+        });
+
+        result.unwrap_or("".to_string())
     }
 
-    //http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-getpropertyvalue
     fn GetPropertyValue(self, property: DOMString) -> DOMString {
-        // 1. Let property be property converted to ASCII lowercase.
+        // Step 1
         let property = Atom::from_slice(property.as_slice().to_ascii_lower().as_slice());
 
-        // 2. If property is a shorthand property, then follow these substeps:
+        // Step 2
         let longhand_properties = longhands_from_shorthand(property.as_slice());
-        if longhand_properties.is_some() {
-            //    1. Let list be a new empty array.
+        if let Some(longhand_properties) = longhand_properties {
+            // Step 2.1
             let mut list = vec!();
 
-            //    2. For each longhand property longhand that property maps to, in canonical order,
-            //       follow these substeps:
-            for longhand in longhand_properties.unwrap().iter() {
-                //       1. If longhand is a case-sensitive match for a property name of a
-                //          CSS declaration in the declarations, let declaration be that CSS
-                //          declaration, or null otherwise.
+            // Step 2.2
+            for longhand in longhand_properties.iter() {
+                // Step 2.2.1
                 let declaration = self.get_declaration(&Atom::from_slice(longhand.as_slice()));
 
-                //       2. If declaration is null, return the empty string and terminate these
-                //          steps.
+                // Step 2.2.2 & 2.2.3
                 match declaration {
-                    //       3. Append the declaration to list.
                     Some(declaration) => list.push(declaration),
                     None => return "".to_string(),
                 }
             }
 
-            //    3. Return the serialization of list and terminate these steps.
+            // Step 2.3
             return serialize_list(&list);
         }
 
-        // 3. If property is a case-sensitive match for a property name of a CSS declaration
-        //    in the declarations, return the result of invoking serialize a CSS value of that
-        //    declaration and terminate these steps.
-        // 4. Return the empty string.
-        let declaration = self.get_declaration(&property);
-        declaration.as_ref()
-                   .map(|declaration| serialize_value(declaration))
-                   .unwrap_or("".to_string())
+        // Step 3 & 4
+        if let Some(ref declaration) = self.get_declaration(&property) {
+            serialize_value(declaration)
+        } else {
+            "".to_string()
+        }
     }
 
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setproperty
     fn SetProperty(self, property: DOMString, value: DOMString,
                    priority: DOMString) -> ErrorResult {
-        // 1. If the readonly flag is set, throw a NoModificationAllowedError exception
-        //    and terminate these steps.
-        //TODO
+        //TODO: disallow modifications if readonly flag is set
 
-        // 2. Let property be property converted to ASCII lowercase.
+        // Step 2
         let property = property.as_slice().to_ascii_lower();
 
-        // 3. If property is not a case-sensitive match for a supported CSS property,
-        //    terminate this algorithm.
+        // Step 3
         if !is_supported_property(property.as_slice()) {
             return Ok(());
         }
 
-        // 4. If value is the empty string, invoke removeProperty() with property as argument
-        //    and terminate this algorithm.
+        // Step 4
         if value.is_empty() {
-            self.RemoveProperty(property.clone());
+            self.RemoveProperty(property);
             return Ok(());
         }
 
-        // 5. If priority is not the empty string and is not an ASCII case-insensitive match
-        //    for the string "important", terminate this algorithm.
+        // Step 5
         let priority = priority.as_slice().to_ascii_lower();
         if priority.as_slice() != "!important" && !priority.is_empty() {
             return Ok(());
         }
 
-        // 6. Let `component value list` be the result of parsing value for property `property`.
+        // Step 6
         let mut synthesized_declaration = String::from_str(property.as_slice());
         synthesized_declaration.push_str(": ");
         synthesized_declaration.push_str(value.as_slice());
@@ -170,7 +162,7 @@ impl<'a> CSSStyleDeclarationMethods for JSRef<'a, CSSStyleDeclaration> {
         let decl_block = parse_style_attribute(synthesized_declaration.as_slice(),
                                                &page.get_url());
 
-        // 7. If `component value list` is null terminate these steps.
+        // Step 7
         if decl_block.normal.len() == 0 {
             return Ok(());
         }
@@ -178,19 +170,9 @@ impl<'a> CSSStyleDeclarationMethods for JSRef<'a, CSSStyleDeclaration> {
         let owner = self.owner.root();
         let element: JSRef<Element> = ElementCast::from_ref(*owner);
 
-        //XXXjdm https://www.w3.org/Bugs/Public/show_bug.cgi?id=27589
-        assert!(decl_block.important.len() == 0);
-
+        // Step 8
         for decl in decl_block.normal.iter() {
-            // 8. If property is a shorthand property, then for each longhand property
-            //    longhand that property maps to, in canonical order, set the CSS
-            //    declaration value longhand to the appropriate value(s) from component
-            //    value list, and with the list of declarations being the declarations.
-
-            // 9. Otherwise, set the CSS declaration value property to the
-            //    value component value list, and with the list of declarations
-            //    being the declarations.
-
+            // Step 9
             element.update_inline_style(decl.clone(), !priority.is_empty());
         }
 
@@ -200,49 +182,48 @@ impl<'a> CSSStyleDeclarationMethods for JSRef<'a, CSSStyleDeclaration> {
         Ok(())
     }
 
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-setpropertyvalue
     fn SetPropertyValue(self, property: DOMString, value: DOMString) -> ErrorResult {
         self.SetProperty(property, value, "".to_string())
     }
 
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-removeproperty
     fn RemoveProperty(self, property: DOMString) -> DOMString {
-        // 1. If the readonly flag is set, throw a NoModificationAllowedError exception
-        //    and terminate these steps.
-        //TODO
+        //TODO: disallow modifications if readonly flag is set
 
-        // 2. Let `property` be property converted to ASCII lowercase.
+        // Step 2
         let property = property.as_slice().to_ascii_lower();
 
-        // 3. Let `value` be the return value of invoking getPropertyValue() with `property`
-        //    as argument.
+        // Step 3
         let value = self.GetPropertyValue(property.clone());
 
-        // 4. If `property` is a shorthand property, for each longhand property `longhand` that
-        //    `property` maps to, invoke removeProperty() with `longhand` as argument.
         let longhand_properties = longhands_from_shorthand(property.as_slice());
         match longhand_properties {
             Some(longhands) => {
+                // Step 4
                 for longhand in longhands.iter() {
                     self.RemoveProperty(longhand.clone());
                 }
             }
 
-            // 5. Otherwise, if `property` is a case-sensitive match for a property name of a
-            //    CSS declaration in the declarations, remove that CSS declaration.
             None => {
+                // Step 5
                 let owner = self.owner.root();
                 let elem: JSRef<Element> = ElementCast::from_ref(*owner);
                 elem.remove_inline_style_property(property)
             }
         }
 
-        // 6. Return value.
+        // Step 6
         value
     }
 
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-cssfloat
     fn CssFloat(self) -> DOMString {
         self.GetPropertyValue("float".to_string())
     }
 
+    // http://dev.w3.org/csswg/cssom/#dom-cssstyledeclaration-cssfloat
     fn SetCssFloat(self, value: DOMString) -> ErrorResult {
         self.SetPropertyValue("float".to_string(), value)
     }
diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs
index fa4d6d5c08a..06de6d4ce29 100644
--- a/components/script/dom/element.rs
+++ b/components/script/dom/element.rs
@@ -528,8 +528,6 @@ impl<'a> ElementHelpers<'a> for JSRef<'a, Element> {
     }
 
     fn remove_inline_style_property(self, property: DOMString) {
-        //FIXME: Rust bug incorrectly thinks inline_declarations doesn't need mut,
-        //       and allow(unused_mut) on this method does nothing.
         let mut inline_declarations = self.style_attribute.borrow_mut();
         inline_declarations.as_mut().map(|declarations| {
             let index = declarations.normal
@@ -557,47 +555,44 @@ impl<'a> ElementHelpers<'a> for JSRef<'a, Element> {
     }
 
     fn update_inline_style(self, property_decl: style::PropertyDeclaration, important: bool) {
-        //FIXME: Rust bug incorrectly thinks inline_declarations doesn't need mut,
-        //       and allow(unused_mut) on this method does nothing.
-        let mut inline_declarations = self.style_attribute.borrow_mut();
-        let exists = inline_declarations.is_some();
-        if exists {
-            let declarations = inline_declarations.deref_mut().as_mut().unwrap();
-            let mut existing_declarations = if important {
-                declarations.important.clone()
+        let mut inline_declarations = self.style_attribute().borrow_mut();
+        if let Some(ref mut declarations) = *inline_declarations.deref_mut() {
+            let existing_declarations = if important {
+                declarations.important.make_unique()
             } else {
-                declarations.normal.clone()
+                declarations.normal.make_unique()
             };
-            for declaration in existing_declarations.make_unique().iter_mut() {
+
+            for declaration in existing_declarations.iter_mut() {
                 if declaration.name() == property_decl.name() {
                     *declaration = property_decl;
                     return;
                 }
             }
-            existing_declarations.make_unique().push(property_decl);
+            existing_declarations.push(property_decl);
+            return;
+        }
+
+        let (important, normal) = if important {
+            (vec!(property_decl), vec!())
         } else {
-            let (important, normal) = if important {
-                (vec!(property_decl), vec!())
-            } else {
-                (vec!(), vec!(property_decl))
-            };
+            (vec!(), vec!(property_decl))
+        };
 
-            *inline_declarations = Some(style::PropertyDeclarationBlock {
-                important: Arc::new(important),
-                normal: Arc::new(normal),
-            });
-        }
+        *inline_declarations = Some(style::PropertyDeclarationBlock {
+            important: Arc::new(important),
+            normal: Arc::new(normal),
+        });
     }
 
     fn get_inline_style_declaration(self, property: &Atom) -> Option<style::PropertyDeclaration> {
-        let mut inline_declarations = self.style_attribute.borrow();
+        let inline_declarations = self.style_attribute.borrow();
         inline_declarations.as_ref().and_then(|declarations| {
-            for declaration in declarations.normal.iter().chain(declarations.important.iter()) {
-                if declaration.matches(property.as_slice()) {
-                    return Some(declaration.clone());
-                }
-            }
-            None
+            declarations.normal
+                        .iter()
+                        .chain(declarations.important.iter())
+                        .find(|decl| decl.matches(property.as_slice()))
+                        .map(|decl| decl.clone())
         })
     }
 }
diff --git a/components/script/dom/htmlelement.rs b/components/script/dom/htmlelement.rs
index e37b37b3c55..48acd297574 100644
--- a/components/script/dom/htmlelement.rs
+++ b/components/script/dom/htmlelement.rs
@@ -72,13 +72,11 @@ impl<'a> PrivateHTMLElementHelpers for JSRef<'a, HTMLElement> {
 
 impl<'a> HTMLElementMethods for JSRef<'a, HTMLElement> {
     fn Style(self) -> Temporary<CSSStyleDeclaration> {
-        if self.style_decl.get().is_none() {
+        self.style_decl.or_init(|| {
             let global = window_from_node(self).root();
-            let style_props = CSS2Properties::new(*global, self).root();
-            let style_decl: JSRef<CSSStyleDeclaration> = CSSStyleDeclarationCast::from_ref(*style_props);
-            self.style_decl.assign(Some(style_decl));
-        }
-        self.style_decl.get().unwrap()
+            let style_props = CSS2Properties::new(*global, self);
+            CSSStyleDeclarationCast::from_temporary(style_props)
+        })
     }
 
     make_getter!(Title)
diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
index 234e1c8c707..50647862007 100644
--- a/components/script/dom/node.rs
+++ b/components/script/dom/node.rs
@@ -194,7 +194,7 @@ pub struct SharedLayoutData {
 /// Encapsulates the abstract layout data.
 pub struct LayoutData {
     chan: Option<LayoutChan>,
-    pub shared_data: SharedLayoutData,
+    _shared_data: SharedLayoutData,
     _data: *const (),
 }
 
diff --git a/components/script/dom/webidls/CSS2Properties.webidl b/components/script/dom/webidls/CSS2Properties.webidl
index 8341cce9926..9e1832cfb25 100644
--- a/components/script/dom/webidls/CSS2Properties.webidl
+++ b/components/script/dom/webidls/CSS2Properties.webidl
@@ -4,7 +4,8 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/.
  *
  * The origin of this IDL file is
- * http://dev.w3.org/csswg/cssom/
+ * http://mxr.mozilla.org/mozilla-central/source/dom/webidl/CSS2Properties.webidl.in
+ * http://mxr.mozilla.org/mozilla-central/source/dom/bindings/GenerateCSS2PropertiesWebIDL.py
  *
  */
 
diff --git a/components/script/dom/webidls/CSSStyleDeclaration.webidl b/components/script/dom/webidls/CSSStyleDeclaration.webidl
index 75b4bbeffc9..35b332ca736 100644
--- a/components/script/dom/webidls/CSSStyleDeclaration.webidl
+++ b/components/script/dom/webidls/CSSStyleDeclaration.webidl
@@ -23,8 +23,7 @@ interface CSSStyleDeclaration {
   //[Throws]
   //void setPropertyPriority(DOMString property, [TreatNullAs=EmptyString] DOMString priority);
   DOMString removeProperty(DOMString property);
-// Not implemented yet:
-//  readonly attribute CSSRule? parentRule;
+  //readonly attribute CSSRule? parentRule;
   [SetterThrows]
            attribute DOMString cssFloat;
 };