Improving the fake clock and using it to fix a flaky STUN timeout test.

Reland of: Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/f5f03e823c2d522e84fce46bc8a7d57f913032ba
Cr-Commit-Position: refs/heads/master@{#13052}

[Taylor Brandstetter, 2016-06-01 15:54:58]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2016-06-01 15:54:59]

https://codereview.webrtc.org/2024813004/diff/1/webrtc/base/gunit.h
File webrtc/base/gunit.h (right):

https://codereview.webrtc.org/2024813004/diff/1/webrtc/base/gunit.h#newcode28
webrtc/base/gunit.h:28: rtc::Thread::Current()->SleepMs(1);
I had to do this because if you do a "WAIT" while using a simulated clock,
ProcessMessages(1) will wait forever for the time to advance a millisecond.

[pthatcher1, 2016-06-01 22:38:13]

pthatcher@webrtc.org changed reviewers:
+ tommi@webrtc.org

[pthatcher1, 2016-06-01 22:38:14]

I think it would be good to have Tommi review this as well since it involves so
much threading magic.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h
File webrtc/base/fakeclock.h (right):

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:45: class ScopedFakeClock {
Does this work in C++?

class ScopedFakeClock : FakeClock {
 public:
  ScopedFakeClock() { SetClock(this); }
  ~ScopedFakeClock() { SetClock(nullptr); }
}

?

That way, the caller would't have to call "sfc.clock()".  They could just use
the clock.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:49: ~ScopedFakeClock() { SetClock(nullptr); }
Should we do this?

public:
 ScopedFakeClock() { old_clock_ = g_clock; SetClock(this); }
 ~ScopedFakeClock() { SetClock(old_clock_); }

private:
 old_clock_ = nullptr;



Otherwise, we might blow away a clock that was supposed to be there.  But I
don't know who will set g_clock to anything but nullptr except a unit test.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h
File webrtc/base/gunit.h (right):

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:28: rtc::Thread::Current()->SleepMs(1);
Did you intend the sleep to be in the for loop?  If so, I think you need some
{}s.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:28: rtc::Thread::Current()->SleepMs(1);
Would it make sense to have an AdvanceTime method like this?

void AdvanceTime(rtc::TimeDelta delta) {
  if (g_clock) {
    g_clock->AdvanceTime(delta);
    return;
  }
  rtc::Thread::Current()->ProcessMessages(0);
  rtc::Thread::Current()->SleepMs(1);
}

That way, the WAIT and SIMULATED_WAIT macros could be the same (and all the
calls to WAIT would magically work if we simply inserted a ScopedFakeClock?)

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:93: #define SIMULATED_WAIT(ex, clock, timeout)            \
Would it make sense for the clock to be the last arg on all of these?  Or the
first?  It seems weird in the middle.

[Taylor Brandstetter, 2016-06-01 23:28:37]

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h
File webrtc/base/fakeclock.h (right):

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:45: class ScopedFakeClock {
On 2016/06/01 22:38:14, pthatcher1 wrote:
> Does this work in C++?
> 
> class ScopedFakeClock : FakeClock {
>  public:
>   ScopedFakeClock() { SetClock(this); }
>   ~ScopedFakeClock() { SetClock(nullptr); }
> }
> 
> ?
> 
> That way, the caller would't have to call "sfc.clock()".  They could just use
> the clock.
> 
> 
> 

Done.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/fakeclock.h#n...
webrtc/base/fakeclock.h:49: ~ScopedFakeClock() { SetClock(nullptr); }
On 2016/06/01 22:38:14, pthatcher1 wrote:
> Should we do this?
> 
> public:
>  ScopedFakeClock() { old_clock_ = g_clock; SetClock(this); }
>  ~ScopedFakeClock() { SetClock(old_clock_); }
> 
> private:
>  old_clock_ = nullptr;
> 
> 
> 
> Otherwise, we might blow away a clock that was supposed to be there.  But I
> don't know who will set g_clock to anything but nullptr except a unit test.

I hadn't *intended* for there to be multiple fake clocks in use in the same
test. But I guess it's possible, so might as well do this.

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h
File webrtc/base/gunit.h (right):

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:28: rtc::Thread::Current()->SleepMs(1);
On 2016/06/01 22:38:14, pthatcher1 wrote:
> Would it make sense to have an AdvanceTime method like this?
> 
> void AdvanceTime(rtc::TimeDelta delta) {
>   if (g_clock) {
>     g_clock->AdvanceTime(delta);
>     return;
>   }
>   rtc::Thread::Current()->ProcessMessages(0);
>   rtc::Thread::Current()->SleepMs(1);
> }
> 
> That way, the WAIT and SIMULATED_WAIT macros could be the same (and all the
> calls to WAIT would magically work if we simply inserted a ScopedFakeClock?)

That seems like too much hidden magic for me. g_clock isn't public and isn't
intended to be. Plus, eventually didn't we plan to get rid of g_clock and have a
ClockInterface passed into everything that needs it?

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:28: rtc::Thread::Current()->SleepMs(1);
On 2016/06/01 22:38:14, pthatcher1 wrote:
> Did you intend the sleep to be in the for loop?  If so, I think you need some
> {}s.

Oops. I blame whoever initially wrote this code and didn't use {}s. :)

https://codereview.webrtc.org/2024813004/diff/20001/webrtc/base/gunit.h#newco...
webrtc/base/gunit.h:93: #define SIMULATED_WAIT(ex, clock, timeout)            \
On 2016/06/01 22:38:14, pthatcher1 wrote:
> Would it make sense for the clock to be the last arg on all of these?  Or the
> first?  It seems weird in the middle.

Sure, I don't have any preference.

[tommi, 2016-06-02 07:59:01]

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:133: // process messages as well.
Is the expectation that when this function finishes execution, that all queues
will be empty?
(if so, I don't see how that's guaranteed)

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:54: ClockInterface* SetClock(ClockInterface* clock);
Can we rename this method to SetClockForTesting?  I think it could have bad
consequences if we allow changing this value outside of tests.  Even with tests,
it could be bad if we have tests executing in parallel.

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils_uni...
File webrtc/base/timeutils_unittest.cc (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils_uni...
webrtc/base/timeutils_unittest.cc:367: Thread::Current()->SleepMs(1000);
A whole second seems like a lot.  Can we synchronize somehow instead?

[Taylor Brandstetter, 2016-06-02 17:19:54]

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/messagequeue....
webrtc/base/messagequeue.cc:133: // process messages as well.
On 2016/06/02 07:59:01, tommi-webrtc wrote:
> Is the expectation that when this function finishes execution, that all queues
> will be empty?
> (if so, I don't see how that's guaranteed)

Not that they're empty, but that they've processed all delayed message up until
the current time. For example, if there are messages queued at 10ms, 20ms, and
30ms, and you advance the time to 20ms, this ensures the first two messages will
be dispatched.

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils.h#n...
webrtc/base/timeutils.h:54: ClockInterface* SetClock(ClockInterface* clock);
On 2016/06/02 07:59:01, tommi-webrtc wrote:
> Can we rename this method to SetClockForTesting?  I think it could have bad
> consequences if we allow changing this value outside of tests.  Even with
tests,
> it could be bad if we have tests executing in parallel.

Done.

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils_uni...
File webrtc/base/timeutils_unittest.cc (right):

https://codereview.webrtc.org/2024813004/diff/40001/webrtc/base/timeutils_uni...
webrtc/base/timeutils_unittest.cc:367: Thread::Current()->SleepMs(1000);
On 2016/06/02 07:59:01, tommi-webrtc wrote:
> A whole second seems like a lot.  Can we synchronize somehow instead?

I picked a long time so that it's guaranteed to be long enough for a thread to
start. There's no way to synchronize unless we expose more of the thread's
current state; the goal is to make sure it's in a socket select(), but there's
no easy way to tell that.

[pthatcher1, 2016-06-02 18:48:04]

lgtm

[tommi, 2016-06-03 05:39:59]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2016-06-03 05:39:59]

lgtm

[commit-bot: I haz the power, 2016-06-03 05:40:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024813004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2024813004/60001

[commit-bot: I haz the power, 2016-06-03 05:40:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 05:40:55]

Try jobs failed on following builders:
  ios64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/253)
  ios64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/248)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14189)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[Taylor Brandstetter, 2016-06-03 22:29:44]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-06-03 22:29:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2024813004/#ps180001 (title:
