Make EventHandler::ActiveIntervalTimer mockable.

Current layout tests for ActiveIntervalTimer use setTimeout, this would enable it to be suspended and fired manually for testing as well as eliminate flakiness.

A more generalized solution for testing Timers would require adding some sort of reflection and/or a mocked system clock.

BUG=319529

[Zeeshan Qureshi, 2013-12-13 02:05:58]

Rick, can you do a first pass, there's a few things I'm not sure of.

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:127: // Just derive from TimerBase and skip the
template-fu?
Deriving from TimerBase might just be cleaner.

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:145: // Should this only work when timer is suspended?
Not sure, maybe only allow this when m_suspended?

[Rick Byers, 2013-12-13 22:52:25]

I think this is a reasonable approach.  The challenge here is striking a
pragmatic balance between overhauling the world with a whole mock-clock system
and enabling some targeted testing.  This is definitely an improvement over the
status quo of not testing this sort of behavior at all, even if it's not exactly
ideal.

At some point I'd like to see us come up with a design for a full mock clock
system (which should be possible to do more cleanly without a bunch of different
hooks), but I don't think we should block testing of the active timer on that.

A few comments.

https://codereview.chromium.org/112023010/diff/1/Source/core/page/EventHandler.h
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/112023010/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.h:151: void suspendActiveIntervalTimer();
To minimize impact on EventHandler API surface, I'd suggest replacing these
three methods with a single method that returns a reference to the
MockableTimer.  To make it clear this is an abstraction violation I'd explicitly
name it something like 'getActiveInternalTimerForTesting'.

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:127: // Just derive from TimerBase and skip the
template-fu?
On 2013/12/13 02:05:58, Zeeshan Qureshi wrote:
> Deriving from TimerBase might just be cleaner.

I like that you don't have to duplicate anything here (like the
m_object/m_function) - I think deriving from Timer is the right way to go.

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:141: void suspend() { m_suspended = true; }
I wonder if we should make these a little scarrier names that clarify their role
- eg. 'setManualModeForTesting' and 'manualFireForTesting' or something?

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:145: // Should this only work when timer is suspended?
On 2013/12/13 02:05:58, Zeeshan Qureshi wrote:
> Not sure, maybe only allow this when m_suspended?

Yeah, probably a good idea to ASSERT(m_suspended) - we should try to avoid
accidental use of these APIs outside of testing.

I think you should also try to preserve the illusion that the timer really
fired.  Eg. if it's active, call stop.  Ideally you could also ASSERT that
'fire' is only being used in place of a single actual fired event - eg. either
the Timer isActive, or fired() was previously invoked and suppressed.

I'm just trying to avoid situations where a test can do something different from
what a real timer would have done.  The only power we want to give the test is
the ability to make the firing time predictable.

[Zeeshan Qureshi, 2014-01-07 19:35:09]

Rick, tried to address your comments.

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:141: void suspend() { m_suspended = true; }
On 2013/12/13 22:52:25, Rick Byers wrote:
> I wonder if we should make these a little scarrier names that clarify their
role
> - eg. 'setManualModeForTesting' and 'manualFireForTesting' or something?

Yes!

https://codereview.chromium.org/112023010/diff/1/Source/platform/Timer.h#newc...
Source/platform/Timer.h:145: // Should this only work when timer is suspended?
On 2013/12/13 22:52:25, Rick Byers wrote:
> On 2013/12/13 02:05:58, Zeeshan Qureshi wrote:
> > Not sure, maybe only allow this when m_suspended?
> 
> Yeah, probably a good idea to ASSERT(m_suspended) - we should try to avoid
> accidental use of these APIs outside of testing.
> 
> I think you should also try to preserve the illusion that the timer really
> fired.  Eg. if it's active, call stop.  Ideally you could also ASSERT that
> 'fire' is only being used in place of a single actual fired event - eg. either
> the Timer isActive, or fired() was previously invoked and suppressed.
> 
> I'm just trying to avoid situations where a test can do something different
from
> what a real timer would have done.  The only power we want to give the test is
> the ability to make the firing time predictable. 

