auto merge of #1760 : Ms2ger/servo/idmap-improvements, r=jdm

src/components/script/dom/attr.rs

@@ -9,8 +9,6 @@ use dom::window::Window;
 use servo_util::namespace::{Namespace, Null};
 use servo_util::str::DOMString;
 
-use std::util;
-
 #[deriving(Encodable)]
 pub struct Attr {
     reflector_: Reflector,
@@ -63,9 +61,8 @@ impl Attr {
         reflect_dom_object(~attr, window, AttrBinding::Wrap)
     }
 
-    pub fn set_value(&mut self, mut value: DOMString) -> DOMString {
+    pub fn set_value(&mut self, value: DOMString) {
-        util::swap(&mut self.value, &mut value);
+        self.value = value;
-        value
     }
 
     pub fn value_ref<'a>(&'a self) -> &'a str {

src/components/script/dom/document.rs

@@ -478,68 +478,30 @@ impl Document {
         self.window.get().wait_until_safe_to_modify_dom();
     }
 
-    pub fn register_nodes_with_id(&mut self, root: &JS<Element>) {
+
-        foreach_ided_elements(root, |id: &DOMString, abstract_node: &JS<Element>| {
+    /// Remove any existing association between the provided id and any elements in this document.
-            // TODO: "in tree order, within the context object's tree"
+    pub fn unregister_named_element(&mut self,
-            // http://dom.spec.whatwg.org/#dom-document-getelementbyid.
+                                    id: DOMString) {
-            self.idmap.find_or_insert(id.clone(), abstract_node.clone());
+        self.idmap.remove(&id);
-        });
     }
 
-    pub fn unregister_nodes_with_id(&mut self, root: &JS<Element>) {
+    /// Associate an element present in this document with the provided id.
-        foreach_ided_elements(root, |id: &DOMString, _| {
+    pub fn register_named_element(&mut self,
-            // TODO: "in tree order, within the context object's tree"
+                                  element: &JS<Element>,
-            // http://dom.spec.whatwg.org/#dom-document-getelementbyid.
+                                  id: DOMString) {
-            self.idmap.pop(id);
+        assert!({
+            let node: JS<Node> = NodeCast::from(element);
+            node.is_in_doc()
         });
-    }
 
-    pub fn update_idmap(&mut self,
+        // TODO: support the case if multiple elements
-                        abstract_self: &JS<Element>,
+        // which haves same id are in the same document.
-                        new_id: Option<DOMString>,
+        self.idmap.mangle(id, element,
-                        old_id: Option<DOMString>) {
+                          |_, new_element: &JS<Element>| -> JS<Element> {
-        // remove old ids:
+                              new_element.clone()
-        // * if the old ones are not same as the new one,
+                          },
-        // * OR if the new one is none.
+                          |_, old_element: &mut JS<Element>, new_element: &JS<Element>| {
-        match old_id {
+                              *old_element = new_element.clone();
-            Some(ref old_id) if new_id.is_none() ||
+                          });
-                                (*new_id.get_ref() != *old_id) => {
-                self.idmap.remove(old_id);
-            }
-            _ => ()
-        }
-
-        match new_id {
-            Some(new_id) => {
-                // TODO: support the case if multiple elements
-                // which haves same id are in the same document.
-                self.idmap.mangle(new_id, abstract_self.clone(),
-                                  |_, new_node: JS<Element>| -> JS<Element> {
-                                      new_node
-                                  },
-                                  |_, old_node: &mut JS<Element>, new_node: JS<Element>| {
-                                      *old_node = new_node;
-                                  });
-            }
-            None => ()
-        }
-    }
-}
-
-#[inline(always)]
-fn foreach_ided_elements(root: &JS<Element>, callback: |&DOMString, &JS<Element>|) {
-    let root: JS<Node> = NodeCast::from(root);
-    for node in root.traverse_preorder() {
-        if !node.is_element() {
-            continue;
-        }
-
-        let element: JS<Element> = ElementCast::to(&node);
-        match element.get().get_attribute(Null, "id") {
-            Some(id) => {
-                callback(&id.get().Value(), &element);
-            }
-            None => ()
-        }
     }
 }

src/components/script/dom/element.rs

@@ -204,26 +204,31 @@ impl Element {
         self.node.wait_until_safe_to_modify_dom();
 
         // FIXME: reduce the time of `value.clone()`.
-        let mut old_raw_value: Option<DOMString> = None;
+        let idx = self.attrs.iter().position(|attr| {
-        for attr in self.attrs.mut_iter() {
+            attr.get().local_name == local_name
-            let attr = attr.get_mut();
+        });
-            if attr.local_name == local_name {
+
-                old_raw_value = Some(attr.set_value(value.clone()));
+        match idx {
-                break;
+            Some(idx) => {
+                if namespace == namespace::Null {
+                    let old_value = self.attrs[idx].get().Value();
+                    self.before_remove_attr(abstract_self, local_name.clone(),
+                                            old_value);
+                }
+                self.attrs[idx].get_mut().set_value(value.clone());
+            }
+            None => {
+                let doc = self.node.owner_doc();
+                let doc = doc.get();
+                let new_attr = Attr::new_ns(doc.window.get(), local_name.clone(), value.clone(),
+                                            name.clone(), namespace.clone(),
+                                            prefix);
+                self.attrs.push(new_attr);
             }
         }
 
-        if old_raw_value.is_none() {
-            let doc = self.node.owner_doc();
-            let doc = doc.get();
-            let new_attr = Attr::new_ns(doc.window.get(), local_name.clone(), value.clone(),
-                                        name.clone(), namespace.clone(),
-                                        prefix);
-            self.attrs.push(new_attr);
-        }
-
         if namespace == namespace::Null {
-            self.after_set_attr(abstract_self, local_name, value, old_raw_value);
+            self.after_set_attr(abstract_self, local_name, value);
         }
         Ok(())
     }
@@ -231,8 +236,7 @@ impl Element {
     fn after_set_attr(&mut self,
                       abstract_self: &JS<Element>,
                       local_name: DOMString,
-                      value: DOMString,
+                      value: DOMString) {
-                      old_value: Option<DOMString>) {
 
         match local_name.as_slice() {
             "style" => {
@@ -247,7 +251,7 @@ impl Element {
                     // "borrowed value does not live long enough"
                     let mut doc = self.node.owner_doc();
                     let doc = doc.get_mut();
-                    doc.update_idmap(abstract_self, Some(value.clone()), old_value);
+                    doc.register_named_element(abstract_self, value.clone());
                 }
             }
             _ => ()
@@ -289,22 +293,22 @@ impl Element {
         match idx {
             None => (),
             Some(idx) => {
-                let removed = self.attrs.remove(idx);
-                let removed_raw_value = Some(removed.get().Value());
-
                 if namespace == namespace::Null {
-                    self.after_remove_attr(abstract_self, local_name, removed_raw_value);
+                    let removed_raw_value = self.attrs[idx].get().Value();
+                    self.before_remove_attr(abstract_self, local_name, removed_raw_value);
                 }
+
+                self.attrs.remove(idx);
             }
         };
 
         Ok(())
     }
 
-    fn after_remove_attr(&mut self,
+    fn before_remove_attr(&mut self,
-                         abstract_self: &JS<Element>,
+                          abstract_self: &JS<Element>,
-                         local_name: DOMString,
+                          local_name: DOMString,
-                         old_value: Option<DOMString>) {
+                          old_value: DOMString) {
         match local_name.as_slice() {
             "style" => {
                 self.style_attribute = None
@@ -316,7 +320,7 @@ impl Element {
                     // "borrowed value does not live long enough"
                     let mut doc = self.node.owner_doc();
                     let doc = doc.get_mut();
-                    doc.update_idmap(abstract_self, None, old_value);
+                    doc.unregister_named_element(old_value);
                 }
             }
             _ => ()
@@ -327,11 +331,11 @@ impl Element {
         match abstract_self.get().node.type_id {
             ElementNodeTypeId(HTMLImageElementTypeId) => {
                 let mut elem: JS<HTMLImageElement> = HTMLImageElementCast::to(abstract_self);
-                elem.get_mut().AfterRemoveAttr(local_name.clone());
+                elem.get_mut().BeforeRemoveAttr(local_name.clone());
             }
             ElementNodeTypeId(HTMLIframeElementTypeId) => {
                 let mut elem: JS<HTMLIFrameElement> = HTMLIFrameElementCast::to(abstract_self);
-                elem.get_mut().AfterRemoveAttr(local_name.clone());
+                elem.get_mut().BeforeRemoveAttr(local_name.clone());
             }
             _ => ()
         }
@@ -646,6 +650,33 @@ impl Element {
     }
 }
 
+pub trait IElement {
+    fn bind_to_tree_impl(&self);
+    fn unbind_from_tree_impl(&self);
+}
+
+impl IElement for JS<Element> {
+    fn bind_to_tree_impl(&self) {
+        match self.get().get_attribute(Null, "id") {
+            Some(attr) => {
+                let mut doc = self.get().node.owner_doc();
+                doc.get_mut().register_named_element(self, attr.get().Value());
+            }
+            _ => ()
+        }
+    }
+
+    fn unbind_from_tree_impl(&self) {
+        match self.get().get_attribute(Null, "id") {
+            Some(attr) => {
+                let mut doc = self.get().node.owner_doc();
+                doc.get_mut().unregister_named_element(attr.get().Value());
+            }
+            _ => ()
+        }
+    }
+}
+
 fn get_attribute_parts(name: DOMString) -> (Option<~str>, ~str) {
     //FIXME: Throw for XML-invalid names
     //FIXME: Throw for XMLNS-invalid names

src/components/script/dom/htmliframeelement.rs

@@ -140,7 +140,7 @@ impl HTMLIFrameElement {
         }
     }
 
-    pub fn AfterRemoveAttr(&mut self, name: DOMString) {
+    pub fn BeforeRemoveAttr(&mut self, name: DOMString) {
         if "sandbox" == name {
             self.sandbox = None;
         }

src/components/script/dom/htmlimageelement.rs

@@ -96,7 +96,7 @@ impl HTMLImageElement {
         }
     }
 
-    pub fn AfterRemoveAttr(&mut self, name: DOMString) {
+    pub fn BeforeRemoveAttr(&mut self, name: DOMString) {
         if "src" == name {
             self.update_image(None, None);
         }

src/components/script/dom/node.rs

@@ -14,7 +14,7 @@ use dom::bindings::utils;
 use dom::characterdata::CharacterData;
 use dom::document::Document;
 use dom::documenttype::DocumentType;
-use dom::element::{Element, ElementTypeId, HTMLAnchorElementTypeId};
+use dom::element::{Element, ElementTypeId, HTMLAnchorElementTypeId, IElement};
 use dom::eventtarget::{EventTarget, NodeTargetTypeId};
 use dom::nodelist::{NodeList};
 use dom::text::Text;
@@ -396,11 +396,13 @@ impl NodeHelpers for JS<Node> {
     // http://dom.spec.whatwg.org/#node-is-inserted
     fn node_inserted(&self) {
         assert!(self.parent_node().is_some());
-        let mut document = self.get().owner_doc();
+        let document = self.get().owner_doc();
 
-        // Register elements having "id" attribute to the owner doc.
+        for node in self.traverse_preorder() {
-        if self.is_element() {
+            if node.is_element() {
-            document.get_mut().register_nodes_with_id(&ElementCast::to(self));
+                let element: JS<Element> = ElementCast::to(&node);
+                element.bind_to_tree_impl();
+            }
         }
 
         document.get().content_changed();
@@ -409,11 +411,13 @@ impl NodeHelpers for JS<Node> {
     // http://dom.spec.whatwg.org/#node-is-removed
     fn node_removed(&self) {
         assert!(self.parent_node().is_none());
-        let mut document = self.get().owner_doc();
+        let document = self.get().owner_doc();
 
-        // Unregister elements having "id".
+        for node in self.traverse_preorder() {
-        if self.is_element() {
+            if node.is_element() {
-            document.get_mut().unregister_nodes_with_id(&ElementCast::to(self));
+                let element: JS<Element> = ElementCast::to(&node);
+                element.unbind_from_tree_impl();
+            }
         }
 
         document.get().content_changed();