Auto merge of #8404 - eefriedman:misc-attributes, r=nox

Misc cleanup for DOM attributes

No functional change; just cleanups related to parsing attributes.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8404)
<!-- Reviewable:end -->

components/script/dom/document.rs

@@ -968,7 +968,9 @@ impl Document {
 
     pub fn set_body_attribute(&self, local_name: &Atom, value: DOMString) {
         if let Some(ref body) = self.GetBody().and_then(Root::downcast::<HTMLBodyElement>) {
-            body.upcast::<Element>().set_string_attribute(local_name, value);
+            let body = body.upcast::<Element>();
+            let value = body.parse_attribute(&ns!(""), &local_name, value);
+            body.set_attribute(local_name, value);
         }
     }
 

components/script/dom/element.rs

@@ -36,18 +36,18 @@ use dom::domrectlist::DOMRectList;
 use dom::domtokenlist::DOMTokenList;
 use dom::event::Event;
 use dom::htmlanchorelement::HTMLAnchorElement;
-use dom::htmlbodyelement::HTMLBodyElement;
+use dom::htmlbodyelement::{HTMLBodyElement, HTMLBodyElementLayoutHelpers};
 use dom::htmlcollection::HTMLCollection;
 use dom::htmlfieldsetelement::HTMLFieldSetElement;
-use dom::htmlfontelement::HTMLFontElement;
+use dom::htmlfontelement::{HTMLFontElement, HTMLFontElementLayoutHelpers};
 use dom::htmlhrelement::{HTMLHRElement, HTMLHRLayoutHelpers};
-use dom::htmliframeelement::HTMLIFrameElement;
+use dom::htmliframeelement::{HTMLIFrameElement, HTMLIFrameElementLayoutMethods};
 use dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers};
 use dom::htmllabelelement::HTMLLabelElement;
 use dom::htmllegendelement::HTMLLegendElement;
 use dom::htmloptgroupelement::HTMLOptGroupElement;
 use dom::htmltablecellelement::{HTMLTableCellElement, HTMLTableCellElementLayoutHelpers};
-use dom::htmltableelement::HTMLTableElement;
+use dom::htmltableelement::{HTMLTableElement, HTMLTableElementLayoutHelpers};
 use dom::htmltablerowelement::HTMLTableRowElement;
 use dom::htmltablesectionelement::HTMLTableSectionElement;
 use dom::htmltemplateelement::HTMLTemplateElement;
@@ -274,9 +274,9 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         where V: VecLike<DeclarationBlock<Vec<PropertyDeclaration>>>
     {
         let bgcolor = if let Some(this) = self.downcast::<HTMLBodyElement>() {
-            (*this.unsafe_get()).get_background_color()
+            this.get_background_color()
         } else if let Some(this) = self.downcast::<HTMLTableElement>() {
-            (*this.unsafe_get()).get_background_color()
+            this.get_background_color()
         } else if let Some(this) = self.downcast::<HTMLTableCellElement>() {
             this.get_background_color()
         } else if let Some(this) = self.downcast::<HTMLTableRowElement>() {
@@ -294,7 +294,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         }
 
         let background = if let Some(this) = self.downcast::<HTMLBodyElement>() {
-            (*this.unsafe_get()).get_background()
+            this.get_background()
         } else {
             None
         };
@@ -306,10 +306,10 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         }
 
         let color = if let Some(this) = self.downcast::<HTMLFontElement>() {
-            (*this.unsafe_get()).get_color()
+            this.get_color()
         } else if let Some(this) = self.downcast::<HTMLBodyElement>() {
             // https://html.spec.whatwg.org/multipage/#the-page:the-body-element-20
-            (*this.unsafe_get()).get_color()
+            this.get_color()
         } else if let Some(this) = self.downcast::<HTMLHRElement>() {
             // https://html.spec.whatwg.org/multipage/#the-hr-element-2:presentational-hints-5
             this.get_color()
@@ -326,7 +326,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         }
 
         let font_family = if let Some(this) = self.downcast::<HTMLFontElement>() {
-            (*this.unsafe_get()).get_face()
+            this.get_face()
         } else {
             None
         };
@@ -341,7 +341,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         }
 
         let font_size = if let Some(this) = self.downcast::<HTMLFontElement>() {
-            (*this.unsafe_get()).get_size()
+            this.get_size()
         } else {
             None
         };
@@ -355,7 +355,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         }
 
         let cellspacing = if let Some(this) = self.downcast::<HTMLTableElement>() {
-            (*this.unsafe_get()).get_cellspacing()
+            this.get_cellspacing()
         } else {
             None
         };
