Bug 1457353 exposed a change in color computation that causes SMIL <animate from="rgb(10,20,30)" by="currentColor" ... /> to fail.

> ::: servo/components/style/values/animated/color.rs:105 > (Diff revision 2) > > + Complex { > > + color: RGBA, > > + // ratios representing the contribution of color and > > + // currentColor to the final color, where: > > + // `(bgRatio, fgRatio) = self.ratios`. > > + ratios: (f32, f32) > > Should we split this tuple into two different variables? Actually during > reviewing this patch I don't see which one is for bg or fg. I've replaced the tuple with a struct with named components. I hope this makes it clean which ratio is related to what color.

> ::: servo/components/style/values/animated/color.rs:230 > (Diff revision 2) > > - self.foreground_ratio > > - .compute_squared_distance(&other.foreground_ratio)?) > > + self_ratios.0.compute_squared_distance(&other_ratios.0)? + > > + self_ratios.1.compute_squared_distance(&other_ratios.1)?) > > + }, > > Oh, I didn't know that we calcurate foreground_ratio as if it's in the same > distance space for color. Because it's possible to have the same `color` and `foreground_ratio` but different `background_ratio` I extended it. The `foreground_ratio` addition was already in the code and I can't comment on whether this is valid or not.

> ::: layout/style/StyleComplexColor.h:66 > (Diff revision 2) > > - bool IsNumericColor() const { return mForegroundRatio == 0; } > > - bool IsCurrentColor() const { return mForegroundRatio == 255; } > > + bool IsAuto() const { return mTag == eAuto; } > > + bool IsCurrentColor() const { return mTag == eCurrent; } > > > > bool operator==(const StyleComplexColor& aOther) const { > > - return mForegroundRatio == aOther.mForegroundRatio && > > - (IsCurrentColor() || mColor == aOther.mColor) && > > + if (mTag != aOther.mTag) { > > + return false; > > Maybe assert in the case when one is complex and the other is numeric or foreground, and they don't happen to have equal value? In discussion on IRC with :xidorn, decided to put assert into construction of `StyleComplexColor`s instead.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #32) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Disable failing SMIL tests. > > https://reviewboard.mozilla.org/r/246350/#review253892 > > ::: servo/components/style/values/animated/color.rs:97 > (Diff revision 2) > > -pub struct Color { > > - pub color: RGBA, > > - pub foreground_ratio: f32, > > +pub enum Color { > > + Numeric { color: RGBA }, > > + Current, > > + Complex { > > + color: RGBA, > > + // ratios representing the contribution of color and > > + // currentColor to the final color, where: > > + // `(bgRatio, fgRatio) = self.ratios`. > > + ratios: (f32, f32) > > + }, > > } > > Given you are bascially duplicating the enum in `animated` and `computed`, > you should move this enum into `generics` mod and make it generic over > `RGBA`, then alias it in the two mods. That would allow you to avoid lots of > duplicates below as well. > > Actually, I'm suspecting that we probably should move `RGBA` into `generics` > as well and make it generic over the field type. I moved the `Color` enum into `generics` and removed and common code from `animated::Color` and `computed::Color`. I didn't move `RGBA` because there's only one definition in this crate. The other `RGBA` comes from the separate `cssparser` crate.

Comment on attachment 8983304 [details] Bug 1465307 - P1: Extend StyleComplexColor to support additive blending. https://reviewboard.mozilla.org/r/249204/#review255362 Looks good to me other than From trait for ComplexColorRatios. Thanks! ::: servo/components/style/values/animated/color.rs:170 (Diff revision 1) > - foreground_ratio: self.foreground_ratio, > - }) > - } > - } else if self.is_currentcolor() && other.is_numeric() { > - Ok(Color { > - color: other.color, > + color: c1.animate(&c2, procedure)?, > + }), > + // Combinations of numeric color and currentColor > + (Color::Foreground, Color::Numeric { color }, _) => Ok(Color::Complex { > + color: color.clone(), > + ratios: (other_weight, this_weight).into(), Unfortunately, using From trait here makes this tuple unclear, which one is for bg or fg. So we should directly use ComplexColorRatios { bg: xx, fg: xx }? ::: servo/components/style/values/computed/color.rs:30 (Diff revision 1) > +impl From<(f32, f32)> for ComplexColorRatios { > + fn from(x: (f32, f32)) -> Self { > + ComplexColorRatios { bg: x.0, fg: x.1 } > + } > +} > > - /// The ratio of currentcolor in complex color. > - pub foreground_ratio: u8, > +impl From<(f64, f64)> for ComplexColorRatios { > + fn from(x: (f64, f64)) -> Self { > + ComplexColorRatios { > + bg: x.0 as f32, > + fg: x.1 as f32, > + } > + } > +} I think these Trait shouldn't be used.

Comment on attachment 8983304 [details] Bug 1465307 - P1: Extend StyleComplexColor to support additive blending. https://reviewboard.mozilla.org/r/249204/#review255664 Alright, looks mostly fine. Some small issues below. ::: layout/style/StyleComplexColor.h:29 (Diff revision 3) > - * the actual algorithm. > * > * It can also represent an "auto" value, which is valid for some > - * properties. See comment of mIsAuto. > + * properties. See comment of `Tag::eAuto`. > */ > -struct StyleComplexColor > +class StyleComplexColor This can probably be marked `final`. ::: layout/style/StyleComplexColor.h:32 (Diff revision 3) > - * properties. See comment of mIsAuto. > + * properties. See comment of `Tag::eAuto`. > */ > -struct StyleComplexColor > +class StyleComplexColor > { > - nscolor mColor; > - uint8_t mForegroundRatio; > +public: > + enum Tag : uint8_t { It seems to me nothing related to `Tag` is exposed, so maybe you can have this enum private as well. ::: layout/style/StyleComplexColor.h:33 (Diff revision 3) > */ > -struct StyleComplexColor > +class StyleComplexColor > { > - nscolor mColor; > - uint8_t mForegroundRatio; > +public: > + enum Tag : uint8_t { > - // Whether the complex color represents a computed-value time auto > + // Whether the complex color represents a computed-value time auto Since it is no longer a flag, it probably makes more sense to remove "Whether" from this comment and below. Something like "This represents a ..." is probably enough. ::: layout/style/StyleComplexColor.h:106 (Diff revision 3) > * the foreground color from aFrame's ComputedStyle. > */ > nscolor CalcColor(const nsIFrame* aFrame) const; > -}; > > +protected: For a final class (which cannot be inherited by anything), `protected` section doesn't make much sense. Please just move it to `private`. ::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:9 (Diff revision 3) > +use gecko_bindings::structs::{StyleComplexColor_Tag_eAuto, StyleComplexColor_Tag_eComplex, > + StyleComplexColor_Tag_eForeground, StyleComplexColor_Tag_eNumeric}; I think you can add `StyleComplexColor_Tag` into `rusty-enums` in `ServoBindings.toml`, then you can just do ```rust use gecko_bindings::structs::StyleComplexColor_Tag as Tag; ``` here, and code below can be much simplified. Maybe this would also avoid the warning you are working around below. ::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:52 (Diff revision 3) > } > > impl From<ComputedColor> for StyleComplexColor { > fn from(other: ComputedColor) -> Self { > + match other { > + ComputedColor::Numeric { color } => convert_rgba_to_nscolor(&color).into(), Maybe it would be better to just `impl From<RGBA> for StyleComplexColor`. ::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:95 (Diff revision 3) > + bg: other.mBgRatio, > + fg: other.mFgRatio, > + }, > + } > + } > + x => panic!("Unsupport StyleComplexColor with tag {}", x), I would prefer `unreachable` in this case over `panic`. Also with `StyleComplexColor_Tag` being a rusty enum, you should be able to explicitly list `Tag::eAuto` here instead. ::: servo/components/style/values/animated/color.rs:109 (Diff revision 3) > - pub color: RGBA, > - pub foreground_ratio: f32, > + Numeric { > + color: RGBA, > + }, > + Foreground, > + Complex { > + color: RGBA, > + ratios: ComplexColorRatios, > + }, I guess you can remove the names here like: ```rust pub enum Color { Numeric(RGBA), Foreground, Complex(RGBA, ComplexColorRatios), } ``` The types themselves should already be pretty clear what they are in this case, and thus the additional names don't seem to add much value. ::: servo/components/style/values/animated/color.rs:223 (Diff revision 3) > > impl ComputeSquaredDistance for Color { > #[inline] > fn compute_squared_distance(&self, other: &Self) -> Result<SquaredDistance, ()> { > // All comments from the Animate impl also applies here. > - if self.foreground_ratio == other.foreground_ratio { > + match (*self, *other) { Since you are returning `Ok` everywhere, you can probably have it wrap the `match` expression at the top level. It's not a big deal, so up to which way you would prefer. ::: servo/components/style/values/animated/color.rs:228 (Diff revision 3) > - } > - } else if self.is_currentcolor() && other.is_numeric() { > - Ok( > - RGBA::transparent().compute_squared_distance(&other.color)? + > - SquaredDistance::from_sqrt(1.), > - ) > + (Color::Foreground, Color::Numeric { color }) => Ok(RGBA::transparent() > + .compute_squared_distance(&color)? > + + SquaredDistance::from_sqrt(1.)), > + (Color::Numeric { color }, Color::Foreground) => Ok(color > + .compute_squared_distance(&RGBA::transparent())? > + + SquaredDistance::from_sqrt(1.)), These two branches can be merged together. `computed_squared_distance` should be symmetric. ::: servo/components/style/values/computed/color.rs:35 (Diff revision 3) > + Numeric { > + /// The color > + color: RGBA, > + }, > + > + /// The current foreground color. > + Foreground, > + > + /// A linear combination of numeric color and currentColor. > + /// The formula is: `color * bg_ratio + currentColor * fg_ratio`. > + Complex { > + /// The background color > + color: RGBA, > + /// The ratios of background and foreground colors in color. > + /// `(bg_ratio, fg_ratio) = ratios` > + ratios: ComplexColorRatios, > + }, As I mentioned above, just using tuples in this enum should be enough. (I still don't get why you cannot move it into generics, but I guess there are reasons. I can probably try it myself afterwards and see why that's going to be a problem.) ::: servo/components/style/values/computed/color.rs:78 (Diff revision 3) > - self.foreground_ratio == 0 > + match *self { > + Color::Numeric { .. } => true, > + _ => false, > + } `matches!(*self, Color::Numeric { .. })` ::: servo/components/style/values/computed/color.rs:86 (Diff revision 3) > - self.foreground_ratio == u8::max_value() > + match *self { > + Color::Foreground => true, > + _ => false, > + } `matches!(*self, Color::Foreground)` ::: servo/components/style/values/computed/color.rs:125 (Diff revision 3) > > let a = p1 * a1 + p2 * a2; > - if a == 0.0 { > + if a <= 0. { > return RGBA::transparent(); > } > + let a = if a > 1. { 1. } else { a }; Maybe `f32::min(a, 1.)`.

Comment on attachment 8983305 [details] Bug 1465307 - P2: Fix nsStyleBorder::mBorderColor for GCC. https://reviewboard.mozilla.org/r/249206/#review255672 Ideally this should be moved to the first patch so that all builds work even between the patches. But it's not a big deal.

Comment on attachment 8983304 [details] Bug 1465307 - P1: Extend StyleComplexColor to support additive blending. https://reviewboard.mozilla.org/r/249204/#review255664 > As I mentioned above, just using tuples in this enum should be enough. > > (I still don't get why you cannot move it into generics, but I guess there are reasons. I can probably try it myself afterwards and see why that's going to be a problem.) Oh, it seems you are moving `Color` to `generics` in a later patch.

Comment on attachment 8983306 [details] Bug 1465307 - P3: Extract {animated,computed}::Color common parts. https://reviewboard.mozilla.org/r/249208/#review255676 There are several issues in part 1 you may want to address first, and this has some issues as well, so canceling review for now. ::: servo/components/style/values/computed/color.rs:12 (Diff revision 3) > use cssparser::{Color as CSSParserColor, RGBA}; > use std::fmt; > use style_traits::{CssWriter, ToCss}; > use values::animated::ToAnimatedValue; > -use values::animated::color::{Color as AnimatedColor, RGBA as AnimatedRGBA}; > - > +use values::animated::color::RGBA as AnimatedRGBA; > +use values::generics::color::{Color as GenericColor, Complex, Foreground, Numeric, Transparent}; You can do ```rust use values::generics::color as generics; ``` then in the rest of places use `generics::Color`. ::: servo/components/style/values/generics/color.rs:9 (Diff revision 3) > +/// A trait to obtain the value of the transparent color for a color representation. > +pub trait Transparent { > + /// Obtain the transparent color. > + fn transparent() -> Self; > +} I don't think this is necessary. It seems that it is only used in the `transparent` function below, not anything else. I think you can just implement the `transparent` functions for the corresponding concrete color types, rather than adding a trait and having it in the generic type. ::: servo/components/style/values/generics/color.rs:32 (Diff revision 3) > + pub const NUMERIC: ComplexColorRatios = ComplexColorRatios { bg: 1., fg: 0. }; > + /// Ratios representing the `Foreground` color. > + pub const FOREGROUND: ComplexColorRatios = ComplexColorRatios { bg: 0., fg: 1. }; > +} > + > +pub use self::Color::*; I don't think we general do this at top mod level. Please avoid it. ::: servo/components/style/values/generics/color.rs:110 (Diff revision 3) > +impl<T> ToAnimatedValue for Color<T> > +where > + T: ToAnimatedValue, I don't see why you need to implement this manually. This seems pretty deriveable. If it complains about `ComplexColorRatios`, I think you can derive `ToAnimatedValue` for that as well.

Comment on attachment 8983307 [details] Bug 1465307 - P4: Enable disabled dom/smil lighting_color tests. https://reviewboard.mozilla.org/r/249210/#review255678

Comment on attachment 8983304 [details] Bug 1465307 - P1: Extend StyleComplexColor to support additive blending. https://reviewboard.mozilla.org/r/249204/#review255664 > I think you can add `StyleComplexColor_Tag` into `rusty-enums` in `ServoBindings.toml`, then you can just do > ```rust > use gecko_bindings::structs::StyleComplexColor_Tag as Tag; > ``` > here, and code below can be much simplified. > > Maybe this would also avoid the warning you are working around below. Thanks. I didn't about that feature in ServoBindings.toml.

Comment on attachment 8983304 [details] Bug 1465307 - P1: Extend StyleComplexColor to support additive blending. https://reviewboard.mozilla.org/r/249204/#review255664 > `matches!(*self, Color::Numeric { .. })` Thanks. I didn't know about that macro.

Comment on attachment 8983306 [details] Bug 1465307 - P3: Extract {animated,computed}::Color common parts. https://reviewboard.mozilla.org/r/249208/#review255676 > I don't think this is necessary. > > It seems that it is only used in the `transparent` function below, not anything else. > > I think you can just implement the `transparent` functions for the corresponding concrete color types, rather than adding a trait and having it in the generic type. I added that when I started making RGBA generic and then forgot to remove when I abandoned that. > I don't think we general do this at top mod level. > > Please avoid it. I was trying to get around the weird behaviour of rust where the pattern matching refers to the generic type name: ```rust impl From<ComputedColor> for StyleComplexColor { fn from(other: ComputedColor) -> Self { match other { // Matching a ComputedColor but have to specify GenericColor GenericColor::Numeric(color) => color.into(), GenericColor::Foreground => Self::current_color(), GenericColor::Complex(color, ratios) => { ... } } } }``` If I import the generic names using `use values::generics::color as generics`, as you suggest, it would become: ```rust impl From<ComputedColor> for StyleComplexColor { fn from(other: ComputedColor) -> Self { match other { // Matching a ComputedColor but have to specify generics::Color generics::Color::Numeric(color) => color.into(), generics::Color::Foreground => Self::current_color(), generics::Color::Complex(color, ratios) => { ... } } } }``` which is gross. Would you be okay with: ```rust impl From<ComputedColor> for StyleComplexColor { fn from(other: ComputedColor) -> Self { use GenericColor::*; match other { Numeric(color) => color.into(), Foreground => Self::current_color(), Complex(color, ratios) => { ... } } } }```

Comment on attachment 8983306 [details] Bug 1465307 - P3: Extract {animated,computed}::Color common parts. https://reviewboard.mozilla.org/r/249208/#review255676 > I was trying to get around the weird behaviour of rust where the pattern matching refers to the generic type name: > ```rust > impl From<ComputedColor> for StyleComplexColor { > fn from(other: ComputedColor) -> Self { > match other { > // Matching a ComputedColor but have to specify GenericColor > GenericColor::Numeric(color) => color.into(), > GenericColor::Foreground => Self::current_color(), > GenericColor::Complex(color, ratios) => { > ... > } > } > } > }``` > > If I import the generic names using `use values::generics::color as generics`, as you suggest, it would become: > ```rust > impl From<ComputedColor> for StyleComplexColor { > fn from(other: ComputedColor) -> Self { > match other { > // Matching a ComputedColor but have to specify generics::Color > generics::Color::Numeric(color) => color.into(), > generics::Color::Foreground => Self::current_color(), > generics::Color::Complex(color, ratios) => { > ... > } > } > } > }``` > > which is gross. > > Would you be okay with: > ```rust > impl From<ComputedColor> for StyleComplexColor { > fn from(other: ComputedColor) -> Self { > use GenericColor::*; > match other { > Numeric(color) => color.into(), > Foreground => Self::current_color(), > Complex(color, ratios) => { > ... > } > } > } > }``` https://github.com/rust-lang/rust/issues/26264

Comment on attachment 8983306 [details] Bug 1465307 - P3: Extract {animated,computed}::Color common parts. https://reviewboard.mozilla.org/r/249208/#review255676 > You can do > ```rust > use values::generics::color as generics; > ``` > then in the rest of places use `generics::Color`. Decided not to do this as you'll see in the adjustments due to other comments.

Comment on attachment 8983306 [details] Bug 1465307 - P3: Extract {animated,computed}::Color common parts. https://reviewboard.mozilla.org/r/249208/#review256030 ::: servo/components/style/values/animated/color.rs:105 (Diff revision 4) > > Ok(ComplexColorRatios { bg, fg }) > } > } > > #[allow(missing_docs)] It'd better have a document here. It can just be something like ``` /// An animated value for `<color>` ``` ::: servo/components/style/values/computed/color.rs:20 (Diff revision 4) > -impl Color { > - /// Returns a numeric color representing the given RGBA value. > +/// This enum represents a combined color from a numeric color and > +/// the current foreground color (currentcolor keyword). This is no longer an enum, just an alias now. Probably something like ``` /// A computed value for `<color>` ``` would be good enough.

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/76d098691ed0 P1: Extend StyleComplexColor to support additive blending. r=hiro,xidorn https://hg.mozilla.org/integration/autoland/rev/31c82145a8e7 P2: Fix nsStyleBorder::mBorderColor for GCC. r=xidorn https://hg.mozilla.org/integration/autoland/rev/85784a996d8b P3: Extract {animated,computed}::Color common parts. r=xidorn https://hg.mozilla.org/integration/autoland/rev/b34fa4440625 P4: Enable disabled dom/smil lighting_color tests. r=xidorn

https://hg.mozilla.org/mozilla-central/rev/76d098691ed0 https://hg.mozilla.org/mozilla-central/rev/31c82145a8e7 https://hg.mozilla.org/mozilla-central/rev/85784a996d8b https://hg.mozilla.org/mozilla-central/rev/b34fa4440625