Add a unit test for timers

Add a unit test for timers

This test will help to ensure current behavior isn't altered
while removing the timer heap.

Original patch by Alex Clarke <alexclarke@chromium.org>;.

BUG=463143
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195235

[Sami, 2015-05-08 17:04:51]

Patchset #2 (id:20001) has been deleted

[Sami, 2015-05-08 17:06:22]

skyostil@chromium.org changed reviewers:
+ philipj@opera.com

[Sami, 2015-05-08 17:06:38]

Philip, would you be up to reviewing this patch?

[philipj_slow, 2015-05-11 16:01:20]

I'm not deeply familiar with Timer (other than having used it) and haven't
written a lot of gmock/gtest, but I've read carefully and it LGTM. If you're
unsure about something you may want to find someone who's worked on the timer
code for a second opinion.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
File Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:181: static SharedTimerFunction s_timerFunction;
Is making this static important to the test? The "shared" in the name made me
suspect that this should be shared between (real) Platform implementations, but
of course setSharedTimerFiredFunction isn't static so that doesn't make much
sense. If it can simply be a member, that seems more clear.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:398: TEST_F(TimerTest,
PostingTimerTwiceWithNewerRunTimeCancelsOrigionalTask)
Typo "origional"

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:408: TEST_F(TimerTest,
PostingTimerTwiceWithLaterRunTimeCancelsOrigionalTask)
Dito

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:462: EXPECT_THAT(m_runTimes,
ElementsAre(m_startTime + 1.0, m_startTime + 2.0, m_startTime + 2.0));
It's not obvious to me why the last one will be exactly m_startTime+2.0, does
that not depend on the argument to setSharedTimerFireInterval? Since there's
repeating timer and you call runUntilIdleOrDeadlinePassed I would have expected
the current time to be >=m_startTime+2.5 at the startOneShot call. I probably
don't understand how this all fits together?

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:595: timer.augmentRepeatInterval(10);
I couldn't guess what this was for and it's not documented, but
DOMTimer::fired() makes it pretty clear it's for rate limiting setTimeouts. If
you want to comment why this influences nextFireInterval() and not just
repeatInterval() as the name might suggest that would be nice.

[Sami, 2015-05-11 17:38:59]

> I'm not deeply familiar with Timer (other than having used it) and haven't
> written a lot of gmock/gtest, but I've read carefully and it LGTM. If you're
> unsure about something you may want to find someone who's worked on the timer
> code for a second opinion.

Thank you for the review. I think the basic problem is that there aren't too
many experts around for the timer code in the first place. It has barely changed
since we forked. My plan is to delete as much of it as we can[1] so we won't
need any experts :)

That said, I don't think there's anything too controversial in this test. It's
essentially just documenting what the current implementation does.

[1] https://codereview.chromium.org/1134523002/

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
File Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:181: static SharedTimerFunction s_timerFunction;
On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> Is making this static important to the test? The "shared" in the name made me
> suspect that this should be shared between (real) Platform implementations,
but
> of course setSharedTimerFiredFunction isn't static so that doesn't make much
> sense. If it can simply be a member, that seems more clear.

Unfortunately the way the shared timer function is registered basically requires
this to be a static :\ More specifically, the first time you call
PlatformThreadData::current(), the callback is set, but there's no way to tear
down PlatformThreadData between tests so we'd lose the callback when the
platform is torn down after the first test.

I briefly started down the path of making PlatformThreadData re-initializable,
but that ended up being a little messy so I decided this would be the path of
least resistance -- especially given that my upcoming patch will remove shared
timers for good: https://codereview.chromium.org/1134523002 :)

I've added a comment here that makes this more explicit.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:398: TEST_F(TimerTest,
PostingTimerTwiceWithNewerRunTimeCancelsOrigionalTask)
On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> Typo "origional"

Done.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:408: TEST_F(TimerTest,
PostingTimerTwiceWithLaterRunTimeCancelsOrigionalTask)
On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> Dito

Done.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:462: EXPECT_THAT(m_runTimes,
ElementsAre(m_startTime + 1.0, m_startTime + 2.0, m_startTime + 2.0));
On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> It's not obvious to me why the last one will be exactly m_startTime+2.0, does
> that not depend on the argument to setSharedTimerFireInterval?

Right, right now the timer implementation works by computing the right value to
pass into setSharedTimerFireInterval. That value will essentially be the time at
which the next pending timer should trigger.

> Since there's
> repeating timer and you call runUntilIdleOrDeadlinePassed I would have
expected
> the current time to be >=m_startTime+2.5 at the startOneShot call. I probably
> don't understand how this all fits together?