@@ -398,9 +398,9 @@ impl LayoutElementHelpers for LayoutJS<Element> {
 
 
         let width = if let Some(this) = self.downcast::<HTMLIFrameElement>() {
-            (*this.unsafe_get()).get_width()
+            this.get_width()
         } else if let Some(this) = self.downcast::<HTMLTableElement>() {
-            (*this.unsafe_get()).get_width()
+            this.get_width()
         } else if let Some(this) = self.downcast::<HTMLTableCellElement>() {
             this.get_width()
         } else {
@@ -425,7 +425,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
 
 
         let height = if let Some(this) = self.downcast::<HTMLIFrameElement>() {
-            (*this.unsafe_get()).get_height()
+            this.get_height()
         } else {
             LengthOrPercentageOrAuto::Auto
         };
@@ -490,7 +490,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
 
 
         let border = if let Some(this) = self.downcast::<HTMLTableElement>() {
-            (*this.unsafe_get()).get_border()
+            this.get_border()
         } else {
             None
         };

components/script/dom/htmlbodyelement.rs

@@ -9,7 +9,7 @@ use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
 use dom::bindings::codegen::Bindings::HTMLBodyElementBinding::{self, HTMLBodyElementMethods};
 use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
 use dom::bindings::inheritance::Castable;
-use dom::bindings::js::Root;
+use dom::bindings::js::{LayoutJS, Root};
 use dom::bindings::reflector::Reflectable;
 use dom::document::Document;
 use dom::element::{AttributeMutation, Element, RawLayoutElementHelpers};
@@ -20,12 +20,11 @@ use dom::virtualmethods::VirtualMethods;
 use msg::constellation_msg::ConstellationChan;
 use msg::constellation_msg::Msg as ConstellationMsg;
 use std::borrow::ToOwned;
-use std::cell::Cell;
 use std::rc::Rc;
 use string_cache::Atom;
 use time;
 use url::{Url, UrlParser};
-use util::str::{self, DOMString};
+use util::str::DOMString;
 
 /// How long we should wait before performing the initial reflow after `<body>` is parsed, in
 /// nanoseconds.
@@ -34,7 +33,6 @@ const INITIAL_REFLOW_DELAY: u64 = 200_000_000;
 #[dom_struct]
 pub struct HTMLBodyElement {
     htmlelement: HTMLElement,
-    background_color: Cell<Option<RGBA>>,
     background: DOMRefCell<Option<Url>>
 }
 
@@ -43,7 +41,6 @@ impl HTMLBodyElement {
                      -> HTMLBodyElement {
         HTMLBodyElement {
             htmlelement: HTMLElement::new_inherited(localName, prefix, document),
-            background_color: Cell::new(None),
             background: DOMRefCell::new(None)
         }
     }
@@ -61,17 +58,13 @@ impl HTMLBodyElementMethods for HTMLBodyElement {
     make_getter!(BgColor, "bgcolor");
 
     // https://html.spec.whatwg.org/multipage/#dom-body-bgcolor
-    make_setter!(SetBgColor, "bgcolor");
+    make_legacy_color_setter!(SetBgColor, "bgcolor");
 
     // https://html.spec.whatwg.org/multipage/#dom-body-text
     make_getter!(Text);
 
     // https://html.spec.whatwg.org/multipage/#dom-body-text
-    fn SetText(&self, value: DOMString) {
+    make_legacy_color_setter!(SetText, "text");
-        let element = self.upcast::<Element>();
-        let color = str::parse_legacy_color(&value).ok();
-        element.set_attribute(&atom!("text"), AttrValue::Color(value, color));
-    }
 
     // https://html.spec.whatwg.org/multipage/#the-body-element
     fn GetOnunload(&self) -> Option<Rc<EventHandlerNonNull>> {
@@ -94,16 +87,27 @@ impl HTMLBodyElementMethods for HTMLBodyElement {
     }
 }
 
+pub trait HTMLBodyElementLayoutHelpers {
+    fn get_background_color(&self) -> Option<RGBA>;
+    fn get_color(&self) -> Option<RGBA>;
+    fn get_background(&self) -> Option<Url>;
+}
 
-impl HTMLBodyElement {
+impl HTMLBodyElementLayoutHelpers for LayoutJS<HTMLBodyElement> {
-    pub fn get_background_color(&self) -> Option<RGBA> {
+    #[allow(unsafe_code)]
-        self.background_color.get()
+    fn get_background_color(&self) -> Option<RGBA> {
+        unsafe {
+            (*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("bgcolor"))
+                .and_then(AttrValue::as_color)
+                .cloned()
+        }
     }
 
     #[allow(unsafe_code)]
-    pub fn get_color(&self) -> Option<RGBA> {
+    fn get_color(&self) -> Option<RGBA> {
         unsafe {
-            self.upcast::<Element>()
+            (*self.upcast::<Element>().unsafe_get())
                 .get_attr_for_layout(&ns!(""), &atom!("text"))
                 .and_then(AttrValue::as_color)
                 .cloned()
@@ -111,9 +115,9 @@ impl HTMLBodyElement {
     }
 
     #[allow(unsafe_code)]
-    pub fn get_background(&self) -> Option<Url> {
+    fn get_background(&self) -> Option<Url> {
         unsafe {
-            self.background.borrow_for_layout().clone()
+            (*self.unsafe_get()).background.borrow_for_layout().clone()
         }
     }
 }
@@ -141,8 +145,9 @@ impl VirtualMethods for HTMLBodyElement {
     }
 
     fn parse_plain_attribute(&self, name: &Atom, value: DOMString) -> AttrValue {
-        match name {
+        match *name {
-            &atom!("text") => AttrValue::from_legacy_color(value),
+            atom!("bgcolor") |
+            atom!("text") => AttrValue::from_legacy_color(value),
             _ => self.super_type().unwrap().parse_plain_attribute(name, value),
         }
     }
@@ -150,11 +155,6 @@ impl VirtualMethods for HTMLBodyElement {
     fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
         self.super_type().unwrap().attribute_mutated(attr, mutation);
         match (attr.local_name(), mutation) {
-            (&atom!(bgcolor), _) => {
-                self.background_color.set(mutation.new_value(attr).and_then(|value| {
-                    str::parse_legacy_color(&value).ok()
-                }));
-            },
             (&atom!(background), _) => {
                 *self.background.borrow_mut() = mutation.new_value(attr).and_then(|value| {
                     let document = document_from_node(self);

components/script/dom/htmlfontelement.rs

@@ -3,25 +3,23 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use cssparser::RGBA;
-use dom::attr::{Attr, AttrValue};
+use dom::attr::AttrValue;
 use dom::bindings::codegen::Bindings::HTMLFontElementBinding;
 use dom::bindings::codegen::Bindings::HTMLFontElementBinding::HTMLFontElementMethods;
 use dom::bindings::inheritance::Castable;
-use dom::bindings::js::Root;
+use dom::bindings::js::{LayoutJS, Root};
 use dom::document::Document;
-use dom::element::{AttributeMutation, Element, RawLayoutElementHelpers};
+use dom::element::{Element, RawLayoutElementHelpers};
 use dom::htmlelement::HTMLElement;
 use dom::node::Node;
 use dom::virtualmethods::VirtualMethods;
-use std::cell::Cell;
 use string_cache::Atom;
 use style::values::specified;
-use util::str::{self, DOMString, parse_legacy_font_size};
+use util::str::{DOMString, parse_legacy_font_size};
 
 #[dom_struct]
 pub struct HTMLFontElement {
     htmlelement: HTMLElement,
-    color: Cell<Option<RGBA>>,
 }
 
 
@@ -29,7 +27,6 @@ impl HTMLFontElement {
     fn new_inherited(localName: DOMString, prefix: Option<DOMString>, document: &Document) -> HTMLFontElement {
         HTMLFontElement {
             htmlelement: HTMLElement::new_inherited(localName, prefix, document),
-            color: Cell::new(None),
         }
     }
 
@@ -47,7 +44,7 @@ impl HTMLFontElementMethods for HTMLFontElement {
     make_getter!(Color, "color");
 
     // https://html.spec.whatwg.org/multipage/#dom-font-color
-    make_setter!(SetColor, "color");
+    make_legacy_color_setter!(SetColor, "color");
 
     // https://html.spec.whatwg.org/multipage/#dom-font-face
     make_getter!(Face);
@@ -71,21 +68,10 @@ impl VirtualMethods for HTMLFontElement {
         Some(self.upcast::<HTMLElement>() as &VirtualMethods)
     }
 
-    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
-        self.super_type().unwrap().attribute_mutated(attr, mutation);
-        match attr.local_name() {
-            &atom!(color) => {
-                self.color.set(mutation.new_value(attr).and_then(|value| {
-                    str::parse_legacy_color(&value).ok()
-                }));
-            },
-            _ => {},
-        }
-    }
-
     fn parse_plain_attribute(&self, name: &Atom, value: DOMString) -> AttrValue {
         match name {
             &atom!("face") => AttrValue::from_atomic(value),
+            &atom!("color") => AttrValue::from_legacy_color(value),
             &atom!("size") => {
                 let length = parse_length(&value);
                 AttrValue::Length(value, length)
@@ -95,16 +81,27 @@ impl VirtualMethods for HTMLFontElement {
     }
 }
 
+pub trait HTMLFontElementLayoutHelpers {
+    fn get_color(&self) -> Option<RGBA>;
+    fn get_face(&self) -> Option<Atom>;
+    fn get_size(&self) -> Option<specified::Length>;
+}
 
-impl HTMLFontElement {
+impl HTMLFontElementLayoutHelpers for LayoutJS<HTMLFontElement> {
-    pub fn get_color(&self) -> Option<RGBA> {
+    #[allow(unsafe_code)]
-        self.color.get()
+    fn get_color(&self) -> Option<RGBA> {
+        unsafe {
+            (*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("color"))
+                .and_then(AttrValue::as_color)
+                .cloned()
+        }
     }
 
     #[allow(unsafe_code)]
-    pub fn get_face(&self) -> Option<Atom> {
+    fn get_face(&self) -> Option<Atom> {
         unsafe {
-            self.upcast::<Element>()
+            (*self.upcast::<Element>().unsafe_get())
                 .get_attr_for_layout(&ns!(""), &atom!("face"))
                 .map(AttrValue::as_atom)
                 .cloned()
@@ -112,9 +109,9 @@ impl HTMLFontElement {
     }
 
     #[allow(unsafe_code)]
-    pub fn get_size(&self) -> Option<specified::Length> {
+    fn get_size(&self) -> Option<specified::Length> {
         unsafe {
-            self.upcast::<Element>()
+            (*self.upcast::<Element>().unsafe_get())
                 .get_attr_for_layout(&ns!(""), &atom!("size"))
                 .and_then(AttrValue::as_length)
                 .cloned()

components/script/dom/htmliframeelement.rs

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use dom::attr::{Attr, AttrHelpersForLayout, AttrValue};
+use dom::attr::{Attr, AttrValue};
 use dom::bindings::codegen::Bindings::HTMLIFrameElementBinding;
 use dom::bindings::codegen::Bindings::HTMLIFrameElementBinding::HTMLIFrameElementMethods;
 use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
@@ -14,7 +14,7 @@ use dom::bindings::js::{Root, LayoutJS};
 use dom::bindings::reflector::Reflectable;
 use dom::customevent::CustomEvent;
 use dom::document::Document;
-use dom::element::{self, AttributeMutation, Element};
+use dom::element::{AttributeMutation, Element, RawLayoutElementHelpers};
 use dom::event::Event;
 use dom::htmlelement::HTMLElement;
 use dom::node::{Node, window_from_node};
@@ -156,28 +156,6 @@ impl HTMLIFrameElement {
         self.subpage_id.set(Some(new_subpage_id));
     }
 
-    #[allow(unsafe_code)]
-    pub fn get_width(&self) -> LengthOrPercentageOrAuto {
-        unsafe {
-            element::get_attr_for_layout(self.upcast(),
-                                         &ns!(""),
-                                         &atom!("width")).map(|attribute| {
-                str::parse_length(&**attribute.value_for_layout())
-            }).unwrap_or(LengthOrPercentageOrAuto::Auto)
-        }
-    }
-
-    #[allow(unsafe_code)]
-    pub fn get_height(&self) -> LengthOrPercentageOrAuto {
-        unsafe {
-            element::get_attr_for_layout(self.upcast(),
-                                         &ns!(""),
-                                         &atom!("height")).map(|attribute| {
-                str::parse_length(&**attribute.value_for_layout())
-            }).unwrap_or(LengthOrPercentageOrAuto::Auto)
-        }
-    }
-
     fn new_inherited(localName: DOMString,
                      prefix: Option<DOMString>,
                      document: &Document) -> HTMLIFrameElement {
@@ -211,6 +189,8 @@ impl HTMLIFrameElement {
 
 pub trait HTMLIFrameElementLayoutMethods {
     fn pipeline_id(self) -> Option<PipelineId>;
+    fn get_width(&self) -> LengthOrPercentageOrAuto;
+    fn get_height(&self) -> LengthOrPercentageOrAuto;
 }
 
 impl HTMLIFrameElementLayoutMethods for LayoutJS<HTMLIFrameElement> {
@@ -221,6 +201,26 @@ impl HTMLIFrameElementLayoutMethods for LayoutJS<HTMLIFrameElement> {
             (*self.unsafe_get()).pipeline_id.get()
         }
     }
+
+    #[allow(unsafe_code)]
+    fn get_width(&self) -> LengthOrPercentageOrAuto {
+        unsafe {
+            (*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("width"))
+                .map(|attribute| str::parse_length(&attribute))
+                .unwrap_or(LengthOrPercentageOrAuto::Auto)
+        }
+    }
+
+    #[allow(unsafe_code)]
+    fn get_height(&self) -> LengthOrPercentageOrAuto {
+        unsafe {
+            (*self.upcast::<Element>().unsafe_get())
+                .get_attr_for_layout(&ns!(""), &atom!("height"))
+                .map(|attribute| str::parse_length(&attribute))
+                .unwrap_or(LengthOrPercentageOrAuto::Auto)
+        }
+    }
 }
 
 pub fn Navigate(iframe: &HTMLIFrameElement, direction: NavigationDirection) -> Fallible<()> {

components/script/dom/htmltableelement.rs

@@ -8,7 +8,7 @@ use dom::bindings::codegen::Bindings::HTMLTableElementBinding;
 use dom::bindings::codegen::Bindings::HTMLTableElementBinding::HTMLTableElementMethods;
 use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
 use dom::bindings::inheritance::Castable;
-use dom::bindings::js::{Root, RootedReference};
+use dom::bindings::js::{LayoutJS, Root, RootedReference};
 use dom::document::Document;
 use dom::element::{AttributeMutation, Element};
 use dom::htmlelement::HTMLElement;
@@ -47,6 +47,10 @@ impl HTMLTableElement {
         let element = HTMLTableElement::new_inherited(localName, prefix, document);
         Node::reflect_node(box element, document, HTMLTableElementBinding::Wrap)
     }
+
+    pub fn get_border(&self) -> Option<u32> {
+        self.border.get()
+    }
 }
 
 impl HTMLTableElementMethods for HTMLTableElement {
@@ -115,22 +119,40 @@ impl HTMLTableElementMethods for HTMLTableElement {
     make_setter!(SetBgColor, "bgcolor");
 }
 
-
+pub trait HTMLTableElementLayoutHelpers {
-impl HTMLTableElement {
+    fn get_background_color(&self) -> Option<RGBA>;
-    pub fn get_background_color(&self) -> Option<RGBA> {
+    fn get_border(&self) -> Option<u32>;
-        self.background_color.get()
+    fn get_cellspacing(&self) -> Option<u32>;
+    fn get_width(&self) -> LengthOrPercentageOrAuto;
 }
 
-    pub fn get_border(&self) -> Option<u32> {
+impl HTMLTableElementLayoutHelpers for LayoutJS<HTMLTableElement> {
-        self.border.get()
+    #[allow(unsafe_code)]
+    fn get_background_color(&self) -> Option<RGBA> {
+        unsafe {
+            (*self.unsafe_get()).background_color.get()
+        }
     }
 
-    pub fn get_cellspacing(&self) -> Option<u32> {
+    #[allow(unsafe_code)]
-        self.cellspacing.get()
+    fn get_border(&self) -> Option<u32> {
+        unsafe {
+            (*self.unsafe_get()).border.get()
+        }
     }
 
-    pub fn get_width(&self) -> LengthOrPercentageOrAuto {
+    #[allow(unsafe_code)]
-        self.width.get()
+    fn get_cellspacing(&self) -> Option<u32> {
+        unsafe {
+            (*self.unsafe_get()).cellspacing.get()
+        }
+    }
+
+    #[allow(unsafe_code)]
+    fn get_width(&self) -> LengthOrPercentageOrAuto {
+        unsafe {
+            (*self.unsafe_get()).width.get()
+        }
     }
 }
 

components/script/dom/macros.rs

@@ -215,6 +215,21 @@ macro_rules! make_atomic_setter(
     );
 );
 
+#[macro_export]
+macro_rules! make_legacy_color_setter(
+    ( $attr:ident, $htmlname:expr ) => (
+        fn $attr(&self, value: DOMString) {
+            use dom::bindings::inheritance::Castable;
+            use dom::element::Element;
+            use string_cache::Atom;
+            let element = self.upcast::<Element>();
+            let value = AttrValue::from_legacy_color(value);
+            // FIXME(pcwalton): Do this at compile time, not at runtime.
+            element.set_attribute(&Atom::from_slice($htmlname), value)
+        }
+    );
+);
+
 /// For use on non-jsmanaged types
 /// Use #[derive(JSTraceable)] on JS managed types
 macro_rules! no_jsmanaged_fields(