Refactoring; negative tests

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

@@ -380,7 +380,7 @@ ${helpers.single_keyword("font-variant-position",
         #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
         pub enum T {
             Normal,
-            Computed(Vec<FeatureTagValue>)
+            Tag(Vec<FeatureTagValue>)
         }
 
         #[derive(Debug, Clone, PartialEq)]
@@ -394,7 +394,7 @@ ${helpers.single_keyword("font-variant-position",
             fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
                 match *self {
                     T::Normal => dest.write_str("normal"),
-                    T::Computed(ref ftvs) => {
+                    T::Tag(ref ftvs) => {
                         let mut iter = ftvs.iter();
                         // handle head element
                         try!(iter.next().unwrap().to_css(dest));
@@ -417,32 +417,34 @@ ${helpers.single_keyword("font-variant-position",
         }
 
         impl FeatureTagValue {
+            /// https://www.w3.org/TR/css-fonts-3/#propdef-font-feature-settings
             /// <string> [ on | off | <integer> ]
             pub fn parse(input: &mut Parser) -> Result<FeatureTagValue, ()> {
-                let tag = try!(input.expect_string()).into_owned();
+                let tag = try!(input.expect_string());
 
                 // allowed strings of length 4 containing chars: <U+20, U+7E>
                 if tag.len() != 4 ||
-                   tag.chars().any(|c| c < ' ' || c > '~') {
-                       return Err(())
-                   }
+                   tag.chars().any(|c| c < ' ' || c > '~')
+                {
+                    return Err(())
+                }
 
                 if let Ok(value) = input.try(|input| input.expect_integer()) {
                     // handle integer, throw if it is negative
                     if value >= 0 {
-                        Ok(FeatureTagValue{ tag: tag, value: value })
+                        Ok(FeatureTagValue { tag: tag.into_owned(), value: value })
                     } else {
                         Err(())
                     }
                 } else if let Ok(_) = input.try(|input| input.expect_ident_matching("on")) {
                     // on is an alias for '1'
-                    Ok(FeatureTagValue{ tag: tag, value: 1 })
+                    Ok(FeatureTagValue { tag: tag.into_owned(), value: 1 })
                 } else if let Ok(_) = input.try(|input| input.expect_ident_matching("off")) {
                     // off is an alias for '0'
-                    Ok(FeatureTagValue{ tag: tag, value: 0 })
+                    Ok(FeatureTagValue { tag: tag.into_owned(), value: 0 })
                 } else {
                     // empty value is an alias for '1'
-                    Ok(FeatureTagValue{ tag:tag, value: 1 })
+                    Ok(FeatureTagValue { tag:tag.into_owned(), value: 1 })
                 }
             }
         }
@@ -459,7 +461,7 @@ ${helpers.single_keyword("font-variant-position",
             Ok(computed_value::T::Normal)
         } else {
             input.parse_comma_separated(computed_value::FeatureTagValue::parse)
-                 .map(computed_value::T::Computed)
+                 .map(computed_value::T::Tag)
         }
     }
 </%helpers:longhand>

tests/unit/style/parsing/font.rs

@@ -19,33 +19,53 @@ fn font_feature_settings_should_parse_properly() {
     assert_eq!(normal, normal_computed);
 
     let on = parse_longhand!(font_feature_settings, "\"abcd\" on");
-    let on_computed = computed_value::T::Computed(vec![
+    let on_computed = computed_value::T::Tag(vec![
         FeatureTagValue { tag: String::from("abcd"), value: 1 }
     ]);
     assert_eq!(on, on_computed);
 
     let off = parse_longhand!(font_feature_settings, "\"abcd\" off");
-    let off_computed = computed_value::T::Computed(vec![
+    let off_computed = computed_value::T::Tag(vec![
         FeatureTagValue { tag: String::from("abcd"), value: 0 }
     ]);
     assert_eq!(off, off_computed);
 
-    let empty = parse_longhand!(font_feature_settings, "\"abcd\"");
-    let empty_computed = computed_value::T::Computed(vec![
+    let no_value = parse_longhand!(font_feature_settings, "\"abcd\"");
+    let no_value_computed = computed_value::T::Tag(vec![
         FeatureTagValue { tag: String::from("abcd"), value: 1 }
     ]);
-    assert_eq!(empty, empty_computed);
+    assert_eq!(no_value, no_value_computed);
 
     let pos_integer = parse_longhand!(font_feature_settings, "\"abcd\" 100");
-    let pos_integer_computed = computed_value::T::Computed(vec![
+    let pos_integer_computed = computed_value::T::Tag(vec![
         FeatureTagValue { tag: String::from("abcd"), value: 100 }
     ]);
     assert_eq!(pos_integer, pos_integer_computed);
 
     let multiple = parse_longhand!(font_feature_settings, "\"abcd\" off, \"efgh\"");
-    let multiple_computed = computed_value::T::Computed(vec![
+    let multiple_computed = computed_value::T::Tag(vec![
         FeatureTagValue { tag: String::from("abcd"), value: 0 },
         FeatureTagValue { tag: String::from("efgh"), value: 1 }
     ]);
     assert_eq!(multiple, multiple_computed);
 }
+
+#[test]
+fn font_feature_settings_should_throw_on_bad_input() {
+    use style::properties::longhands::font_feature_settings;
+
+    let url = Url::parse("http://localhost").unwrap();
+    let context = ParserContext::new(Origin::Author, &url, Box::new(CSSErrorReporterTest));
+
+    let mut empty = Parser::new("");
+    assert!(font_feature_settings::parse(&context, &mut empty).is_err());
+
+    let mut negative = Parser::new("\"abcd\" -1");
+    assert!(font_feature_settings::parse(&context, &mut negative).is_err());
+
+    let mut short_tag = Parser::new("\"abc\"");
+    assert!(font_feature_settings::parse(&context, &mut short_tag).is_err());
+
+    let mut illegal_tag = Parser::new("\"abcó\"");
+    assert!(font_feature_settings::parse(&context, &mut illegal_tag).is_err());
+}