Closed Bug 1163845 Opened 8 years ago Closed 8 years ago

Unify handling of time in APZ gtests

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(1 file, 1 obsolete file)

We have two kinds of time variables in the APZ gtests:

  - 'TimeStamp APZCBasicTester::testStartTime', which gets passed
    into SampleContentTransformForFrame()

  - A local 'int time' variable which gets passed into many helper
    functions like Pan().

Having both is confusing and unnecessary - we should unify them.
This was mostly a straightforward change, but there were two subtleties:

  - The MultiTouchInput constructor wants an integer timestamp in addition
    to a TimeStamp (this is why we were using the 'int time'). To get an
    integer out of a TimeStamp, I had to introduce a reference "startup"
    TimeStamp which I subtract to get a duration.

  - The tests were relying on SetFrameTime(testStartTime) being called at 
    the beginning of the test (which established the mLastSampleTime for
    animations), and then SampleContentTransformForFrame being called with 
    'testStartTime + delta' to run the animation for a known delta. With
    my approach, things like Pan() modify the time variable that replaced
    testStartTime (mTime), so this no longer works. To fix this, I changed
    GetFrameTime() to a virtual function, which the tests override to return
    mTime, so that time as seen by APZ is under control of the test without
    it having to SetFrameTime() every time it changes mTime.
Attached file There shall be only one time! (obsolete) —
/r/8599 - Bug 1163845 - Unify handling of time in APZ gtests. r=kats

Pull down this commit:

hg pull -r e50146a0f8fa5ec2bb5237aeb9495044bcd19e90 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8604383 - Flags: review?(bugmail.mozilla)
Attachment #8604383 - Attachment description: MozReview Request: bz://1163845/botond → There shall be only one time!
Assignee: nobody → botond
https://reviewboard.mozilla.org/r/8599/#review7221

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:385
(Diff revision 1)
>  static TimeStamp sFrameTime;

Remove sFrameTime

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp:2386
(Diff revision 1)
> -  AsyncPanZoomController::SetFrameTime(testStartTime + TimeDuration::FromMilliseconds(1000));
> +  mTime += TimeDuration::FromMilliseconds(1000);

This is not quite equivalent since mTime has already been incremented. Not sure if it affects the semantics of the test.

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp:564
(Diff revision 1)
> -  aTime += TIME_BETWEEN_TOUCH_EVENT;
> +  // Don't increment the time here. Animations started on touch-up, such as

As this is a functional change that affects the semantics of the tests I would rather bump it to another patch. Or even better, leave the old time increment in, but make the increment a parameter with a default value of 100 so it is equivalent to what it was before but can be overridden if needed.
Comment on attachment 8604383 [details]
There shall be only one time!

I totally hit Ship It without any open issues, so wtf mozreview!
Attachment #8604383 - Flags: review?(bugmail.mozilla) → review+
Status: REOPENED → NEW
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> https://reviewboard.mozilla.org/r/8599/#review7221
> 
> ::: gfx/layers/apz/src/AsyncPanZoomController.cpp:385
> (Diff revision 1)
> >  static TimeStamp sFrameTime;
> 
> Remove sFrameTime
> 
> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp:2386
> (Diff revision 1)
> > -  AsyncPanZoomController::SetFrameTime(testStartTime + TimeDuration::FromMilliseconds(1000));
> > +  mTime += TimeDuration::FromMilliseconds(1000);
> 
> This is not quite equivalent since mTime has already been incremented. Not
> sure if it affects the semantics of the test.

It doesn't affect the semantics (but it was good to verify this, thanks for pointing it out). The purpose of the 1000 ms increment is to ensure that timestamp of tasks posted to TaskThrottler in the second half of the test is greater than the timestamp of tasks posted in the first half by an amount that exceeds the "max wait" threshold, which is 500 ms. Doing additional increments of mTime in the first or second halves doesn't upset that.

> ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp:564
> (Diff revision 1)
> > -  aTime += TIME_BETWEEN_TOUCH_EVENT;
> > +  // Don't increment the time here. Animations started on touch-up, such as
> 
> As this is a functional change that affects the semantics of the tests I
> would rather bump it to another patch. Or even better, leave the old time
> increment in, but make the increment a parameter with a default value of 100
> so it is equivalent to what it was before but can be overridden if needed.

I don't think any callers rely on the 100 ms increment at the end of the pan, as evidenced by the fact that all tests continue to pass as a result of this change. In contrast, a lot of tests fail without it, so a lot of places would have to be changed to pass in the non-default value of zero. Therefore, I propose removing the increment (but I'm happy to split that out into a separate patch, which would apply before the main patch).
Try push that includes this change:

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

It contains a static analysis failure which I've fixed locally, and a Windows gtest timeout which this other push [1] demonstrates is not caused by this patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3220595fa11b
(In reply to Botond Ballo [:botond] from comment #8)
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3220595fa11b

This contains none of your (non-WALDO) patches and is green. So doesn't it demonstrate that the problem is introduced by one of your patches? Not sure how you concluded it wasn't caused by this one.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #8)
> > [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3220595fa11b
> 
> This contains none of your (non-WALDO) patches and is green. So doesn't it
> demonstrate that the problem is introduced by one of your patches? Not sure
> how you concluded it wasn't caused by this one.

Oh whoops, you're right - I mixed up the Try pushes! (It doesn't help that they don't show the try revision's parent revision if it was part of a previous Try push...)

Here's the Try push with just this patch applied, and it does indeed have the Windows gtest timeout, so it appears to be caused by this patch:

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

I'll do a local Windows build to try to reproduce and investigate.
The problem was that I added a static global variable to TestAsyncPanZoomController.cpp whose initializer was 'TimeStamp::Now()'. This was running before TimeStamp::Startup() had run, and that was causing a crash. Fixed locally to use a function-scope static variable which is initialized on first use instead.
https://hg.mozilla.org/mozilla-central/rev/36d9c1b097a8
Status: NEW → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attachment #8604383 - Attachment is obsolete: true
Attachment #8620270 - Flags: review+
You need to log in before you can comment on or make changes to this bug.