Done, it's a little tricky to reason about pending fires for repeating timers so
I'm thinking that a manual fire clears all pending ones and thus the boolean
instead of counting them.

[Rick Byers, 2014-01-10 21:46:49]

Sorry for the delay!  This is looking good!  Just a couple minor suggestions.

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:136: [RaisesException] void
suspendActiveIntervalTimer(Document document);
combining suspend and resume into a single
'setActiveIntervalTimerSuspended(Document, bool)' function would be more
consistent with other APIs here (Eg. see all the set...Enabled functions that
take a bool to enable and disable).

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Time...
File Source/core/testing/TimerTest.cpp (right):

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Time...
Source/core/testing/TimerTest.cpp:88:
ASSERT_FALSE(m_timer.inManualModeForTesting());
This repeats the two tests from above, right?  I'd either remove the above two
tests (combining them into this one), or just do the minimum necessary to get
back to the same state.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:127: // Just derive from TimerBase and skip the
template-fu?
I think you need the template-fu to get the timer-fired behavior wired up in a
way consistent with other uses of Timer.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:128: template <typename TimerFiredClass>
I'd add a comment here saying the idea is to allow specific timers to be
controlled manually from tests.  Maybe mention that ideally we'd have a full
mock-clock system that would make this unnecessary.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:140: bool inManualModeForTesting() const { return
m_manualMode; }
blink coding guidelines say to use bare words for getters that match the name of
the variable.  So I guess we want to rename the field to m_manualModeForTesting,
and make the getter 'bool manualModeForTesting()'.

Actually, this method is used only by the timer unit test, and in a pretty
trivial way, right?  Perhaps we should just remove it entirely - you can unit
test the timer by testing it's behavior, not it's internal state.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:142: bool firePendingForTesting() const { return
m_firePending; }
Again, this is used only by the unit test, right?  Does it really add enough
value to justify the public surface area do you think?  You can mostly test the
behavior here, but I guess you can't really verify your ASSERT conditions
without this.  Up to you...

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:144: void enterManualModeForTesting() { m_manualMode =
true; }
collapse enter/leave into a single 'setManualModeForTesting' function.

[Zeeshan Qureshi, 2014-01-10 22:20:41]

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:136: [RaisesException] void
suspendActiveIntervalTimer(Document document);
On 2014/01/10 21:46:50, Rick Byers wrote:
> combining suspend and resume into a single
> 'setActiveIntervalTimerSuspended(Document, bool)' function would be more
> consistent with other APIs here (Eg. see all the set...Enabled functions that
> take a bool to enable and disable).

Sorry, I didn't pay attention to the surrounding code. Also, how about using
name similar to the timer function i.e. setActiveIntervalTimerInManualMode?

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Time...
File Source/core/testing/TimerTest.cpp (right):

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Time...
Source/core/testing/TimerTest.cpp:88:
ASSERT_FALSE(m_timer.inManualModeForTesting());
On 2014/01/10 21:46:50, Rick Byers wrote:
> This repeats the two tests from above, right?  I'd either remove the above two
> tests (combining them into this one), or just do the minimum necessary to get
> back to the same state.

Yes, the last two tests are somewhat redundant, combining these 3 into one makes
sense.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:127: // Just derive from TimerBase and skip the
template-fu?
On 2014/01/10 21:46:50, Rick Byers wrote:
> I think you need the template-fu to get the timer-fired behavior wired up in a
> way consistent with other uses of Timer.

I think we already decided on this, forgot to remove comment.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:128: template <typename TimerFiredClass>
On 2014/01/10 21:46:50, Rick Byers wrote:
> I'd add a comment here saying the idea is to allow specific timers to be
> controlled manually from tests.  Maybe mention that ideally we'd have a full
> mock-clock system that would make this unnecessary.

