Restore perf improvement from #1554

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

5 files changed, 34 insertions, 37 deletions

diff --git a/src/components/main/layout/wrapper.rs b/src/components/main/layout/wrapper.rs
index eddc50e5894..34bd613e957 100644
--- a/src/components/main/layout/wrapper.rs
+++ b/src/components/main/layout/wrapper.rs
@@ -14,7 +14,6 @@
 //! (2) Layout is not allowed to see anything with `Abstract` in the name, because it could hang
 //!     onto these objects and cause use-after-free.
 
-use std::ascii::StrAsciiExt;
 use extra::url::Url;
 use script::dom::element::{Element, HTMLAreaElementTypeId, HTMLAnchorElementTypeId};
 use script::dom::element::{HTMLLinkElementTypeId};
@@ -26,7 +25,7 @@ use script::dom::node::{AbstractNode, DocumentNodeTypeId, ElementNodeTypeId, Nod
 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`.
@@ -282,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> {
@@ -379,33 +393,22 @@ 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) }
     }
 
-    fn match_attr(&self, ns_url: Option<~str>, name: &str, test: &|&str| -> bool) -> bool {
-        let temp;
-        let name = if self.element.html_element_in_html_document() {
-            temp = name.to_ascii_lower();
-            temp.as_slice()
-        } else {
-            name
-        };
-        match self.get_attr(ns_url, name) {
-            Some(value) => (*test)(value),
-            None => false,
-        }
-    }
-
     fn get_link(&self) -> Option<~str> {
         // FIXME: This is HTML only.
         match self.element.node.type_id {
diff --git a/src/components/style/node.rs b/src/components/style/node.rs
index c11f1180b56..455e1d1e93d 100644
--- a/src/components/style/node.rs
+++ b/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,11 +18,12 @@ 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 {
     fn get_attr(&self, namespace: Option<~str>, attr: &str) -> Option<&'static str>;
-    fn match_attr(&self, ns_url: Option<~str>, name: &str, test: &|&str| -> bool) -> bool;
     fn get_link(&self) -> Option<~str>;
     fn get_local_name<'a>(&'a self) -> &'a str;
     fn get_namespace_url<'a>(&'a self) -> &'a str;
diff --git a/src/components/style/selector_matching.rs b/src/components/style/selector_matching.rs
index fd37b684c56..a3984f8603a 100644
--- a/src/components/style/selector_matching.rs
+++ b/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,18 +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,
-                   test: |&str| -> bool)
-                   -> bool {
-    element.with_element(|element: &E| {
-        // FIXME: avoid .clone() here? See #1367
-        element.match_attr(attr.namespace.clone(), attr.name, &test)
-    })
-}
 
 #[cfg(test)]
 mod tests {
diff --git a/src/components/style/selectors.rs b/src/components/style/selectors.rs
index 860c4a6d6bc..24b2d0e4b20 100644
--- a/src/components/style/selectors.rs
+++ b/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,
         },
     };
diff --git a/src/components/style/style.rc b/src/components/style/style.rc
index a8e39526fa1..c5ae2428064 100644
--- a/src/components/style/style.rc
+++ b/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;