style: Refactor font-feature-settings and font-variation-settings.

This fixes all known issues with serialization and parsing of these two
properties, and in particular calc handling and such:

  https://bugzilla.mozilla.org/show_bug.cgi?id=1434692
  https://bugzilla.mozilla.org/show_bug.cgi?id=1434724

Also does a fair amount of cleanup and all that, which was needed.

components/style/values/computed/font.rs

@@ -22,13 +22,13 @@ use std::slice;
 use style_traits::{CssWriter, ParseError, ToCss};
 use values::CSSFloat;
 use values::animated::{ToAnimatedValue, ToAnimatedZero};
-use values::computed::{Context, NonNegativeLength, ToComputedValue};
-use values::generics::font::{FontSettings, FontSettingTagInt};
+use values::computed::{Context, NonNegativeLength, ToComputedValue, Integer, Number};
+use values::generics::font::{FontSettings, FeatureTagValue, VariationValue};
 use values::specified::font as specified;
 use values::specified::length::{FontBaseSize, NoCalcLength};
 
 pub use values::computed::Length as MozScriptMinSize;
-pub use values::specified::font::{XTextZoom, XLang, MozScriptSizeMultiplier, FontSynthesis, FontVariantSettings};
+pub use values::specified::font::{XTextZoom, XLang, MozScriptSizeMultiplier, FontSynthesis};
 
 /// As of CSS Fonts Module Level 3, only the following values are
 /// valid: 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900
@@ -712,8 +712,11 @@ pub type FontVariantLigatures = specified::VariantLigatures;
 /// Use VariantNumeric as computed type of FontVariantNumeric
 pub type FontVariantNumeric = specified::VariantNumeric;
 
-/// Use FontSettings as computed type of FontFeatureSettings
-pub type FontFeatureSettings = FontSettings<FontSettingTagInt>;
+/// Use FontSettings as computed type of FontFeatureSettings.
+pub type FontFeatureSettings = FontSettings<FeatureTagValue<Integer>>;
+
+/// The computed value for font-variation-settings.
+pub type FontVariationSettings = FontSettings<VariationValue<Number>>;
 
 /// font-language-override can only have a single three-letter
 /// OpenType "language system" tag, so we should be able to compute

components/style/values/computed/mod.rs

@@ -40,7 +40,7 @@ pub use self::background::{BackgroundSize, BackgroundRepeat};
 pub use self::border::{BorderImageSlice, BorderImageWidth, BorderImageSideWidth};
 pub use self::border::{BorderRadius, BorderCornerRadius, BorderSpacing};
 pub use self::font::{FontSize, FontSizeAdjust, FontSynthesis, FontWeight, FontVariantAlternates};
-pub use self::font::{FontFamily, FontLanguageOverride, FontVariantSettings, FontVariantEastAsian};
+pub use self::font::{FontFamily, FontLanguageOverride, FontVariationSettings, FontVariantEastAsian};
 pub use self::font::{FontVariantLigatures, FontVariantNumeric, FontFeatureSettings};
 pub use self::font::{MozScriptLevel, MozScriptMinSize, MozScriptSizeMultiplier, XTextZoom, XLang};
 pub use self::box_::{AnimationIterationCount, AnimationName, Display, OverscrollBehavior, Contain};

components/style/values/generics/font.rs

@@ -4,128 +4,116 @@
 
 //! Generic types for font stuff.
 
-use std::fmt::{self, Write};
 use cssparser::Parser;
+use num_traits::One;
 use parser::{Parse, ParserContext};
-use style_traits::{Comma, CssWriter, OneOrMoreSeparated, ParseError, ToCss, StyleParseErrorKind};
+use std::fmt::{self, Write};
+use style_traits::{CssWriter, ParseError, ToCss};
+use values::distance::{ComputeSquaredDistance, SquaredDistance};
 use values::specified::font::FontTag;
 
