Closed Bug 1299741 Opened 8 years ago Closed 8 years ago

Implement interpolation between numeric color and currentcolor

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox51 --- fix-optional
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: dev-doc-complete)

Attachments

(12 files, 6 obsolete files)

Bug 1299741 part 1 - Factor out ComputeColorDistance.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1299741 part 2 - Move ClampColor to nsColor.h.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1299741 part 10 - Make nsStyleContext.cpp:ExtractColor return a Maybe so that ExtractColorLenient can reuse that.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1299741 part 3 - Add LinearBlendColors function for linear blending two colors.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1299741 part 4 - Add StyleComplexColor type for storing color combining numeric color and currentcolor.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1299741 part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor.
58 bytes, text/x-review-board-request
dbaron
: review+
Details
Bug 1299741 part 12 - Add test for new currentcolor interpolation.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1299741 part 5 - Add css::RGBAColorData struct.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1299741 part 6 - Use RGBAColorData for color calculation in StyleAnimationValue.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1299741 part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue.
58 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1299741 part 8 - Change StyleDataAtOffset to template to simplify code.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1299741 part 9 - Implement interpolation between numeric color and currentcolor.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
      No description provided.
Assignee: nobody → xidorn+moz
Comment on attachment 8787236 [details]
Bug 1299741 part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor.

https://reviewboard.mozilla.org/r/75938/#review73994

::: layout/mathml/nsMathMLmfracFrame.cpp:631
(Diff revision 1)
>    
> -  nsCSSPropertyID colorProp = mFrame->StyleContext()->GetTextFillColorProp();
>    ColorPattern color(ToDeviceColor(
> -                                 mFrame->GetVisitedDependentColor(colorProp)));
> +    mFrame->GetVisitedDependentColor(eCSSProperty__webkit_text_fill_color)));
>   
>    // draw the slash as a parallelogram 

I'll remove the tailing spaces in these lines.
Comment on attachment 8787227 [details]
Bug 1299741 part 1 - Factor out ComputeColorDistance.

https://reviewboard.mozilla.org/r/75920/#review74264