"Fixing double linkage of testclock.cc.")

[commit-bot: I haz the power, 2016-06-03 22:29:51]

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

[commit-bot: I haz the power, 2016-06-03 22:31:33]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-03 22:31:41]

Description was changed from

==========
Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
==========

to

==========
Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

[commit-bot: I haz the power, 2016-06-03 22:31:42]

Patchset 10 (id:??) landed as
https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}

[Taylor Brandstetter, 2016-06-03 23:05:14]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.webrtc.org/2038213002/ by deadbeef@webrtc.org.

The reason for reverting is: There seems to be a TSan warning that wasn't caught
by the trybot:
https://build.chromium.org/p/client.webrtc/builders/Linux%20Tsan%20v2/builds/...

Apparently a thread is still alive even after destroying WebRTCSession. Need to
think of a way to fix this, without adding a critical section around g_clock
(since that would hurt performance)..

[Taylor Brandstetter, 2016-06-06 17:16:43]

Description was changed from

==========
Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

to

==========
Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

[Taylor Brandstetter, 2016-06-06 17:37:55]

Description was changed from

==========
Improving the fake clock and using it to fix a flaky STUN timeout test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

to

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

[Taylor Brandstetter, 2016-06-06 17:38:09]

Description was changed from

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
Cr-Commit-Position: refs/heads/master@{#13043}
==========

to

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
==========

[Taylor Brandstetter, 2016-06-06 17:38:25]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-06-06 17:38:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2024813004/#ps200001 (title:
"Fixing another TSan warning. WebRtcSession wasn't completely shut down.")