-/// A settings tag, defined by a four-character tag and a setting value
-///
-/// For font-feature-settings, this is a tag and an integer, for
-/// font-variation-settings this is a tag and a float.
-#[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
-pub struct FontSettingTag<T> {
-    /// A four-character tag, packed into a u32 (one byte per character)
+/// https://drafts.csswg.org/css-fonts-4/#feature-tag-value
+#[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue)]
+pub struct FeatureTagValue<Integer> {
+    /// A four-character tag, packed into a u32 (one byte per character).
     pub tag: FontTag,
     /// The actual value.
-    pub value: T,
+    pub value: Integer,
 }
 
-impl<T> OneOrMoreSeparated for FontSettingTag<T> {
-    type S = Comma;
-}
-
-impl<T: Parse> Parse for FontSettingTag<T> {
-    /// <https://www.w3.org/TR/css-fonts-3/#propdef-font-feature-settings>
-    /// <https://drafts.csswg.org/css-fonts-4/#low-level-font-variation->
-    /// settings-control-the-font-variation-settings-property
-    /// <string> [ on | off | <integer> ]
-    /// <string> <number>
-    fn parse<'i, 't>(
-        context: &ParserContext,
-        input: &mut Parser<'i, 't>,
-    ) -> Result<Self, ParseError<'i>> {
-        let tag = FontTag::parse(context, input)?;
-        let value = T::parse(context, input)?;
-
-        Ok(Self { tag, value })
-    }
-}
-
-/// A font settings value for font-variation-settings or font-feature-settings
-#[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
-pub enum FontSettings<T> {
-    /// No settings (default)
-    Normal,
-    /// Set of settings
-    Tag(Vec<FontSettingTag<T>>)
-}
-
-impl <T> FontSettings<T> {
-    #[inline]
-    /// Default value of font settings as `normal`
-    pub fn normal() -> Self {
-        FontSettings::Normal
-    }
-}
-
-impl<T: Parse> Parse for FontSettings<T> {
-    /// <https://www.w3.org/TR/css-fonts-3/#propdef-font-feature-settings>
-    fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        if input.try(|i| i.expect_ident_matching("normal")).is_ok() {
-            return Ok(FontSettings::Normal);
-        }
-        Vec::parse(context, input).map(FontSettings::Tag)
-    }
-}
-
-/// An integer that can also parse "on" and "off",
-/// for font-feature-settings
-#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue)]
-pub struct FontSettingTagInt(pub u32);
-
-/// A number value to be used for font-variation-settings.
-///
-/// FIXME(emilio): The spec only says <integer>, so we should be able to reuse
-/// the other code:
-///
-/// https://drafts.csswg.org/css-fonts-4/#propdef-font-variation-settings
-#[cfg_attr(feature = "gecko", derive(Animate, ComputeSquaredDistance))]
-#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
-pub struct FontSettingTagFloat(pub f32);
-
-impl ToCss for FontSettingTagInt {
+impl<Integer> ToCss for FeatureTagValue<Integer>
+where
+    Integer: One + ToCss + PartialEq,
+{
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: Write,
     {
-        match self.0 {
-            1 => Ok(()),
-            0 => dest.write_str("off"),
-            x => x.to_css(dest),
+        self.tag.to_css(dest)?;
+        // Don't serialize the default value.
+        if self.value != Integer::one() {
+            dest.write_char(' ')?;
+            self.value.to_css(dest)?;
         }
+
+        Ok(())
     }
 }
 
-impl Parse for FontSettingTagInt {
-    fn parse<'i, 't>(_context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        // FIXME(emilio): This doesn't handle calc properly.
-        if let Ok(value) = input.try(|input| input.expect_integer()) {
-            // handle integer, throw if it is negative
-            if value >= 0 {
-                Ok(FontSettingTagInt(value as u32))
-            } else {
-                Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+/// Variation setting for a single feature, see:
+///
+/// https://drafts.csswg.org/css-fonts-4/#font-variation-settings-def
+#[derive(Animate, Clone, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue, ToCss)]
+pub struct VariationValue<Number> {
+    /// A four-character tag, packed into a u32 (one byte per character).
+    #[animation(constant)]
+    pub tag: FontTag,
+    /// The actual value.
+    pub value: Number,
+}
+
+impl<Number> ComputeSquaredDistance for VariationValue<Number>
+where
+    Number: ComputeSquaredDistance,
+{
+    #[inline]
+    fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
+        if self.tag != other.tag {
+            return Err(());
+        }
+        self.value.compute_squared_distance(&other.value)
+    }
+}
+
+/// A value both for font-variation-settings and font-feature-settings.
+#[derive(Clone, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue)]
+pub struct FontSettings<T>(pub Box<[T]>);
+
+impl<T> FontSettings<T> {
+    /// Default value of font settings as `normal`.
+    #[inline]
+    pub fn normal() -> Self {
+        FontSettings(vec![].into_boxed_slice())
+    }
+}
+
+impl<T: Parse> Parse for FontSettings<T> {
+    /// https://drafts.csswg.org/css-fonts-4/#descdef-font-face-font-feature-settings
+    /// https://drafts.csswg.org/css-fonts-4/#font-variation-settings-def
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        if input.try(|i| i.expect_ident_matching("normal")).is_ok() {
+            return Ok(Self::normal());
+        }
+
+        Ok(FontSettings(
+            input.parse_comma_separated(|i| T::parse(context, i))?.into_boxed_slice()
+        ))
+    }
+}
+
+impl<T: ToCss> ToCss for FontSettings<T> {
+    /// https://drafts.csswg.org/css-fonts-4/#descdef-font-face-font-feature-settings
+    /// https://drafts.csswg.org/css-fonts-4/#font-variation-settings-def
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        if self.0.is_empty() {
+            return dest.write_str("normal");
+        }
+
+        let mut first = true;
+        for item in self.0.iter() {
+            if !first {
+                dest.write_str(", ")?;
             }
-        } else if let Ok(_) = input.try(|input| input.expect_ident_matching("on")) {
-            // on is an alias for '1'
-            Ok(FontSettingTagInt(1))
-        } else if let Ok(_) = input.try(|input| input.expect_ident_matching("off")) {
-            // off is an alias for '0'
-            Ok(FontSettingTagInt(0))
-        } else {
-            // empty value is an alias for '1'
-            Ok(FontSettingTagInt(1))
+            first = false;
+            item.to_css(dest)?;
         }
-    }
-}
 
-impl Parse for FontSettingTagFloat {
-    fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        // FIXME(emilio): Should handle calc() using Number::parse.
-        //
-        // Also why is this not in font.rs?
-        input.expect_number().map(FontSettingTagFloat).map_err(|e| e.into())
+        Ok(())
     }
 }

components/style/values/specified/font.rs

@@ -21,8 +21,8 @@ use style_traits::{CssWriter, ParseError, StyleParseErrorKind, ToCss};
 use values::CustomIdent;
 use values::computed::{font as computed, Context, Length, NonNegativeLength, ToComputedValue};
 use values::computed::font::{SingleFontFamily, FontFamilyList, FamilyName};
-use values::generics::font::{FontSettings, FontSettingTagFloat};
-use values::specified::{AllowQuirks, LengthOrPercentage, NoCalcLength, Number};
+use values::generics::font::{FontSettings, FeatureTagValue, VariationValue};
+use values::specified::{AllowQuirks, Integer, LengthOrPercentage, NoCalcLength, Number};
 use values::specified::length::{AU_PER_PT, AU_PER_PX, FontBaseSize};
 
 const DEFAULT_SCRIPT_MIN_SIZE_PT: u32 = 8;
@@ -164,8 +164,8 @@ impl From<LengthOrPercentage> for FontSize {
     }
 }
 
+/// Specifies a prioritized list of font family names or generic family names.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
-/// Specifies a prioritized list of font family names or generic family names
 pub enum FontFamily {
     /// List of `font-family`
     Values(FontFamilyList),
@@ -1709,13 +1709,15 @@ impl Parse for FontVariantNumeric {
     }
 }
 
-#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
-#[derive(Clone, Debug, PartialEq, ToCss)]
-/// Define initial settings that apply when the font defined
-/// by an @font-face rule is rendered.
+/// This property provides low-level control over OpenType or TrueType font variations.
+pub type SpecifiedFontFeatureSettings = FontSettings<FeatureTagValue<Integer>>;
+
+/// Define initial settings that apply when the font defined by an @font-face
+/// rule is rendered.
+#[derive(Clone, Debug, MallocSizeOf, PartialEq, ToCss)]
 pub enum FontFeatureSettings {
     /// Value of `FontSettings`
-    Value(computed::FontFeatureSettings),
+    Value(SpecifiedFontFeatureSettings),
     /// System font
     System(SystemFont)
 }
@@ -1724,7 +1726,7 @@ impl FontFeatureSettings {
     #[inline]
     /// Get default value of `font-feature-settings` as normal
     pub fn normal() -> FontFeatureSettings {
-        FontFeatureSettings::Value(FontSettings::Normal)
+        FontFeatureSettings::Value(FontSettings::normal())
     }
 
     /// Get `font-feature-settings` with system font
@@ -1745,12 +1747,12 @@ impl FontFeatureSettings {
 impl ToComputedValue for FontFeatureSettings {
     type ComputedValue = computed::FontFeatureSettings;
 
-    fn to_computed_value(&self, _context: &Context) -> computed::FontFeatureSettings {
+    fn to_computed_value(&self, context: &Context) -> computed::FontFeatureSettings {
         match *self {
-            FontFeatureSettings::Value(ref v) => v.clone(),
+            FontFeatureSettings::Value(ref v) => v.to_computed_value(context),
             FontFeatureSettings::System(_) => {
                 #[cfg(feature = "gecko")] {
-                    _context.cached_system_font.as_ref().unwrap().font_feature_settings.clone()
+                    context.cached_system_font.as_ref().unwrap().font_feature_settings.clone()
                 }
                 #[cfg(feature = "servo")] {
                     unreachable!()
@@ -1760,7 +1762,7 @@ impl ToComputedValue for FontFeatureSettings {
     }
 
     fn from_computed_value(other: &computed::FontFeatureSettings) -> Self {
-        FontFeatureSettings::Value(other.clone())
+        FontFeatureSettings::Value(ToComputedValue::from_computed_value(other))
     }
 }
 
@@ -1770,7 +1772,7 @@ impl Parse for FontFeatureSettings {
         context: &ParserContext,
         input: &mut Parser<'i, 't>
     ) -> Result<FontFeatureSettings, ParseError<'i>> {
-        computed::FontFeatureSettings::parse(context, input).map(FontFeatureSettings::Value)
+        SpecifiedFontFeatureSettings::parse(context, input).map(FontFeatureSettings::Value)
     }
 }
 
@@ -2006,9 +2008,51 @@ impl ToCss for FontTag {
     }
 }
 
+/// This property provides low-level control over OpenType or TrueType font
+/// variations.
+pub type FontVariationSettings = FontSettings<VariationValue<Number>>;
+
+fn parse_one_feature_value<'i, 't>(
+    context: &ParserContext,
+    input: &mut Parser<'i, 't>,
+) -> Result<Integer, ParseError<'i>> {
+    if let Ok(integer) = input.try(|i| Integer::parse_non_negative(context, i)) {
+        return Ok(integer)
+    }
+
+    try_match_ident_ignore_ascii_case! { input,
+        "on" => Ok(Integer::new(1)),
+        "off" => Ok(Integer::new(0)),
+    }
+}
+
+impl Parse for FeatureTagValue<Integer> {
+    /// https://drafts.csswg.org/css-fonts-4/#feature-tag-value
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        let tag = FontTag::parse(context, input)?;
+        let value = input.try(|i| parse_one_feature_value(context, i))
+            .unwrap_or_else(|_| Integer::new(1));
+
+        Ok(Self { tag, value })
+    }
+}
+
+impl Parse for VariationValue<Number> {
+    /// This is the `<string> <number>` part of the font-variation-settings
+    /// syntax.
+    fn parse<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Self, ParseError<'i>> {
+        let tag = FontTag::parse(context, input)?;
+        let value = Number::parse(context, input)?;
+        Ok(Self { tag, value })
+    }
+}
 
