Add a Clock and TickClock interface for mocking out time

Add a Clock and TickClock interface for mocking out time

Add DefaultClock and DefaultTickClock implementations.

Add SimpleTestClock and SimpleTestTickClock implementations for test.

Port some classes in sync/ and media/ to use SimpleTest{,Tick}Clock.

BUG=166469
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183865

[akalin, 2012-12-17 19:44:20]

+brettw for base/ and overall review
+rlarocque for sync/
+scherkus for media/

[Ryan Sleevi, 2012-12-17 19:54:45]

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc
File base/default_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc#newcode12
base/default_clock.cc:12: LazyInstance<DefaultClock> default_clock =
LAZY_INSTANCE_INITIALIZER;
DESIGN: Should this be Leaky, to match the worker-safety afforded by
Time::Now/TimeTicks::Now()

https://codereview.chromium.org/11607003/diff/1/base/default_tick_clock.cc
File base/default_tick_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/default_tick_clock.cc#ne...
base/default_tick_clock.cc:12: LazyInstance<DefaultTickClock> default_tick_clock
= LAZY_INSTANCE_INITIALIZER;
DESIGN: Same here

[awong, 2012-12-17 19:55:11]

drive-by nits. No need to wait for my LG.

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc
File base/default_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc#newcode12
base/default_clock.cc:12: LazyInstance<DefaultClock> default_clock =
LAZY_INSTANCE_INITIALIZER;
nit: g_default_clock

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_clock.h
File base/test/simple_test_clock.h (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_clock.h...
base/test/simple_test_clock.h:17: // the returned Time objects.
Describe thread safety

[jar (doing other things), 2012-12-17 20:29:21]

It bugs me a bit that we can't control the global TimeTicks, and hence we have
some competing clocks in the system (for stuff like PostDelayedTask() etc.). 
That said, having a mock clock for folks that can isolate themselves does seem
useful.

Drive-by nits below.

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
File base/test/simple_test_tick_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
base/test/simple_test_tick_clock.cc:20: now_ticks_ += delta;
nit: consider DCHECK_GE(delta, TimeDelta());

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
File base/test/simple_test_tick_clock.h (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
base/test/simple_test_tick_clock.h:31: // Protects |now_|.
nit: now_ --> now_ticks_

https://codereview.chromium.org/11607003/diff/1/sync/notifier/ack_tracker_uni...
File sync/notifier/ack_tracker_unittest.cc (right):

https://codereview.chromium.org/11607003/diff/1/sync/notifier/ack_tracker_uni...
sync/notifier/ack_tracker_unittest.cc:55: tick_clock_(tick_clock) {
nit: I like the use of "tick_clock" here.  It helps clarify that this is more
kindred of TimeTicks() than of Time() (which historically has been a point of
confusion).  Perchance you should use a base of TickClock() as a class name
rather than Clock() in some of your other code.

[rlarocque, 2012-12-17 20:38:44]

Sync changes LGTM.

[akalin, 2012-12-17 22:38:31]

Comments addressed, PTAL

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc
File base/default_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc#newcode12
base/default_clock.cc:12: LazyInstance<DefaultClock> default_clock =
LAZY_INSTANCE_INITIALIZER;
On 2012/12/17 19:54:46, Ryan Sleevi wrote:
> DESIGN: Should this be Leaky, to match the worker-safety afforded by
> Time::Now/TimeTicks::Now()

Brett didn't like the lazy instance, so I removed it.

https://codereview.chromium.org/11607003/diff/1/base/default_clock.cc#newcode12
base/default_clock.cc:12: LazyInstance<DefaultClock> default_clock =
LAZY_INSTANCE_INITIALIZER;
On 2012/12/17 19:55:11, awong wrote:
> nit: g_default_clock

removed lazy instance

https://codereview.chromium.org/11607003/diff/1/base/default_tick_clock.cc
File base/default_tick_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/default_tick_clock.cc#ne...
base/default_tick_clock.cc:12: LazyInstance<DefaultTickClock> default_tick_clock
= LAZY_INSTANCE_INITIALIZER;
On 2012/12/17 19:54:46, Ryan Sleevi wrote:
> DESIGN: Same here

also removed this

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_clock.h
File base/test/simple_test_clock.h (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_clock.h...
base/test/simple_test_clock.h:17: // the returned Time objects.
On 2012/12/17 19:55:11, awong wrote:
> Describe thread safety

Done.

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
File base/test/simple_test_tick_clock.cc (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
base/test/simple_test_tick_clock.cc:20: now_ticks_ += delta;
On 2012/12/17 20:29:21, jar wrote:
> nit: consider DCHECK_GE(delta, TimeDelta());

Done.  Although it has to be DCHECK( >= ) since TimeDelta doesn't overload <<.

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
File base/test/simple_test_tick_clock.h (right):

https://codereview.chromium.org/11607003/diff/1/base/test/simple_test_tick_cl...
base/test/simple_test_tick_clock.h:31: // Protects |now_|.
On 2012/12/17 20:29:21, jar wrote:
> nit: now_ --> now_ticks_

Done.

https://codereview.chromium.org/11607003/diff/1/sync/notifier/ack_tracker_uni...
File sync/notifier/ack_tracker_unittest.cc (right):

https://codereview.chromium.org/11607003/diff/1/sync/notifier/ack_tracker_uni...
sync/notifier/ack_tracker_unittest.cc:55: tick_clock_(tick_clock) {
On 2012/12/17 20:29:21, jar wrote:
> Perchance you should use a base of TickClock() as a class name
> rather than Clock() in some of your other code.  

Yeah, I've made sure to have 'TickClock' in the name for anything that inherits
from TickClock.  If I run into places where TickClock is called for, I'll
probably name it 'tick_clock_' or similar.

[akalin, 2012-12-17 23:20:10]

Pasting in Brett's comments from the other thread:

On Mon, Dec 17, 2012 at 12:15 PM, Brett Wilson <brettw@google.com> wrote:
> As I said, I'm not particularly wedded to one approach or another. I
> can see some disadvantages by doing the big global thing, but having
> to have each object you need to mock have a separate clock interface
> sounds like a pain.
> 
> I'm not a fan of the leaky singleton in Fred's proposed patch:
> https://codereview.chromium.org/11607003
> In the old "global clock override" patch there is a pointer that's
> just non-null if you've instantiated a mock clock object, and this
> gets cleaned up when the test goes out of scope. I think this is a
> cleaner solution.
> 
> I'm also concerned about all the new files in the new patch, it seems
> like 6 new files in base is "a lot" for something that seems
> relatively simple in my mind.
> 
> I prefer the original global mock solution for these reasons.
> 
> Brett

[scherkus (not reviewing), 2012-12-17 23:25:47]

media/ lgtm

https://codereview.chromium.org/11607003/diff/21001/media/base/pipeline_unitt...
File media/base/pipeline_unittest.cc (left):

https://codereview.chromium.org/11607003/diff/21001/media/base/pipeline_unitt...
media/base/pipeline_unittest.cc:586: static base::Time StaticClockFunction() {
haha wow am I ever glad this is getting deleted

[akalin, 2012-12-17 23:30:12]

Pasting in my reply to Brett's comments:

On Mon, Dec 17, 2012 at 12:50 PM, Fred Akalin <akalin@google.com> wrote:
> On Mon, Dec 17, 2012 at 12:15 PM, Brett Wilson <brettw@google.com> wrote:
> As I said, I'm not particularly wedded to one approach or another. I
> can see some disadvantages by doing the big global thing, but having
> to have each object you need to mock have a separate clock interface
> sounds like a pain.

But that's what we've been doing until now!  The only pain seems to have been
the lack of a single interface in base/ and everyone writing their own Clock
class.
 
> I'm not a fan of the leaky singleton in Fred's proposed patch:
> https://codereview.chromium.org/11607003

The leaky instance (not singleton) is just a convenience, and can be removed. 
Although that would mean that there will be a bunch of DefaultSystemClock
objects living in various classes.  But that might be okay.

Also, some folks pointed out in the code review that I may be able to get rid of
the leaky part.

> In the old "global clock override" patch there is a pointer that's
> just non-null if you've instantiated a mock clock object, and this
> gets cleaned up when the test goes out of scope. I think this is a
> cleaner solution.

I don't think having a global clock override is incompatible with having a Clock
interface.  I just don't want to add a global clock override until we have a use
case for it.  11453012 wasn't a very strong case; it was a unit test, and a
global clock override seems more suited to an integration test.  Plus, it seemed
like there were classes that could have had time dep-injected instead of having
the global override, but the global clock override was put in just because of
time pressure.
 
> I'm also concerned about all the new files in the new patch, it seems
> like 6 new files in base is "a lot" for something that seems
> relatively simple in my mind.

Measuring complexity by number of files seems dubious to me, since each file is
quite short and simple.  I suppose I could get it down to 3 new files (and/or
classes) if you really wanted.  But it's worth pointing out that I will be
getting rid of MockTimeProvider (in a follow-up patch) so that's a net of 4 new
files, or 2 if I combine Clock and TickClock.  That's about the same as the
solution in 11453012, which added 2 files and touched all the time.h
implementations.
 
> I prefer the original global mock solution for these reasons.

To summarize my position:
- We're already mocking out time in unit tests, but doing so in an ad hoc way. 
I simply want to combine those.
- Having a Clock class isn't incompatible with having a global clock override,
although I'd like to avoid doing the latter until we have a solid use case for
it.
- Having static instances for DefaultClock/DefaultTickClock is a convenience and
can be removed.

[akalin, 2012-12-17 23:32:23]

Pasting in Scott's reply:

> On Mon, Dec 17, 2012 at 2:25 PM, Scott Violet <sky@google.com> wrote:
> On Mon, Dec 17, 2012 at 12:15 PM, Brett Wilson <brettw@google.com> wrote:
> > As I said, I'm not particularly wedded to one approach or another. I
> > can see some disadvantages by doing the big global thing, but having
> > to have each object you need to mock have a separate clock interface
> > sounds like a pain.
> >
> > I'm not a fan of the leaky singleton in Fred's proposed patch:
> > https://codereview.chromium.org/11607003
> > In the old "global clock override" patch there is a pointer that's
> > just non-null if you've instantiated a mock clock object, and this
> > gets cleaned up when the test goes out of scope. I think this is a
> > cleaner solution.
> >
> > I'm also concerned about all the new files in the new patch, it seems
> > like 6 new files in base is "a lot" for something that seems
> > relatively simple in my mind.
> >
> > I prefer the original global mock solution for these reasons.
> 
> +1
>

[Denis Kuznetsov (DE-MUC), 2012-12-18 13:40:57]

On 2012/12/17 23:32:23, akalin wrote:
> Pasting in Scott's reply:
> 
> > On Mon, Dec 17, 2012 at 2:25 PM, Scott Violet <mailto:sky@google.com> wrote:
> > On Mon, Dec 17, 2012 at 12:15 PM, Brett Wilson <mailto:brettw@google.com>
wrote:
> > > As I said, I'm not particularly wedded to one approach or another. I
> > > can see some disadvantages by doing the big global thing, but having
> > > to have each object you need to mock have a separate clock interface
> > > sounds like a pain.
> > >
> > > I'm not a fan of the leaky singleton in Fred's proposed patch:
> > > https://codereview.chromium.org/11607003
> > > In the old "global clock override" patch there is a pointer that's
> > > just non-null if you've instantiated a mock clock object, and this
> > > gets cleaned up when the test goes out of scope. I think this is a
> > > cleaner solution.
> > >
> > > I'm also concerned about all the new files in the new patch, it seems
> > > like 6 new files in base is "a lot" for something that seems
> > > relatively simple in my mind.
> > >
> > > I prefer the original global mock solution for these reasons.
> > 
> > +1
> >
Just my two cents: this approach would not help in testing done in
https://codereview.chromium.org/11453012/.
It has a high-level test like "press power button, wait for lock animation to go
half way through, release button, and make sure that callback associated with
animation end (actual locking) is not called".
As animation should be stopped half way through, LayerAnimator's property
"disable_timer_for_test_" can not be used, as it finishes animations instantly.
So there is a need to call timer's callback manually, and advance time somehow
to make animator believe it progresses. Animator code uses TimeTicks::Now across
several places : ui/compositor/layer_animator and
ui/base/animation/animation_container. 
However, there are several layers of classes between test itself and
AnimationContainer, several of them are constructed implicitly, so passing
substituted Clock object can be non-trivial.

[akalin, 2012-12-18 16:24:10]

On 2012/12/18 13:40:57, Denis Kuznetsov wrote:
> Just my two cents: this approach would not help in testing done in
> https://codereview.chromium.org/11453012/.
> It has a high-level test like "press power button, wait for lock animation to
go
> half way through, release button, and make sure that callback associated with
> animation end (actual locking) is not called".
> As animation should be stopped half way through, LayerAnimator's property
> "disable_timer_for_test_" can not be used, as it finishes animations
instantly.
> So there is a need to call timer's callback manually, and advance time somehow
> to make animator believe it progresses. Animator code uses TimeTicks::Now
across
> several places : ui/compositor/layer_animator and
> ui/base/animation/animation_container. 
> However, there are several layers of classes between test itself and
> AnimationContainer, several of them are constructed implicitly, so passing
> substituted Clock object can be non-trivial.

$ git grep 'TimeTicks::Now' ui/
ui/aura/bench/bench_main.cc:      start_time_ = TimeTicks::Now();
ui/aura/bench/bench_main.cc:        TimeTicks now = TimeTicks::Now();
ui/base/animation/animation_container.cc:    : last_tick_time_(TimeTicks::Now())
ui/base/animation/animation_container.cc:    last_tick_time_ = TimeTicks::Now();
ui/base/animation/animation_container.cc:  TimeTicks current_time = TimeTicks::N
ui/compositor/layer_animator.cc:      last_step_time_ = base::TimeTicks::Now();
ui/compositor/layer_animator.cc:  // All clear, actually start the sequence. Not
ui/compositor/layer_animator.cc:    start_time = base::TimeTicks::Now();
ui/compositor/layer_animator_unittest.cc:  base::TimeTicks now = base::TimeTicks
ui/compositor/layer_animator_unittest.cc:  delta = base::TimeTicks::Now() - anim
ui/views/corewm/window_animations_unittest.cc:  element->Step(base::TimeTicks::N
ui/views/corewm/window_animations_unittest.cc:      base::TimeTicks::Now() + bas
ui/views/corewm/window_animations_unittest.cc:      Step(base::TimeTicks::Now() 
ui/views/corewm/window_animations_unittest.cc:      base::TimeTicks::Now() + bas
ui/views/corewm/window_animations_unittest.cc:      Step(base::TimeTicks::Now() 

So there are 8 places where TimeTIcks::Now() is called in ui code (minus tests).
 Changing those places to use e.g. tick_clock->Now() seems doable to me.

[Denis Kuznetsov (DE-MUC), 2012-12-19 13:41:57]

On 2012/12/18 16:24:10, akalin wrote:
> On 2012/12/18 13:40:57, Denis Kuznetsov wrote:
> > Just my two cents: this approach would not help in testing done in
> > https://codereview.chromium.org/11453012/.
> > It has a high-level test like "press power button, wait for lock animation
to
> go
> > half way through, release button, and make sure that callback associated
with
> > animation end (actual locking) is not called".
> > As animation should be stopped half way through, LayerAnimator's property
> > "disable_timer_for_test_" can not be used, as it finishes animations
> instantly.
> > So there is a need to call timer's callback manually, and advance time
somehow
> > to make animator believe it progresses. Animator code uses TimeTicks::Now
> across
> > several places : ui/compositor/layer_animator and
> > ui/base/animation/animation_container. 
> > However, there are several layers of classes between test itself and
> > AnimationContainer, several of them are constructed implicitly, so passing
> > substituted Clock object can be non-trivial.
> 
> $ git grep 'TimeTicks::Now' ui/
> ui/aura/bench/bench_main.cc:      start_time_ = TimeTicks::Now();
> ui/aura/bench/bench_main.cc:        TimeTicks now = TimeTicks::Now();
> ui/base/animation/animation_container.cc:    :
last_tick_time_(TimeTicks::Now())
> ui/base/animation/animation_container.cc:    last_tick_time_ =
TimeTicks::Now();
> ui/base/animation/animation_container.cc:  TimeTicks current_time =
TimeTicks::N
> ui/compositor/layer_animator.cc:      last_step_time_ =
base::TimeTicks::Now();
> ui/compositor/layer_animator.cc:  // All clear, actually start the sequence.
Not
> ui/compositor/layer_animator.cc:    start_time = base::TimeTicks::Now();
> ui/compositor/layer_animator_unittest.cc:  base::TimeTicks now =
base::TimeTicks
> ui/compositor/layer_animator_unittest.cc:  delta = base::TimeTicks::Now() -
anim
> ui/views/corewm/window_animations_unittest.cc: 
element->Step(base::TimeTicks::N
> ui/views/corewm/window_animations_unittest.cc:      base::TimeTicks::Now() +
bas
> ui/views/corewm/window_animations_unittest.cc:     
Step(base::TimeTicks::Now() 
> ui/views/corewm/window_animations_unittest.cc:      base::TimeTicks::Now() +
bas
> ui/views/corewm/window_animations_unittest.cc:     
Step(base::TimeTicks::Now() 
> 
> So there are 8 places where TimeTIcks::Now() is called in ui code (minus
tests).
>  Changing those places to use e.g. tick_clock->Now() seems doable to me.

There is no problem with changing TimeTicks::Now to tick_clock->Now(), but
passing that Clock to the place can be troublesome.
To ensure that time is monotonic, Clock need to be specified in constructor,
e.g. in constructor of AnimationContainer.
But LayerAnimator creates AnimationContainer as a static variable inside the
function (see layer_animator.cc:39).
LayerAnimator is lazily created in Layer, and layer is created when 
Window::Init method is called, and so forth. So it is not clear what is the
entry point for setting either default Clock or test one.
Or we can choose to start that objects with default Clock, and provide a method
for replacing that on fly, despite no guarantee of monotonicity - but we will
have to look up all those objects : at some point find all windows, take their
layers, get their animators and substitute Clocks for them.

[Denis Kuznetsov (DE-MUC), 2012-12-19 16:40:33]

In other words, this approach is nice when you need to test time-dependant class
directly. However, in more complex tests, when the class to be tested depends on
time indirectly, via several other classes, especially when some of them are
either static or lazily-created, this approach shows it's weak sides: need to
collect all objects that need Clock to be mocked or need to introduce some
static single point that would let to override Clocks for these classes at once
- but this is exactly the situation this CL tries to avoid.

[Nikita (slow), 2012-12-21 11:47:59]

+Ian for considerations on animations testing (see latest comments by Denis)

[Denis Kuznetsov (DE-MUC), 2012-12-21 12:41:45]

I think that correct time mocking should look like this:
* Mocking  should be installed as a first part of the test, and removed as the
last part (e.g. in SetUp/TearDown) - it will isolate tests with mocked time.
* Mocking should preempt "normal time", e.g. Time::Now() after mocking >=
Time::Now() right before mocking.
* Mocking should provide API that let change time in only "normal" way - at most
ToManualMode / Advance / ToAutoMode / SetScaleFromNormal (latter can be used
with instrumented tests to compensate introduced slowness in timeouts without
changing code).
* Mocking should affect Time::Now() / TimeTicks::Now(), as well as something in
Timer/ThreadTaskRunnerHandle/MessageLoop pipe to mock Timer behavior as well.
* Mocking should perform additional checks for correct use (e.g. Time::Now()
calls can check that thread they are called in has Timer mocked).

It looks a bit of overkill for simple unittests that test just one class, but
allow reuse in larger level - something close to test_util :
RunAllPendingInMessageLoop(), but with fine Timer handling.

[akalin, 2012-12-22 07:50:38]

On Fri, Dec 21, 2012 at 4:41 AM, <antrim@chromium.org> wrote:

> I think that correct time mocking should look like this:
> * Mocking  should be installed as a first part of the test, and removed as
> the
> last part (e.g. in SetUp/TearDown) - it will isolate tests with mocked
> time.
> * Mocking should preempt "normal time", e.g. Time::Now() after mocking >=
> Time::Now() right before mocking.
> * Mocking should provide API that let change time in only "normal" way -
> at most
> ToManualMode / Advance / ToAutoMode / SetScaleFromNormal (latter can be
> used
> with instrumented tests to compensate introduced slowness in timeouts
> without
> changing code).
> * Mocking should affect Time::Now() / TimeTicks::Now(), as well as
> something in
> Timer/ThreadTaskRunnerHandle/**MessageLoop pipe to mock Timer behavior as
> well.
> * Mocking should perform additional checks for correct use (e.g.
> Time::Now()
> calls can check that thread they are called in has Timer mocked).
>
> It looks a bit of overkill for simple unittests that test just one class,
> but
> allow reuse in larger level - something close to test_util :
> RunAllPendingInMessageLoop(), but with fine Timer handling.
>
>
https://codereview.chromium.**org/11607003/<https://codereview.chromium.org/1...
>

This seems quite restrictive.  Time::Now() has no restrictions as to what
it can return -- time can jump backwards if the user sets his clock.
 TimeTicks::Now() has only the restriction that it is monotonic -- time can
stand still if the computer is suspended.  Time::Now() and TimeTicks::Now()
can change independently, so it doesn't make sense to tie them together.
 If tests need more restrictions, they can enforce it themselves.

I think allowing TimeTicks to be overridden on a per-MessageLoop basis gets
you 90% of where you want to be, and avoids my concern with rampant
globals.  (Note that MessageLoops don't use Time::Now, and [jar@ can tell
you more] classes shouldn't be relying on Time::Now for timing-sensitive
things.)  I wonder if that's the correct way forward.

[Denis Kuznetsov (DE-MUC), 2012-12-26 12:08:58]

On 2012/12/22 07:50:38, akalin wrote:
> On Fri, Dec 21, 2012 at 4:41 AM, <mailto:antrim@chromium.org> wrote:
> 
> > I think that correct time mocking should look like this:
> > * Mocking  should be installed as a first part of the test, and removed as
> > the
> > last part (e.g. in SetUp/TearDown) - it will isolate tests with mocked
> > time.
> > * Mocking should preempt "normal time", e.g. Time::Now() after mocking >=
> > Time::Now() right before mocking.
> > * Mocking should provide API that let change time in only "normal" way -
> > at most
> > ToManualMode / Advance / ToAutoMode / SetScaleFromNormal (latter can be
> > used
> > with instrumented tests to compensate introduced slowness in timeouts
> > without
> > changing code).
> > * Mocking should affect Time::Now() / TimeTicks::Now(), as well as
> > something in
> > Timer/ThreadTaskRunnerHandle/**MessageLoop pipe to mock Timer behavior as
> > well.
> > * Mocking should perform additional checks for correct use (e.g.
> > Time::Now()
> > calls can check that thread they are called in has Timer mocked).
> >
> > It looks a bit of overkill for simple unittests that test just one class,
> > but
> > allow reuse in larger level - something close to test_util :
> > RunAllPendingInMessageLoop(), but with fine Timer handling.
> >
> >
>
https://codereview.chromium.**org/11607003/%3Chttps://codereview.chromium.org...>
> >
> 
> This seems quite restrictive.  Time::Now() has no restrictions as to what
> it can return -- time can jump backwards if the user sets his clock.
>  TimeTicks::Now() has only the restriction that it is monotonic -- time can
> stand still if the computer is suspended.  Time::Now() and TimeTicks::Now()
> can change independently, so it doesn't make sense to tie them together.
>  If tests need more restrictions, they can enforce it themselves.

I started with most restrictive approach to show that time mocking can be done
in a somewhat safe way, and to find possible additional values of time mocking,
i.e. compensating slowness of *san builds to avoid timeouts.

By the way, it seems that there are lots of places that misuse Time::Now()
instead of Timeticks::Now() - timeouts, metrics, etc. They can break when, for
instance, time gets updated via NTP.
 
> I think allowing TimeTicks to be overridden on a per-MessageLoop basis gets
> you 90% of where you want to be, and avoids my concern with rampant
> globals.  (Note that MessageLoops don't use Time::Now, and [jar@ can tell
> you more] classes shouldn't be relying on Time::Now for timing-sensitive
> things.)  I wonder if that's the correct way forward.

Still, we need to agree on a way to implement that: easy to use in tests, mock
only what it tests, and protection from using it in production code. 
I feel that some private static time provider in TimeTicks::Now() that can be
only set from friend-class in test namespace will work for TimeTicks, but I'm
not sure about a way to deal with MessageLoops.

[akalin, 2013-02-20 21:32:55]

Hello, everyone.  I'm resurrecting this CL.  Brett, do you still have concerns? 
I think everyone else is okay with this interface.  Also, this is blocking a few
other items: https://code.google.com/p/chromium/issues/detail?id=166469

[brettw, 2013-02-20 23:37:18]

I spoke to some people. I think it's the same: nobody really cares that much
either way enough to stop you, but nobody is pushing for it either.

I've been trying to make the root base directory smaller and this is adding a
bunch more stuff. I think we should add a new base/time/ directory for this
stuff and move the current time/timer/perftimer stuff in there.

We should also be sure to update the existing mock time thingy which you wrote a
TODO for.

So if you're can add your stuff to the new directory and promise to help out
with moving the other stuff (future, obviously), I will promise to
enthusiastically support this addition.

[akalin, 2013-02-21 01:11:43]

On 2013/02/20 23:37:18, brettw wrote:
> I spoke to some people. I think it's the same: nobody really cares that much
> either way enough to stop you, but nobody is pushing for it either.

Okay, fair enough.

> I've been trying to make the root base directory smaller and this is adding a
> bunch more stuff. I think we should add a new base/time/ directory for this
> stuff and move the current time/timer/perftimer stuff in there.

Done!  Also added TODO to move time* in time/. (I can do that in another CL.)

> We should also be sure to update the existing mock time thingy which you wrote
a
> TODO for.

What do you mean exactly?  There are only a few places that actually use that
thing, and I have CLs to port them to using these classes and then remove it.

> So if you're can add your stuff to the new directory and promise to help out
> with moving the other stuff (future, obviously), I will promise to
> enthusiastically support this addition.

You have a deal!

(Please take another look.)

[brettw, 2013-02-21 18:15:06]

base lgtm

[commit-bot: I haz the power, 2013-02-21 18:19:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/11607003/39001

[commit-bot: I haz the power, 2013-02-21 19:04:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-21 19:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/11607003/44004

[akalin, 2013-02-21 21:50:52]

Committed patchset #6 manually as r183865 (presubmit successful).