Yes, adding.

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:142: bool firePendingForTesting() const { return
m_firePending; }
On 2014/01/10 21:46:50, Rick Byers wrote:
> Again, this is used only by the unit test, right?  Does it really add enough
> value to justify the public surface area do you think?  You can mostly test
the
> behavior here, but I guess you can't really verify your ASSERT conditions
> without this.  Up to you...

Removing the above methods seems fine, I was under the impression we needed to
have accessors for most of state.

[Rick Byers, 2014-01-10 22:24:35]

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
File Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/112023010/diff/20001/Source/core/testing/Inte...
Source/core/testing/Internals.idl:136: [RaisesException] void
suspendActiveIntervalTimer(Document document);
On 2014/01/10 22:20:41, Zeeshan Qureshi wrote:
> On 2014/01/10 21:46:50, Rick Byers wrote:
> > combining suspend and resume into a single
> > 'setActiveIntervalTimerSuspended(Document, bool)' function would be more
> > consistent with other APIs here (Eg. see all the set...Enabled functions
that
> > take a bool to enable and disable).
> 
> Sorry, I didn't pay attention to the surrounding code. Also, how about using
> name similar to the timer function i.e. setActiveIntervalTimerInManualMode?

Sure, sounds good!

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/20001/Source/platform/Timer.h#...
Source/platform/Timer.h:142: bool firePendingForTesting() const { return
m_firePending; }
On 2014/01/10 22:20:41, Zeeshan Qureshi wrote:
> On 2014/01/10 21:46:50, Rick Byers wrote:
> > Again, this is used only by the unit test, right?  Does it really add enough
> > value to justify the public surface area do you think?  You can mostly test
> the
> > behavior here, but I guess you can't really verify your ASSERT conditions
> > without this.  Up to you...
> 
> Removing the above methods seems fine, I was under the impression we needed to
> have accessors for most of state.

Nope, encapsulation is your friend!

[Rick Byers, 2014-01-13 21:53:36]

lgtm

update subject/description to remove 'WIP' and also add a bit more discussion
about the motivation/scope to the description (i.e. just for activer timer
testing right now, a more general solution would require a full mock-clock
system, etc.).

[Zeeshan Qureshi, 2014-01-14 18:13:24]

eseidel@chromium.org: Please review changes in 

Timer.h

[eseidel, 2014-01-14 22:12:14]

This feels like a very odd layer to hook in at.

But I support the idea of making our testing better!

How soon is the long-term plan of mocking in a system clock?  How does Chromium
do these mocks?  We sit on top of the message loop, maybe we should mock there
isntead?

[Zeeshan Qureshi, 2014-01-14 23:33:55]

> This feels like a very odd layer to hook in at.
> 
> But I support the idea of making our testing better!
> 
> How soon is the long-term plan of mocking in a system clock?

We've thrown around the idea a bit but haven't looked into actual solutions.
Right now, I don't know the system well enough to estimate a timeline.

Doing some sort of dependency injection at test time, tracking the caller and
then
suspending a specific timer instance via JS is one approach. This would remove
the
need to plumb calls for each new Timer explicitly although I haven't quite
figured
out how to identify which instance we'd like to suspend / test.

Mocking the system clock as Rick suggested is another approach although we still
have to explore the idea more.

> How does Chromium do these mocks?

As far as I can see, Chromium tests just sleep.
e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer_u...

> We sit on top of the message loop, maybe we should mock there isntead?

I'll have to look into the message loop a bit more to see how it can be used for
this.
Creating another timer that supported testing seemed like a low hanging fruit
that would
remove flakes in the short term.

[Rick Byers, 2014-01-15 14:32:11]