[commit-bot: I haz the power, 2016-06-06 17:38:32]

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

[commit-bot: I haz the power, 2016-06-06 18:16:09]

Description was changed from

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
==========

to

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
==========

[commit-bot: I haz the power, 2016-06-06 18:16:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-06 18:16:18]

Description was changed from

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
==========

to

==========
Reland of: Improving the fake clock and using it to fix a flaky STUN timeout
test.

When the fake clock's time is advanced, it now ensures all pending
queued messages have been dispatched. This allows us to write a
"SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
until the target time.

Useful in this case, where we know the STUN timeout should take a total
of 9500ms, but it would be overly complex to write test code that waits
for each individual timeout, ensures a STUN packet has been
retransmited, etc.

(The test described above *should* be written, but it belongs in
p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).

Committed: https://crrev.com/f5f03e823c2d522e84fce46bc8a7d57f913032ba
Cr-Commit-Position: refs/heads/master@{#13052}
==========

[commit-bot: I haz the power, 2016-06-06 18:16:19]

Patchset 11 (id:??) landed as
https://crrev.com/f5f03e823c2d522e84fce46bc8a7d57f913032ba
Cr-Commit-Position: refs/heads/master@{#13052}

[nisse-webrtc, 2016-06-08 09:47:28]

nisse@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2016-06-08 09:47:30]

Hi, here are a few late comments.

I haven't tried to really understand the interaction between the fake clock and
the thread machinery. 

Ideally, when using a fake clock, Thread::SleepMs() should just advance the fake
clock, but I have no idea how to make that work.

https://codereview.webrtc.org/2024813004/diff/200001/webrtc/base/fakeclock.cc
File webrtc/base/fakeclock.cc (right):

https://codereview.webrtc.org/2024813004/diff/200001/webrtc/base/fakeclock.cc...
webrtc/base/fakeclock.cc:47: SetClockForTesting(prev_clock_);
I think it's simpler to only ever allow one fake clock to be in use. I.e., let
the constructor RTC_CHECK that the old value was null, an let the destructor set
it back to null.

Or else, add an RTC_CHECK to the destructor to verify that the return value of
SetClockForTesting is |this|.

https://codereview.webrtc.org/2024813004/diff/200001/webrtc/base/timeutils.h
File webrtc/base/timeutils.h (right):

https://codereview.webrtc.org/2024813004/diff/200001/webrtc/base/timeutils.h#...
webrtc/base/timeutils.h:51: // TODO(deadbeef): Instead of having functions that
access this global
I don't think it's a good idea to pass around the clock everywhere (even if
there's some code using webrtc::Clock doing that).

Except maybe if we can cut down a lot on the number of places where the clock is
read. And in many cases, I think passing the current time as an argument
downstream makes more sense than passing down a clock object.