::: layout/style/StyleAnimationValue.cpp:485
(Diff revision 1)
> +double
> +StyleAnimationValue::ComputeColorDistance(nscolor aStartColor,
> +                                          nscolor aEndColor)
> +{
> +    // http://www.w3.org/TR/smil-animation/#animateColorElement says
> +    // that we should use Euclidean RGB cube distance.  However, we
> +    // have to extend that to RGBA.  For now, we'll just use the
> +    // Euclidean distance in the (part of the) 4-cube of premultiplied
> +    // colors.

Nit: The body of this method appears to be indented four spaces instead of two.

::: layout/style/StyleAnimationValue.cpp:494
(Diff revision 1)
> +    // FIXME (spec): The CSS transitions spec doesn't say whether
> +    // colors are premultiplied, but things work better when they are,
> +    // so use premultiplication.  Spec issue is still open per
> +    // http://lists.w3.org/Archives/Public/www-style/2009Jul/0050.html

This is now specified so can we drop this comment?
Attachment #8787227 - Flags: review?(bbirtles) → review+
Comment on attachment 8787228 [details]
Bug 1299741 part 2 - Move ClampColor to nsColor.h.

https://reviewboard.mozilla.org/r/75922/#review74268

::: gfx/src/nsColor.h:38
(Diff revision 1)
>  #define NS_GET_G(_rgba) ((uint8_t) (((_rgba) >> 8) & 0xff))
>  #define NS_GET_B(_rgba) ((uint8_t) (((_rgba) >> 16) & 0xff))
>  #define NS_GET_A(_rgba) ((uint8_t) (((_rgba) >> 24) & 0xff))
>  
> +template<typename T>
> +inline uint_fast8_t NS_ClampColor(T aColor)

Let's not use the NS_ prefix.
Attachment #8787228 - Flags: review?(jmuizelaar) → review-
Comment on attachment 8787228 [details]
Bug 1299741 part 2 - Move ClampColor to nsColor.h.

https://reviewboard.mozilla.org/r/75922/#review74268

> Let's not use the NS_ prefix.

Would you prefer using mozilla:: or mozilla::gfx:: for this?
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> 
> Would you prefer using mozilla:: or mozilla::gfx:: for this?

Yeah, either is fine.

Also, I'm going to be away for a week starting tomorrow, so if you want reviews sooner you can probably get them from mstange.
Comment on attachment 8787229 [details]
Bug 1299741 part 3 - Invoke DiluteColor from AddWeightedColors so that we can reuse AddWeightedColors directly.

https://reviewboard.mozilla.org/r/75924/#review74272
Attachment #8787229 - Flags: review?(bbirtles) → review+
Comment on attachment 8787230 [details]
Bug 1299741 part 10 - Make nsStyleContext.cpp:ExtractColor return a Maybe so that ExtractColorLenient can reuse that.

https://reviewboard.mozilla.org/r/75926/#review74274
Attachment #8787230 - Flags: review?(bbirtles) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> Also, I'm going to be away for a week starting tomorrow, so if you want
> reviews sooner you can probably get them from mstange.

OK, thanks for the information. I guess you can continue reviewing the rest of patches r?you as they don't depend on other patches :)
Attachment #8787237 - Flags: review?(bbirtles)
Comment on attachment 8787237 [details]
Bug 1299741 part 10 - Implement interpolation between numeric color and currentcolor.

https://reviewboard.mozilla.org/r/75940/#review74276

::: layout/style/StyleAnimationValue.cpp:628
(Diff revision 1)
> +    }
> +    case eUnit_ComplexColor: {
> +      StyleComplexColor color1 = ExtractComplexColorValue(aStartValue);
> +      StyleComplexColor color2 = ExtractComplexColorValue(aEndValue);
> +
> +      // Common cases that interpolating between a color and a currentcolor.

Nit: Common case is interpolating...

::: layout/style/StyleAnimationValue.cpp:640
(Diff revision 1)
> +      // XXX Would we ever get here?
> +      // Compute distance based on colors premultipled with
> +      // the non-foreground ratio.
> +      auto alpha1 = FastDivideBy255(NS_GET_A(color1.mColor) *
> +                                    (255 - color1.mForegroundRatio));
> +      auto alpha2 = FastDivideBy255(NS_GET_A(color2.mColor) *
> +                                    (255 - color2.mForegroundRatio));
> +      double dist =
> +        ComputeColorDistance(NS_ColorWithAlpha(color1.mColor, alpha1),
> +                             NS_ColorWithAlpha(color2.mColor, alpha2));
> +      double diffRatio = (1.0 / 255.0) * color1.mForegroundRatio -
> +                         (1.0 / 255.0) * color2.mForegroundRatio;
> +      aDistance = sqrt(dist * dist + diffRatio * diffRatio);

I *think* we won't ever get here. The reason is I think we only ever end up with these complex colors (where the foreground ratio is neither 0 nor 255) after we've done interpolation. However, we only call ComputeDistance on values prior to interpolation. As far as I can tell, this is true for both SMIL and within dom/animation.

So, I think we can just drop this block of code and put an MOZ_ASSERT_UNREACHABLE here instead.

::: layout/style/StyleAnimationValue.cpp:2475
(Diff revision 1)
> +    }
> +    case eUnit_ComplexColor: {
> +      StyleComplexColor color1 = ExtractComplexColorValue(aValue1);
> +      StyleComplexColor color2 = ExtractComplexColorValue(aValue2);
> +      StyleComplexColor result;
> +      // Common cases that interpolating between a color and a currentcolor.

Nit: // Common case is interpolating...

::: layout/style/StyleAnimationValue.cpp:2483
(Diff revision 1)
> +        result.mForegroundRatio = NS_ClampColor(aCoeff2 * 255);
> +      } else if (color1.IsCurrentColor() && color2.IsNumericColor()) {
> +        result.mColor = color2.mColor;
> +        result.mForegroundRatio = NS_ClampColor(aCoeff1 * 255);
> +      } else {
> +        // XXX Would we ever get here?

Yes, I think we can, although I'm not sure if we do yet--it depends on how we're handling currentColor in SVG.

Basically, when we have additive animations you can interpolate with the underlying value. So you could have an animation stack like the following:

#0: [ currentColor -> numericA ]
#1: [ numericB -> numericC ]

In SMIL you can say that #1 is additive="sum". In Web Animations you can say either/both of numericB/numericC are additive.

So when we go to calculate the animation value at, say, progress = 0.5, we'll first interpolate #0 and produce a complex color with foreground ratio 128.

Then, in SMIL we'll interpolate numericB -> numericC, produce an intermediate numeric color then call StyleAnimationValue::Add with aCoeff1 and aCoeff2 set to 1.0 (this happens from the SandwichAdd call).

In Web Animations we're still in the middle of implementing this, but I believe we'll end up adding either/both of numericB/numericC before interpolating (depending on which is additive).

So, I think we need this code.
Attachment #8787237 - Flags: review+
Attachment #8787234 - Flags: review?(dbaron)
Comment on attachment 8787238 [details]
Bug 1299741 part 12 - Add reftest for currentcolor interpolation.

https://reviewboard.mozilla.org/r/76084/#review74282

I didn't calculate the color values by hand since I trust you did that, but otherwise this looks good.

::: layout/reftests/css-transitions/currentcolor-interpolation-1-ref.html:15
(Diff revision 1)
> +}
> +div {
> +  display: inline-block;
> +}
> +#test1 {
> +  color: #00fc00;

