1063815 - Use SetCalcValue in StyleAnimationValue::ExtractComputedValue() case for 'background-position', addressing XXXbz comment

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

StyleAnimationValue::ExtractComputedValue has this in its
"case eCSSProperty_background_position" section:
> 3170             // XXXbz is there a good reason we can't just
> 3171             // SetCalcValue(&pos.mXPosition, item->mXValue) here?
[...]
> 3174             if (!pos.mXPosition.mHasPercent) {
> 3175               NS_ABORT_IF_FALSE(pos.mXPosition.mPercent == 0.0f,
> 3176                                 "Shouldn't have mPercent!");
> 3177               nscoordToCSSValue(pos.mXPosition.mLength, xValue);
> 3178             } else if (pos.mXPosition.mLength == 0) {
> 3179               xValue.SetPercentValue(pos.mXPosition.mPercent);
> 3180             } else {
> 3181               SetCalcValue(&pos.mXPosition, xValue);
> 3182             }
http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=8430feba6148#3170

We should use SetCalcValue there, per the XXX comment.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: part 1

Comment 2

[Daniel Holbert [:dholbert]]

There is a minor functional difference between the existing code and SetCalcValue.

The existing code will just use a simple length-valued or percent-valued nsCSSValue, if pos.mXPosition is really just a length or a percent; whereas SetCalcValue will give us a slightly-bulkier nsCSSValue with eCSSUnit_Calc (representing e.g. calc(50px + 0%)).

So this makes us marginally less efficient, but the reduction in code-complexity is worth it. If it matters, we could also introduce a new function like "SetCalcOrSimpleValue()" which does what the current code does, and use that here.

Comment 3

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=cd78a2640ae4

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: part 2: Use AddCSSValueCanonicalCalc(), now that our extracted values will reliably be in Calc form

On the plus side, now that we'll always have eCSSUnit_Calc in these values, that means we can use AddCSSValueCanonicalCalc() to add them.  This considerably simplifies the AddWeighted logic for background-position.

(Technically, we could do this part on its own, even without part 1, because AddCSSValueCanonicalCalc() actually accepts pixel- and percent-valued inputs, despite its name and documentation. (since ExtractCalcValue has cases to handle those units))

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8485251 [details] [diff] [review]
part 1

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8485295 [details] [diff] [review]
part 2: Use AddCSSValueCanonicalCalc(), now that our extracted values will reliably be in Calc form

Hmm.  I guess this is ok, since it could happen anyway, but what about the "restrictions" bit?

Comment 7

[Daniel Holbert [:dholbert]]

I think that was only there to allow us to call AddCSSValuePixelPercentCalc(), which requires it.

In practice, it doesn't matter, because 'background-position' doesn't specify any restrictions in nsCSSPropList.h[1].

In particular: the only value-restrictions that the StyleAnimation code seems to care about are CSS_PROPERTY_VALUE_NONNEGATIVE and CSS_PROPERTY_VALUE_AT_LEAST_ONE [2], and we don't have either of those specified for background-position in nsCSSPropList.h.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSPropList.h?rev=0bab389db590#562
[2] http://mxr.mozilla.org/mozilla-central/source/layout/style/StyleAnimationValue.cpp?rev=8430feba6148#935

Comment 8

[Daniel Holbert [:dholbert]]

Landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d32d903d95a2
 https://hg.mozilla.org/integration/mozilla-inbound/rev/966d4281eea8

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d32d903d95a2
https://hg.mozilla.org/mozilla-central/rev/966d4281eea8