On 2014/01/14 23:33:55, Zeeshan Qureshi wrote:
> > This feels like a very odd layer to hook in at.
> > 
> > But I support the idea of making our testing better!
> > 
> > How soon is the long-term plan of mocking in a system clock?
> 
> We've thrown around the idea a bit but haven't looked into actual solutions.
> Right now, I don't know the system well enough to estimate a timeline.
> 
> Doing some sort of dependency injection at test time, tracking the caller and
> then
> suspending a specific timer instance via JS is one approach. This would remove
> the
> need to plumb calls for each new Timer explicitly although I haven't quite
> figured
> out how to identify which instance we'd like to suspend / test.
> 
> Mocking the system clock as Rick suggested is another approach although we
still
> have to explore the idea more.
> 
> > How does Chromium do these mocks?
> 
> As far as I can see, Chromium tests just sleep.
> e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer_u...
> 
> > We sit on top of the message loop, maybe we should mock there isntead?
> 
> I'll have to look into the message loop a bit more to see how it can be used
for
> this.
> Creating another timer that supported testing seemed like a low hanging fruit
> that would
> remove flakes in the short term.

I took a brief look into this.  It doesn't look like there are any existing
mock-clock hooks in MessageLoop.  In particular, I think the main places we'd
need to mock are:
 - MesageLoop::DoDelayedWork - it uses a hard-coded call to TimeTicks::Now() to
decide what to run
 - IncomingTaskQueue::CalculateDelayedRuntime - it calls TimeTicks::Now() to set
the run time of a task

And/or perhaps we should be building some mechanism for switching to an
alternate (mock) implementation of TimeTicks.  Eg. we could have a
TimeTicks::MockableNow() and global functions to 'freeze' and manually advance
the clock.  Switching back to free-running might be tricky (maybe add a constant
offset to all future times to avoid going backwards).

Either way I was concerned that it was potentially too hairy to reasonably ask
Zeeshan to do now as a Noogler (I can just imagine the sort of nasty bugs that
could arise when some code path is accidentally getting a real clock time
instead of a mock clock time).  Maybe I'm wrong and this won't actually be that
hard though - it's intriguing and I'd love to see someone take a crack at it at
some point.  I've filed http://crbug.com/334639 to track with some of these
thoughts.

[Rick Byers, 2014-01-16 20:44:42]

On 2014/01/15 14:32:11, Rick Byers wrote:
> On 2014/01/14 23:33:55, Zeeshan Qureshi wrote:
> > > This feels like a very odd layer to hook in at.
> > > 
> > > But I support the idea of making our testing better!
> > > 
> > > How soon is the long-term plan of mocking in a system clock?
> > 
> > We've thrown around the idea a bit but haven't looked into actual solutions.
> > Right now, I don't know the system well enough to estimate a timeline.
> > 
> > Doing some sort of dependency injection at test time, tracking the caller
and
> > then
> > suspending a specific timer instance via JS is one approach. This would
remove
> > the
> > need to plumb calls for each new Timer explicitly although I haven't quite
> > figured
> > out how to identify which instance we'd like to suspend / test.
> > 
> > Mocking the system clock as Rick suggested is another approach although we
> still
> > have to explore the idea more.
> > 
> > > How does Chromium do these mocks?
> > 
> > As far as I can see, Chromium tests just sleep.
> > e.g.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer_u...
> > 
> > > We sit on top of the message loop, maybe we should mock there isntead?
> > 
> > I'll have to look into the message loop a bit more to see how it can be used
> for
> > this.
> > Creating another timer that supported testing seemed like a low hanging
fruit
> > that would
> > remove flakes in the short term.
> 
> I took a brief look into this.  It doesn't look like there are any existing
> mock-clock hooks in MessageLoop.  In particular, I think the main places we'd
> need to mock are:
>  - MesageLoop::DoDelayedWork - it uses a hard-coded call to TimeTicks::Now()
to
> decide what to run
>  - IncomingTaskQueue::CalculateDelayedRuntime - it calls TimeTicks::Now() to
set
> the run time of a task
> 
> And/or perhaps we should be building some mechanism for switching to an
> alternate (mock) implementation of TimeTicks.  Eg. we could have a
> TimeTicks::MockableNow() and global functions to 'freeze' and manually advance
> the clock.  Switching back to free-running might be tricky (maybe add a
constant
> offset to all future times to avoid going backwards).
> 
> Either way I was concerned that it was potentially too hairy to reasonably ask
> Zeeshan to do now as a Noogler (I can just imagine the sort of nasty bugs that
> could arise when some code path is accidentally getting a real clock time
> instead of a mock clock time).  Maybe I'm wrong and this won't actually be
that
> hard though - it's intriguing and I'd love to see someone take a crack at it
at
> some point.  I've filed http://crbug.com/334639 to track with some of these
> thoughts.