-/// This property provides low-level control over OpenType or TrueType font variations.
-pub type FontVariantSettings = FontSettings<FontSettingTagFloat>;
 
 #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue)]
 /// text-zoom. Enable if true, disable if false

components/style/values/specified/mod.rs

@@ -9,6 +9,7 @@
 use Namespace;
 use context::QuirksMode;
 use cssparser::{Parser, Token, serialize_identifier};
+use num_traits::One;
 use parser::{ParserContext, Parse};
 use self::url::SpecifiedUrl;
 #[allow(unused_imports)] use std::ascii::AsciiExt;
@@ -33,7 +34,7 @@ pub use self::background::{BackgroundRepeat, BackgroundSize};
 pub use self::border::{BorderCornerRadius, BorderImageSlice, BorderImageWidth};
 pub use self::border::{BorderImageSideWidth, BorderRadius, BorderSideWidth, BorderSpacing};
 pub use self::font::{FontSize, FontSizeAdjust, FontSynthesis, FontWeight, FontVariantAlternates};
-pub use self::font::{FontFamily, FontLanguageOverride, FontVariantSettings, FontVariantEastAsian};
+pub use self::font::{FontFamily, FontLanguageOverride, FontVariationSettings, FontVariantEastAsian};
 pub use self::font::{FontVariantLigatures, FontVariantNumeric, FontFeatureSettings};
 pub use self::font::{MozScriptLevel, MozScriptMinSize, MozScriptSizeMultiplier, XTextZoom, XLang};
 pub use self::box_::{AnimationIterationCount, AnimationName, Display, OverscrollBehavior, Contain};
