Keep custom-ident and string separate in animation/keyframes name.

components/script/dom/csskeyframesrule.rs

@@ -5,7 +5,7 @@
 use cssparser::Parser;
 use dom::bindings::codegen::Bindings::CSSKeyframesRuleBinding;
 use dom::bindings::codegen::Bindings::CSSKeyframesRuleBinding::CSSKeyframesRuleMethods;
-use dom::bindings::error::{Error, ErrorResult};
+use dom::bindings::error::ErrorResult;
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{MutNullableJS, Root};
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
@@ -20,7 +20,7 @@ use std::sync::Arc;
 use style::keyframes::{Keyframe, KeyframeSelector};
 use style::shared_lock::{Locked, ToCssWithGuard};
 use style::stylesheets::KeyframesRule;
-use style::values::CustomIdent;
+use style::values::KeyframesName;
 
 #[dom_struct]
 pub struct CSSKeyframesRule {
@@ -107,15 +107,15 @@ impl CSSKeyframesRuleMethods for CSSKeyframesRule {
     // https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name
     fn Name(&self) -> DOMString {
         let guard = self.cssrule.shared_lock().read();
-        DOMString::from(&*self.keyframesrule.read_with(&guard).name.0)
+        DOMString::from(&**self.keyframesrule.read_with(&guard).name.as_atom())
     }
 
     // https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name
     fn SetName(&self, value: DOMString) -> ErrorResult {
-        // https://github.com/w3c/csswg-drafts/issues/801
+        // Spec deviation: https://github.com/w3c/csswg-drafts/issues/801
-        // Setting this property to a CSS-wide keyword or `none` will
+        // Setting this property to a CSS-wide keyword or `none` does not throw,
-        // throw a Syntax Error.
+        // it stores a value that serializes as a quoted string.
-        let name = CustomIdent::from_ident(value.into(), &["none"]).map_err(|()| Error::Syntax)?;
+        let name = KeyframesName::from_ident(value.into());
         let mut guard = self.cssrule.shared_lock().write();
         self.keyframesrule.write_with(&mut guard).name = name;
         Ok(())

components/style/animation.rs

@@ -464,14 +464,20 @@ pub fn maybe_start_animations(context: &SharedStyleContext,
 
     let box_style = new_style.get_box();
     for (i, name) in box_style.animation_name_iter().enumerate() {
+        let name = if let Some(atom) = name.as_atom() {
+            atom
+        } else {
+            continue
+        };
+
         debug!("maybe_start_animations: name={}", name);
         let total_duration = box_style.animation_duration_mod(i).seconds();
         if total_duration == 0. {
             continue
         }
 
-        if let Some(ref anim) = context.stylist.animations().get(&name.0 .0) {
+        if let Some(ref anim) = context.stylist.animations().get(name) {
-            debug!("maybe_start_animations: animation {} found", name.0 .0);
+            debug!("maybe_start_animations: animation {} found", name);
 
             // If this animation doesn't have any keyframe, we can just continue
             // without submitting it to the compositor, since both the first and
@@ -506,7 +512,7 @@ pub fn maybe_start_animations(context: &SharedStyleContext,
 
 
             new_animations_sender
-                .send(Animation::Keyframes(node, name.0 .0.clone(), KeyframesAnimationState {
+                .send(Animation::Keyframes(node, name.clone(), KeyframesAnimationState {
                     started_at: animation_start,
                     duration: duration as f64,
                     delay: delay as f64,
@@ -584,9 +590,10 @@ pub fn update_style_for_animation(context: &SharedStyleContext,
 
             debug_assert!(!animation.steps.is_empty());
 
-            let maybe_index = style.get_box()
+            let maybe_index = style
+                .get_box()
                 .animation_name_iter()
-                                   .position(|animation_name| *name == animation_name.0 .0);
+                .position(|animation_name| Some(name) == animation_name.as_atom());
 
             let index = match maybe_index {
                 Some(index) => index,

components/style/gecko_string_cache/mod.rs

@@ -163,6 +163,12 @@ impl WeakAtom {
         }
     }
 
+    /// Returns whether this atom is the empty string.
+    #[inline]
+    pub fn is_empty(&self) -> bool {
+        self.len() == 0
+    }
+
     /// Returns the atom as a mutable pointer.
     #[inline]
     pub fn as_ptr(&self) -> *mut nsIAtom {

components/style/matching.rs

@@ -560,7 +560,7 @@ trait PrivateMatchMethods: TElement {
                                pseudo: Option<&PseudoElement>) -> bool {
         let ref new_box_style = new_values.get_box();
         let has_new_animation_style = new_box_style.animation_name_count() >= 1 &&
-                                      new_box_style.animation_name_at(0).0 .0.len() != 0;
+                                      new_box_style.animation_name_at(0).0.is_some();
         let has_animations = self.has_css_animations(pseudo);
 
         old_values.as_ref().map_or(has_new_animation_style, |ref old| {

components/style/properties/gecko.mako.rs

@@ -62,7 +62,7 @@ use std::ptr;
 use std::sync::Arc;
 use std::cmp;
 use values::computed::ToComputedValue;
-use values::{Either, Auto, CustomIdent};
+use values::{Either, Auto, KeyframesName};
 use computed_values::border_style;
 
 pub mod style_structs {
@@ -2202,16 +2202,22 @@ fn static_assert() {
         self.gecko.mAnimationNameCount = v.0.len() as u32;
         for (servo, gecko) in v.0.into_iter().zip(self.gecko.mAnimations.iter_mut()) {
             // TODO This is inefficient. We should fix this in bug 1329169.
-            gecko.mName.assign(servo.0 .0.as_slice());
+            gecko.mName.assign(match servo.0 {
+                Some(ref name) => name.as_atom().as_slice(),
+                None => &[],  // Empty string for 'none'
+            });
         }
     }
     pub fn animation_name_at(&self, index: usize)
         -> longhands::animation_name::computed_value::SingleComputedValue {
-        use Atom;
         use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
         // XXX: Is there any effective ways?
-        AnimationName(CustomIdent(Atom::from(
+        let atom = &self.gecko.mAnimations[index].mName;
-            String::from_utf16_lossy(&self.gecko.mAnimations[index].mName[..]))))
+        if atom.is_empty() {
+            AnimationName(None)
+        } else {
+            AnimationName(Some(KeyframesName::from_ident(atom.to_string())))
+        }
     }
     pub fn copy_animation_name_from(&mut self, other: &Self) {
         unsafe { self.gecko.mAnimations.ensure_len(other.gecko.mAnimations.len()) };

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

@@ -796,7 +796,7 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
     use std::ops::Deref;
     use style_traits::ToCss;
     use values::computed::ComputedValueAsSpecified;
-    use values::{HasViewportPercentage, CustomIdent};
+    use values::{HasViewportPercentage, KeyframesName};
 
     pub mod computed_value {
         pub use super::SpecifiedValue as T;
@@ -804,7 +804,14 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
     #[derive(Clone, Debug, Hash, Eq, PartialEq)]
     #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-    pub struct SpecifiedValue(pub CustomIdent);
+    pub struct SpecifiedValue(pub Option<KeyframesName>);
+
+    impl SpecifiedValue {
+        /// As an Atom
+        pub fn as_atom(&self) -> Option< &Atom> {
+            self.0.as_ref().map(|n| n.as_atom())
+        }
+    }
 
     #[inline]
     pub fn get_initial_value() -> computed_value::T {
@@ -813,45 +820,33 @@ ${helpers.single_keyword("overflow-x", "visible hidden scroll auto",
 
     #[inline]
     pub fn get_initial_specified_value() -> SpecifiedValue {
-        SpecifiedValue(CustomIdent(atom!("")))
+        SpecifiedValue(None)
     }
 
     impl fmt::Display for SpecifiedValue {
         fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-            self.0 .0.fmt(f)
+            self.to_css(f)
         }
     }
 
     impl ToCss for SpecifiedValue {
         fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-            if self.0 .0 == atom!("") {
+            if let Some(ref name) = self.0 {
-                dest.write_str("none")
+                name.to_css(dest)
             } else {
-                self.0.to_css(dest)
+                dest.write_str("none")
             }
         }
     }
 
     impl Parse for SpecifiedValue {
-        fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result<Self, ()> {
+        fn parse(context: &ParserContext, input: &mut ::cssparser::Parser) -> Result<Self, ()> {
-            use cssparser::Token;
+            if let Ok(name) = input.try(|input| KeyframesName::parse(context, input)) {
-            use std::ascii::AsciiExt;
+                Ok(SpecifiedValue(Some(name)))
-            use values::CustomIdent;
-
-            let atom = match input.next() {
-                Ok(Token::Ident(value)) => {
-                    if value.eq_ignore_ascii_case("none") {
-                        // FIXME We may want to support `@keyframes ""` at some point.
-                        CustomIdent(atom!(""))
             } else {
-                        CustomIdent::from_ident(value, &[])?
+                input.expect_ident_matching("none").map(|()| SpecifiedValue(None))
             }
         }
-                Ok(Token::QuotedString(value)) => CustomIdent(Atom::from(value)),
-                _ => return Err(()),
-            };
-            Ok(SpecifiedValue(atom))
-        }
     }
     no_viewport_percentage!(SpecifiedValue);
 

components/style/properties/properties.mako.rs

@@ -1617,7 +1617,7 @@ pub mod style_structs {
             /// Returns whether there is any animation specified with
             /// animation-name other than `none`.
             pub fn specifies_animations(&self) -> bool {
-                self.animation_name_iter().any(|name| name.0 .0 != atom!(""))
+                self.animation_name_iter().any(|name| name.0.is_some())
             }
 
             /// Returns whether there are any transitions specified.

components/style/stylesheets.rs

@@ -6,10 +6,10 @@
 
 #![deny(missing_docs)]
 
-use {Atom, Prefix, Namespace};
+use {Prefix, Namespace};
 use counter_style::{CounterStyleRule, parse_counter_style_name, parse_counter_style_body};
 use cssparser::{AtRuleParser, Parser, QualifiedRuleParser};
-use cssparser::{AtRuleType, RuleListParser, Token, parse_one_rule};
+use cssparser::{AtRuleType, RuleListParser, SourcePosition, parse_one_rule};
 use cssparser::ToCss as ParserToCss;
 use error_reporting::{ParseErrorReporter, NullReporter};
 #[cfg(feature = "servo")]
@@ -41,7 +41,7 @@ use str::starts_with_ignore_ascii_case;
 use style_traits::ToCss;
 use stylist::FnvHashMap;
 use supports::SupportsCondition;
-use values::CustomIdent;
+use values::{CustomIdent, KeyframesName};
 use values::specified::url::SpecifiedUrl;
 use viewport::ViewportRule;
 
@@ -519,7 +519,7 @@ impl ToCssWithGuard for ImportRule {
 #[derive(Debug)]
 pub struct KeyframesRule {
     /// The name of the current animation.
-    pub name: CustomIdent,
+    pub name: KeyframesName,
     /// The keyframes specified for this CSS rule.
     pub keyframes: Vec<Arc<Locked<Keyframe>>>,
     /// Vendor prefix type the @keyframes has.
@@ -931,7 +931,7 @@ enum AtRulePrelude {
     /// A @viewport rule prelude.
     Viewport,
     /// A @keyframes rule, with its animation name and vendor prefix if exists.
-    Keyframes(CustomIdent, Option<VendorPrefix>),
+    Keyframes(KeyframesName, Option<VendorPrefix>),
     /// A @page rule prelude.
     Page,
 }
@@ -1139,11 +1139,7 @@ impl<'a, 'b> AtRuleParser for NestedRuleParser<'a, 'b> {
                     // Servo should not support @-moz-keyframes.
                     return Err(())
                 }
-                let name = match input.next() {
+                let name = KeyframesName::parse(self.context, input)?;
-                    Ok(Token::Ident(value)) => CustomIdent::from_ident(value, &["none"])?,
-                    Ok(Token::QuotedString(value)) => CustomIdent(Atom::from(value)),
-                    _ => return Err(())
-                };
 
                 Ok(AtRuleType::WithBlock(AtRulePrelude::Keyframes(name, prefix)))
             },

components/style/stylist.rs

@@ -349,13 +349,13 @@ impl Stylist {
 
                     // Don't let a prefixed keyframes animation override a non-prefixed one.
                     let needs_insertion = keyframes_rule.vendor_prefix.is_none() ||
-                        self.animations.get(&keyframes_rule.name.0).map_or(true, |rule|
+                        self.animations.get(keyframes_rule.name.as_atom()).map_or(true, |rule|
                             rule.vendor_prefix.is_some());
                     if needs_insertion {
                         let animation = KeyframesAnimation::from_keyframes(
                             &keyframes_rule.keyframes, keyframes_rule.vendor_prefix.clone(), guard);
                         debug!("Found valid keyframe animation: {:?}", animation);
-                        self.animations.insert(keyframes_rule.name.0.clone(), animation);
+                        self.animations.insert(keyframes_rule.name.as_atom().clone(), animation);
                     }
                 }
                 CssRule::FontFace(ref rule) => {

components/style/values/mod.rs

@@ -9,7 +9,7 @@
 #![deny(missing_docs)]
 
 use Atom;
-pub use cssparser::{RGBA, Parser, serialize_identifier};
+pub use cssparser::{RGBA, Token, Parser, serialize_identifier, serialize_string};
 use parser::{Parse, ParserContext};
 use std::ascii::AsciiExt;
 use std::borrow::Cow;
@@ -240,6 +240,52 @@ impl ToCss for CustomIdent {
     }
 }
 
+/// https://drafts.csswg.org/css-animations/#typedef-keyframes-name
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+pub enum KeyframesName {
+    /// <custom-ident>
+    Ident(CustomIdent),
+    /// <string>
+    QuotedString(Atom),
+}
+
+impl KeyframesName {
+    /// https://drafts.csswg.org/css-animations/#dom-csskeyframesrule-name
+    pub fn from_ident(value: String) -> Self {
+        match CustomIdent::from_ident((&*value).into(), &["none"]) {
+            Ok(ident) => KeyframesName::Ident(ident),
+            Err(()) => KeyframesName::QuotedString(value.into()),
+        }
+    }
+
+    /// The name as an Atom
+    pub fn as_atom(&self) -> &Atom {
+        match *self {
+            KeyframesName::Ident(ref ident) => &ident.0,
+            KeyframesName::QuotedString(ref atom) => atom,
+        }
+    }
+}
+
+impl Parse for KeyframesName {
+    fn parse(_context: &ParserContext, input: &mut Parser) -> Result<Self, ()> {
+        match input.next() {
+            Ok(Token::Ident(s)) => Ok(KeyframesName::Ident(CustomIdent::from_ident(s, &["none"])?)),
+            Ok(Token::QuotedString(s)) => Ok(KeyframesName::QuotedString(s.into())),
+            _ => Err(())
+        }
+    }
+}
+
+impl ToCss for KeyframesName {
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            KeyframesName::Ident(ref ident) => ident.to_css(dest),
+            KeyframesName::QuotedString(ref atom) => serialize_string(&atom.to_string(), dest),
+        }
+    }
+}
 
 // A type for possible values for min- and max- flavors of width,
 // height, block-size, and inline-size.

tests/unit/style/lib.rs

@@ -13,7 +13,7 @@ extern crate parking_lot;
 extern crate rayon;
 extern crate rustc_serialize;
 extern crate selectors;
-#[macro_use] extern crate servo_atoms;
+extern crate servo_atoms;
 extern crate servo_config;
 extern crate servo_url;
 extern crate style;

tests/unit/style/parsing/animation.rs

@@ -7,7 +7,7 @@ use servo_atoms::Atom;
 use style::parser::Parse;
 use style::properties::longhands::animation_iteration_count::single_value::computed_value::T as AnimationIterationCount;
 use style::properties::longhands::animation_name;
-use style::values::CustomIdent;
+use style::values::{KeyframesName, CustomIdent};
 use style_traits::ToCss;
 
 #[test]
@@ -15,13 +15,13 @@ fn test_animation_name() {
     use self::animation_name::single_value::SpecifiedValue as SingleValue;
     let other_name = Atom::from("other-name");
     assert_eq!(parse_longhand!(animation_name, "none"),
-               animation_name::SpecifiedValue(vec![SingleValue(CustomIdent(atom!("")))]));
+               animation_name::SpecifiedValue(vec![SingleValue(None)]));
     assert_eq!(parse_longhand!(animation_name, "other-name, none, 'other-name', \"other-name\""),
                animation_name::SpecifiedValue(
-                   vec![SingleValue(CustomIdent(other_name.clone())),
+                   vec![SingleValue(Some(KeyframesName::Ident(CustomIdent(other_name.clone())))),
-                        SingleValue(CustomIdent(atom!(""))),
+                        SingleValue(None),
-                        SingleValue(CustomIdent(other_name.clone())),
+                        SingleValue(Some(KeyframesName::QuotedString(other_name.clone()))),
-                        SingleValue(CustomIdent(other_name.clone()))]));
+                        SingleValue(Some(KeyframesName::QuotedString(other_name.clone())))]));
 }
 
 #[test]

tests/unit/style/stylesheets.rs

@@ -23,7 +23,7 @@ use style::properties::longhands::animation_play_state;
 use style::shared_lock::SharedRwLock;
 use style::stylesheets::{Origin, Namespaces};
 use style::stylesheets::{Stylesheet, NamespaceRule, CssRule, CssRules, StyleRule, KeyframesRule};
-use style::values::CustomIdent;
+use style::values::{KeyframesName, CustomIdent};
 use style::values::specified::{LengthOrPercentageOrAuto, Percentage};
 
 pub fn block_from<I>(iterable: I) -> PropertyDeclarationBlock
@@ -222,7 +222,7 @@ fn test_parse_stylesheet() {
                 ]))),
             }))),
             CssRule::Keyframes(Arc::new(stylesheet.shared_lock.wrap(KeyframesRule {
-                name: CustomIdent("foo".into()),
+                name: KeyframesName::Ident(CustomIdent("foo".into())),
                 keyframes: vec![
                     Arc::new(stylesheet.shared_lock.wrap(Keyframe {
                         selector: KeyframeSelector::new_for_unit_testing(