Based on discussion with jamesr in http://crbug.com/334639, it sounds like a
full mock clock system is going to be too hard, and so we should proceed with
this higher-level approach for testing specific timers.  Do you agree Eric?

[jamesr, 2014-01-16 20:46:41]

I think you misunderstood.  A full mock time is hard, of course, but mocking
time in only some systems and not others is going to be a lot worse.  Why does
this particular thing need to be faked out?

[Rick Byers, 2014-01-16 21:35:54]

On 2014/01/16 20:46:41, jamesr wrote:
> I think you misunderstood.  A full mock time is hard, of course, but mocking
> time in only some systems and not others is going to be a lot worse.  Why does
> this particular thing need to be faked out?

What's the harm in mocking one particular timer?  This particular timer has no
time-based interaction with anything else in the system.  I agree that
interactions could be a problem in theory, but tackling a case at time which we
believe is safe to control manually seems fine.  We can't reliably test this
timer's full behavior without some way to make it predictable when under test
(note that several test scenarios here were disabled before Zeeshan added manual
control).

[eseidel, 2014-01-21 21:24:41]

I'm OK with this, as long as there is a larger plan to use this for more things.
 As a one-off it's somewhat expensive, but I can see that it enables a testing
use case we previously didn't cover.

Thank you for testing!

https://codereview.chromium.org/112023010/diff/510001/Source/core/page/EventH...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/112023010/diff/510001/Source/core/page/EventH...
Source/core/page/EventHandler.h:152: MockableTimer<EventHandler>*
getActiveIntervalTimerForTesting();
Normally we don't use "get" in our getters in Blink.

https://codereview.chromium.org/112023010/diff/510001/Source/platform/Timer.h
File Source/platform/Timer.h (right):

https://codereview.chromium.org/112023010/diff/510001/Source/platform/Timer.h...
Source/platform/Timer.h:130: // a mocked system clock that would make this
unnecessary.
If this is no longer the plan, we should update this comment.

[Zeeshan Qureshi, 2014-01-21 21:56:53]

On 2014/01/21 21:24:41, eseidel wrote:
> I'm OK with this, as long as there is a larger plan to use this for more
things.
>  As a one-off it's somewhat expensive, but I can see that it enables a testing
> use case we previously didn't cover.

We're certainly going to add more tests soon covering :active, :hover and
related actions.

>
https://codereview.chromium.org/112023010/diff/510001/Source/core/page/EventH...
> File Source/core/page/EventHandler.h (right):
> 
>
https://codereview.chromium.org/112023010/diff/510001/Source/core/page/EventH...
> Source/core/page/EventHandler.h:152: MockableTimer<EventHandler>*
> getActiveIntervalTimerForTesting();
> Normally we don't use "get" in our getters in Blink.

Done.


> https://codereview.chromium.org/112023010/diff/510001/Source/platform/Timer.h
> File Source/platform/Timer.h (right):
> 
>
https://codereview.chromium.org/112023010/diff/510001/Source/platform/Timer.h...
> Source/platform/Timer.h:130: // a mocked system clock that would make this
> unnecessary.
> If this is no longer the plan, we should update this comment.

Removed, I'm still going to take a stab at it later and see what happens.
Considering that we pause blink while debugging, having the blink clock out of
sync for tests might not be a big issue.