I don't think the result of interpolating |color| is actually observable here. Does it make sense to make that observable (e.g. through background-color: currentColor)? I guess not since that's not what we're testing? If not, perhaps we don't need these values here? (Although it doesn't hurt and I guess adds documentation)
Attachment #8787238 - Flags: review?(bbirtles) → review+
Attachment #8787235 - Flags: review?(dbaron)
Comment on attachment 8787237 [details]
Bug 1299741 part 10 - Implement interpolation between numeric color and currentcolor.

https://reviewboard.mozilla.org/r/75940/#review74276

> I *think* we won't ever get here. The reason is I think we only ever end up with these complex colors (where the foreground ratio is neither 0 nor 255) after we've done interpolation. However, we only call ComputeDistance on values prior to interpolation. As far as I can tell, this is true for both SMIL and within dom/animation.
> 
> So, I think we can just drop this block of code and put an MOZ_ASSERT_UNREACHABLE here instead.

I'd prefer keep this block of code and put an MOZ_ASSERT_UNREACHABLE here at the same time, so that even if the assertion fails, we can still have reasonable result. Also this code might be useful in the future when we do have a way to express intermediate value.
Attachment #8787231 - Attachment is obsolete: true
Attachment #8787231 - Flags: review?(jmuizelaar)
Attachment #8787238 - Attachment is obsolete: true
Comment on attachment 8787536 [details]
Bug 1299741 part 12 - Add test for new currentcolor interpolation.

https://reviewboard.mozilla.org/r/76262/#review74340

Looks good. Did we need to test rgba values too?
Attachment #8787536 - Flags: review?(bbirtles) → review+
Comment on attachment 8787536 [details]
Bug 1299741 part 12 - Add test for new currentcolor interpolation.

https://reviewboard.mozilla.org/r/76262/#review74340

Probably. But the percentage of alpha channel is pretty hard to predict. There are two levels of integer <-> floating-point conversion, so the precision isn't good enough to match manually calculating result, especially given that 255 is not divisible by 2 at all. It is probably testable in one level, though.
Comment on attachment 8787228 [details]
Bug 1299741 part 2 - Move ClampColor to nsColor.h.

https://reviewboard.mozilla.org/r/75922/#review74502

::: gfx/src/nsColor.h:40
(Diff revision 2)
>  #define NS_GET_A(_rgba) ((uint8_t) (((_rgba) >> 24) & 0xff))
>  
> +namespace mozilla {
> +
> +template<typename T>
> +inline uint_fast8_t ClampColor(T aColor)

Are you benefiting from using uint_fast8_t over uint8_t? If not, let's make this uint8_t, so that readers aren't required to look up the meaning of this type. (For example, I had never encountered this type before.)
Attachment #8787228 - Flags: review?(mstange) → review+
Comment on attachment 8787232 [details]
Bug 1299741 part 3 - Add LinearBlendColors function for linear blending two colors.

https://reviewboard.mozilla.org/r/75930/#review74506

I'm a little confused that aFgRatio is not a float between 0.0 and 1.0.
Attachment #8787232 - Flags: review?(mstange) → review+
Comment on attachment 8787233 [details]
Bug 1299741 part 6 - Make unit of StyleAnimationValue and nsCSSValue uint16_t and add an extra data field.

https://reviewboard.mozilla.org/r/75932/#review77124

::: layout/style/StyleAnimationValue.h:321
(Diff revision 2)
>      eUnit_UnparsedString // nsStringBuffer* (never null)
>    };
>  
>  private:
>    Unit mUnit;
> +  uint16_t mExtra;

I think you should have a comment explaining that this is an extra part of mValue.  The comment should point out that for any unit types where mExtra is meaningful, the operator== is responsible for comparing it.  However, it is always copied in operator= and the copy constructor.

It might even be worth calling it mValueExtra.

(And the same for the one in nsCSSValue.)
Attachment #8787233 - Flags: review?(dbaron) → review+
Comment on attachment 8787234 [details]
Bug 1299741 part 4 - Add StyleComplexColor type for storing color combining numeric color and currentcolor.

https://reviewboard.mozilla.org/r/75934/#review77126

::: layout/style/StyleComplexColor.h:16
(Diff revision 2)
> +// A struct represents a color combined from a numeric color and the
> +// current foreground color (currentcolor keyword).
> +// The target color is roughly "color * (1 - p) + currentcolor * p".
> +// where p is mForegroundRatio.

This should be a comment beginning with /** and ending with */, since it describes the class.

Also "A struct represents" -> "This struct represents" and "combined from" -> "combined with".

Also, why "roughly"?  Is the formula not accurate (modulo rounding)?
Attachment #8787234 - Flags: review?(dbaron) → review+
Comment on attachment 8787235 [details]
Bug 1299741 part 8 - Add support for storing complex color in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/75936/#review77128

::: layout/style/StyleAnimationValue.cpp:4354
(Diff revision 2)
>      case eUnit_Color:
> +    case eUnit_ComplexColor:
>        mValue.mColor = aOther.mValue.mColor;
>        break;

comment that mExtra is copied above?

::: layout/style/nsCSSValue.cpp:158
(Diff revision 2)
> -  else if (IsIntegerColorUnit()) {
> +  else if (IsIntegerColorUnit() || eCSSUnit_ComplexColor == mUnit) {
>      mValue.mColor = aCopy.mValue.mColor;
>    }

similar comment here?

::: layout/style/nsCSSValue.cpp:1588
(Diff revision 2)
> +    if (color.IsCurrentColor()) {
> +      serializable.SetIntValue(NS_COLOR_CURRENTCOLOR, eCSSUnit_EnumColor);
> +    } else if (color.IsNumericColor()) {
> +      serializable.SetColorValue(color.mColor);
> +    } else {
> +      MOZ_ASSERT_UNREACHABLE("Cannot serialize a complex color");

If this is unreachable, why isn't the whole else-if?   Seems like the whole 10 lines should be replaced with just the assert -- that way if it's possible to hit the assertion, you're more likely to notice.
Attachment #8787235 - Flags: review?(dbaron) → review+
Comment on attachment 8787234 [details]
Bug 1299741 part 4 - Add StyleComplexColor type for storing color combining numeric color and currentcolor.

https://reviewboard.mozilla.org/r/75934/#review77126

> This should be a comment beginning with /** and ending with */, since it describes the class.
> 
> Also "A struct represents" -> "This struct represents" and "combined from" -> "combined with".
> 
> Also, why "roughly"?  Is the formula not accurate (modulo rounding)?

This formula is the concept but there are differences in details: the calculation is applied per component, and the range of p is actually [0, 255] rather than [0.0, 1.0].
Comment on attachment 8787235 [details]
Bug 1299741 part 8 - Add support for storing complex color in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/75936/#review77128

> If this is unreachable, why isn't the whole else-if?   Seems like the whole 10 lines should be replaced with just the assert -- that way if it's possible to hit the assertion, you're more likely to notice.

The other parts are actually reachable, because devtools may want to display the start and end value of an animation derived from StyleAnimationValue. Probably I can change nsCSSValue::SetComplexColorValue to do the same thing as StyleAnimationValue::SetComplexColorValue that uses the simple units for these cases.
Comment on attachment 8787236 [details]
Bug 1299741 part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor.

https://reviewboard.mozilla.org/r/75938/#review77142

::: layout/style/nsRuleNode.cpp:1112
(Diff revision 2)
> +  if (unit == eCSSUnit_Null) {
> +    return true;
> +  }

This should return false.

(Or maybe the function should return void?)

::: layout/style/nsRuleNode.cpp:1119
(Diff revision 2)
> +  }
> +
> +  if (unit == eCSSUnit_Unset) {
> +    if (UnsetTo == eUnsetInitial) {
> +      unit = eCSSUnit_Initial;
> +    } else if (UnsetTo == eUnsetInherit) {

This should be an else rather than an else if, and inside should assert (is a static_assert possible?) the condition.

Or, alternatively, maybe it makes it more like similar code to not modify unit, but instead just write:

 if (unit == eCSSUnit_Initial ||
     (UnsetTo == eUnsetInitial && unit == eCSSUnit_Unset))

etc., below.

::: layout/style/nsRuleNode.cpp:1134
(Diff revision 2)
> +    aResult.mForegroundRatio = 0;
> +    return SetColor(aValue, aParentColor.mColor, aPresContext,
> +                    nullptr, aResult.mColor, aConditions);

Modifying the result and returning false is poor practice here.  Instead, you should do:

if (!SetColor(...)) {
  return false;
}
aResult.mForegroundRatio = 0;
Attachment #8787236 - Flags: review?(dbaron) → review+
Comment on attachment 8787236 [details]
Bug 1299741 part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor.

https://reviewboard.mozilla.org/r/75938/#review77142

> This should be an else rather than an else if, and inside should assert (is a static_assert possible?) the condition.
> 
> Or, alternatively, maybe it makes it more like similar code to not modify unit, but instead just write:
> 
>  if (unit == eCSSUnit_Initial ||
>      (UnsetTo == eUnsetInitial && unit == eCSSUnit_Unset))
> 
> etc., below.

I don't think static_assert is possible... before we have "constexpr if" :)
Comment on attachment 8787236 [details]
Bug 1299741 part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor.

https://reviewboard.mozilla.org/r/75938/#review77142

> I don't think static_assert is possible... before we have "constexpr if" :)

It is possible, though, to do static_assert that UnsetTo is one of the two values, in the function body. But I think conditional static_assert would only work with "constexpr if". I'd prefer expending the if statements.
The patches landed in bug 1216843 make large part of this patchset needs to be reworked. Probably we no longer need mValueExtra anymore as bug 1216843 does unconditional allocation for color values, which I believe is a bad idea, though.

But if we don't really care about the performance on color interpolation, we may just use float instead of uint8_t for the ratio, and just store color with ratio in a form like nsCSSValueFloatColor.
Depends on: 1216843
Blocks: 1303241
Attachment #8787229 - Attachment is obsolete: true
Attachment #8787233 - Attachment is obsolete: true
Attachment #8787235 - Attachment is obsolete: true
Attachment #8787237 - Attachment is obsolete: true
Sorry for the delay in these reviews, I'm currently attending TPAC but will hopefully get through them today.

One high-level comment, however, is that I believe we intend to drop the mozilla::css namespace long-term so we shouldn't add new types to this namespace if we can help it.
Comment on attachment 8791869 [details]
Bug 1299741 part 6 - Use RGBAColorData for color calculation in StyleAnimationValue.

https://reviewboard.mozilla.org/r/79162/#review78054

Nice!

::: layout/style/StyleAnimationValue.cpp:1180
(Diff revision 1)
>    // FIXME (spec): The CSS transitions spec doesn't say whether
>    // colors are premultiplied, but things work better when they are,
>    // so use premultiplication.  Spec issue is still open per
>    // http://lists.w3.org/Archives/Public/www-style/2009Jul/0050.html

Do we still need this comment?

::: layout/style/StyleAnimationValue.cpp:1185
(Diff revision 1)
>    // To save some math, scale the alpha down to a 0-1 scale, but
>    // leave the color components on a 0-255 scale.

Does this comment still apply?

::: layout/style/StyleAnimationValue.cpp:1192
(Diff revision 1)
>  
> -  double R1, G1, B1, A1;
> -  Tie(R1, G1, B1, A1) = GetPremultipliedColorComponents(aValue1);
> -  double R2, G2, B2, A2;
> -  Tie(R2, G2, B2, A2) = GetPremultipliedColorComponents(aValue2);
> -  double Aresf = (A1 * aCoeff1 + A2 * aCoeff2);
> +  float factor1 = aValue1.mA * aCoeff1;
> +  float factor2 = aValue2.mA * aCoeff2;
> +  float resultA = factor1 + factor2;
> +  if (resultA <= 0.0) {
> +    return NS_RGBA(0, 0, 0, 0);

Could we just return { 0, 0, 0, 0 } here?

Rather than build up an nscolor and then pass to the RGBAColorData constructor?
Attachment #8791869 - Flags: review?(bbirtles) → review+
Comment on attachment 8791871 [details]
Bug 1299741 part 8 - Change StyleDataAtOffset to template to simplify code.

https://reviewboard.mozilla.org/r/79166/#review78114
Attachment #8791871 - Flags: review?(bbirtles) → review+
Comment on attachment 8791872 [details]
Bug 1299741 part 9 - Implement interpolation between numeric color and currentcolor.

https://reviewboard.mozilla.org/r/79168/#review78116

::: layout/style/StyleAnimationValue.cpp:537
(Diff revision 3)
> +      return ComplexColorData(NS_RGBA(0, 0, 0, 0), 1.0f);
> +    case StyleAnimationValue::eUnit_ComplexColor:
> +      return ComplexColorData(aValue.GetComplexColorData());
> +    default:
> +      MOZ_ASSERT_UNREACHABLE("Unknown unit");
> +      return ComplexColorData(NS_RGBA(0, 0, 0, 0), 0.0f);

Would it be slightly simpler to use { 0.f, 0.f, 0.f, 0.f } than NS_RGBA and avoid going to nscolor and back in the two cases here?

::: layout/style/StyleAnimationValue.cpp:663
(Diff revision 3)
> +      // We shouldn't ever get here but the following code may still be
> +      // useful in the future, and be a fallback if the assertion fails.
> +      MOZ_ASSERT_UNREACHABLE("We shouldn't get here as we only call "
> +                             "ComputeDistance on pre-interpolation values");
> +      float alpha1 = color1.mColor.mA * (1.0f - color1.mForegroundRatio);
> +      float alpha2 = color2.mColor.mA * (1.0f - color2.mForegroundRatio);
> +      double dist = ComputeColorDistance(color1.mColor.WithAlpha(alpha1),
> +                                         color2.mColor.WithAlpha(alpha2));
> +      double diffRatio = color1.mForegroundRatio - color2.mForegroundRatio;
> +      aDistance = sqrt(dist * dist + diffRatio * diffRatio);
> +      return true;

I personally don't like putting untested (and untestable) code in the tree, especially when it's this complicated but if you're sure this is worth adding I'll leave that to your judgement.

Personally, I'd just add the MOZ_ASSERT_UNREACHABLE with, aDistance = 0.0 and return false and then possibly a comment with a link to a Bugzilla comment describing the code we should add if the assertion fails.
Attachment #8791872 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #74)
> Sorry for the delay in these reviews, I'm currently attending TPAC but will
> hopefully get through them today.
> 
> One high-level comment, however, is that I believe we intend to drop the
> mozilla::css namespace long-term so we shouldn't add new types to this
> namespace if we can help it.

I saw Grid-related data struct there, so I suppose we are still adding new things there... And I'm not quite sure whether moving those names into mozilla namespace is a good idea.
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #77)
> Personally, I'd just add the MOZ_ASSERT_UNREACHABLE with, aDistance = 0.0
> and return false and then possibly a comment with a link to a Bugzilla
> comment describing the code we should add if the assertion fails.

That sounds like a good approach. Let me paste that code here:
> float alpha1 = color1.mColor.mA * (1.0f - color1.mForegroundRatio);
> float alpha2 = color2.mColor.mA * (1.0f - color2.mForegroundRatio);
> double dist = ComputeColorDistance(color1.mColor.WithAlpha(alpha1),
>                                    color2.mColor.WithAlpha(alpha2));
> double diffRatio = color1.mForegroundRatio - color2.mForegroundRatio;
> aDistance = sqrt(dist * dist + diffRatio * diffRatio);
Comment on attachment 8791868 [details]
Bug 1299741 part 5 - Add css::RGBAColorData struct.

https://reviewboard.mozilla.org/r/79160/#review78320

::: layout/style/nsCSSValue.h:302
(Diff revision 1)
>      ~FontFamilyListRefCnt() {
>          MOZ_COUNT_DTOR(FontFamilyListRefCnt);
>      }
>  };
>  
> +struct RGBAColorData

Seems a shame to have two similar classes (RGBAColorData and the less-strongly-typed, refcounted nsCSSValueFloatColor) that aren't related to each other, but it's probably not worth trying to re-use one in the other.

::: layout/style/nsCSSValue.h:314
(Diff revision 1)
> +    : mR(NS_GET_R(aColor) * (1.0f / 255.0f))
> +    , mG(NS_GET_G(aColor) * (1.0f / 255.0f))
> +    , mB(NS_GET_B(aColor) * (1.0f / 255.0f))
> +    , mA(NS_GET_A(aColor) * (1.0f / 255.0f))

I'm not sure if there is any difference between this and just

  : mR(NS_GET_R(aColor) / 255.0f)

but it looks like we use this |* (1.0f / 255.0f)| pattern elsewhere...
Attachment #8791868 - Flags: review?(cam) → review+
Comment on attachment 8791868 [details]
Bug 1299741 part 5 - Add css::RGBAColorData struct.

https://reviewboard.mozilla.org/r/79160/#review78320

> I'm not sure if there is any difference between this and just
> 
>   : mR(NS_GET_R(aColor) / 255.0f)
> 
> but it looks like we use this |* (1.0f / 255.0f)| pattern elsewhere...

They are identical in terms of math, but they are different in terms of floating-point calculation. "* (1.0f / 255.0f)" is faster than "/ 255.0f" because multiply is faster than division. And compiler cannot do this optimization because the former hurts accuracy. GCC (probably Clang as well) does this optimization only when -funsafe-math-optimizations is enabled, while the flag is not enabled in Gecko (and I suspect that enabling that flag is unsafe in Gecko). (See https://gcc.gnu.org/wiki/FloatingPointMath )

That says, while using integer division with constant divisor should be encouraged (over "fast" hacks, e.g. FAST_DIVIDE_BY_255), using floating-point division should still be avoided.
Comment on attachment 8791870 [details]
Bug 1299741 part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/79164/#review78324

::: layout/style/StyleAnimationValue.h:440
(Diff revision 3)
> +  StyleComplexColor GetComplexColorValue() const {
> +    return GetComplexColorData().ToComplexColor();
> +  }
> +

I think this function is confusingly named, since it looks like it should return a ComplexColorValue object but instead returns a StyleComplexColor.  What about GetStyleComplexColor?

::: layout/style/nsCSSValue.h:390
(Diff revision 3)
> +  // Just redirect any parameter to the data struct.
> +  template<typename... Args>
> +  explicit ComplexColorValue(Args&&... aArgs)
> +    : ComplexColorData(Forward<Args>(aArgs)...)
> +  {
> +    MOZ_COUNT_CTOR(ComplexColorValue);

MOZ_COUNT_CTOR/DTOR are not needed for refcounted objects that use NS_..._REFCOUNTING macros, as that machinery does this checking for us:

http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/xpcom/glue/nsISupportsImpl.h#141-148

http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/xpcom/glue/nsISupportsImpl.h#502

::: layout/style/nsCSSValue.h:394
(Diff revision 3)
> +  bool operator==(const ComplexColorValue& aOther) const
> +    { return ComplexColorData::operator==(aOther); }
> +  bool operator!=(const ComplexColorValue& aOther) const
> +    { return ComplexColorData::operator!=(aOther); }

I guess you can just rely on inheritance here rather than redefining them.

::: layout/style/nsCSSValue.cpp:1601
(Diff revision 3)
> +    if (color.IsCurrentColor()) {
> +      serializable.SetIntValue(NS_COLOR_CURRENTCOLOR, eCSSUnit_EnumColor);
> +    } else if (color.IsNumericColor()) {
> +      serializable.SetColorValue(color.mColor);
> +    } else {
> +      MOZ_ASSERT_UNREACHABLE("Cannot serialize a complex color");

Is this really unreachable?  What if we inspect computed style in the middle of an animation?  Does the spec say what should be returned here?
Comment on attachment 8791870 [details]
Bug 1299741 part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/79164/#review78324

> I think this function is confusingly named, since it looks like it should return a ComplexColorValue object but instead returns a StyleComplexColor.  What about GetStyleComplexColor?

Naming here is pretty hard, because other similiar getters all use Get*Value form, and if we want a method to get ComplexColorValue, it may be named GetComplexColorValueValue.

Probably GetStyleComplexColorValue would be fine. WDYT?

> MOZ_COUNT_CTOR/DTOR are not needed for refcounted objects that use NS_..._REFCOUNTING macros, as that machinery does this checking for us:
> 
> http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/xpcom/glue/nsISupportsImpl.h#141-148
> 
> http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/xpcom/glue/nsISupportsImpl.h#502

Oh, okay, I didn't know that. It seems there are several other classes doing this.

> Is this really unreachable?  What if we inspect computed style in the middle of an animation?  Does the spec say what should be returned here?

As far as I tested, it doesn't seem to be reachable. It seems that inspector may query the start value and the end value (thus the branches for currentcolor and numeric color), but it doesn't seem inspector would query any intermediate value. Even if it does, we cannot provide any useful information here, I think.

The spec doesn't define any syntax for this currently. blenda() in CSS Colors 4 may be a candidate for this, but I don't think we should use it before its syntax and definition get stable and we fully implement it.
Comment on attachment 8791870 [details]
Bug 1299741 part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/79164/#review78324

> Naming here is pretty hard, because other similiar getters all use Get*Value form, and if we want a method to get ComplexColorValue, it may be named GetComplexColorValueValue.
> 
> Probably GetStyleComplexColorValue would be fine. WDYT?

Sounds good.

> As far as I tested, it doesn't seem to be reachable. It seems that inspector may query the start value and the end value (thus the branches for currentcolor and numeric color), but it doesn't seem inspector would query any intermediate value. Even if it does, we cannot provide any useful information here, I think.
> 
> The spec doesn't define any syntax for this currently. blenda() in CSS Colors 4 may be a candidate for this, but I don't think we should use it before its syntax and definition get stable and we fully implement it.

OK, understood.  So the reason we can't get in here is that this is for serializing specified values, and it's not possible to write a specified value like this.
Comment on attachment 8791870 [details]
Bug 1299741 part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue.

https://reviewboard.mozilla.org/r/79164/#review78370
Attachment #8791870 - Flags: review?(cam) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1424770e8cac
part 1 - Factor out ComputeColorDistance. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b335b4195e35
part 2 - Move ClampColor to nsColor.h. r=mstange
https://hg.mozilla.org/integration/autoland/rev/60e40fa6a9c3
part 3 - Add LinearBlendColors function for linear blending two colors. r=mstange
https://hg.mozilla.org/integration/autoland/rev/ee7bca9676a2
part 4 - Add StyleComplexColor type for storing color combining numeric color and currentcolor. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/617d96f12f2d
part 5 - Add css::RGBAColorData struct. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f24aadc98d8d
part 6 - Use RGBAColorData for color calculation in StyleAnimationValue. r=birtles
https://hg.mozilla.org/integration/autoland/rev/14dc16efee19
part 7 - Support storing ComplexColor in nsCSSValue and StyleAnimationValue. r=heycam
https://hg.mozilla.org/integration/autoland/rev/67272a3c64aa
part 8 - Change StyleDataAtOffset to template to simplify code. r=birtles
https://hg.mozilla.org/integration/autoland/rev/204f2b90e5e1
part 9 - Implement interpolation between numeric color and currentcolor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/eae9410d0c8f
part 10 - Make nsStyleContext.cpp:ExtractColor return a Maybe so that ExtractColorLenient can reuse that. r=birtles
https://hg.mozilla.org/integration/autoland/rev/de69417de414
part 11 - Change text-{emphasis,fill,stroke}-color to using StyleComplexColor. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/f4fb6313812c
part 12 - Add test for new currentcolor interpolation. r=birtles
Mark 51 as fix-optional. If it's worth uplift to 51, feel free to nominate it.
status-firefox51: affected → fix-optional
No, this is too big to uplift. I wouldn't risk it.
This looks like a bunch of internal changes, not requiring any updates to MDN, so I'm setting the dev-doc-needed flag to complete. Let me know if I'm wrong. Cheers!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: