auto merge of #1560 : SimonSapin/servo/restore-1554, r=pcwalton

Lower-case attribute names when parsing selectors rather than when matching. This avoid one allocation in matching code.

Also do not lowercase names for *AttributeNS APIs. (Move lower-casing to callers.)

src/components/main/layout/wrapper.rs

@@ -19,12 +19,13 @@ use script::dom::element::{Element, HTMLAreaElementTypeId, HTMLAnchorElementType
 use script::dom::element::{HTMLLinkElementTypeId};
 use script::dom::htmliframeelement::HTMLIFrameElement;
 use script::dom::htmlimageelement::HTMLImageElement;
+use script::dom::namespace;
 use script::dom::namespace::Namespace;
 use script::dom::node::{AbstractNode, DocumentNodeTypeId, ElementNodeTypeId, Node, NodeTypeId};
 use script::dom::text::Text;
 use servo_msg::constellation_msg::{PipelineId, SubpageId};
 use std::cast;
-use style::{PropertyDeclarationBlock, TElement, TNode};
+use style::{PropertyDeclarationBlock, TElement, TNode, AttrSelector};
 
 /// A wrapper so that layout can access only the methods that it should have access to. Layout must
 /// only ever see these and must never see instances of `AbstractNode`.
@@ -246,7 +247,7 @@ impl<'ln> TNode<LayoutElement<'ln>> for LayoutNode<'ln> {
             self.node.node().prev_sibling.map(|node| self.new_with_this_lifetime(node))
         }
     }
-    
+
     fn next_sibling(&self) -> Option<LayoutNode<'ln>> {
         unsafe {
             self.node.node().next_sibling.map(|node| self.new_with_this_lifetime(node))
@@ -280,6 +281,21 @@ impl<'ln> TNode<LayoutElement<'ln>> for LayoutNode<'ln> {
             }
         })
     }
+
+    fn match_attr(&self, attr: &AttrSelector, test: |&str| -> bool) -> bool {
+        self.with_element(|element| {
+            let name = if element.element.html_element_in_html_document() {
+                attr.lower_name.as_slice()
+            } else {
+                attr.name.as_slice()
+            };
+            // FIXME: avoid .clone() here? See #1367
+            match element.get_attr(attr.namespace.clone(), name) {
+                Some(value) => test(value),
+                None => false,
+            }
+        })
+    }
 }
 
 pub struct LayoutNodeChildrenIterator<'a> {
@@ -377,14 +393,17 @@ impl<'le> LayoutElement<'le> {
 }
 
 impl<'le> TElement for LayoutElement<'le> {
+    #[inline]
     fn get_local_name<'a>(&'a self) -> &'a str {
         self.element.tag_name.as_slice()
     }
 
+    #[inline]
     fn get_namespace_url<'a>(&'a self) -> &'a str {
         self.element.namespace.to_str().unwrap_or("")
     }
 
+    #[inline]
     fn get_attr(&self, ns_url: Option<~str>, name: &str) -> Option<&'static str> {
         let namespace = Namespace::from_str(ns_url);
         unsafe { self.element.get_attr_val_for_layout(namespace, name) }
@@ -398,7 +417,8 @@ impl<'le> TElement for LayoutElement<'le> {
             ElementNodeTypeId(HTMLAnchorElementTypeId) |
             ElementNodeTypeId(HTMLAreaElementTypeId) |
             ElementNodeTypeId(HTMLLinkElementTypeId) => {
-                self.get_attr(None, "href").map(|val| val.to_owned())
+                unsafe { self.element.get_attr_val_for_layout(namespace::Null, "href") }
+                .map(|val| val.to_owned())
             }
             _ => None,
         }

src/components/script/dom/element.rs

@@ -7,7 +7,7 @@
 use dom::attr::Attr;
 use dom::attrlist::AttrList;
 use dom::bindings::utils::{Reflectable, DOMString, ErrorResult, Fallible, Reflector};
