Closed Bug 1255022 Opened 4 years ago Closed 4 years ago

Port test_transformed_scrolling_repaints* to mochitest-plain

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

From https://bugzilla.mozilla.org/show_bug.cgi?id=1254806#c30 I think these tests should be converted to mochitest-plain:

layout/base/tests/chrome/test_transformed_scrolling_repaints.html
layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
layout/base/tests/chrome/test_transformed_scrolling_repaints_3.html
Attached patch Convert to mochitest-plain (obsolete) — Splinter Review
Seems to work locally. I'll do a try push
Try push had two types of failures. On Windows there was a windows-specific codepath in one test that tried using Components.classes. I think that codepath can just be taken out so I tried doing that, have another try push going.

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

The other failure was in the test_transformed_scrolling_repaints_3.html test on Windows and Linux only. I can repro easily on Linux but having trouble figuring out the problem. I know that it's caused by subpixel offsets on the layer which is somehow caused by APZ being enabled. If I disable APZ the test passes. I chased it a bit further and it seems like with APZ disabled, some of the ScrollByLines(1) calls scroll by 1102 app units with APZ disabled, and 1103 app units with APZ enabled. I chased it a bit more and it seems like with APZ disabled, mScrollPosForLayerPixelAlignment remains 1103 each time through ScrollToImpl but with APZ enabled it advances - 1103, 2206, 3309, and so on. This value gets used in ClampAndAlignWithLayerPixels, and seems to be implicated in the failure.

I don't really understand the layer pixel alignment code, at least not at this level, so it would be better to hand this off to somebody else. I'll verify that my Components.classes removal works first though.
Good news, after rebasing my patch to master the failure went away. Love it when that happens.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9a05f81d498
Try seems green, although "Add Jobs" is broken so I'll need to do a new push for android :(
Attachment #8730332 - Attachment is obsolete: true
Attachment #8736939 - Flags: review?(mstange)
Comment on attachment 8736939 [details] [diff] [review]
Convert to mochitest-plain

Review of attachment 8736939 [details] [diff] [review]:
-----------------------------------------------------------------

nice
Attachment #8736939 - Flags: review?(mstange) → review+
Well that was pointless, the entire layout/base/tests directory is skipped on android. So this is good to land, I think.
https://hg.mozilla.org/mozilla-central/rev/05cd77a4d02b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Good news, after rebasing my patch to master the failure went away. Love it
> when that happens.
> 

Indeed! \o/
Marking 47 as affected since according to Johnny's email I'm assuming the goal is to have all testing stuff uplifted to 47. I'll need to check that the failure doesn't happen on 47 though, before requesting uplift.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Good news, after rebasing my patch to master the failure went away. Love it
> when that happens.

Whatever fixed this isn't on aurora yet.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5770844a7cd0&group_state=expanded

We can just let this ride the trains.
You need to log in before you can comment on or make changes to this bug.