@@ -382,12 +383,28 @@ impl ToComputedValue for Opacity {
 /// An specified `<integer>`, optionally coming from a `calc()` expression.
 ///
 /// <https://drafts.csswg.org/css-values/#integers>
-#[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, PartialOrd)]
+#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, PartialOrd)]
 pub struct Integer {
     value: CSSInteger,
     was_calc: bool,
 }
 
+impl One for Integer {
+    #[inline]
+    fn one() -> Self {
+        Self::new(1)
+    }
+}
+
+// This is not great, because it loses calc-ness, but it's necessary for One.
+impl ::std::ops::Mul<Integer> for Integer {
+    type Output = Self;
+
+    fn mul(self, other: Self) -> Self {
+        Self::new(self.value * other.value)
+    }
+}
+
 impl Integer {
     /// Trivially constructs a new `Integer` value.
     pub fn new(val: CSSInteger) -> Self {
@@ -435,7 +452,7 @@ impl Integer {
     pub fn parse_with_minimum<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
-        min: i32
+        min: i32,
     ) -> Result<Integer, ParseError<'i>> {
         match Integer::parse(context, input) {
             // FIXME(emilio): The spec asks us to avoid rejecting it at parse
@@ -498,10 +515,11 @@ impl ToCss for Integer {
 pub type IntegerOrAuto = Either<Integer, Auto>;
 
 impl IntegerOrAuto {
-    #[allow(missing_docs)]
-    pub fn parse_positive<'i, 't>(context: &ParserContext,
-                                  input: &mut Parser<'i, 't>)
-                                  -> Result<IntegerOrAuto, ParseError<'i>> {
+    /// Parse `auto` or a positive integer.
+    pub fn parse_positive<'i, 't>(
+        context: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<IntegerOrAuto, ParseError<'i>> {
         match IntegerOrAuto::parse(context, input) {
             Ok(Either::First(integer)) if integer.value() <= 0 => {
                 Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))