1163845 - Unify handling of time in APZ gtests

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

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.

Comment 1

[Botond Ballo [:botond]]

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.

Comment 2

[Botond Ballo [:botond]]

Attachment: There shall be only one time!

/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/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8604383 [details]
There shall be only one time!

I totally hit Ship It without any open issues, so wtf mozreview!

Comment 5

[Mark Côté [:mcote]]

Sorry, same old issue--you have to Ship It on the parent review request (https://reviewboard.mozilla.org/r/8597/) in order to get an r+ in the attachment.  We know this is entirely nonobvious; we're switching this behaviour very soon. Duping this bug to that one so you can follow along.

Comment 6

[Mark Côté [:mcote]]

Oops sorry, wrong bug to dup!

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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).

Comment 8

[Botond Ballo [:botond]]

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 11

[Botond Ballo [:botond]]

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36d9c1b097a8

Comment 13

[Botond Ballo [:botond]]

(Clean Try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cdfbf778323)

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/36d9c1b097a8

Comment 16

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1163845 - Unify handling of time in APZ gtests. r=kats