I guess the confusing part is that runUntilIdleOrDeadlinePassed() will stop at
the last point in time when something executed -- not counting the deadline
itself. In this test the current time after runUntilIdleOrDeadlinePassed()
returns is exactly start + 2.0, not start + 2.5.

I think I agree this is pretty misleading so I've now changed
runUntilIdleOrDeadlinePassed() to stop at the deadline instead.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:595: timer.augmentRepeatInterval(10);
On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> I couldn't guess what this was for and it's not documented, but
> DOMTimer::fired() makes it pretty clear it's for rate limiting setTimeouts. If
> you want to comment why this influences nextFireInterval() and not just
> repeatInterval() as the name might suggest that would be nice.

I believe your guess is as good as mine here -- IIRC this code goes back to the
early days of WebKit. I think the reason why both are modified is that this way
we don't lose the phase offset of the timer. For example if you augment a
repeating timer from 10ms to 20ms and then back to 10ms, you won't miss a beat
during the transition.

[Sami, 2015-05-12 09:29:39]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-05-12 09:29:39]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1130423002/#ps80001
(title: "Review comments.")

[commit-bot: I haz the power, 2015-05-12 09:30:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130423002/80001

[philipj_slow, 2015-05-12 10:07:58]

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
File Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:181: static SharedTimerFunction s_timerFunction;
On 2015/05/11 17:38:59, Sami wrote:
> On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> > Is making this static important to the test? The "shared" in the name made
me
> > suspect that this should be shared between (real) Platform implementations,
> but
> > of course setSharedTimerFiredFunction isn't static so that doesn't make much
> > sense. If it can simply be a member, that seems more clear.
> 
> Unfortunately the way the shared timer function is registered basically
requires
> this to be a static :\ More specifically, the first time you call
> PlatformThreadData::current(), the callback is set, but there's no way to tear
> down PlatformThreadData between tests so we'd lose the callback when the
> platform is torn down after the first test.
> 
> I briefly started down the path of making PlatformThreadData re-initializable,
> but that ended up being a little messy so I decided this would be the path of
> least resistance -- especially given that my upcoming patch will remove shared
> timers for good: https://codereview.chromium.org/1134523002 :)
> 
> I've added a comment here that makes this more explicit.

Acknowledged.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:462: EXPECT_THAT(m_runTimes,
ElementsAre(m_startTime + 1.0, m_startTime + 2.0, m_startTime + 2.0));
On 2015/05/11 17:38:59, Sami wrote:
> On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> > It's not obvious to me why the last one will be exactly m_startTime+2.0,
does
> > that not depend on the argument to setSharedTimerFireInterval?
> 
> Right, right now the timer implementation works by computing the right value
to
> pass into setSharedTimerFireInterval. That value will essentially be the time
at
> which the next pending timer should trigger.
> 
> > Since there's
> > repeating timer and you call runUntilIdleOrDeadlinePassed I would have
> expected
> > the current time to be >=m_startTime+2.5 at the startOneShot call. I
probably
> > don't understand how this all fits together?
> 
> I guess the confusing part is that runUntilIdleOrDeadlinePassed() will stop at
> the last point in time when something executed -- not counting the deadline
> itself. In this test the current time after runUntilIdleOrDeadlinePassed()
> returns is exactly start + 2.0, not start + 2.5.
> 
> I think I agree this is pretty misleading so I've now changed
> runUntilIdleOrDeadlinePassed() to stop at the deadline instead.

Nice, that makes more sense to me.

https://codereview.chromium.org/1130423002/diff/60001/Source/platform/TimerTe...
Source/platform/TimerTest.cpp:595: timer.augmentRepeatInterval(10);
On 2015/05/11 17:38:59, Sami wrote:
> On 2015/05/11 16:01:19, philipj_UTC2 wrote:
> > I couldn't guess what this was for and it's not documented, but
> > DOMTimer::fired() makes it pretty clear it's for rate limiting setTimeouts.
If
> > you want to comment why this influences nextFireInterval() and not just
> > repeatInterval() as the name might suggest that would be nice.
> 
> I believe your guess is as good as mine here -- IIRC this code goes back to
the
> early days of WebKit. I think the reason why both are modified is that this
way
> we don't lose the phase offset of the timer. For example if you augment a
> repeating timer from 10ms to 20ms and then back to 10ms, you won't miss a beat
> during the transition.

OK, a test to verify the behavior is better than no test even if the naming
doesn't reflect what the call does.

[commit-bot: I haz the power, 2015-05-12 10:36:35]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195235