Bug 1695369 - Simplify StyleColor representation. r=jwatt

There's no need for CurrentColor / Numeric variants when we can
represent them with the complex form.

Differential Revision: https://phabricator.services.mozilla.com/D106690

layout/style/StyleColor.cpp

@@ -50,9 +50,7 @@ template <>
 bool StyleColor::MaybeTransparent() const {
   // We know that the color is opaque when it's a numeric color with
   // alpha == 255.
-  // TODO(djg): Should we extend this to check Complex with bgRatio =
-  // 0, and fgRatio * alpha >= 255?
-  return !IsNumeric() || AsNumeric().alpha != 255;
+  return ratios != StyleComplexColorRatios::NUMERIC || color.alpha != 255;
 }
 
 template <>
@@ -62,32 +60,31 @@ nscolor StyleColor::CalcColor(nscolor aColor) const {
 
 template <>
 nscolor StyleColor::CalcColor(const StyleRGBA& aForegroundColor) const {
-  if (IsNumeric()) {
-    return AsNumeric().ToColor();
+  if (ratios == StyleComplexColorRatios::NUMERIC) {
+    return color.ToColor();
   }
-  if (IsCurrentColor()) {
+  if (ratios == StyleComplexColorRatios::CURRENT_COLOR) {
     return aForegroundColor.ToColor();
   }
-  MOZ_ASSERT(IsComplex());
-  const auto& complex = AsComplex();
-  return LinearBlendColors(complex.color, complex.ratios.bg, aForegroundColor,
-                           complex.ratios.fg);
+  return LinearBlendColors(color, ratios.bg, aForegroundColor, ratios.fg);
 }
 
 template <>
 nscolor StyleColor::CalcColor(const ComputedStyle& aStyle) const {
   // Common case that is numeric color, which is pure background, we
   // can skip resolving StyleText().
-  // TODO(djg): Is this optimization worth it?
-  if (IsNumeric()) {
-    return AsNumeric().ToColor();
+  if (ratios == StyleComplexColorRatios::NUMERIC) {
+    return color.ToColor();
   }
   return CalcColor(aStyle.StyleText()->mColor);
 }
 
 template <>
 nscolor StyleColor::CalcColor(const nsIFrame* aFrame) const {
-  return CalcColor(*aFrame->Style());
+  if (ratios == StyleComplexColorRatios::NUMERIC) {
+    return color.ToColor();
+  }
+  return CalcColor(aFrame->StyleText()->mColor);
 }
 
 }  // namespace mozilla

layout/style/StyleColorInlines.h

@@ -27,7 +27,18 @@ inline StyleRGBA StyleRGBA::Transparent() { return {0, 0, 0, 0}; }
 
 template <>
 inline StyleColor StyleColor::FromColor(nscolor aColor) {
-  return StyleColor::Numeric(StyleRGBA::FromColor(aColor));
+  return StyleColor{
+      StyleRGBA::FromColor(aColor),
+      StyleComplexColorRatios::NUMERIC,
+  };
+}
+
+template <>
+inline StyleColor StyleColor::CurrentColor() {
+  return StyleColor{
+      StyleRGBA::Transparent(),
+      StyleComplexColorRatios::CURRENT_COLOR,
+  };
 }
 
 template <>

servo/components/style/values/animated/color.rs

@@ -35,17 +35,14 @@ impl RGBA {
     #[inline]
     pub fn new(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
         RGBA {
-            red: red,
-            green: green,
-            blue: blue,
-            alpha: alpha,
+            red,
+            green,
+            blue,
+            alpha,
         }
     }
 }
 
-/// Unlike Animate for computed colors, we don't clamp any component values.
-///
-/// FIXME(nox): Why do computed colors even implement Animate?
 impl Animate for RGBA {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
@@ -57,15 +54,11 @@ impl Animate for RGBA {
         }
 
         alpha = alpha.min(1.);
-        let red =
-            (self.red * self.alpha).animate(&(other.red * other.alpha), procedure)? * 1. / alpha;
-        let green = (self.green * self.alpha).animate(&(other.green * other.alpha), procedure)? *
-            1. /
-            alpha;
-        let blue =
-            (self.blue * self.alpha).animate(&(other.blue * other.alpha), procedure)? * 1. / alpha;
-
-        Ok(RGBA::new(red, green, blue, alpha))
+        let red = (self.red * self.alpha).animate(&(other.red * other.alpha), procedure)?;
+        let green = (self.green * self.alpha).animate(&(other.green * other.alpha), procedure)?;
+        let blue = (self.blue * self.alpha).animate(&(other.blue * other.alpha), procedure)?;
+        let inv = 1. / alpha;
+        Ok(RGBA::new(red * inv, green * inv, blue * inv, alpha))
     }
 }
 
@@ -97,21 +90,34 @@ pub type Color = GenericColor<RGBA>;
 
 impl Color {
     fn effective_intermediate_rgba(&self) -> RGBA {
-        match *self {
-            GenericColor::Numeric(color) => color,
-            GenericColor::CurrentColor => RGBA::transparent(),
-            GenericColor::Complex { color, ratios } => RGBA {
-                alpha: color.alpha * ratios.bg,
-                ..color.clone()
-            },
+        if self.ratios.bg == 0. {
+            return RGBA::transparent();
+        }
+
+        if self.ratios.bg == 1. {
+            return self.color;
+        }
+
+        RGBA {
+            alpha: self.color.alpha * self.ratios.bg,
+            ..self.color
         }
     }
 
-    fn effective_ratios(&self) -> ComplexColorRatios {
-        match *self {
-            GenericColor::Numeric(..) => ComplexColorRatios::NUMERIC,
-            GenericColor::CurrentColor => ComplexColorRatios::CURRENT_COLOR,
-            GenericColor::Complex { ratios, .. } => ratios,
+    fn scaled_rgba(&self) -> RGBA {
+        if self.ratios.bg == 0. {
+            return RGBA::transparent();
+        }
+
+        if self.ratios.bg == 1. {
+            return self.color;
+        }
+
+        RGBA {
+            red: self.color.red * self.ratios.bg,
+            green: self.color.green * self.ratios.bg,
+            blue: self.color.blue * self.ratios.bg,
+            alpha: self.color.alpha * self.ratios.bg,
         }
     }
 }
@@ -119,58 +125,51 @@ impl Color {
 impl Animate for Color {
     #[inline]
     fn animate(&self, other: &Self, procedure: Procedure) -> Result<Self, ()> {
-        use self::GenericColor::*;
+        let self_numeric = self.is_numeric();
+        let other_numeric = other.is_numeric();
 
-        // Common cases are interpolating between two numeric colors,
-        // two currentcolors, and a numeric color and a currentcolor.
-        let (this_weight, other_weight) = procedure.weights();
+        if self_numeric && other_numeric {
+            return Ok(Self::rgba(self.color.animate(&other.color, procedure)?));
+        }
 
-        Ok(match (*self, *other, procedure) {
-            // Any interpolation of currentcolor with currentcolor returns currentcolor.
-            (CurrentColor, CurrentColor, Procedure::Interpolate { .. }) => CurrentColor,
-            // Animating two numeric colors.
-            (Numeric(c1), Numeric(c2), _) => Numeric(c1.animate(&c2, procedure)?),
-            // Combinations of numeric color and currentcolor
-            (CurrentColor, Numeric(color), _) => Self::with_ratios(
-                color,
-                ComplexColorRatios {
-                    bg: other_weight as f32,
-                    fg: this_weight as f32,
-                },
-            ),
-            (Numeric(color), CurrentColor, _) => Self::with_ratios(
-                color,
-                ComplexColorRatios {
-                    bg: this_weight as f32,
-                    fg: other_weight as f32,
-                },
-            ),
+        let self_currentcolor = self.is_currentcolor();
+        let other_currentcolor = other.is_currentcolor();
 
-            // Any other animation of currentcolor with currentcolor.
-            (CurrentColor, CurrentColor, _) => Self::with_ratios(
+        if self_currentcolor && other_currentcolor {
+            let (self_weight, other_weight) = procedure.weights();
+            return Ok(Self::new(
                 RGBA::transparent(),
                 ComplexColorRatios {
                     bg: 0.,
-                    fg: (this_weight + other_weight) as f32,
+                    fg: (self_weight + other_weight) as f32,
                 },
-            ),
+            ));
+        }
+
+        // FIXME(emilio): Without these special cases tests fail, looks fairly
+        // sketchy!
+        if (self_currentcolor && other_numeric) || (self_numeric && other_currentcolor) {
+            let (self_weight, other_weight) = procedure.weights();
+            return Ok(if self_numeric {
+                Self::new(
+                    self.color,
+                    ComplexColorRatios {
+                        bg: self_weight as f32,
+                        fg: other_weight as f32,
+                    },
+                )
+            } else {
+                Self::new(
+                    other.color,
+                    ComplexColorRatios {
+                        bg: other_weight as f32,
+                        fg: self_weight as f32,
+                    },
+                )
+            });
+        }
 
-            // Defer to complex calculations
-            _ => {
         // Compute the "scaled" contribution for `color`.
-                fn scaled_rgba(color: &Color) -> RGBA {
-                    match *color {
-                        GenericColor::Numeric(color) => color,
-                        GenericColor::CurrentColor => RGBA::transparent(),
-                        GenericColor::Complex { color, ratios } => RGBA {
-                            red: color.red * ratios.bg,
-                            green: color.green * ratios.bg,
-                            blue: color.blue * ratios.bg,
-                            alpha: color.alpha * ratios.bg,
-                        },
-                    }
-                }
-
         // Each `Color`, represents a complex combination of foreground color and
         // background color where fg and bg represent the overall
         // contributions. ie:
@@ -206,53 +205,60 @@ impl Animate for Color {
         //
         //    = { bg_color, fg_color }
         //    = { 1 * (bg_color1 op bg_color2), (fg1 op fg2) * foreground }
-
+        //
         // To perform the operation on two complex colors, we need to
         // generate the scaled contributions of each background color
         // component.
-                let bg_color1 = scaled_rgba(self);
-                let bg_color2 = scaled_rgba(other);
+        let bg_color1 = self.scaled_rgba();
+        let bg_color2 = other.scaled_rgba();
+
         // Perform bg_color1 op bg_color2
         let bg_color = bg_color1.animate(&bg_color2, procedure)?;
 
         // Calculate the final foreground color ratios; perform
         // animation on effective fg ratios.
-                let ComplexColorRatios { fg: fg1, .. } = self.effective_ratios();
-                let ComplexColorRatios { fg: fg2, .. } = other.effective_ratios();
-                // Perform fg1 op fg2
-                let fg = fg1.animate(&fg2, procedure)?;
+        let fg = self.ratios.fg.animate(&other.ratios.fg, procedure)?;
 
-                Self::with_ratios(bg_color, ComplexColorRatios { bg: 1., fg })
-            },
-        })
+        Ok(Self::new(bg_color, ComplexColorRatios { bg: 1., fg }))
     }
 }
 
 impl ComputeSquaredDistance for Color {
     #[inline]
     fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> {
-        use self::GenericColor::*;
+        // All comments from the Animate impl also apply here.
+        let self_numeric = self.is_numeric();
+        let other_numeric = other.is_numeric();
 
-        // All comments from the Animate impl also applies here.
-        Ok(match (*self, *other) {
-            (CurrentColor, CurrentColor) => SquaredDistance::from_sqrt(0.),
-            (Numeric(c1), Numeric(c2)) => c1.compute_squared_distance(&c2)?,
-            (CurrentColor, Numeric(color)) | (Numeric(color), CurrentColor) => {
+        if self_numeric && other_numeric {
+            return self.color.compute_squared_distance(&other.color);
+        }
+
+        let self_currentcolor = self.is_currentcolor();
+        let other_currentcolor = other.is_currentcolor();
+        if self_currentcolor && other_currentcolor {
+            return Ok(SquaredDistance::from_sqrt(0.));
+        }
+
+        if (self_currentcolor && other_numeric) || (self_numeric && other_currentcolor) {
+            let color = if self_numeric {
+                &self.color
+            } else {
+                &other.color
+            };
             // `computed_squared_distance` is symmetric.
-                color.compute_squared_distance(&RGBA::transparent())? +
-                    SquaredDistance::from_sqrt(1.)
-            },
-            (_, _) => {
+            return Ok(color.compute_squared_distance(&RGBA::transparent())? +
+                SquaredDistance::from_sqrt(1.));
+        }
+
         let self_color = self.effective_intermediate_rgba();
         let other_color = other.effective_intermediate_rgba();
-                let self_ratios = self.effective_ratios();
-                let other_ratios = other.effective_ratios();
+        let self_ratios = self.ratios;
+        let other_ratios = other.ratios;
 
-                self_color.compute_squared_distance(&other_color)? +
+        Ok(self_color.compute_squared_distance(&other_color)? +
             self_ratios.bg.compute_squared_distance(&other_ratios.bg)? +
-                    self_ratios.fg.compute_squared_distance(&other_ratios.fg)?
-            },
-        })
+            self_ratios.fg.compute_squared_distance(&other_ratios.fg)?)
     }
 }
 

servo/components/style/values/computed/color.rs

@@ -29,13 +29,18 @@ impl Color {
     /// Combine this complex color with the given foreground color into
     /// a numeric RGBA color. It currently uses linear blending.
     pub fn to_rgba(&self, fg_color: RGBA) -> RGBA {
-        let (color, ratios) = match *self {
-            // Common cases that the complex color is either pure numeric
-            // color or pure currentcolor.
-            GenericColor::Numeric(color) => return color,
-            GenericColor::CurrentColor => return fg_color,
-            GenericColor::Complex { color, ratios } => (color, ratios),
-        };
+        // Common cases that the complex color is either pure numeric color or
+        // pure currentcolor.
+        if self.is_numeric() {
+            return self.color;
+        }
+
+        if self.is_currentcolor() {
+            return fg_color;
+        }
+
+        let ratios = &self.ratios;
+        let color = &self.color;
 
         // For the more complicated case that the alpha value differs,
         // we use the following formula to compute the components:
@@ -59,13 +64,14 @@ impl Color {
         if a <= 0. {
             return RGBA::transparent();
         }
-        let a = f32::min(a, 1.);
+        let a = a.min(1.);
 
-        let inverse_a = 1. / a;
-        let r = (p1 * r1 + p2 * r2) * inverse_a;
-        let g = (p1 * g1 + p2 * g2) * inverse_a;
-        let b = (p1 * b1 + p2 * b2) * inverse_a;
-        return RGBA::from_floats(r, g, b, a);
+        let inv = 1. / a;
+
+        let r = (p1 * r1 + p2 * r2) * inv;
+        let g = (p1 * g1 + p2 * g2) * inv;
+        let b = (p1 * b1 + p2 * b2) * inv;
+        RGBA::from_floats(r, g, b, a)
     }
 }
 
@@ -74,11 +80,13 @@ impl ToCss for Color {
     where
         W: fmt::Write,
     {
-        match *self {
-            GenericColor::Numeric(color) => color.to_css(dest),
-            GenericColor::CurrentColor => CSSParserColor::CurrentColor.to_css(dest),
-            _ => Ok(()),
+        if self.is_currentcolor() {
+            return CSSParserColor::CurrentColor.to_css(dest);
         }
+        if self.is_numeric() {
+            return self.color.to_css(dest);
+        }
+        Ok(())
     }
 }
 

servo/components/style/values/generics/color.rs

@@ -6,6 +6,11 @@
 
 /// Ratios representing the contribution of color and currentcolor to
 /// the final color value.
+///
+/// NOTE(emilio): For animated colors, the sum of these two might be more than
+/// one (because the background color would've been scaled down already). So
+/// beware that it is not generally safe to assume that if bg is 1 then fg is 0,
+/// for example.
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToAnimatedValue, ToShmem)]
 #[repr(C)]
 pub struct ComplexColorRatios {
@@ -22,59 +27,52 @@ impl ComplexColorRatios {
     pub const CURRENT_COLOR: ComplexColorRatios = ComplexColorRatios { bg: 0., fg: 1. };
 }
 
-/// This enum represents a combined color from a numeric color and
+/// This struct represents a combined color from a numeric color and
 /// the current foreground color (currentcolor keyword).
 #[derive(Clone, Copy, Debug, MallocSizeOf, PartialEq, ToAnimatedValue, ToShmem)]
-#[repr(C, u8)]
-pub enum GenericColor<RGBA> {
-    ///  Numeric RGBA color.
-    Numeric(RGBA),
-
-    /// The current foreground color.
-    CurrentColor,
-
-    /// A linear combination of numeric color and currentcolor.
-    /// The formula is: `color * ratios.bg + currentcolor * ratios.fg`.
-    Complex {
+#[repr(C)]
+pub struct GenericColor<RGBA> {
     /// The actual numeric color.
-        color: RGBA,
+    pub color: RGBA,
     /// The ratios of mixing between numeric and currentcolor.
-        ratios: ComplexColorRatios,
-    },
+    /// The formula is: `color * ratios.bg + currentcolor * ratios.fg`.
+    pub ratios: ComplexColorRatios,
 }
 
 pub use self::GenericColor as Color;
 
+impl Color<cssparser::RGBA> {
+    /// Returns a color value representing currentcolor.
+    pub fn currentcolor() -> Self {
+        Color {
+            color: cssparser::RGBA::transparent(),
+            ratios: ComplexColorRatios::CURRENT_COLOR,
+        }
+    }
+}
+
 impl<RGBA> Color<RGBA> {
     /// Create a color based upon the specified ratios.
-    pub fn with_ratios(color: RGBA, ratios: ComplexColorRatios) -> Self {
-        if ratios == ComplexColorRatios::NUMERIC {
-            Color::Numeric(color)
-        } else if ratios == ComplexColorRatios::CURRENT_COLOR {
-            Color::CurrentColor
-        } else {
-            Color::Complex { color, ratios }
-        }
+    pub fn new(color: RGBA, ratios: ComplexColorRatios) -> Self {
+        Self { color, ratios }
     }
 
     /// Returns a numeric color representing the given RGBA value.
     pub fn rgba(color: RGBA) -> Self {
-        Color::Numeric(color)
+        Self {
+            color,
+            ratios: ComplexColorRatios::NUMERIC,
         }
-
-    /// Returns a complex color value representing currentcolor.
-    pub fn currentcolor() -> Self {
-        Color::CurrentColor
     }
 
     /// Whether it is a numeric color (no currentcolor component).
     pub fn is_numeric(&self) -> bool {
-        matches!(*self, Color::Numeric(..))
+        self.ratios == ComplexColorRatios::NUMERIC
     }
 
     /// Whether it is a currentcolor value (no numeric color component).
     pub fn is_currentcolor(&self) -> bool {
-        matches!(*self, Color::CurrentColor)
+        self.ratios == ComplexColorRatios::CURRENT_COLOR
     }
 }
 

servo/components/style/values/resolved/color.rs

@@ -20,7 +20,7 @@ impl ToResolvedValue for computed::Color {
 
     #[inline]
     fn from_resolved_value(resolved: Self::ResolvedValue) -> Self {
-        generics::Color::Numeric(resolved)
+        generics::Color::rgba(resolved)
     }
 }
 
@@ -33,7 +33,7 @@ impl ToResolvedValue for computed::ColorOrAuto {
     fn to_resolved_value(self, context: &Context) -> Self::ResolvedValue {
         let color = match self {
             generics::ColorOrAuto::Color(color) => color,
-            generics::ColorOrAuto::Auto => generics::Color::CurrentColor,
+            generics::ColorOrAuto::Auto => generics::Color::currentcolor(),
         };
         color.to_resolved_value(context)
     }

servo/components/style/values/specified/color.rs

@@ -9,7 +9,7 @@ use super::AllowQuirks;
 use crate::gecko_bindings::structs::nscolor;
 use crate::parser::{Parse, ParserContext};
 use crate::values::computed::{Color as ComputedColor, Context, ToComputedValue};
-use crate::values::generics::color::{Color as GenericColor, ColorOrAuto as GenericColorOrAuto};
+use crate::values::generics::color::{ColorOrAuto as GenericColorOrAuto};
 use crate::values::specified::calc::CalcNode;
 use cssparser::{AngleOrNumber, Color as CSSParserColor, Parser, Token, RGBA};
 use cssparser::{BasicParseErrorKind, NumberOrPercentage, ParseErrorKind};
@@ -585,11 +585,13 @@ impl ToComputedValue for Color {
     }
 
     fn from_computed_value(computed: &ComputedColor) -> Self {
-        match *computed {
-            GenericColor::Numeric(color) => Color::rgba(color),
-            GenericColor::CurrentColor => Color::currentcolor(),
-            GenericColor::Complex { .. } => Color::Complex(*computed),
+        if computed.is_numeric() {
+            return Color::rgba(computed.color);
         }
+        if computed.is_currentcolor() {
+            return Color::currentcolor();
+        }
+        Color::Complex(*computed)
     }
 }
 

servo/ports/geckolib/cbindgen.toml

@@ -490,6 +490,7 @@ renaming_overrides_prefixing = true
 """
 
 "GenericColor" = """
+  static inline StyleGenericColor CurrentColor();
   static inline StyleGenericColor FromColor(nscolor);
   static inline StyleGenericColor Black();
   static inline StyleGenericColor White();