CSS angle unit conversions should consistently use the same associativity

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

CSS angle unit conversions should consistently use the same associativity

[Keith Miller]

Created attachment 409516 [details]
Patch

[Keith Miller]

I'm not 100% sure the image passes are real. If they fail on the bots I'll change them to [ PASS ImageOnlyFailure ] instead.

[Maciej Stachowiak]

Comment on attachment 409516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409516&action=review

> Source/WebCore/ChangeLog:21
> +        This patch changes the WTF functions to operate on a single
> +        scale factor when converting to/from the canonical
> +        unit. Conversions between non-canonical units first convert to the
> +        canonical unit (degree). Also,
> +        conversionToCanonicalUnitsScaleFactor now gets the scale factor
> +        from the WTF functions for consistency.

Ideally the ChangeLog should explain why this matter.

> Source/WTF/wtf/MathExtras.h:103
> +// We compute the scale factor first because it allows us to extract scale factors via these functions. This is important because FP is not associative.

e.g. the explanation in this comment would be good in the ChangeLog.

> Source/WTF/wtf/MathExtras.h:104
> +inline double deg2rad(double d)  { return d * (piDouble / 180.0); }

How about extracting the conversion factors into named constants? That would make the comment unnecessary here (there would't be weird parens to explain) and it would avoid the need to call these functions with a 1.0 parameter to generate conversion factors, which seems more awkward than just using conversion factor constants.

> Source/WTF/wtf/MathExtras.h:121
> +inline float rad2grad(float r) { return deg2grad(rad2deg(r)); }
> +inline float grad2rad(float g) { return deg2rad(grad2deg(g)); }

I guess this is right for CSS, but is it right for all uses of these math functions?

> Source/WebCore/css/CSSPrimitiveValue.cpp:761
> +        factor = grad2deg(1.0);

As mentioned above, it would be even nicer if this referenced a named constant instead of calling the conversion function with a 1.0 parameter.

[Keith Miller]

Comment on attachment 409516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409516&action=review

>> Source/WebCore/ChangeLog:21
>> +        from the WTF functions for consistency.
> 
> Ideally the ChangeLog should explain why this matter.

Sure.

>> Source/WTF/wtf/MathExtras.h:103
>> +// We compute the scale factor first because it allows us to extract scale factors via these functions. This is important because FP is not associative.
> 
> e.g. the explanation in this comment would be good in the ChangeLog.

Copied.

>> Source/WTF/wtf/MathExtras.h:104
>> +inline double deg2rad(double d)  { return d * (piDouble / 180.0); }
> 
> How about extracting the conversion factors into named constants? That would make the comment unnecessary here (there would't be weird parens to explain) and it would avoid the need to call these functions with a 1.0 parameter to generate conversion factors, which seems more awkward than just using conversion factor constants.

Fair enough. Done.

>> Source/WTF/wtf/MathExtras.h:121
>> +inline float grad2rad(float g) { return deg2rad(grad2deg(g)); }
> 
> I guess this is right for CSS, but is it right for all uses of these math functions?

The only place using these is in SVG. And changing this does appear to cause a test to pass.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:761
>> +        factor = grad2deg(1.0);
> 
> As mentioned above, it would be even nicer if this referenced a named constant instead of calling the conversion function with a 1.0 parameter.

Sure.

[Keith Miller]

Created attachment 409522 [details]
Patch

[Keith Miller]

Created attachment 409586 [details]
Patch

[Keith Miller]

Created attachment 409602 [details]
Patch

[Keith Miller]

Comment on attachment 409516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409516&action=review

>>> Source/WTF/wtf/MathExtras.h:121
>>> +inline float grad2rad(float g) { return deg2rad(grad2deg(g)); }
>> 
>> I guess this is right for CSS, but is it right for all uses of these math functions?
> 
> The only place using these is in SVG. And changing this does appear to cause a test to pass.

I lied, that appears to have been a flakey pass... :/

[Maciej Stachowiak]

Comment on attachment 409602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409602&action=review

r=me

> LayoutTests/imported/w3c/ChangeLog:13
> +        * web-platform-tests/css/css-values/minmax-angle-computed-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-audioparam-interface/k-rate-panner-connections-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-audioparam-interface/k-rate-panner-expected.txt:
> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-basic-expected.txt:

You may want to explain that these are either progressions or irrelevant changes.

Does one of these tests cover what you are actually fixing? I assume the minmax-angle-computed-expected one?

[Keith Miller]

Comment on attachment 409602 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409602&action=review

>> LayoutTests/imported/w3c/ChangeLog:13
>> +        * web-platform-tests/webaudio/the-audio-api/the-pannernode-interface/panner-automation-basic-expected.txt:
> 
> You may want to explain that these are either progressions or irrelevant changes.
> 
> Does one of these tests cover what you are actually fixing? I assume the minmax-angle-computed-expected one?

Yeah, the minmax-angle-computed test covers this edge case. Will add that comment, I'll also clarify that the other tests are simply changes to the rounding error of the panner computation.

[Keith Miller]

Created attachment 409623 [details]
Patch for landing

[EWS]

Committed r267551: <https://trac.webkit.org/changeset/267551>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409623 [details].

[Radar WebKit Bug Importer]

<rdar://problem/69530589>