-use dom::bindings::utils::{null_str_as_empty, NamespaceError};
+use dom::bindings::utils::NamespaceError;
 use dom::bindings::utils::{InvalidCharacter, QName, Name, InvalidXMLName, xml_name_type};
 use dom::htmlcollection::HTMLCollection;
 use dom::clientrect::ClientRect;
@@ -137,30 +137,23 @@ impl Element {
         }
     }
 
-    pub fn normalize_attr_name(&self, name: Option<DOMString>) -> ~str {
-        //FIXME: Throw for XML-invalid names
+    pub fn html_element_in_html_document(&self) -> bool {
         let owner = self.node.owner_doc();
-        if owner.document().doctype == document::HTML { // && self.namespace == Namespace::HTML
-            null_str_as_empty(&name).to_ascii_lower()
-        } else {
-            null_str_as_empty(&name)
-        }
+        self.namespace == namespace::HTML &&
+        // FIXME: check that this matches what the spec calls "is in an HTML document"
+        owner.document().doctype == document::HTML
     }
 
     pub fn get_attribute(&self,
                          namespace: Namespace,
                          name: &str) -> Option<@mut Attr> {
-        // FIXME: only case-insensitive in the HTML namespace (as opposed to SVG, etc.)
-        let name = name.to_ascii_lower();
         self.attrs.iter().find(|attr| {
             name == attr.local_name && attr.namespace == namespace
         }).map(|&x| x)
     }
 
-    pub unsafe fn get_attr_val_for_layout(&self, namespace: Namespace, name: &str) 
+    pub unsafe fn get_attr_val_for_layout(&self, namespace: Namespace, name: &str)
                                           -> Option<&'static str> {
-        // FIXME: only case-insensitive in the HTML namespace (as opposed to SVG, etc.)
-        let name = name.to_ascii_lower();
         self.attrs.iter().find(|attr: & &@mut Attr| {
             // unsafely avoid a borrow because this is accessed by many tasks
             // during parallel layout
@@ -174,8 +167,6 @@ impl Element {
 
     pub fn set_attr(&mut self, abstract_self: AbstractNode, name: DOMString, value: DOMString)
                     -> ErrorResult {
-        // FIXME: HTML-in-HTML only.
-        let name = name.to_ascii_lower();
         self.set_attribute(abstract_self, namespace::Null, name, value)
     }
 
@@ -402,6 +393,11 @@ impl Element {
     }
 
     pub fn GetAttribute(&self, name: DOMString) -> Option<DOMString> {
+        let name = if self.html_element_in_html_document() {
+            name.to_ascii_lower()
+        } else {
+            name
+        };
         self.get_attribute(Null, name).map(|s| s.Value())
     }
 
@@ -413,6 +409,12 @@ impl Element {
 
     pub fn SetAttribute(&mut self, abstract_self: AbstractNode, name: DOMString, value: DOMString)
                         -> ErrorResult {
+        // FIXME: If name does not match the Name production in XML, throw an "InvalidCharacterError" exception.
+        let name = if self.html_element_in_html_document() {
+            name.to_ascii_lower()
+        } else {
+            name
+        };
         self.set_attr(abstract_self, name, value)
     }
 
@@ -435,6 +437,11 @@ impl Element {
     pub fn RemoveAttribute(&mut self,
                            abstract_self: AbstractNode,
                            name: DOMString) -> ErrorResult {
+        let name = if self.html_element_in_html_document() {
+            name.to_ascii_lower()
+        } else {
+            name
+        };
         self.remove_attribute(abstract_self, namespace::Null, name)
     }
 

src/components/style/node.rs

@@ -5,6 +5,9 @@
 //! Traits that nodes must implement. Breaks the otherwise-cyclic dependency between layout and
 //! style.
 
+use selectors::AttrSelector;
+
+
 pub trait TNode<E:TElement> : Clone {
     fn parent_node(&self) -> Option<Self>;
     fn prev_sibling(&self) -> Option<Self>;
@@ -15,6 +18,8 @@ pub trait TNode<E:TElement> : Clone {
 
     /// FIXME(pcwalton): This should not use the `with` pattern.
     fn with_element<'a, R>(&self, f: |&E| -> R) -> R;
+
+    fn match_attr(&self, attr: &AttrSelector, test: |&str| -> bool) -> bool;
 }
 
 pub trait TElement {

src/components/style/selector_matching.rs

@@ -513,22 +513,22 @@ fn matches_simple_selector<E:TElement,N:TNode<E>>(selector: &SimpleSelector, ele
             })
         }
 
-        AttrExists(ref attr) => match_attribute(attr, element, |_| true),
-        AttrEqual(ref attr, ref value) => match_attribute(attr, element, |v| v == value.as_slice()),
-        AttrIncludes(ref attr, ref value) => match_attribute(attr, element, |attr_value| {
+        AttrExists(ref attr) => element.match_attr(attr, |_| true),
+        AttrEqual(ref attr, ref value) => element.match_attr(attr, |v| v == value.as_slice()),
+        AttrIncludes(ref attr, ref value) => element.match_attr(attr, |attr_value| {
             attr_value.split(SELECTOR_WHITESPACE).any(|v| v == value.as_slice())
         }),
         AttrDashMatch(ref attr, ref value, ref dashing_value)
-        => match_attribute(attr, element, |attr_value| {
+        => element.match_attr(attr, |attr_value| {
             attr_value == value.as_slice() || attr_value.starts_with(dashing_value.as_slice())
         }),
-        AttrPrefixMatch(ref attr, ref value) => match_attribute(attr, element, |attr_value| {
+        AttrPrefixMatch(ref attr, ref value) => element.match_attr(attr, |attr_value| {
             attr_value.starts_with(value.as_slice())
         }),
-        AttrSubstringMatch(ref attr, ref value) => match_attribute(attr, element, |attr_value| {
+        AttrSubstringMatch(ref attr, ref value) => element.match_attr(attr, |attr_value| {
             attr_value.contains(value.as_slice())
         }),
-        AttrSuffixMatch(ref attr, ref value) => match_attribute(attr, element, |attr_value| {
+        AttrSuffixMatch(ref attr, ref value) => element.match_attr(attr, |attr_value| {
             attr_value.ends_with(value.as_slice())
         }),
 
@@ -696,21 +696,6 @@ fn matches_last_child<E:TElement,N:TNode<E>>(element: &N) -> bool {
     }
 }
 
-#[inline]
-fn match_attribute<E:TElement,
-                   N:TNode<E>>(
-                   attr: &AttrSelector,
-                   element: &N,
-                   f: |&str| -> bool)
-                   -> bool {
-    element.with_element(|element: &E| {
-        // FIXME: avoid .clone() here? See #1367
-        match element.get_attr(attr.namespace.clone(), attr.name) {
-            None => false,
-            Some(value) => f(value)
-        }
-    })
-}
 
 #[cfg(test)]
 mod tests {

src/components/style/selectors.rs

@@ -88,6 +88,7 @@ pub enum SimpleSelector {
 #[deriving(Eq, Clone)]
 pub struct AttrSelector {
     name: ~str,
+    lower_name: ~str,
     namespace: Option<~str>,
 }
 
@@ -423,6 +424,7 @@ fn parse_attribute_selector(content: ~[ComponentValue], namespaces: &NamespaceMa
         QualifiedName(_, None) => fail!("Implementation error, this should not happen."),
         QualifiedName(namespace, Some(local_name)) => AttrSelector {
             namespace: namespace,
+            lower_name: local_name.to_ascii_lower(),
             name: local_name,
         },
     };

src/components/style/style.rc

@@ -23,7 +23,7 @@ pub use properties::{cascade, PropertyDeclaration, ComputedValues, computed_valu
 pub use properties::{PropertyDeclarationBlock, parse_style_attribute};  // Style attributes
 pub use errors::with_errors_silenced;
 pub use node::{TElement, TNode};
-pub use selectors::{PseudoElement, Before, After};
+pub use selectors::{PseudoElement, Before, After, AttrSelector};
 
 mod stylesheets;
 mod errors;