Bug 1563315 - Simplify parsing and storage of SVG paint server fallback. r=boris
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 06 Jul 2019 08:24:39 +0000
changeset 541198 72070e8f6b08dc6d8698c4a16ab1d4cf26ba7c65
parent 541197 d9db827fb8d5582c83fd4a770b2d6d3f969b244d
child 541199 bb29c8eeb41c73b5ddafc4396ac65b1f41437790
push id11533
push userarchaeopteryx@coole-files.de
push dateMon, 08 Jul 2019 18:18:03 +0000
treeherdermozilla-beta@f4452e031aed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersboris
bugs1563315
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1563315 - Simplify parsing and storage of SVG paint server fallback. r=boris Depends on D36805 Differential Revision: https://phabricator.services.mozilla.com/D36806
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/animated/svg.rs
servo/components/style/values/computed/svg.rs
servo/components/style/values/generics/svg.rs
servo/components/style/values/specified/mod.rs
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -520,26 +520,25 @@ def set_gecko_property(ffi_name, expr):
                 SVGOpacity::ContextStrokeOpacity
             }
         }
     }
 </%def>
 
 <%def name="impl_svg_paint(ident, gecko_ffi_name)">
     #[allow(non_snake_case)]
-    pub fn set_${ident}(&mut self, mut v: longhands::${ident}::computed_value::T) {
-        use crate::values::generics::svg::SVGPaintKind;
+    pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
+        use crate::values::generics::svg::{SVGPaintKind, SVGPaintFallback};
         use self::structs::nsStyleSVGPaintType;
         use self::structs::nsStyleSVGFallbackType;
 
         let ref mut paint = ${get_gecko_property(gecko_ffi_name)};
         unsafe {
             bindings::Gecko_nsStyleSVGPaint_Reset(paint);
         }
-        let fallback = v.fallback.take();
         match v.kind {
             SVGPaintKind::None => return,
             SVGPaintKind::ContextFill => {
                 paint.mType = nsStyleSVGPaintType::eStyleSVGPaintType_ContextFill;
             }
             SVGPaintKind::ContextStroke => {
                 paint.mType = nsStyleSVGPaintType::eStyleSVGPaintType_ContextStroke;
             }
@@ -554,25 +553,27 @@ def set_gecko_property(ffi_name, expr):
             SVGPaintKind::Color(color) => {
                 paint.mType = nsStyleSVGPaintType::eStyleSVGPaintType_Color;
                 unsafe {
                     *paint.mPaint.mColor.as_mut() = color.into();
                 }
             }
         }
 
-        paint.mFallbackType = match fallback {
-            Some(Either::First(color)) => {
-                paint.mFallbackColor = color.into();
+        paint.mFallbackType = match v.fallback {
+            SVGPaintFallback::Color(c) => {
+                paint.mFallbackColor = c.into();
                 nsStyleSVGFallbackType::eStyleSVGFallbackType_Color
             },
-            Some(Either::Second(_)) => {
+            SVGPaintFallback::None => {
                 nsStyleSVGFallbackType::eStyleSVGFallbackType_None
             },
-            None => nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet
+            SVGPaintFallback::Unset => {
+                nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet
+            }
         };
     }
 
     #[allow(non_snake_case)]
     pub fn copy_${ident}_from(&mut self, other: &Self) {
         unsafe {
             bindings::Gecko_nsStyleSVGPaint_CopyFrom(
                 &mut ${get_gecko_property(gecko_ffi_name)},
@@ -583,29 +584,31 @@ def set_gecko_property(ffi_name, expr):
 
     #[allow(non_snake_case)]
     pub fn reset_${ident}(&mut self, other: &Self) {
         self.copy_${ident}_from(other)
     }
 
     #[allow(non_snake_case)]
     pub fn clone_${ident}(&self) -> longhands::${ident}::computed_value::T {
-        use crate::values::generics::svg::{SVGPaint, SVGPaintKind};
+        use crate::values::generics::svg::{SVGPaint, SVGPaintKind, SVGPaintFallback};
         use self::structs::nsStyleSVGPaintType;
         use self::structs::nsStyleSVGFallbackType;
         let ref paint = ${get_gecko_property(gecko_ffi_name)};
 
         let fallback = match paint.mFallbackType {
             nsStyleSVGFallbackType::eStyleSVGFallbackType_Color => {
-                Some(Either::First(paint.mFallbackColor.into()))
+                SVGPaintFallback::Color(paint.mFallbackColor.into())
             },
             nsStyleSVGFallbackType::eStyleSVGFallbackType_None => {
-                Some(Either::Second(None_))
+                SVGPaintFallback::None
             },
-            nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet => None,
+            nsStyleSVGFallbackType::eStyleSVGFallbackType_NotSet => {
+                SVGPaintFallback::Unset
+            }
         };
 
         let kind = match paint.mType {
             nsStyleSVGPaintType::eStyleSVGPaintType_None => SVGPaintKind::None,
             nsStyleSVGPaintType::eStyleSVGPaintType_ContextFill => SVGPaintKind::ContextFill,
             nsStyleSVGPaintType::eStyleSVGPaintType_ContextStroke => SVGPaintKind::ContextStroke,
             nsStyleSVGPaintType::eStyleSVGPaintType_Server => {
                 SVGPaintKind::PaintServer(unsafe {
--- a/servo/components/style/values/animated/svg.rs
+++ b/servo/components/style/values/animated/svg.rs
@@ -1,33 +1,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! Animation implementations for various SVG-related types.
 
-use super::{Animate, Procedure, ToAnimatedZero};
+use super::{Animate, Procedure};
 use crate::properties::animated_properties::ListAnimation;
-use crate::values::animated::color::Color as AnimatedColor;
-use crate::values::computed::url::ComputedUrl;
 use crate::values::distance::{ComputeSquaredDistance, SquaredDistance};
-use crate::values::generics::svg::{SVGPaint, SVGStrokeDashArray};
-
-/// Animated SVGPaint.
-pub type IntermediateSVGPaint = SVGPaint<AnimatedColor, ComputedUrl>;
-
-impl ToAnimatedZero for IntermediateSVGPaint {
-    #[inline]
-    fn to_animated_zero(&self) -> Result<Self, ()> {
-        Ok(IntermediateSVGPaint {
-            kind: self.kind.to_animated_zero()?,
-            fallback: self.fallback.and_then(|v| v.to_animated_zero().ok()),
-        })
-    }
-}
+use crate::values::generics::svg::SVGStrokeDashArray;
 
 /// <https://www.w3.org/TR/SVG11/painting.html#StrokeDasharrayProperty>
 impl<L> Animate for SVGStrokeDashArray<L>
 where
     L: Clone + Animate,
 {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
--- a/servo/components/style/values/computed/svg.rs
+++ b/servo/components/style/values/computed/svg.rs
@@ -15,32 +15,23 @@ pub use crate::values::specified::SVGPai
 
 pub use crate::values::specified::MozContextProperties;
 
 /// Computed SVG Paint value
 pub type SVGPaint = generic::SVGPaint<Color, ComputedUrl>;
 /// Computed SVG Paint Kind value
 pub type SVGPaintKind = generic::SVGPaintKind<Color, ComputedUrl>;
 
-impl Default for SVGPaint {
-    fn default() -> Self {
-        SVGPaint {
-            kind: generic::SVGPaintKind::None,
-            fallback: None,
-        }
-    }
-}
-
 impl SVGPaint {
     /// Opaque black color
     pub fn black() -> Self {
         let rgba = RGBA::from_floats(0., 0., 0., 1.).into();
         SVGPaint {
             kind: generic::SVGPaintKind::Color(rgba),
-            fallback: None,
+            fallback: generic::SVGPaintFallback::Unset,
         }
     }
 }
 
 /// <length> | <percentage> | <number> | context-value
 pub type SVGLength = generic::SVGLength<LengthPercentage>;
 
 impl SVGLength {
--- a/servo/components/style/values/generics/svg.rs
+++ b/servo/components/style/values/generics/svg.rs
@@ -1,57 +1,95 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! Generic types for CSS values in SVG
 
 use crate::parser::{Parse, ParserContext};
-use crate::values::{Either, None_};
 use cssparser::Parser;
-use style_traits::{ParseError, StyleParseErrorKind};
+use style_traits::ParseError;
+
+/// The fallback of an SVG paint server value.
+#[derive(
+    Animate,
+    Clone,
+    ComputeSquaredDistance,
+    Debug,
+    MallocSizeOf,
+    PartialEq,
+    Parse,
+    SpecifiedValueInfo,
+    ToAnimatedValue,
+    ToAnimatedZero,
+    ToComputedValue,
+    ToCss,
+    ToResolvedValue,
+    ToShmem,
+)]
+pub enum SVGPaintFallback<C> {
+    /// The `none` keyword.
+    None,
+    /// A magic value that represents no fallback specified and serializes to
+    /// the empty string.
+    #[css(skip)]
+    Unset,
+    /// A color.
+    Color(C),
+}
 
 /// An SVG paint value
 ///
 /// <https://www.w3.org/TR/SVG2/painting.html#SpecifyingPaint>
 #[animation(no_bound(UrlPaintServer))]
 #[derive(
     Animate,
     Clone,
     ComputeSquaredDistance,
     Debug,
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToAnimatedValue,
+    ToAnimatedZero,
     ToComputedValue,
     ToCss,
     ToResolvedValue,
     ToShmem,
 )]
 pub struct SVGPaint<ColorType, UrlPaintServer> {
-    /// The paint source
+    /// The paint source.
     pub kind: SVGPaintKind<ColorType, UrlPaintServer>,
-    /// The fallback color. It would be empty, the `none` keyword or <color>.
-    pub fallback: Option<Either<ColorType, None_>>,
+    /// The fallback color.
+    pub fallback: SVGPaintFallback<ColorType>,
+}
+
+impl<C, U> Default for SVGPaint<C, U> {
+    fn default() -> Self {
+        Self {
+            kind: SVGPaintKind::None,
+            fallback: SVGPaintFallback::Unset,
+        }
+    }
 }
 
 /// An SVG paint value without the fallback
 ///
 /// Whereas the spec only allows PaintServer
 /// to have a fallback, Gecko lets the context
 /// properties have a fallback as well.
 #[animation(no_bound(UrlPaintServer))]
 #[derive(
     Animate,
     Clone,
     ComputeSquaredDistance,
     Debug,
     MallocSizeOf,
     PartialEq,
+    Parse,
     SpecifiedValueInfo,
     ToAnimatedValue,
     ToAnimatedZero,
     ToComputedValue,
     ToCss,
     ToResolvedValue,
     ToShmem,
 )]
@@ -65,75 +103,32 @@ pub enum SVGPaintKind<ColorType, UrlPain
     #[animation(error)]
     PaintServer(UrlPaintServer),
     /// `context-fill`
     ContextFill,
     /// `context-stroke`
     ContextStroke,
 }
 
-impl<ColorType, UrlPaintServer> SVGPaintKind<ColorType, UrlPaintServer> {
-    /// Parse a keyword value only
-    fn parse_ident<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        try_match_ident_ignore_ascii_case! { input,
-            "none" => Ok(SVGPaintKind::None),
-            "context-fill" => Ok(SVGPaintKind::ContextFill),
-            "context-stroke" => Ok(SVGPaintKind::ContextStroke),
-        }
-    }
-}
-
-/// Parse SVGPaint's fallback.
-/// fallback is keyword(none), Color or empty.
-/// <https://svgwg.org/svg2-draft/painting.html#SpecifyingPaint>
-fn parse_fallback<'i, 't, ColorType: Parse>(
-    context: &ParserContext,
-    input: &mut Parser<'i, 't>,
-) -> Option<Either<ColorType, None_>> {
-    if input.try(|i| i.expect_ident_matching("none")).is_ok() {
-        Some(Either::Second(None_))
-    } else {
-        if let Ok(color) = input.try(|i| ColorType::parse(context, i)) {
-            Some(Either::First(color))
-        } else {
-            None
-        }
-    }
-}
-
 impl<ColorType: Parse, UrlPaintServer: Parse> Parse for SVGPaint<ColorType, UrlPaintServer> {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
-        if let Ok(url) = input.try(|i| UrlPaintServer::parse(context, i)) {
-            Ok(SVGPaint {
-                kind: SVGPaintKind::PaintServer(url),
-                fallback: parse_fallback(context, input),
-            })
-        } else if let Ok(kind) = input.try(SVGPaintKind::parse_ident) {
-            if let SVGPaintKind::None = kind {
-                Ok(SVGPaint {
-                    kind: kind,
-                    fallback: None,
-                })
-            } else {
-                Ok(SVGPaint {
-                    kind: kind,
-                    fallback: parse_fallback(context, input),
-                })
-            }
-        } else if let Ok(color) = input.try(|i| ColorType::parse(context, i)) {
-            Ok(SVGPaint {
-                kind: SVGPaintKind::Color(color),
-                fallback: None,
-            })
-        } else {
-            Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+        let kind = SVGPaintKind::parse(context, input)?;
+        if matches!(kind, SVGPaintKind::None | SVGPaintKind::Color(..)) {
+            return Ok(SVGPaint {
+                kind,
+                fallback: SVGPaintFallback::Unset
+            });
         }
+        let fallback = input
+            .try(|i| SVGPaintFallback::parse(context, i))
+            .unwrap_or(SVGPaintFallback::Unset);
+        Ok(SVGPaint { kind, fallback })
     }
 }
 
 /// An SVG length value supports `context-value` in addition to length.
 #[derive(
     Animate,
     Clone,
     ComputeSquaredDistance,
--- a/servo/components/style/values/specified/mod.rs
+++ b/servo/components/style/values/specified/mod.rs
@@ -337,18 +337,16 @@ impl Parse for GreaterThanOrEqualToOneNu
         parse_number_with_clamping_mode(context, input, AllowedNumericType::AtLeastOne)
             .map(GreaterThanOrEqualToOne::<Number>)
     }
 }
 
 /// <number> | <percentage>
 ///
 /// Accepts only non-negative numbers.
-///
-/// FIXME(emilio): Should probably use Either.
 #[allow(missing_docs)]
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, SpecifiedValueInfo, ToCss, ToShmem)]
 pub enum NumberOrPercentage {
     Percentage(Percentage),
     Number(Number),
 }
 
 impl NumberOrPercentage {