Closed Bug 1457353 Opened 7 years ago Closed 7 years ago

nsStyleSVGReset: Replace nsColor with StyleComplexColor

Categories

(Core :: CSS Parsing and Computation, defect, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1457353 - P1: Change nscolor to StyleComplexColor.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457353 - P2: Simple reftests.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1457353 - P3: Disable failing SMIL tests.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
No description provided.
Assignee: nobody → dglastonbury
Blocks: 760345
Status: NEW → ASSIGNED
Attachment #8971479 - Flags: review?(cam) → review?(xidorn+moz)
Attachment #8971480 - Flags: review?(cam) → review?(xidorn+moz)
Comment on attachment 8971478 [details] Bug 1457353 - P1: Change nscolor to StyleComplexColor. https://reviewboard.mozilla.org/r/240238/#review246242 ::: layout/style/nsStyleStruct.cpp:1332 (Diff revision 1) > +// If color `c` is just a numeric color, we can skip resolving > +// `StyleColor()`. > +#define CALC_COMPLEX_COLOR(c) \ > + c.IsNumericColor() ? c.mColor : aStyle->StyleColor()->CalcComplexColor(c) I don't think resolving `StyleColor` is that bad. Let's don't bother optimizing this for now unless it really turns out to be a problem. We probably want to move `CalcComplexColor` into `ComputedStyle` instead at some point, so that we can do the right thing everywhere instead of requiring every callsites to have this kind of micro optimization. Especially given that we may want to just embed `color` into `ComputedStyle` directly (see bug 1453224). Could you just remove the optimization and file a followup bug for moving around `CalcComplexColor`? And, with the `IsNumericColor` check removed, I don't think it's really worth much to have methods for them. Could you just inline `CalcComplexColor` call to the callsites?
Attachment #8971478 - Flags: review?(xidorn+moz)
Comment on attachment 8971479 [details] Bug 1457353 - P2: Stylo changes due to StyleComplexColor change. https://reviewboard.mozilla.org/r/240240/#review246244 ::: servo/components/style/properties/longhand/svg.mako.rs:25 (Diff revision 1) > // Section 13 - Gradients and Patterns > > ${helpers.predefined_type( > "stop-color", > - "RGBAColor", > - "RGBA::new(0, 0, 0, 255)", > + "Color", > + "computed_value::T::rgba(RGBA::new(0, 0, 0, 255))", `RGBA::new(0, 0, 0, 255).into()` should be enough here. ::: servo/components/style/properties/longhand/svg.mako.rs:41 (Diff revision 1) > // Section 15 - Filter Effects > > ${helpers.predefined_type( > "flood-color", > - "RGBAColor", > - "RGBA::new(0, 0, 0, 255)", > + "Color", > + "computed_value::T::rgba(RGBA::new(0, 0, 0, 255))", ditto. ::: servo/components/style/properties/longhand/svg.mako.rs:54 (Diff revision 1) > spec="https://www.w3.org/TR/SVG/filters.html#FloodOpacityProperty")} > > ${helpers.predefined_type( > "lighting-color", > - "RGBAColor", > - "RGBA::new(255, 255, 255, 255)", > + "Color", > + "computed_value::T::rgba(RGBA::new(255, 255, 255, 255))", ditto.
Attachment #8971479 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8971478 [details] Bug 1457353 - P1: Change nscolor to StyleComplexColor. https://reviewboard.mozilla.org/r/240238/#review246248 ::: commit-message-63a0e:1 (Diff revision 1) > +Bug 1457353 - P1: Change nscolor to StyleComplexColor. r?xidorn > + > +Change mStopColor, mFloodColor, and mLightingColor in nsStyleSVGReset. Also, I believe this should be squashed with P2, because I don't think this part can build as itself.
Comment on attachment 8971480 [details] Bug 1457353 - P2: Simple reftests. https://reviewboard.mozilla.org/r/240242/#review246250
Attachment #8971480 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8971478 [details] Bug 1457353 - P1: Change nscolor to StyleComplexColor. https://reviewboard.mozilla.org/r/240238/#review246248 > Also, I believe this should be squashed with P2, because I don't think this part can build as itself. Also you may need to update test_transitions_per_property.html to reflect that these properties now transition with currentcolor with the new approach. I suspect test_transitions_per_property.html should be failing with your patch here, in that case the test should be updated in this part, otherwise it can be updated in the testing part.
Summary: nsSVGStyleReset: Replace nsColor with StyleComplexColor → nsStyleSVGReset: Replace nsColor with StyleComplexColor
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7) > Also, I believe this should be squashed with P2, because I don't think this > part can build as itself. I split out the servo changes into a separate patch just in case they needed to be landed separately. Will squash into one patch.
Attachment #8971479 - Attachment is obsolete: true
Comment on attachment 8971478 [details] Bug 1457353 - P1: Change nscolor to StyleComplexColor. https://reviewboard.mozilla.org/r/240238/#review246308 Looks good, thanks.
Attachment #8971478 - Flags: review?(xidorn+moz) → review+
Priority: -- → P3
Depends on: 1457810
Depends on: 1463603
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review252464 I haven't looked into this patch very closely, but given its complexity (and independency to this bug itself), I would suggest we do this in a separate bug. You can either make it a blocker of this bug, or land the other patches in this bug with the corresponding SMIL tests marked failing. I do have some suggestion to the patch below. ::: layout/style/StyleComplexColor.h:23 (Diff revision 1) > * Conceptually, the formula is "color * (1 - p) + currentcolor * p" > * where p is mForegroundRatio. See mozilla::LinearBlendColors for > * the actual algorithm. The formula would need update. ::: layout/style/StyleComplexColor.h:30 (Diff revision 1) > struct StyleComplexColor > { > nscolor mColor; > - uint8_t mForegroundRatio; > + float mColorRatio; > + float mForegroundRatio; > // Whether the complex color represents a computed-value time auto > // value. This is a flag indicating that this value should not be > // interpolatable with other colors. When this flag is set, other > // fields represent a currentcolor. Properties can decide whether > // that should be used. > bool mIsAuto; Given the structure of StyleComplexColor is becoming more complicated, it's probably better make all the fields private for better encapsulation, so that users of this type wouldn't accidently make assumption on the value in it. This may require moving some of the current user code into it, e.g. `IsOpaqueBorderEdge` may need something like `MayBeTransparent`. Also we may want to make this type be `class` rather than `struct` since the latter is usually used only for types which allows direct access to fields. (Google C++ Style Guide includes such suggestion, but it doesn't seem our coding style has it.) ::: layout/style/StyleComplexColor.cpp:28 (Diff revision 1) > - if (aFgRatio == 0) { > + if (aFgRatio == 0.) { > return aBg; > } > - if (aFgRatio == 255) { > + if (aBgRatio == 0.) { > return aFg; > } As I've mentioned on IRC, these fast paths are wrong. You need to also ensure the other ratio is one. ::: layout/style/StyleComplexColor.cpp:36 (Diff revision 1) > - auto r = BlendColorComponent(NS_GET_R(aBg), NS_GET_R(aFg), aFgRatio); > - auto g = BlendColorComponent(NS_GET_G(aBg), NS_GET_G(aFg), aFgRatio); > - auto b = BlendColorComponent(NS_GET_B(aBg), NS_GET_B(aFg), aFgRatio); > + auto bgRatio = NSToIntRound(aBgRatio * 255); > + auto fgRatio = NSToIntRound(aFgRatio * 255); > + auto r = BlendColorComponent(NS_GET_R(aBg), bgRatio, NS_GET_R(aFg), fgRatio); > + auto g = BlendColorComponent(NS_GET_G(aBg), bgRatio, NS_GET_G(aFg), fgRatio); > + auto b = BlendColorComponent(NS_GET_B(aBg), bgRatio, NS_GET_B(aFg), fgRatio); Maybe just have them multiply the ratio directly. Note that you also need clamp here. It wasn't a problem, but when the sum of ratio go beyond one, it would become a problem. ::: layout/style/StyleComplexColor.cpp:71 (Diff revision 1) > + // In majority of cases, background-color should just be a numeric color. > + // In that case, we can skip resolving StyleColor(). This comment shouldn't mention `background-color`. ::: layout/style/StyleComplexColor.cpp:73 (Diff revision 1) > + if (IsNumericColor()) { > + return mColor; > + } I guess we should probably just remove `IsNumericColor` and move (or duplicate) the fast path here.
Attachment #8980195 - Flags: review?(xidorn+moz)
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review253858 Thanks! I am happy that AnimatedColor finally can represent currentColor! Though I just reviewed only animation parts, it looks good. Would you mind adding some test cases in testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js, or file a followup bug for the test cases? ::: 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. ::: servo/components/style/values/animated/color.rs:148 (Diff revision 2) > // Note: this algorithm assumes self_portion + other_portion > // equals to one, so it may be broken for additive operation. > // To properly support additive color interpolation, we would > // need two ratio fields in computed color types. This comment is no longer needed? ::: servo/components/style/values/animated/color.rs:193 (Diff revision 2) > - // the final alpha value from the effective alpha value. > + // the final alpha value from the effective alpha value. > - let foreground_ratio = self.foreground_ratio > - .animate(&other.foreground_ratio, procedure)?; > - let alpha = color.alpha / (1. - foreground_ratio); > - Ok(Color { > - color: RGBA { > + let self_ratios = self.effective_ratios(); > + let other_ratios = other.effective_ratios(); > + let ratios = (self_ratios.0.animate(&other_ratios.0, procedure)?, > + self_ratios.1.animate(&other_ratios.1, procedure)?); > + let alpha = color.alpha / ratios.0; I am not convinced this is reasonable. I can imagine that 'ratio.0 + ratio.1 != 1', for additive case the result would be basically greater than 1, and theoritically would be an arbitary real number. Also, the ratio (bg/fg) might have been changed by above animate() functions because in the case of additive |procedure| isn't a linear function, I guess. So, this should be let alpha = color.alpha / (ratios.0 / (ratios.0 + ratios.1)) ? (And we should handle ratios.0 == 0 case?) But anyway, I am totally unsure what we should do for currentColor. So if Xidorn is fine with this, I am fine too. :) ::: 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.
Attachment #8980195 - Flags: review?(hikezoe) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #18) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Extend StyleComplexColor to support additive blending. > > https://reviewboard.mozilla.org/r/246350/#review252464 > > I haven't looked into this patch very closely, but given its complexity (and > independency to this bug itself), I would suggest we do this in a separate > bug. You can either make it a blocker of this bug, or land the other patches > in this bug with the corresponding SMIL tests marked failing. The main reason I suggested you to put it in a separate bug is to have this land separately from the other patches here. I hope this patch to land separately because it contains certain risk itself (as for any refactoring), and having them separate makes it easier to bisect when regression happens, and also allow us to move some part forward even if we have to backout the other. But if you insist on landing everything in this bug... I guess I'm fine as well.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #23) > The main reason I suggested you to put it in a separate bug is to have this > land separately from the other patches here. I hope this patch to land > separately because it contains certain risk itself (as for any refactoring), > and having them separate makes it easier to bisect when regression happens, > and also allow us to move some part forward even if we have to backout the > other. Oh, I missed this before. Excellent suggestion.
Moved P3 to Bug 1465307
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review253872 ::: layout/style/StyleComplexColor.h:34 (Diff revision 2) > - // value. This is a flag indicating that this value should not be > + // value. This is a flag indicating that this value should not be > - // interpolatable with other colors. When this flag is set, other > + // interpolatable with other colors. When this flag is set, other > - // fields represent a currentcolor. Properties can decide whether > + // fields represent a currentcolor. Properties can decide whether > - // that should be used. > + // that should be used. s/flag/tag? ::: layout/style/StyleComplexColor.h:40 (Diff revision 2) > - // interpolatable with other colors. When this flag is set, other > + // interpolatable with other colors. When this flag is set, other > - // fields represent a currentcolor. Properties can decide whether > + // fields represent a currentcolor. Properties can decide whether > - // that should be used. > + // that should be used. > - bool mIsAuto; > + eAuto, > + // Whether the complex color represents a numeric color; no > + // currentColor component. `currentcolor` is preferred over `currentColor` (different in case of 'c'), here and below. The latter is the old name of the keyword, and the spec has been changed to make all lowercase the canonical form. ::: layout/style/StyleComplexColor.h:44 (Diff revision 2) > + // Whether the complex color represents a numeric color; no > + // currentColor component. > + eNumeric, > + // Whether the complex color represents the foreground color, > + // currentColor; no numeric color component. > + eCurrent, `Current` feels a bit vague on what it should be... Probably `Foreground` instead. ::: 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? ::: layout/style/StyleComplexColor.h:70 (Diff revision 2) > - return mForegroundRatio == aOther.mForegroundRatio && > - (IsCurrentColor() || mColor == aOther.mColor) && > - mIsAuto == aOther.mIsAuto; > + if (mTag != aOther.mTag) { > + return false; > + } > + > + switch (mTag) { > + default: I would prefer you write down explicitly the two tags here. ::: layout/style/nsStyleStruct.cpp:4321 (Diff revision 2) > { > MOZ_COUNT_CTOR(nsStyleTextReset); > } > > nsStyleTextReset::nsStyleTextReset(const nsStyleTextReset& aSource) > + : mTextDecorationLine(aSource.mTextDecorationLine) You forget to clone `mTextOverflow`. ::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:58 (Diff revision 2) > - StyleComplexColor { > + StyleComplexColor { > - mColor: convert_rgba_to_nscolor(&other.color).into(), > - mForegroundRatio: other.foreground_ratio, > - mIsAuto: false, > + mColor: convert_rgba_to_nscolor(&color).into(), > + mBgRatio: ratios.0, > + mFgRatio: ratios.1, > + mTag: StyleComplexColor_Tag_eComplex, > + }, Maybe worth adding a `debug_assert` that `mBgRatio` and `mFgRatio` is not `(0, 1)` or `(1, 0)`? ::: servo/components/style/gecko_bindings/sugar/style_complex_color.rs:71 (Diff revision 2) > - debug_assert!(!other.mIsAuto); > - ComputedColor { > + match other.mTag { > + StyleComplexColor_Tag_eNumeric => > + ComputedColor::Numeric { > + color: convert_nscolor_to_rgba(other.mColor) > + }, > + StyleComplexColor_Tag_eCurrent => > + ComputedColor::Current, > + StyleComplexColor_Tag_eComplex => > + ComputedColor::Complex { > - color: convert_nscolor_to_rgba(other.mColor), > + color: convert_nscolor_to_rgba(other.mColor), > - foreground_ratio: other.mForegroundRatio, > + ratios: (other.mBgRatio, other.mFgRatio), > + }, Maybe add a `debug_assert` in each branch to check that other fields match what the tag says. ::: servo/components/style/values/animated/color.rs:133 (Diff revision 2) > - > - fn is_numeric(&self) -> bool { > - self.foreground_ratio <= 0. > } > > - fn effective_intermediate_rgba(&self) -> RGBA { > + fn effective_ratios(&self) -> (f32, f32) { If you want to combine the two ratios, you should have a struct for it rather than a tuple. That would make code easier to read.
Attachment #8980195 - Flags: review?(xidorn+moz)
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review253858 > 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 agree with hiro that we shouldn't be using tuple here. Either splitting it into two fields, or having a struct for two ratios would be much better.
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review253872 > 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? Maybe not worth... your call.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #27) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Extend StyleComplexColor to support additive blending. > > https://reviewboard.mozilla.org/r/246350/#review253858 > > > 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 agree with hiro that we shouldn't be using tuple here. Either splitting it > into two fields, or having a struct for two ratios would be much better. Yeah, a new struct sounds nicer. :)
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review253858 > I am not convinced this is reasonable. I can imagine that 'ratio.0 + ratio.1 != 1', for additive case the result would be basically greater than 1, and theoritically would be an arbitary real number. Also, the ratio (bg/fg) might have been changed by above animate() functions because in the case of additive |procedure| isn't a linear function, I guess. So, this should be > > let alpha = color.alpha / (ratios.0 / (ratios.0 + ratios.1)) > > ? > > (And we should handle ratios.0 == 0 case?) > > But anyway, I am totally unsure what we should do for currentColor. So if Xidorn is fine with this, I am fine too. :) So, given the original code, I think this is a reasonable conversion. `color.alpha / (ratios.0 / (ratios.0 + ratios.1))` doesn't seem to be consistent with the other blend function, so I would lean towards `color.alpha / ratios.0`. I agree that we may need to handle the case of `ratios.0 == 0`, but that looks like an existing issue which we can probably fix separately?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Extend StyleComplexColor to support additive blending. > > https://reviewboard.mozilla.org/r/246350/#review253858 > > > I am not convinced this is reasonable. I can imagine that 'ratio.0 + ratio.1 != 1', for additive case the result would be basically greater than 1, and theoritically would be an arbitary real number. Also, the ratio (bg/fg) might have been changed by above animate() functions because in the case of additive |procedure| isn't a linear function, I guess. So, this should be > > > > let alpha = color.alpha / (ratios.0 / (ratios.0 + ratios.1)) > > > > ? > > > > (And we should handle ratios.0 == 0 case?) > > > > But anyway, I am totally unsure what we should do for currentColor. So if Xidorn is fine with this, I am fine too. :) > > So, given the original code, I think this is a reasonable conversion. > `color.alpha / (ratios.0 / (ratios.0 + ratios.1))` doesn't seem to be > consistent with the other blend function, so I would lean towards > `color.alpha / ratios.0`. OK, that's fair.
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. ::: servo/components/style/values/computed/color.rs:80 (Diff revision 2) > - let bg_ratio = (u8::max_value() - fg_alpha) as u32; > - let fg_ratio = fg_alpha as u32; > - let color = bg as u32 * bg_ratio + fg as u32 * fg_ratio; > + let bg_ratio = ratios.0; > + let fg_ratio = ratios.1; > + let color = bg as f32 * bg_ratio + fg as f32 * fg_ratio; > // Rounding divide the number by 255 > - ((color + 127) / 255) as u8 > + ((color.max(0.).min(255.) + 127.) / 255.) as u8 This looks wrong... In the original version of this function, the ratio is an `u8`, which has the meaning of `fg_alpha as f32 / 255.`, so after you multiply the two `u8`s, you need to divide `255` for converting it back. However, here the ratios are already `f32` in `[0, 1]`, so you should just round and clamp the result. ::: servo/components/style/values/computed/color.rs:91 (Diff revision 2) > > + let (color, ratios) = match self { > + // Common cases that the complex color is either pure numeric > + // color or pure currentcolor. > + Color::Numeric { color } => return *color, > + Color::Current => return fg_color.clone(), The `.clone()` here is redundant. You are returning something you have, it should be fine. ::: servo/components/style/values/computed/color.rs:96 (Diff revision 2) > // Common case that alpha channel is equal (usually both are opaque). > - let fg_ratio = self.foreground_ratio; > - if self.color.alpha == fg_color.alpha { > - let r = blend_color_component(self.color.red, fg_color.red, fg_ratio); > - let g = blend_color_component(self.color.green, fg_color.green, fg_ratio); > + if color.alpha == fg_color.alpha { > + let r = blend_color_component(color.red, fg_color.red, ratios); > + let g = blend_color_component(color.green, fg_color.green, ratios); > + let b = blend_color_component(color.blue, fg_color.blue, ratios); > - let b = blend_color_component(self.color.blue, fg_color.blue, fg_ratio); > return RGBA::new(r, g, b, fg_color.alpha); > } So it seems to me you cannot do this optimization like how it was handled here, given that the sum of the two ratios can exceed one. Probably you should just remove this optimization like what you did in the C++ version. ::: servo/components/style/values/computed/color.rs:137 (Diff revision 2) > fn eq(&self, other: &Color) -> bool { > - self.foreground_ratio == other.foreground_ratio && > - (self.is_currentcolor() || self.color == other.color) > + match (self, other) { > + (Color::Numeric { color: c1 }, Color::Numeric { color: c2 }) > + => c1 == c2, > + (Color::Current, Color::Current) > + => true, > + (Color::Complex { color: c1, ratios: r1 }, > + Color::Complex { color: c2, ratios: r2 }) => > + r1 == r2 && c1 == c2, > + _ => false > + } > } This is a derivable implementation of `PartialEq`. Just remove this `impl` block and add `PartialEq` into `#[derive]` of `Color` should be enough. ::: servo/components/style/values/computed/color.rs:170 (Diff revision 2) > impl ToAnimatedValue for Color { > type AnimatedValue = AnimatedColor; > > #[inline] > fn to_animated_value(self) -> Self::AnimatedValue { > - AnimatedColor { > - color: self.color.to_animated_value(), > - foreground_ratio: self.foreground_ratio as f32 * (1. / 255.), > + match self { > + Color::Numeric { color } => AnimatedColor::Numeric { > + color: color.to_animated_value() > + }, > + Color::Current => AnimatedColor::Current, > + Color::Complex { color, ratios, } => AnimatedColor::Complex { > + color: color.to_animated_value(), > + ratios, > + }, > } > } > > #[inline] > fn from_animated_value(animated: Self::AnimatedValue) -> Self { > - Color { > - color: RGBA::from_animated_value(animated.color), > - foreground_ratio: (animated.foreground_ratio * 255.).round() as u8, > + match animated { > + AnimatedColor::Numeric { color } => > + Color::Numeric { > + color: RGBA::from_animated_value(color) > + }, > + AnimatedColor::Current => Color::Current, > + AnimatedColor::Complex { color, ratios } => > + Color::Complex { > + color: RGBA::from_animated_value(color), > + ratios > + }, > } > } > } If you follow the suggestion above to put `Color` into `generics` and make it generic over `RGBA`, the implementation of `ToAnimatedValue` should be deriveable.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Extend StyleComplexColor to support additive blending. > ::: layout/style/StyleComplexColor.h:44 > (Diff revision 2) > > + // Whether the complex color represents a numeric color; no > > + // currentColor component. > > + eNumeric, > > + // Whether the complex color represents the foreground color, > > + // currentColor; no numeric color component. > > + eCurrent, > > `Current` feels a bit vague on what it should be... Probably `Foreground` > instead. Does that mean a numeric color is a `background` color? > ::: 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? Should we check Complex == Numeric and Complex == Current? The old code checked equality for mForegroundRatio (ie. Numeric==Numeric, Current==Current or Complex==Complex) and then the other fields, which is what I've copied here. > ::: servo/components/style/values/animated/color.rs:133 > (Diff revision 2) > > - > > - fn is_numeric(&self) -> bool { > > - self.foreground_ratio <= 0. > > } > > > > - fn effective_intermediate_rgba(&self) -> RGBA { > > + fn effective_ratios(&self) -> (f32, f32) { > > If you want to combine the two ratios, you should have a struct for it > rather than a tuple. That would make code easier to read. At the time I wrote it, I considered making it a struct. The more I wrote .0 and .1 the more confusing it got.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #32) > Comment on attachment 8980195 [details] > Bug 1457353 - P3: Extend StyleComplexColor to support additive blending. > > This is a derivable implementation of `PartialEq`. Just remove this `impl` > block and add `PartialEq` into `#[derive]` of `Color` should be enough. If we derive `PartialEq` then it should match the same logic used in StyleComplexColor::operator==().
Comment on attachment 8980195 [details] Bug 1457353 - P3: Disable failing SMIL tests. https://reviewboard.mozilla.org/r/246350/#review254270 I'm okay with this, and I'm also okay with just comment out the testcases involving currentcolor from "color" directly. It seems that is still used by `fill`, but we may want to switch `fill` to use complex color as well. ::: dom/smil/test/db_smilCSSFromBy.js:65 (Diff revision 3) > + isServoEnabled ? "rgba(225, 225, 225, 0.8)" > + : "rgba(197, 197, 197, 0.8)", Please remove the non-servo branch. It is being removed in the last part of bug 1465107.
Attachment #8980195 - Flags: review?(xidorn+moz) → review+
Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f82bde34ccde P1: Change nscolor to StyleComplexColor. r=xidorn https://hg.mozilla.org/integration/autoland/rev/9ca706d68d3f P2: Simple reftests. r=xidorn https://hg.mozilla.org/integration/autoland/rev/76b492ce7828 P3: Disable failing SMIL tests. r=hiro,xidorn
https://hg.mozilla.org/mozilla-central/rev/f82bde34ccde https://hg.mozilla.org/mozilla-central/rev/9ca706d68d3f https://hg.mozilla.org/mozilla-central/rev/76b492ce7828
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: