Auto merge of #5575 - nox:namednodemap-remove, r=Ms2ger

components/script/dom/element.rs

@@ -627,11 +627,13 @@ pub trait AttributeHandlers {
 
     /// Removes the first attribute with any given namespace and case-sensitive local
     /// name, if any.
-    fn remove_attribute(self, namespace: &Namespace, local_name: &Atom);
+    fn remove_attribute(self, namespace: &Namespace, local_name: &Atom)
+                        -> Option<Temporary<Attr>>;
     /// Removes the first attribute with any namespace and given case-sensitive name.
-    fn remove_attribute_by_name(self, name: &Atom);
+    fn remove_attribute_by_name(self, name: &Atom) -> Option<Temporary<Attr>>;
     /// Removes the first attribute that satisfies `find`.
-    fn do_remove_attribute<F>(self, find: F) where F: Fn(JSRef<Attr>) -> bool;
+    fn do_remove_attribute<F>(self, find: F) -> Option<Temporary<Attr>>
+        where F: Fn(JSRef<Attr>) -> bool;
 
     fn has_class(self, name: &Atom) -> bool;
 
@@ -764,22 +766,25 @@ impl<'a> AttributeHandlers for JSRef<'a, Element> {
         }
     }
 
-    fn remove_attribute(self, namespace: &Namespace, local_name: &Atom) {
+    fn remove_attribute(self, namespace: &Namespace, local_name: &Atom)
+                        -> Option<Temporary<Attr>> {
         self.do_remove_attribute(|attr| {
             attr.namespace() == namespace && attr.local_name() == local_name
-        });
+        })
     }
 
-    fn remove_attribute_by_name(self, name: &Atom) {
+    fn remove_attribute_by_name(self, name: &Atom) -> Option<Temporary<Attr>> {
-        self.do_remove_attribute(|attr| attr.name() == name);
+        self.do_remove_attribute(|attr| attr.name() == name)
     }
 
-    fn do_remove_attribute<F>(self, find: F) where F: Fn(JSRef<Attr>) -> bool {
+    fn do_remove_attribute<F>(self, find: F) -> Option<Temporary<Attr>>
+        where F: Fn(JSRef<Attr>) -> bool
+    {
         let idx = self.attrs.borrow().iter()
                                      .map(|attr| attr.root())
                                      .position(|attr| find(attr.r()));
 
-        if let Some(idx) = idx {
+        idx.map(|idx| {
             let attr = (*self.attrs.borrow())[idx].root();
             if attr.r().namespace() == &ns!("") {
                 vtable_for(&NodeCast::from_ref(self)).before_remove_attr(attr.r());
@@ -798,7 +803,8 @@ impl<'a> AttributeHandlers for JSRef<'a, Element> {
                 };
                 document.r().content_changed(node, damage);
             }
-        };
+            Temporary::from_rooted(attr.r())
+        })
     }
 
     fn has_class(self, name: &Atom) -> bool {

components/script/dom/namednodemap.rs

@@ -5,6 +5,7 @@
 use dom::attr::Attr;
 use dom::bindings::codegen::Bindings::NamedNodeMapBinding;
 use dom::bindings::codegen::Bindings::NamedNodeMapBinding::NamedNodeMapMethods;
+use dom::bindings::error::{Error, Fallible};
 use dom::bindings::global::GlobalRef;
 use dom::bindings::js::{JS, JSRef, Temporary};
 use dom::bindings::utils::{Reflector, reflect_dom_object};
@@ -64,12 +65,32 @@ impl<'a> NamedNodeMapMethods for JSRef<'a, NamedNodeMap> {
     }
 
     // https://dom.spec.whatwg.org/#dom-namednodemap-getnameditemns
-    fn GetNamedItemNS(self, namespace: Option<DOMString>, name: DOMString) -> Option<Temporary<Attr>> {
+    fn GetNamedItemNS(self, namespace: Option<DOMString>, local_name: DOMString)
+                     -> Option<Temporary<Attr>> {
         let owner = self.owner.root();
         // FIXME(https://github.com/rust-lang/rust/issues/23338)
         let owner = owner.r();
         let ns = namespace::from_domstring(namespace);
-        owner.get_attribute(&ns, &Atom::from_slice(&name))
+        owner.get_attribute(&ns, &Atom::from_slice(&local_name))
+    }
+
+    // https://dom.spec.whatwg.org/#dom-namednodemap-removenameditem
+    fn RemoveNamedItem(self, name: DOMString) -> Fallible<Temporary<Attr>> {
+        let owner = self.owner.root();
+        // FIXME(https://github.com/rust-lang/rust/issues/23338)
+        let owner = owner.r();
+        let name = owner.parsed_name(name);
+        owner.remove_attribute_by_name(&Atom::from_slice(&name)).ok_or(Error::NotFound)
+    }
+
+    // https://dom.spec.whatwg.org/#dom-namednodemap-removenameditemns
+    fn RemoveNamedItemNS(self, namespace: Option<DOMString>, local_name: DOMString)
+                      -> Fallible<Temporary<Attr>> {
+        let owner = self.owner.root();
+        // FIXME(https://github.com/rust-lang/rust/issues/23338)
+        let owner = owner.r();
+        let ns = namespace::from_domstring(namespace);
+        owner.remove_attribute(&ns, &Atom::from_slice(&local_name)).ok_or(Error::NotFound)
     }
 
     fn IndexedGetter(self, index: u32, found: &mut bool) -> Option<Temporary<Attr>> {

components/script/dom/webidls/NamedNodeMap.webidl

@@ -11,8 +11,8 @@ interface NamedNodeMap {
   //Attr? setNamedItem(Attr attr);
   //[Throws]
   //Attr? setNamedItemNS(Attr attr);
-  //[Throws]
+  [Throws]
-  //Attr removeNamedItem(DOMString name);
+  Attr removeNamedItem(DOMString name);
-  //[Throws]
+  [Throws]
-  //Attr removeNamedItemNS(DOMString? namespace, DOMString name);
+  Attr removeNamedItemNS(DOMString? namespace, DOMString name);
 };

tests/wpt/metadata/dom/interfaces.html.ini

@@ -345,12 +345,6 @@
   [NamedNodeMap interface: operation setNamedItemNS(Attr)]
     expected: FAIL
 
-  [NamedNodeMap interface: operation removeNamedItem(DOMString)]
-    expected: FAIL
-
-  [NamedNodeMap interface: operation removeNamedItemNS(DOMString,DOMString)]
-    expected: FAIL
-
   [CharacterData interface: attribute previousElementSibling]
     expected: FAIL