From 33792db657e0053c415c26b761cbdc2cf5ae2060 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 16:04:25 +0100 Subject: [PATCH 1/6] Move ID management on binding to and unbinding from tree into Element. --- src/components/script/dom/document.rs | 51 ++++++++++++--------------- src/components/script/dom/element.rs | 27 ++++++++++++++ src/components/script/dom/node.rs | 22 +++++++----- 3 files changed, 62 insertions(+), 38 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index 9854bb7e386..b699f63f50f 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -478,20 +478,31 @@ impl Document { self.window.get().wait_until_safe_to_modify_dom(); } - pub fn register_nodes_with_id(&mut self, root: &JS) { - foreach_ided_elements(root, |id: &DOMString, abstract_node: &JS| { - // TODO: "in tree order, within the context object's tree" - // http://dom.spec.whatwg.org/#dom-document-getelementbyid. - self.idmap.find_or_insert(id.clone(), abstract_node.clone()); - }); + + /// Remove any existing association between the provided id and any elements in this document. + pub fn unregister_named_element(&mut self, + id: DOMString) { + self.idmap.remove(&id); } - pub fn unregister_nodes_with_id(&mut self, root: &JS) { - foreach_ided_elements(root, |id: &DOMString, _| { - // TODO: "in tree order, within the context object's tree" - // http://dom.spec.whatwg.org/#dom-document-getelementbyid. - self.idmap.pop(id); + /// Associate an element present in this document with the provided id. + pub fn register_named_element(&mut self, + element: &JS, + id: DOMString) { + assert!({ + let node: JS = NodeCast::from(element); + node.is_in_doc() }); + + // TODO: support the case if multiple elements + // which haves same id are in the same document. + self.idmap.mangle(id, element, + |_, new_element: &JS| -> JS { + new_element.clone() + }, + |_, old_element: &mut JS, new_element: &JS| { + *old_element = new_element.clone(); + }); } pub fn update_idmap(&mut self, @@ -525,21 +536,3 @@ impl Document { } } } - -#[inline(always)] -fn foreach_ided_elements(root: &JS, callback: |&DOMString, &JS|) { - let root: JS = NodeCast::from(root); - for node in root.traverse_preorder() { - if !node.is_element() { - continue; - } - - let element: JS = ElementCast::to(&node); - match element.get().get_attribute(Null, "id") { - Some(id) => { - callback(&id.get().Value(), &element); - } - None => () - } - } -} diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index d7bbaa065a6..eed0bc9b237 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -646,6 +646,33 @@ impl Element { } } +pub trait IElement { + fn bind_to_tree_impl(&self); + fn unbind_from_tree_impl(&self); +} + +impl IElement for JS { + 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 diff --git a/src/components/script/dom/node.rs b/src/components/script/dom/node.rs index a0357c92ab4..419b02c844a 100644 --- a/src/components/script/dom/node.rs +++ b/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 { // 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. - if self.is_element() { - document.get_mut().register_nodes_with_id(&ElementCast::to(self)); + for node in self.traverse_preorder() { + if node.is_element() { + let element: JS = ElementCast::to(&node); + element.bind_to_tree_impl(); + } } document.get().content_changed(); @@ -409,11 +411,13 @@ impl NodeHelpers for JS { // 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". - if self.is_element() { - document.get_mut().unregister_nodes_with_id(&ElementCast::to(self)); + for node in self.traverse_preorder() { + if node.is_element() { + let element: JS = ElementCast::to(&node); + element.unbind_from_tree_impl(); + } } document.get().content_changed(); From 58b317837b315884014fc189e16a0a00aef99a60 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 16:18:13 +0100 Subject: [PATCH 2/6] Cleanup Element::set_attribute. --- src/components/script/dom/attr.rs | 7 ++----- src/components/script/dom/element.rs | 31 ++++++++++++++-------------- 2 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/components/script/dom/attr.rs b/src/components/script/dom/attr.rs index 3cf978d69c4..906e1b7f02c 100644 --- a/src/components/script/dom/attr.rs +++ b/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 { - util::swap(&mut self.value, &mut value); - value + pub fn set_value(&mut self, value: DOMString) { + self.value = value; } pub fn value_ref<'a>(&'a self) -> &'a str { diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index eed0bc9b237..cda20ef84a5 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -204,22 +204,23 @@ impl Element { self.node.wait_until_safe_to_modify_dom(); // FIXME: reduce the time of `value.clone()`. - let mut old_raw_value: Option = None; - for attr in self.attrs.mut_iter() { - let attr = attr.get_mut(); - if attr.local_name == local_name { - old_raw_value = Some(attr.set_value(value.clone())); - break; - } - } + let idx = self.attrs.iter().position(|attr| { + attr.get().local_name == local_name + }); + let old_raw_value = idx.map(|idx| self.attrs[idx].get().Value()); - 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); + match idx { + Some(idx) => { + 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 namespace == namespace::Null { From fdd17b41399cda3822cb004b3adee8fb1ff08883 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 17:15:31 +0100 Subject: [PATCH 3/6] Pass a non-Option DOMString to after_remove_attr. --- src/components/script/dom/element.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index cda20ef84a5..629fa5ed30c 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -291,7 +291,7 @@ impl Element { None => (), Some(idx) => { let removed = self.attrs.remove(idx); - let removed_raw_value = Some(removed.get().Value()); + let removed_raw_value = removed.get().Value(); if namespace == namespace::Null { self.after_remove_attr(abstract_self, local_name, removed_raw_value); @@ -305,7 +305,7 @@ impl Element { fn after_remove_attr(&mut self, abstract_self: &JS, local_name: DOMString, - old_value: Option) { + old_value: DOMString) { match local_name.as_slice() { "style" => { self.style_attribute = None @@ -317,7 +317,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.update_idmap(abstract_self, None, Some(old_value)); } } _ => () From 432e9cb3a7c895b7065cb067ad3e752d453c2d90 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 17:18:47 +0100 Subject: [PATCH 4/6] Call after_remove_attr before removing the attribute, and rename it before_remove_attr. --- src/components/script/dom/element.rs | 20 +++++++++---------- .../script/dom/htmliframeelement.rs | 2 +- src/components/script/dom/htmlimageelement.rs | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index 629fa5ed30c..cc347ced7a8 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -290,22 +290,22 @@ impl Element { match idx { None => (), Some(idx) => { - let removed = self.attrs.remove(idx); - let removed_raw_value = 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, - abstract_self: &JS, - local_name: DOMString, - old_value: DOMString) { + fn before_remove_attr(&mut self, + abstract_self: &JS, + local_name: DOMString, + old_value: DOMString) { match local_name.as_slice() { "style" => { self.style_attribute = None @@ -328,11 +328,11 @@ impl Element { match abstract_self.get().node.type_id { ElementNodeTypeId(HTMLImageElementTypeId) => { let mut elem: JS = HTMLImageElementCast::to(abstract_self); - elem.get_mut().AfterRemoveAttr(local_name.clone()); + elem.get_mut().BeforeRemoveAttr(local_name.clone()); } ElementNodeTypeId(HTMLIframeElementTypeId) => { let mut elem: JS = HTMLIFrameElementCast::to(abstract_self); - elem.get_mut().AfterRemoveAttr(local_name.clone()); + elem.get_mut().BeforeRemoveAttr(local_name.clone()); } _ => () } diff --git a/src/components/script/dom/htmliframeelement.rs b/src/components/script/dom/htmliframeelement.rs index 92e0c63dfc7..4f25371772b 100644 --- a/src/components/script/dom/htmliframeelement.rs +++ b/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; } diff --git a/src/components/script/dom/htmlimageelement.rs b/src/components/script/dom/htmlimageelement.rs index 4f18597449c..1b336282b3d 100644 --- a/src/components/script/dom/htmlimageelement.rs +++ b/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); } From 86899b6aaf25bc15be4475fa62d2cd70a82dfa83 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 17:23:20 +0100 Subject: [PATCH 5/6] Call unregister_named_element rather update_idmap from before_remove_attr. --- src/components/script/dom/element.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index cc347ced7a8..d0bc904afe6 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -317,7 +317,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, Some(old_value)); + doc.unregister_named_element(old_value); } } _ => () From 25ee5270d195ca055e2788d6f89b8f57a8994ef1 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Tue, 25 Feb 2014 17:39:46 +0100 Subject: [PATCH 6/6] Only register the new ID from after_set_attr, and remove update_idmap. --- src/components/script/dom/document.rs | 31 --------------------------- src/components/script/dom/element.rs | 13 ++++++----- 2 files changed, 8 insertions(+), 36 deletions(-) diff --git a/src/components/script/dom/document.rs b/src/components/script/dom/document.rs index b699f63f50f..3f917023c88 100644 --- a/src/components/script/dom/document.rs +++ b/src/components/script/dom/document.rs @@ -504,35 +504,4 @@ impl Document { *old_element = new_element.clone(); }); } - - pub fn update_idmap(&mut self, - abstract_self: &JS, - new_id: Option, - old_id: Option) { - // remove old ids: - // * if the old ones are not same as the new one, - // * OR if the new one is none. - match old_id { - 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| -> JS { - new_node - }, - |_, old_node: &mut JS, new_node: JS| { - *old_node = new_node; - }); - } - None => () - } - } } diff --git a/src/components/script/dom/element.rs b/src/components/script/dom/element.rs index d0bc904afe6..b3d5d06f38a 100644 --- a/src/components/script/dom/element.rs +++ b/src/components/script/dom/element.rs @@ -207,10 +207,14 @@ impl Element { let idx = self.attrs.iter().position(|attr| { attr.get().local_name == local_name }); - let old_raw_value = idx.map(|idx| self.attrs[idx].get().Value()); match idx { 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 => { @@ -224,7 +228,7 @@ impl Element { } 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(()) } @@ -232,8 +236,7 @@ impl Element { fn after_set_attr(&mut self, abstract_self: &JS, local_name: DOMString, - value: DOMString, - old_value: Option) { + value: DOMString) { match local_name.as_slice() { "style" => { @@ -248,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()); } } _ => ()