stylo: Create separate Attr type

components/style/properties/gecko.mako.rs

@@ -4401,12 +4401,12 @@ clip-path
                                     as_utf16_and_forget(&value);
                             }
                         }
-                        ContentItem::Attr(ns, val) => {
+                        ContentItem::Attr(attr) => {
                             self.gecko.mContents[i].mType = eStyleContentType_Attr;
-                            let s = if let Some((_, ns)) = ns {
+                            let s = if let Some((_, ns)) = attr.namespace {
-                                format!("{}|{}", ns, val)
+                                format!("{}|{}", ns, attr.attribute)
                             } else {
-                                val.into()
+                                attr.attribute.into()
                             };
                             unsafe {
                                 // NB: we share allocators, so doing this is fine.

components/style/properties/longhand/counters.mako.rs

@@ -15,7 +15,7 @@
     #[cfg(feature = "gecko")]
     use values::specified::url::SpecifiedUrl;
     #[cfg(feature = "gecko")]
-    use gecko_string_cache::namespace::Namespace;
+    use values::specified::Attr;
 
     #[cfg(feature = "servo")]
     use super::list_style_type;
@@ -37,8 +37,9 @@
         type CounterStyleType = super::super::list_style_type::computed_value::T;
         #[cfg(feature = "gecko")]
         type CounterStyleType = ::values::generics::CounterStyleOrNone;
+
         #[cfg(feature = "gecko")]
-        use gecko_string_cache::namespace::Namespace;
+        use values::specified::Attr;
 
         #[derive(Debug, PartialEq, Eq, Clone)]
         #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
@@ -60,7 +61,7 @@
 
             % if product == "gecko":
                 /// `attr([namespace? `|`]? ident)`
-                Attr(Option<(Namespace, u32)>, String),
+                Attr(Attr),
                 /// `url(url)`
                 Url(SpecifiedUrl),
             % endif
@@ -94,14 +95,8 @@
                     ContentItem::NoCloseQuote => dest.write_str("no-close-quote"),
 
                     % if product == "gecko":
-                        ContentItem::Attr(ref ns, ref attr) => {
+                        ContentItem::Attr(ref attr) => {
-                            dest.write_str("attr(")?;
+                            attr.to_css(dest)
-                            if let Some(ref ns) = *ns {
-                                cssparser::Token::Ident(ns.0.to_string().into()).to_css(dest)?;
-                                dest.write_str("|")?;
-                            }
-                            cssparser::Token::Ident((&**attr).into()).to_css(dest)?;
-                            dest.write_str(")")
                         }
                         ContentItem::Url(ref url) => url.to_css(dest),
                     % endif
@@ -207,46 +202,7 @@
                         }),
                         % if product == "gecko":
                             "attr" => input.parse_nested_block(|input| {
-                                // Syntax is `[namespace? `|`]? ident`
+                                Ok(ContentItem::Attr(Attr::parse_function(context, input)?))
-                                // no spaces allowed
-                                // FIXME (bug 1346693) we should be checking that
-                                // this is a valid namespace and encoding it as a namespace
-                                // number from the map
-                                let first = input.try(|i| i.expect_ident()).ok();
-                                if let Ok(token) = input.try(|i| i.next_including_whitespace()) {
-                                    match token {
-                                        Token::Delim('|') => {
-                                            // must be followed by an ident
-                                            let tok2 = input.next_including_whitespace()?;
-                                            if let Token::Ident(second) = tok2 {
-                                                let first: Option<Namespace> = first.map(|i| i.into());
-                                                let first_with_id = match (first, context.namespaces) {
-                                                    (Some(prefix), Some(map)) => {
-                                                        let map = map.read();
-                                                        if let Some(ref entry) = map.prefixes.get(&prefix.0) {
-                                                            Some((prefix, entry.1))
-                                                        } else {
-                                                            return Err(())
-                                                        }
-                                                    }
-                                                    // if we don't have a namespace map (e.g. in CSSOM)
-                                                    // we can't parse namespaces
-                                                    (Some(_), None) => return Err(()),
-                                                    _ => None
-                                                };
-                                                return Ok(ContentItem::Attr(first_with_id, second.into_owned()))
-                                            } else {
-                                                return Err(())
-                                            }
-                                        }
-                                        _ => return Err(())
-                                    }
-                                }
-                                if let Some(first) = first {
-                                    Ok(ContentItem::Attr(None, first.into_owned()))
-                                } else {
-                                    Err(())
-                                }
                             }),
                         % endif
                         _ => return Err(())

components/style/stylesheets.rs

@@ -51,6 +51,7 @@ use stylearc::Arc;
 use stylist::FnvHashMap;
 use supports::SupportsCondition;
 use values::{CustomIdent, KeyframesName};
+use values::specified::NamespaceId;
 use values::specified::url::SpecifiedUrl;
 use viewport::ViewportRule;
 
@@ -97,12 +98,12 @@ pub enum Origin {
 
 /// A set of namespaces applying to a given stylesheet.
 ///
-/// The u32 is the namespace id, used in gecko
+/// The namespace id is used in gecko
 #[derive(Clone, Default, Debug)]
 #[allow(missing_docs)]
 pub struct Namespaces {
-    pub default: Option<(Namespace, u32)>,
+    pub default: Option<(Namespace, NamespaceId)>,
-    pub prefixes: FnvHashMap<Prefix, (Namespace, u32)>,
+    pub prefixes: FnvHashMap<Prefix, (Namespace, NamespaceId)>,
 }
 
 /// Like gecko_bindings::structs::MallocSizeOf, but without the Option<> wrapper. Note that
@@ -1240,8 +1241,9 @@ impl Stylesheet {
         let mut rules = Vec::new();
         let mut input = Parser::new(css);
         let mut context = ParserContext::new_with_line_number_offset(origin, url_data, error_reporter,
-                                                                line_number_offset, PARSING_MODE_DEFAULT,
+                                                                     line_number_offset,
-                                                                quirks_mode);
+                                                                     PARSING_MODE_DEFAULT,
+                                                                     quirks_mode);
         context.namespaces = Some(namespaces);
         let rule_parser = TopLevelRuleParser {
             stylesheet_origin: origin,
@@ -1529,18 +1531,18 @@ enum AtRulePrelude {
 
 
 #[cfg(feature = "gecko")]
-fn register_namespace(ns: &Namespace) -> Result<u32, ()> {
+fn register_namespace(ns: &Namespace) -> Result<i32, ()> {
     let id = unsafe { ::gecko_bindings::bindings::Gecko_RegisterNamespace(ns.0.as_ptr()) };
     if id == -1 {
         Err(())
     } else {
-        Ok(id as u32)
+        Ok(id)
     }
 }
 
 #[cfg(feature = "servo")]
-fn register_namespace(ns: &Namespace) -> Result<u32, ()> {
+fn register_namespace(_: &Namespace) -> Result<(), ()> {
-    Ok(1) // servo doesn't use namespace ids
+    Ok(()) // servo doesn't use namespace ids
 }
 
 impl<'a> AtRuleParser for TopLevelRuleParser<'a> {

components/style/values/specified/mod.rs

@@ -6,9 +6,10 @@
 //!
 //! TODO(emilio): Enhance docs.
 
+use Namespace;
 use app_units::Au;
 use context::QuirksMode;
-use cssparser::{self, Parser, Token};
+use cssparser::{self, Parser, Token, serialize_identifier};
 use itoa;
 use parser::{ParserContext, Parse};
 use self::grid::TrackSizeOrRepeat;
@@ -24,6 +25,7 @@ use super::computed::{self, Context};
 use super::computed::{Shadow as ComputedShadow, ToComputedValue};
 use super::generics::grid::{TrackBreadth as GenericTrackBreadth, TrackSize as GenericTrackSize};
 use super::generics::grid::TrackList as GenericTrackList;
+use values::computed::ComputedValueAsSpecified;
 use values::specified::calc::CalcNode;
 
 #[cfg(feature = "gecko")]
@@ -1363,3 +1365,108 @@ impl AllowQuirks {
         self == AllowQuirks::Yes && quirks_mode == QuirksMode::Quirks
     }
 }
+
+#[cfg(feature = "gecko")]
+/// A namespace ID
+pub type NamespaceId = i32;
+
+
+#[cfg(feature = "servo")]
+/// A namespace ID (used by gecko only)
+pub type NamespaceId = ();
+
+/// An attr(...) rule
+///
+/// `[namespace? `|`]? ident`
+#[derive(Clone, PartialEq, Eq, Debug)]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+pub struct Attr {
+    /// Optional namespace
+    pub namespace: Option<(Namespace, NamespaceId)>,
+    /// Attribute name
+    pub attribute: String,
+}
+
+impl Parse for Attr {
+    fn parse(context: &ParserContext, input: &mut Parser) -> Result<Attr, ()> {
+        input.expect_function_matching("attr")?;
+        input.parse_nested_block(|i| Attr::parse_function(context, i))
+    }
+}
+
+#[cfg(feature = "gecko")]
+/// Get the namespace id from the namespace map
+pub fn get_id_for_namespace(namespace: &Namespace, context: &ParserContext) -> Result<NamespaceId, ()> {
+    if let Some(map) = context.namespaces {
+        if let Some(ref entry) = map.read().prefixes.get(&namespace.0) {
+            Ok(entry.1)
+        } else {
+            Err(())
+        }
+    } else {
+        // if we don't have a namespace map (e.g. in inline styles)
+        // we can't parse namespaces
+        Err(())
+    }
+}
+
+#[cfg(feature = "servo")]
+/// Get the namespace id from the namespace map
+pub fn get_id_for_namespace(_: &Namespace, _: &ParserContext) -> Result<NamespaceId, ()> {
+    Ok(())
+}
+
+impl Attr {
+    /// Parse contents of attr() assuming we have already parsed `attr` and are
+    /// within a parse_nested_block()
+    pub fn parse_function(context: &ParserContext, input: &mut Parser) -> Result<Attr, ()> {
+        // Syntax is `[namespace? `|`]? ident`
+        // no spaces allowed
+        let first = input.try(|i| i.expect_ident()).ok();
+        if let Ok(token) = input.try(|i| i.next_including_whitespace()) {
+            match token {
+                Token::Delim('|') => {
+                    // must be followed by an ident
+                    let second_token = match input.next_including_whitespace()? {
+                        Token::Ident(second) => second,
+                        _ => return Err(()),
+                    };
+                    let ns_with_id = if let Some(ns) = first {
+                        let ns: Namespace = ns.into();
+                        let id = get_id_for_namespace(&ns, context)?;
+                        Some((ns, id))
+                    } else {
+                        None
+                    };
+                    return Ok(Attr {
+                        namespace: ns_with_id,
+                        attribute: second_token.into_owned(),
+                    })
+                }
+                _ => return Err(())
+            }
+        }
+        if let Some(first) = first {
+            Ok(Attr {
+                namespace: None,
+                attribute: first.into_owned(),
+            })
+        } else {
+            Err(())
+        }
+    }
+}
+
+impl ToCss for Attr {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        dest.write_str("attr(")?;
+        if let Some(ref ns) = self.namespace {
+            serialize_identifier(&ns.0.to_string(), dest)?;
+            dest.write_str("|")?;
+        }
+        serialize_identifier(&self.attribute, dest)?;
+        dest.write_str(")")
+    }
+}
+
+impl ComputedValueAsSpecified for Attr {}

tests/unit/style/parsing/selectors.rs

@@ -9,7 +9,7 @@ use style::stylesheets::{Origin, Namespaces};
 
 fn parse_selector(input: &mut Parser) -> Result<SelectorList<SelectorImpl>, ()> {
     let mut ns = Namespaces::default();
-    ns.prefixes.insert("svg".into(), ns!(svg));
+    ns.prefixes.insert("svg".into(), (ns!(svg), ()));
     let parser = SelectorParser {
         stylesheet_origin: Origin::UserAgent,
         namespaces: &ns,

tests/unit/style/stylesheets.rs

@@ -70,7 +70,7 @@ fn test_parse_stylesheet() {
     let stylesheet = Stylesheet::from_str(css, url.clone(), Origin::UserAgent, media, lock,
                                           None, &CSSErrorReporterTest, QuirksMode::NoQuirks, 0u64);
     let mut namespaces = Namespaces::default();
-    namespaces.default = Some(ns!(html));
+    namespaces.default = Some((ns!(html), ()));
     let expected = Stylesheet {
         origin: Origin::UserAgent,
         media: Arc::new(stylesheet.shared_lock.wrap(MediaList::empty())),