Closed Bug 1355380 Opened 6 years ago Closed 6 years ago

stylo: re-enable reftests for image-orientation property with -45deg and 315 deg

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: chenpighead, Assigned: jyc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1355380 - Part 1: Have nsStyleImageOrientation::CreateAsAngleAndFlip handle negative angles correctly.
59 bytes, text/x-review-board-request
manishearth
: review+
Details
Bug 1355380 - Part 2: Make Servo's rounding of image-orientation values agree with Gecko's, and pass orientations directly as an enum instead of as angles.
59 bytes, text/x-review-board-request
manishearth
: review+
Details 
In Bug 1341758, we have following 2 reftest regressions:
 
REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-orientation-explicit.html?-45 == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-orientation-explicit.html?-45 | image comparison, max difference: 255, number of differing pixels: 75001 [log…]

REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-orientation-explicit.html?315 == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/image/image-orientation-explicit.html?315 | image comparison, max difference: 255, number of differing pixels: 75001 [log…]


As I mentioned in Bug 1341758 comment 52, the 2 regressions might be caused by precision errors between float and double, since we compute/round with f32 in Servo but with f64 in Gecko. The normalize_angle [1] computation might introduce some potential rounding errors I guess.

What we can do is, align Servo's normalize_angle with Gecko. To simplify the glue code for stylo, it might be worthy to make coumputed value storage of image-orientation in Servo to use u8 as well.



[1] http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/servo/components/style/properties/longhand/inherited_box.mako.rs#147
Depends on: 1341758
Priority: -- → P3
This seems to be the Gecko equivalent: https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h?q=nsStyleImageOrientation&redirect_type=direct#2025 . Storing the computed value as one of the actual rounded-to-90 things makes more sense too.
Assignee: nobody → jyc
Made implementations agree and added handling of negative angles on the Gecko side. Try run here, seems to finally be passing reftests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a235199e3818d85d981e308b43b302ccf133d56

Formatting commits now.
Comment on attachment 8884422 [details]
Bug 1355380 - Part 1: Have nsStyleImageOrientation::CreateAsAngleAndFlip handle negative angles correctly.

https://reviewboard.mozilla.org/r/155338/#review160426

::: layout/style/nsStyleStruct.h:2023
(Diff revision 1)
>      uint8_t orientation(0);
>  
>      // Compute the final angle value, rounding to the closest quarter turn.
>      double roundedAngle = fmod(aRadians, 2 * M_PI);
> +    if (roundedAngle < 0) {
> +      roundedAngle = fmod(roundedAngle + 2 * M_PI, 2 * M_PI);

what if the angle is smaller than -2pi?
Attachment #8884422 - Flags: review?(manishearth) → review+
Comment on attachment 8884423 [details]
Bug 1355380 - Part 2: Make Servo's rounding of image-orientation values agree with Gecko's, and pass orientations directly as an enum instead of as angles.

https://reviewboard.mozilla.org/r/155340/#review160428
Attachment #8884423 - Flags: review?(manishearth) → review+
Servo PR is here now: https://github.com/servo/servo/pull/17654
Comment on attachment 8884422 [details]
Bug 1355380 - Part 1: Have nsStyleImageOrientation::CreateAsAngleAndFlip handle negative angles correctly.

https://reviewboard.mozilla.org/r/155336/#review161396
Comment on attachment 8884423 [details]
Bug 1355380 - Part 2: Make Servo's rounding of image-orientation values agree with Gecko's, and pass orientations directly as an enum instead of as angles.

https://reviewboard.mozilla.org/r/155340/#review161398
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5f2d33a9de3
Part 1: Have nsStyleImageOrientation::CreateAsAngleAndFlip handle negative angles correctly. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/f2836ff575eb
Part 2: Make Servo's rounding of image-orientation values agree with Gecko's, and pass orientations directly as an enum instead of as angles. r=manishearth
Backed out as discussed on IRC for failing mochitest layout/style/test/test_value_computation.html:

https://hg.mozilla.org/integration/autoland/rev/66d21dbbe9160da0df4a0329f6370a25d2eb78dc (servo change)
https://hg.mozilla.org/integration/autoland/rev/be9cdda2269c36778d4113ac9a639c243ab2ba7a
https://hg.mozilla.org/integration/autoland/rev/a0fe405581d6e925a239b9ac31968b47fd47d847

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f2836ff575eb04b5c9a2fc44e9e47a5ed35ddfdb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=113447434&repo=autoland

[task 2017-07-11T21:01:38.811776Z] 21:01:38     INFO - TEST-PASS | layout/style/test/test_value_computation.html | should be testing with values that compute to different things for 'image-orientation' 
[task 2017-07-11T21:01:38.813673Z] 21:01:38     INFO - TEST-PASS | layout/style/test/test_value_computation.html | should be testing with values that compute to different things for 'image-orientation' 
[task 2017-07-11T21:01:38.815521Z] 21:01:38     INFO - TEST-PASS | layout/style/test/test_value_computation.html | should not get empty value for 'image-orientation:-90deg' 
[task 2017-07-11T21:01:38.817319Z] 21:01:38     INFO - TEST-PASS | layout/style/test/test_value_computation.html | should not get empty value for 'image-orientation:-90deg' 
[task 2017-07-11T21:01:38.821642Z] 21:01:38     INFO - Buffered messages finished
[task 2017-07-11T21:01:39.133784Z] 21:01:39     INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should get initial value for 'image-orientation:-90deg' - got "270deg", expected "0deg"
[task 2017-07-11T21:01:39.136793Z] 21:01:39     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-07-11T21:01:39.138407Z] 21:01:39     INFO -     test_value@layout/style/test/test_value_computation.html:177:6
[task 2017-07-11T21:01:39.140696Z] 21:01:39     INFO -     test_property@layout/style/test/test_value_computation.html:222:5
[task 2017-07-11T21:01:39.142203Z] 21:01:39     INFO -     do_one@layout/style/test/test_value_computation.html:239:5
[task 2017-07-11T21:01:39.146620Z] 21:01:39     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-07-11T21:01:39.153140Z] 21:01:39     INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_value_computation.html | should get initial value for 'image-orientation:-90deg' - got "270deg", expected "0deg"
[task 2017-07-11T21:01:39.155528Z] 21:01:39     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-07-11T21:01:39.160979Z] 21:01:39     INFO -     test_value@layout/style/test/test_value_computation.html:180:6
[task 2017-07-11T21:01:39.163250Z] 21:01:39     INFO -     test_property@layout/style/test/test_value_computation.html:222:5
[task 2017-07-11T21:01:39.168781Z] 21:01:39     INFO -     do_one@layout/style/test/test_value_computation.html:239:5
Flags: needinfo?(jyc)
I am going to sleep, but don't know if the automatic sync will run while I am sleeping. So I'm adding the checkin-needed flag so that after https://github.com/servo/servo/pull/17696 is merged, someone might be able to check this in. If it doesn't work, please back it out. Sorry I couldn't do this myself.
Just merged the servo side!
Flags: needinfo?(jyc)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/055d1bde2a28
Part 1: Have nsStyleImageOrientation::CreateAsAngleAndFlip handle negative angles correctly. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/c3844f8b2be0
Part 2: Make Servo's rounding of image-orientation values agree with Gecko's, and pass orientations directly as an enum instead of as angles. r=manishearth
You need to log in before you can comment on or make changes to this bug.