Address review comments

components/script/dom/htmlformelement.rs

@@ -7,7 +7,7 @@ use dom::bindings::codegen::Bindings::EventTargetBinding::EventTargetMethods;
 use dom::bindings::codegen::Bindings::HTMLFormElementBinding;
 use dom::bindings::codegen::Bindings::HTMLFormElementBinding::HTMLFormElementMethods;
 use dom::bindings::codegen::Bindings::HTMLInputElementBinding::HTMLInputElementMethods;
-use dom::bindings::codegen::InheritTypes::{ElementCast, EventTargetCast, HTMLFormElementDerived, NodeCast};
+use dom::bindings::codegen::InheritTypes::{EventTargetCast, HTMLFormElementDerived, NodeCast};
 use dom::bindings::codegen::InheritTypes::HTMLInputElementCast;
 use dom::bindings::global::Window;
 use dom::bindings::js::{JSRef, Temporary};
@@ -64,46 +64,19 @@ impl<'a> HTMLFormElementMethods for JSRef<'a, HTMLFormElement> {
     make_setter!(SetAcceptCharset, "accept-charset")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-action
-    fn Action(self) -> DOMString {
-        let element: JSRef<Element> = ElementCast::from_ref(self);
-        let url = element.get_url_attribute("action");
-        match url.as_slice() {
-            "" => {
-                let window = window_from_node(self).root();
-                window.get_url().serialize()
-            },
-            _ => url
-        }
-    }
+    make_url_or_base_getter!(Action)
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-action
     make_setter!(SetAction, "action")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-form-autocomplete
-    fn Autocomplete(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let ac = elem.get_string_attribute("autocomplete").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-form-autocomplete
-        match ac.as_slice() {
-            "off" => ac,
-            _ => "on".to_string()
-        }
-    }
+    make_enumerated_getter!(Autocomplete, "on", "off")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-form-autocomplete
     make_setter!(SetAutocomplete, "autocomplete")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-enctype
-    fn Enctype(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let enctype = elem.get_string_attribute("enctype").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-fs-enctype
-        match enctype.as_slice() {
-            "text/plain" | "multipart/form-data" => enctype,
-            _ => "application/x-www-form-urlencoded".to_string()
-        }
-    }
-
+    make_enumerated_getter!(Enctype, "application/x-www-form-urlencoded", "text/plain" | "multipart/form-data")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-enctype
     make_setter!(SetEnctype, "enctype")
@@ -119,15 +92,7 @@ impl<'a> HTMLFormElementMethods for JSRef<'a, HTMLFormElement> {
     }
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-method
-    fn Method(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let method = elem.get_string_attribute("method").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-fs-method
-        match method.as_slice() {
-            "post" | "dialog" => method,
-            _ => "get".to_string()
-        }
-    }
+    make_enumerated_getter!(Method, "get", "post" | "dialog")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-fs-method
     make_setter!(SetMethod, "method")
@@ -384,28 +349,14 @@ impl<'a> FormSubmitter<'a> {
     fn action(&self) -> DOMString {
         match *self {
             FormElement(form) => form.Action(),
-            InputElement(input_element) => {
-                let element: JSRef<Element> = ElementCast::from_ref(input_element);
-                if element.has_attribute("formaction") {
-                    input_element.FormAction()
-                } else {
-                    input_element.form_owner().map_or("".to_string(), |f_o| f_o.Action())
-                }
-            }
+            InputElement(input_element) => input_element.get_form_attribute("formaction", |i| i.FormAction(), |f| f.Action())
         }
     }
 
     fn enctype(&self) -> FormEncType {
         let attr = match *self {
             FormElement(form) => form.Enctype(),
-            InputElement(input_element) => {
-                let element: JSRef<Element> = ElementCast::from_ref(input_element);
-                if element.has_attribute("formenctype") {
-                    input_element.FormEnctype()
-                } else {
-                    input_element.form_owner().map_or("".to_string(), |f_o| f_o.Enctype())
-                }
-            }
+            InputElement(input_element) => input_element.get_form_attribute("formenctype", |i| i.FormEnctype(), |f| f.Enctype())
         };
         match attr.as_slice() {
             "multipart/form-data" => FormDataEncoded,
@@ -419,14 +370,7 @@ impl<'a> FormSubmitter<'a> {
     fn method(&self) -> FormMethod {
         let attr = match *self {
             FormElement(form) => form.Method(),
-            InputElement(input_element) => {
-                let element: JSRef<Element> = ElementCast::from_ref(input_element);
-                if element.has_attribute("formmethod") {
-                    input_element.FormMethod()
-                } else {
-                    input_element.form_owner().map_or("".to_string(), |f_o| f_o.Method())
-                }
-            }
+            InputElement(input_element) => input_element.get_form_attribute("formmethod", |i| i.FormMethod(), |f| f.Method())
         };
         match attr.as_slice() {
             "dialog" => FormDialog,
@@ -438,18 +382,21 @@ impl<'a> FormSubmitter<'a> {
     fn target(&self) -> DOMString {
         match *self {
             FormElement(form) => form.Target(),
-            InputElement(input_element) => {
-                let element: JSRef<Element> = ElementCast::from_ref(input_element);
-                if element.has_attribute("formtarget") {
-                    input_element.FormTarget()
-                } else {
-                    input_element.form_owner().map_or("".to_string(), |f_o| f_o.Target())
-                }
-            }
+            InputElement(input_element) => input_element.get_form_attribute("formtarget", |i| i.FormTarget(), |f| f.Target())
         }
     }
 }
 
-pub trait FormOwner<'a> {
-    fn form_owner(self) -> Option<JSRef<'a, HTMLFormElement>>;
+pub trait FormOwner<'a> : Copy {
+    fn form_owner(self) -> Option<Temporary<HTMLFormElement>>;
+    fn get_form_attribute(self, attr: &str,
+                          input: |Self| -> DOMString,
+                          owner: |JSRef<HTMLFormElement>| -> DOMString) -> DOMString {
+        if self.to_element().has_attribute(attr) {
+            input(self)
+        } else {
+            self.form_owner().map_or("".to_string(), |t| owner(*t.root()))
+        }
+    }
+    fn to_element(self) -> JSRef<'a, Element>;
 }

components/script/dom/htmlinputelement.rs

@@ -137,21 +137,13 @@ impl<'a> HTMLInputElementMethods for JSRef<'a, HTMLInputElement> {
     make_uint_setter!(SetSize, "size")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-type
-    fn Type(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let ty = elem.get_string_attribute("type").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-input-type
-        match ty.as_slice() {
-            "hidden" | "search" | "tel" |
-            "url" | "email" | "password" |
-            "datetime" | "date" | "month" |
-            "week" | "time" | "datetime-local" |
-            "number" | "range" | "color" |
-            "checkbox" | "radio" | "file" |
-            "submit" | "image" | "reset" | "button" => ty,
-            _ => "text".to_string()
-        }
-    }
+    make_enumerated_getter!(Type, "text", "hidden" | "search" | "tel" |
+                                  "url" | "email" | "password" |
+                                  "datetime" | "date" | "month" |
+                                  "week" | "time" | "datetime-local" |
+                                  "number" | "range" | "color" |
+                                  "checkbox" | "radio" | "file" |
+                                  "submit" | "image" | "reset" | "button")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-type
     make_setter!(SetType, "type")
@@ -171,45 +163,19 @@ impl<'a> HTMLInputElementMethods for JSRef<'a, HTMLInputElement> {
     make_setter!(SetName, "name")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formaction
-    fn FormAction(self) -> DOMString {
-        let element: JSRef<Element> = ElementCast::from_ref(self);
-        let url = element.get_url_attribute("formaction");
-        match url.as_slice() {
-            "" => {
-                let window = window_from_node(self).root();
-                window.get_url().serialize()
-            },
-            _ => url
-        }
-    }
+    make_url_or_base_getter!(FormAction)
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formaction
     make_setter!(SetFormAction, "formaction")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formenctype
-    fn FormEnctype(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let enctype = elem.get_string_attribute("formenctype").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-fs-enctype
-        match enctype.as_slice() {
-            "text/plain" | "multipart/form-data" => enctype,
-            _ => "application/x-www-form-urlencoded".to_string()
-        }
-    }
+    make_enumerated_getter!(FormEnctype, "application/x-www-form-urlencoded", "text/plain" | "multipart/form-data")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formenctype
     make_setter!(SetFormEnctype, "formenctype")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formmethod
-    fn FormMethod(self) -> DOMString {
-        let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let method = elem.get_string_attribute("formmethod").into_ascii_lower();
-        // https://html.spec.whatwg.org/multipage/forms.html#attr-fs-method
-        match method.as_slice() {
-            "post" | "dialog" => method,
-            _ => "get".to_string()
-        }
-    }
+        make_enumerated_getter!(FormMethod, "get", "post" | "dialog")
 
     // https://html.spec.whatwg.org/multipage/forms.html#dom-input-formmethod
     make_setter!(SetFormMethod, "formmethod")
@@ -422,7 +388,7 @@ impl<'a> VirtualMethods for JSRef<'a, HTMLInputElement> {
                 InputRadio => self.SetChecked(true),
                 InputButton(Some(DEFAULT_SUBMIT_VALUE)) => {
                     self.form_owner().map(|o| {
-                        o.submit(false, InputElement(self.clone()))
+                        o.root().submit(false, InputElement(self.clone()))
                     });
                 }
                 _ => {}
@@ -440,10 +406,10 @@ impl Reflectable for HTMLInputElement {
 impl<'a> FormOwner<'a> for JSRef<'a, HTMLInputElement> {
     // FIXME: This is wrong (https://github.com/servo/servo/issues/3553)
     //        but we need html5ever to do it correctly
-    fn form_owner(self) -> Option<JSRef<'a, HTMLFormElement>> {
+    fn form_owner(self) -> Option<Temporary<HTMLFormElement>> {
         // https://html.spec.whatwg.org/multipage/forms.html#reset-the-form-owner
         let elem: JSRef<Element> = ElementCast::from_ref(self);
-        let owner = elem.get_string_attribute("owner");
+        let owner = elem.get_string_attribute("form");
         if !owner.is_empty() {
             let doc = document_from_node(self).root();
             let owner = doc.GetElementById(owner).root();
@@ -451,13 +417,18 @@ impl<'a> FormOwner<'a> for JSRef<'a, HTMLInputElement> {
                 Some(o) => {
                     let maybe_form: Option<JSRef<HTMLFormElement>> = HTMLFormElementCast::to_ref(*o);
                     if maybe_form.is_some() {
-                        return maybe_form;
+                        return maybe_form.map(Temporary::from_rooted);
                     }
                 },
                 _ => ()
             }
         }
         let node: JSRef<Node> = NodeCast::from_ref(self);
-        node.ancestors().filter_map(|a| HTMLFormElementCast::to_ref(a)).next().clone()
+        node.ancestors().filter_map(|a| HTMLFormElementCast::to_ref(a)).next()
+            .map(Temporary::from_rooted)
+    }
+
+    fn to_element(self) -> JSRef<'a, Element> {
+        ElementCast::from_ref(self)
     }
 }

components/script/dom/macros.rs

@@ -70,6 +70,52 @@ macro_rules! make_url_getter(
     }
 )
 
+#[macro_export]
+macro_rules! make_url_or_base_getter(
+    ( $attr:ident, $htmlname:expr ) => (
+        fn $attr(self) -> DOMString {
+            use dom::element::{Element, AttributeHandlers};
+            use dom::bindings::codegen::InheritTypes::ElementCast;
+            #[allow(unused_imports)]
+            use std::ascii::StrAsciiExt;
+            let element: JSRef<Element> = ElementCast::from_ref(self);
+            let url = element.get_url_attribute($htmlname);
+            match url.as_slice() {
+                "" => {
+                    let window = window_from_node(self).root();
+                    window.get_url().serialize()
+                },
+                _ => url
+            }
+        }
+    );
+    ($attr:ident) => {
+        make_url_or_base_getter!($attr, stringify!($attr).to_ascii_lower().as_slice())
+    }
+)
+
+#[macro_export]
+macro_rules! make_enumerated_getter(
+    ( $attr:ident, $htmlname:expr, $default:expr, $($choices: pat)|+) => (
+        fn $attr(self) -> DOMString {
+            use dom::element::{Element, AttributeHandlers};
+            use dom::bindings::codegen::InheritTypes::ElementCast;
+            #[allow(unused_imports)]
+            use std::ascii::StrAsciiExt;
+            let element: JSRef<Element> = ElementCast::from_ref(self);
+            let val = element.get_string_attribute($htmlname).into_ascii_lower();
+            // https://html.spec.whatwg.org/multipage/forms.html#attr-fs-method
+            match val.as_slice() {
+                $($choices)|+ => val,
+                _ => $default.to_string()
+            }
+        }
+    );
+    ($attr:ident, $default:expr, $($choices: pat)|+) => {
+        make_enumerated_getter!($attr, stringify!($attr).to_ascii_lower().as_slice(), $default, $($choices)|+)
+    }
+)
+
 // concat_idents! doesn't work for function name positions, so
 // we have to specify both the content name and the HTML name here
 #[macro_export]