1390046 - stylo: test_transform_limits.html fails with expected "matrix(1, 0, 0, 1, 3.40282e+38, 0)" but got "matrix(1, 0, 0, 1, 1.78957e+7, 0)"

Status: RESOLVED FIXED

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

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Running locally I get:

INFO TEST-UNEXPECTED-FAIL | dom/animation/test/mozilla/test_transform_limits.html | Test that the parameter of transform translate is clamped - Test that the parameter of transform translate is clamped: assert_equals: expected "matrix(1, 0, 0, 1, 3.40282e+38, 0)" but got "matrix(1, 0, 0, 1, 1.78957e+7, 0)"

Some sort of precision issue?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Yeah, servo parses translate() as AppUnit (Au) value which is represented by i32. We can separate the test case as a crash test, I think.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I think we could probably keep this as a mochitest but perhaps just check that the translateX value is sufficiently large (and, I guess, not Infinity).

Alternatively, we could hard-code in the max we expect:

- const max_float = 3.40282e+38;
+ const MAX_TRANSLATE_COMPONENT = isServo ? 1.78957e+7 : 3.40282e+38;

But I guess we'll want to change the parsing of translate() in future to use higher precision?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #2)
> I think we could probably keep this as a mochitest but perhaps just check
> that the translateX value is sufficiently large (and, I guess, not Infinity).
> 
> Alternatively, we could hard-code in the max we expect:
> 
> - const max_float = 3.40282e+38;
> + const MAX_TRANSLATE_COMPONENT = isServo ? 1.78957e+7 : 3.40282e+38;

I'd prefer this even if it might be changed in future, because we can know the exact limit value in this test case.

Comment 4

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93fb8a0286f424b7af05eb7ca322322026bf0262

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1390046: Fix test fail.

Review commit: https://reviewboard.mozilla.org/r/169506/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169506/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #4)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=93fb8a0286f424b7af05eb7ca322322026bf0262

You don't need to speficy linux64-haz, you need to specify --artifact instead.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8898149 [details]
Bug 1390046: Fix test fail.

https://reviewboard.mozilla.org/r/169506/#review174780

::: commit-message-5c79f:1
(Diff revision 1)
> +Bug 1390046: Fix test fail. r?hiro

You should try to write descriprive commit message. :)

'Use max integer value for the translate limit value on stylo' or something.

::: dom/animation/test/mozilla/file_transform_limits.html:11
(Diff revision 1)
>  'use strict';
>  
>  // We clamp +infinity or -inifinity value in floating point to
>  // maximum floating point value or -maximum floating point value.
> -const max_float = 3.40282e+38;
> +const MAX_FLOAT = 3.40282e+38;
> +const MAX_TRANSLATE_COMPONENT = isStyledByServo() ? "1.78957e+7" : MAX_FLOAT;

If we add a const value for this max int, it would be more clear what the value is.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Okay, thanks Hiro!

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8898149 [details]
Bug 1390046: Fix test fail.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169506/diff/1-2/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8898149 [details]
Bug 1390046: Fix test fail.

https://reviewboard.mozilla.org/r/169506/#review174784

::: commit-message-5c79f:1
(Diff revision 2)
> +Bug 1390046: Fix test fail. r?hiro
> +
> +Use max integer value for the translate limit value on stylo.

I meant that using this 'Use ..' in the first place, and it would be nice to have the reason here.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8898149 [details]
Bug 1390046: Fix test fail.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169506/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1aaa19e64c89
Fix test fail. r=hiro

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1aaa19e64c89

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Enable test_transform_limits.html on stylo

We did forget enabling the test itself.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b806833b4ac747a5c50107591c869e3082debc33

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab92c4aa737b
Enable test_transform_limits.html on stylo. r=jdm

Comment 16

[Daisuke Akatsuka (:daisuke)]

wao, thank you very much, Hiro, Josh!

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ab92c4aa737b