Closed Bug 1062609 Opened 5 years ago Closed 5 months ago

Mochitests Are Unable to Read Realtime Scroll Position with APZ enabled

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: kip, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Using nsDOMWindowUtils::AdvanceTimeAndRefresh is not effective in enabling framerate sensitive Mochitests to verify correct motion of APZ driven animation and scrolling.

TimeStamp::Now() is being used throughout AsyncPanZoomController.cpp (via GetFrameTime()) rather than respecting the time reported by the refresh driver while it is under test control.

Additionally, APZCTMController::RequestContentRepaint calls are not being followed by APZCTMController::RequestContentRepaintEvent::Run until after the refresh drive is released from test control as they are dependent on the main thread's message pump which may throttle events and/or be blocked by the javascript executing in the mochitest.

Functionality is required to enable mochitests to know with certainty that the main thread has received all frame metrics updates from the compositor before testing them and allowing the refresh driver to advance.
This is blocking the Mochitest added in Bug 1026023 from being enabled for APZ.  The smooth scrolling feature added in Bug 1026023 has been implemented for APZ in Bug 1022825.
See Also: → 1026023, 1022825
(In reply to :kip (Kearwood Gilbert) from comment #0)
> TimeStamp::Now() is being used throughout AsyncPanZoomController.cpp (via
> GetFrameTime()) rather than respecting the time reported by the refresh
> driver while it is under test control.

The APZ gtests use the static function AsyncPanZoomController::SetFrameTime() to override what GetFrameTime() returns [1]. Perhaps these tests could do the same?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp?rev=9a507b307d1d#202
(In reply to :kip (Kearwood Gilbert) from comment #0)
> Using nsDOMWindowUtils::AdvanceTimeAndRefresh is not effective in enabling
> framerate sensitive Mochitests to verify correct motion of APZ driven
> animation and scrolling.

Yes, I think this is more or less by design. The APZ animation runs on the compositor thread and so is by design decoupled from main-thread control. It seems to me that a better approach might be to use the APZ test data framework (bug 961289 and friends). Botond can help you with that and/or augment the test data framework if needed to support your use cases.

This test is still broken with APZ enabled, and is basically not running because of the early-exit condition. I'll try and take a look at it.

Assignee: nobody → kats

Yeah, sorry I didn't realize this bug didn't actually refer to the test.

Replacing the waitForAllPaintsFlushed calls with waitForApzFlushedRepaints gets us most of the way there. It passes for me locally, and the try push just shows some intermittent failures on Linux. I'll try and debug that a bit.

Ah, when we start a smooth scroll animation the mLastSampleTime is initialized to TimeStamp::Now() here instead of using the test sample time in the CompositorBridgeParent. So in some cases the time delta of the animation's first sampling is shorter than what the test expects it to be.

The test itself is reasonable, except that it reads scroll positions
from the main thread. With APZ enabled we need to flush any pending APZ
repaints before reading these scroll positions. So changing the
waitForAllPaints to the APZ-flushing variant makes that happen, and the
test passes.

Depends on D29921

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dc4761caa7d4
Ensure that APZCTreeManager::GetFrameTime respects the test sample time. r=botond
https://hg.mozilla.org/integration/autoland/rev/c705a64ce83c
Make scroll-behaviour test work with APZ enabled. r=botond
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1561570
You need to log in before you can comment on or make changes to this bug.