Introduce VirtualMethods::attribute_mutated()

This replaces before_remove_attr(), after_remove_attr() and after_set_attr().
The virtual method takes the mutated attribute and an AttributeMutation value
to disambiguate between "attribute is changed", "attribute is added" and
"attribute is removed".

In the case of "attribute is changed", the mutation value contains a reference
to the old value of the mutated attribute, which is used to unregister outdated
named elements when the "id" attribute is changed on an element.

This greatly simplifies the handling of attributes, which in many cases don't
have any specific behaviour whether they are removed or changed or added. It
also fixes a few bugs where things were put in before_remove_attr() instead of
after_remove_attr() (e.g. when removing an href attribute from a base element).

A few helper functions in Element were also renamed and made private.

components/script/dom/element.rs

@@ -6,7 +6,7 @@
 
 use dom::activation::Activatable;
 use dom::attr::AttrValue;
-use dom::attr::{Attr, AttrSettingType, AttrHelpersForLayout};
+use dom::attr::{Attr, AttrHelpersForLayout};
 use dom::bindings::cell::DOMRefCell;
 use dom::bindings::codegen::Bindings::AttrBinding::AttrMethods;
 use dom::bindings::codegen::Bindings::ElementBinding;
@@ -825,6 +825,22 @@ impl Element {
 
 
 impl Element {
+    pub fn push_new_attribute(&self,
+                              local_name: Atom,
+                              value: AttrValue,
+                              name: Atom,
+                              namespace: Namespace,
+                              prefix: Option<Atom>) {
+        let window = window_from_node(self);
+        let in_empty_ns = namespace == ns!("");
+        let attr = Attr::new(&window, local_name, value, name, namespace, prefix, Some(self));
+        self.attrs.borrow_mut().push(JS::from_rooted(&attr));
+        if in_empty_ns {
+            vtable_for(NodeCast::from_ref(self)).attribute_mutated(
+                &attr, AttributeMutation::Set(None));
+        }
+    }
+
     pub fn get_attribute(&self, namespace: &Namespace, local_name: &Atom) -> Option<Root<Attr>> {
         self.attrs.borrow().iter().map(JS::root).find(|attr| {
             attr.local_name() == local_name && attr.namespace() == namespace
@@ -839,9 +855,9 @@ impl Element {
     }
 
     pub fn set_attribute_from_parser(&self,
-                                 qname: QualName,
-                                 value: DOMString,
-                                 prefix: Option<Atom>) {
+                                     qname: QualName,
+                                     value: DOMString,
+                                     prefix: Option<Atom>) {
         // Don't set if the attribute already exists, so we can handle add_attrs_if_missing
         if self.attrs.borrow().iter().map(JS::root)
                 .any(|a| *a.r().local_name() == qname.local && *a.r().namespace() == qname.ns) {
@@ -856,15 +872,16 @@ impl Element {
             },
         };
         let value = self.parse_attribute(&qname.ns, &qname.local, value);
-        self.do_set_attribute(qname.local, value, name, qname.ns, prefix, |_| false)
+        self.push_new_attribute(qname.local, value, name, qname.ns, prefix);
     }
 
     pub fn set_attribute(&self, name: &Atom, value: AttrValue) {
         assert!(&**name == name.to_ascii_lowercase());
         assert!(!name.contains(":"));
 
-        self.do_set_attribute(name.clone(), value, name.clone(),
-            ns!(""), None, |attr| attr.local_name() == name);
+        self.set_first_matching_attribute(
+            name.clone(), value, name.clone(), ns!(""), None,
+            |attr| attr.local_name() == name);
     }
 
     // https://html.spec.whatwg.org/multipage/#attr-data-*
@@ -878,34 +895,27 @@ impl Element {
         // Steps 2-5.
         let name = Atom::from_slice(&name);
         let value = self.parse_attribute(&ns!(""), &name, value);
-        self.do_set_attribute(name.clone(), value, name.clone(), ns!(""), None, |attr| {
-            *attr.name() == name && *attr.namespace() == ns!("")
-        });
+        self.set_first_matching_attribute(
+            name.clone(), value, name.clone(), ns!(""), None,
+            |attr| *attr.name() == name && *attr.namespace() == ns!(""));
         Ok(())
     }
 
-    pub fn do_set_attribute<F>(&self,
-                           local_name: Atom,
-                           value: AttrValue,
-                           name: Atom,
-                           namespace: Namespace,
-                           prefix: Option<Atom>,
-                           cb: F)
-        where F: Fn(&Attr) -> bool
-    {
-        let idx = self.attrs.borrow().iter().map(JS::root).position(|attr| cb(&attr));
-        let (idx, set_type) = match idx {
-            Some(idx) => (idx, AttrSettingType::ReplacedAttr),
-            None => {
-                let window = window_from_node(self);
-                let attr = Attr::new(window.r(), local_name, value.clone(),
-                                     name, namespace.clone(), prefix, Some(self));
-                self.attrs.borrow_mut().push(JS::from_rooted(&attr));
-                (self.attrs.borrow().len() - 1, AttrSettingType::FirstSetAttr)
-            }
+    fn set_first_matching_attribute<F>(&self,
+                                       local_name: Atom,
+                                       value: AttrValue,
+                                       name: Atom,
+                                       namespace: Namespace,
+                                       prefix: Option<Atom>,
+                                       find: F)
+                                       where F: Fn(&Attr)
+                                       -> bool {
+        let attr = self.attrs.borrow().iter().map(JS::root).find(|attr| find(&attr));
+        if let Some(attr) = attr {
+            attr.set_value(value, self);
+        } else {
+            self.push_new_attribute(local_name, value, name, namespace, prefix);
         };
-
-        (*self.attrs.borrow())[idx].root().r().set_value(set_type, value, self);
     }
 
     pub fn parse_attribute(&self, namespace: &Namespace, local_name: &Atom,
@@ -920,41 +930,27 @@ impl Element {
 
     pub fn remove_attribute(&self, namespace: &Namespace, local_name: &Atom)
                         -> Option<Root<Attr>> {
-        self.do_remove_attribute(|attr| {
+        self.remove_first_matching_attribute(|attr| {
             attr.namespace() == namespace && attr.local_name() == local_name
         })
     }
 
     pub fn remove_attribute_by_name(&self, name: &Atom) -> Option<Root<Attr>> {
-        self.do_remove_attribute(|attr| attr.name() == name)
+        self.remove_first_matching_attribute(|attr| attr.name() == name)
     }
 
-    pub fn do_remove_attribute<F>(&self, find: F) -> Option<Root<Attr>>
+    fn remove_first_matching_attribute<F>(&self, find: F) -> Option<Root<Attr>>
         where F: Fn(&Attr) -> bool
     {
         let idx = self.attrs.borrow().iter().map(JS::root).position(|attr| find(&attr));
 
         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());
-            }
-
             self.attrs.borrow_mut().remove(idx);
-            attr.r().set_owner(None);
-            if attr.r().namespace() == &ns!("") {
-                vtable_for(&NodeCast::from_ref(self)).after_remove_attr(attr.r().name());
-            }
-
+            attr.set_owner(None);
             let node = NodeCast::from_ref(self);
-            if node.is_in_doc() {
-                let document = document_from_node(self);
-                let damage = if attr.r().local_name() == &atom!("style") {
-                    NodeDamage::NodeStyleDamaged
-                } else {
-                    NodeDamage::OtherNodeDamage
-                };
-                document.r().content_changed(node, damage);
+            if attr.namespace() == &ns!("") {
+                vtable_for(node).attribute_mutated(&attr, AttributeMutation::Removed);
             }
             attr
         })
@@ -1168,9 +1164,9 @@ impl ElementMethods for Element {
 
         // Step 3-5.
         let value = self.parse_attribute(&ns!(""), &name, value);
-        self.do_set_attribute(name.clone(), value, name.clone(), ns!(""), None, |attr| {
-            *attr.name() == name
-        });
+        self.set_first_matching_attribute(
+            name.clone(), value, name.clone(), ns!(""), None,
+            |attr| *attr.name() == name);
         Ok(())
     }
 
@@ -1183,11 +1179,9 @@ impl ElementMethods for Element {
             try!(validate_and_extract(namespace, &qualified_name));
         let qualified_name = Atom::from_slice(&qualified_name);
         let value = self.parse_attribute(&namespace, &local_name, value);
-        self.do_set_attribute(local_name.clone(), value, qualified_name,
-                              namespace.clone(), prefix, |attr| {
-            *attr.local_name() == local_name &&
-            *attr.namespace() == namespace
-        });
+        self.set_first_matching_attribute(
+            local_name.clone(), value, qualified_name, namespace.clone(), prefix,
+            |attr| *attr.local_name() == local_name && *attr.namespace() == namespace);
         Ok(())
     }
 
@@ -1454,96 +1448,52 @@ impl VirtualMethods for Element {
         Some(node as &VirtualMethods)
     }
 
-    fn after_set_attr(&self, attr: &Attr) {
-        if let Some(ref s) = self.super_type() {
-            s.after_set_attr(attr);
-        }
-
+    fn attribute_mutated(&self, attr: &Attr, mutation: AttributeMutation) {
+        self.super_type().unwrap().attribute_mutated(attr, mutation);
         let node = NodeCast::from_ref(self);
-        match attr.local_name() {
-            &atom!("style") => {
+        let doc = node.owner_doc();
+        let damage = match attr.local_name() {
+            &atom!(style) => {
                 // Modifying the `style` attribute might change style.
-                let doc = document_from_node(self);
-                let base_url = doc.r().base_url();
-                let value = attr.value();
-                let style = Some(parse_style_attribute(&value, &base_url));
-                *self.style_attribute.borrow_mut() = style;
-
-                if node.is_in_doc() {
-                    doc.r().content_changed(node, NodeDamage::NodeStyleDamaged);
-                }
-            }
-            &atom!("class") => {
+                *self.style_attribute.borrow_mut() =
+                    mutation.new_value(attr).map(|value| {
+                        parse_style_attribute(&value, &doc.base_url())
+                    });
+                NodeDamage::NodeStyleDamaged
+            },
+            &atom!(class) => {
                 // Modifying a class can change style.
+                NodeDamage::NodeStyleDamaged
+            },
+            &atom!(id) => {
                 if node.is_in_doc() {
-                    let document = document_from_node(self);
-                    document.r().content_changed(node, NodeDamage::NodeStyleDamaged);
-                }
-            }
-            &atom!("id") => {
-                // Modifying an ID might change style.
-                let value = attr.value();
-                if node.is_in_doc() {
-                    let doc = document_from_node(self);
-                    if !value.is_empty() {
-                        let value = value.atom().unwrap().clone();
-                        doc.r().register_named_element(self, value);
+                    let value = attr.value().atom().unwrap().clone();
+                    match mutation {
+                        AttributeMutation::Set(old_value) => {
+                            if let Some(old_value) = old_value {
+                                let old_value = old_value.atom().unwrap().clone();
+                                doc.unregister_named_element(self, old_value);
+                            }
+                            if value != atom!("") {
+                                doc.register_named_element(self, value);
+                            }
+                        },
+                        AttributeMutation::Removed => {
+                            if value != atom!("") {
+                                doc.unregister_named_element(self, value);
+                            }
+                        }
                     }
-                    doc.r().content_changed(node, NodeDamage::NodeStyleDamaged);
                 }
-            }
+                NodeDamage::NodeStyleDamaged
+            },
             _ => {
                 // Modifying any other attribute might change arbitrary things.
-                if node.is_in_doc() {
-                    let document = document_from_node(self);
-                    document.r().content_changed(node, NodeDamage::OtherNodeDamage);
-                }
-            }
-        }
-    }
-
-    fn before_remove_attr(&self, attr: &Attr) {
-        if let Some(ref s) = self.super_type() {
-            s.before_remove_attr(attr);
-        }
-
-        let node = NodeCast::from_ref(self);
-        match attr.local_name() {
-            &atom!("style") => {
-                // Modifying the `style` attribute might change style.
-                *self.style_attribute.borrow_mut() = None;
-
-                if node.is_in_doc() {
-                    let doc = document_from_node(self);
-                    doc.r().content_changed(node, NodeDamage::NodeStyleDamaged);
-                }
-            }
-            &atom!("id") => {
-                // Modifying an ID can change style.
-                let value = attr.value();
-                if node.is_in_doc() {
-                    let doc = document_from_node(self);
-                    if !value.is_empty() {
-                        let value = value.atom().unwrap().clone();
-                        doc.r().unregister_named_element(self, value);
-                    }
-                    doc.r().content_changed(node, NodeDamage::NodeStyleDamaged);
-                }
-            }
-            &atom!("class") => {
-                // Modifying a class can change style.
-                if node.is_in_doc() {
-                    let document = document_from_node(self);
-                    document.r().content_changed(node, NodeDamage::NodeStyleDamaged);
-                }
-            }
-            _ => {
-                // Modifying any other attribute might change arbitrary things.
-                if node.is_in_doc() {
-                    let doc = document_from_node(self);
-                    doc.r().content_changed(node, NodeDamage::OtherNodeDamage);
-                }
-            }
+                NodeDamage::OtherNodeDamage
+            },
+        };
+        if node.is_in_doc() {
+            doc.content_changed(node, damage);
         }
     }
 
@@ -1853,3 +1803,23 @@ impl Element {
         self.set_click_in_progress(false);
     }
 }
+
+#[derive(Clone, Copy, PartialEq)]
+pub enum AttributeMutation<'a> {
+    /// The attribute is set, keep track of old value.
+    /// https://dom.spec.whatwg.org/#attribute-is-set
+    Set(Option<&'a AttrValue>),
+
+    /// The attribute is removed.
+    /// https://dom.spec.whatwg.org/#attribute-is-removed
+    Removed
+}
+
+impl<'a> AttributeMutation<'a> {
+    pub fn new_value<'b>(&self, attr: &'b Attr) -> Option<Ref<'b, AttrValue>> {
+        match *self {
+            AttributeMutation::Set(_) => Some(attr.value()),
+            AttributeMutation::Removed => None,
+        }
+    }
+}