Add support in TestMockTimeTaskRunner for vending out mock Time and mock Clocks.

base/test/test_mock_time_task_runner.cc

Index: base/test/test_mock_time_task_runner.cc
diff --git a/base/test/test_mock_time_task_runner.cc b/base/test/test_mock_time_task_runner.cc
index 442d99190b0b87060337b6b67d9d60f7f1d47051..7042bfe51c757accb5883d2c0e990d4b1e1a486d 100644
--- a/base/test/test_mock_time_task_runner.cc
+++ b/base/test/test_mock_time_task_runner.cc
@@ -6,13 +6,17 @@
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/time/clock.h"
+#include "base/time/tick_clock.h"
 
 namespace base {
 
 namespace {
 
-// TickClock that always returns the then-current mock time of |task_runner| as
-// the current time.
+// MockTickClock --------------------------------------------------------------
+
+// TickClock that always returns the then-current mock time ticks of
+// |task_runner| as the current time ticks.
 class MockTickClock : public TickClock {
  public:
   explicit MockTickClock(
@@ -37,18 +41,49 @@ MockTickClock::~MockTickClock() {
 }
 
 TimeTicks MockTickClock::NowTicks() {
-  return task_runner_->GetCurrentMockTime();
+  return task_runner_->NowTicks();
+}
+
+// MockClock ------------------------------------------------------------------
+
+// Clock that always returns the then-current mock time of |task_runner| as the
+// current time.
+class MockClock : public Clock {
+ public:
+  explicit MockClock(scoped_refptr<const TestMockTimeTaskRunner> task_runner);
+  ~MockClock() override;

[bartfab (slow), 2015/02/12 09:50:55]

Nit: No need to override the destructor if your new destructor is empty (the
same applies to MockTickClock).

[engedy, 2015/02/12 11:27:03]

Done.

+
+  // Clock:
+  Time Now() override;
+
+ private:
+  scoped_refptr<const TestMockTimeTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockClock);
+};
+
+MockClock::MockClock(scoped_refptr<const TestMockTimeTaskRunner> task_runner)
+    : task_runner_(task_runner) {
+}
+
+MockClock::~MockClock() {
+}
+
+Time MockClock::Now() {
+  return task_runner_->Now();
 }
 
 }  // namespace
 
+// TestMockTimeTaskRunner -----------------------------------------------------
+
 bool TestMockTimeTaskRunner::TemporalOrder::operator()(
     const TestPendingTask& first_task,
     const TestPendingTask& second_task) const {
   return first_task.GetTimeToRun() > second_task.GetTimeToRun();
 }
 
-TestMockTimeTaskRunner::TestMockTimeTaskRunner() {
+TestMockTimeTaskRunner::TestMockTimeTaskRunner() : now_(Time::UnixEpoch()) {
 }
 
 TestMockTimeTaskRunner::~TestMockTimeTaskRunner() {
@@ -56,42 +91,37 @@ TestMockTimeTaskRunner::~TestMockTimeTaskRunner() {
 
 void TestMockTimeTaskRunner::FastForwardBy(TimeDelta delta) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_GE(delta, TimeDelta());

[bartfab (slow), 2015/02/12 09:50:55]

Nit 1: Expected value first, actual value second.
Nit 2: This will be DCHECK()ed by ProcessAllTasksNoLaterThan() for you.

[engedy, 2015/02/12 11:27:03]

I believe the current order is better. DCHECK makes no distinction between the
first and second argument. On failure, it will print:

Check failed:
  delta >= TimeDelta()

Which seems to be more in line with prevalent Chromium style that the constant
is on the right hand side in comparisons/equivalence tests.  
I would prefer to also have a DCHECK here. I like it because it formalizes the
precondition mentioned in the declaration-side comment that the delta must be
non-negative. It also makes life easier for those who misuse this class: as
opposed to seeing a DCHECK fail in an internal implementation detail, they see a
DCHECK fail in a public method, so it is even more obvious they are using the
API incorrectly.

[bartfab (slow), 2015/02/12 12:36:07]

You are right that DCHECK_xx() does not eforce an argument order. But for
consistency with the semantically strongly related EXPECT_xx() and ASSERT_xx(),
which are sensitive to argument order in the messages they print, it is
customary to put the expected value first.

Ack on having a DCHECK() here, in the public method.

[engedy, 2015/02/13 11:08:05]

Yes, but there is also EXPECT_THAT, which in turn requires the actual value
first and the expectation second. So, instead of going into lengthy arguments
about which semantical relationships are stronger, let us be consistent with
other usages of DCHECK_GE itself in src/base:   

https://code.google.com/p/chromium/codesearch#search/&q=DCHECK_GE%20package:%...

This shows about 40-50 usages; and actually, there is not a single usage with
the expectation first. Therefore, I would propose keeping the current order.

[bartfab (slow), 2015/02/16 13:40:34]

Acknowledged.

 
-  OnBeforeSelectingTask();
-
-  const base::TimeTicks original_now = now_;
-  TestPendingTask task_info;
-  while (DequeueNextTask(original_now, delta, &task_info)) {
-    if (task_info.GetTimeToRun() - now_ > base::TimeDelta()) {
-      now_ = task_info.GetTimeToRun();
-      OnAfterTimePassed();
-    }
-
-    task_info.task.Run();
-
-    OnAfterTaskRun();
-    OnBeforeSelectingTask();
-  }
-
-  if (!delta.is_max() && now_ - original_now < delta) {
-    now_ = original_now + delta;
-    OnAfterTimePassed();
-  }
+  TimeTicks original_now_ticks = now_ticks_;

[bartfab (slow), 2015/02/12 09:50:55]

Nit: const.

[engedy, 2015/02/12 11:27:03]

Done.

+  ProcessAllTasksNoLaterThan(delta);
+  ForwardClocksUntilTickTime(original_now_ticks + delta);
 }
 
 void TestMockTimeTaskRunner::RunUntilIdle() {
-  FastForwardBy(TimeDelta());
+  ProcessAllTasksNoLaterThan(TimeDelta());

[bartfab (slow), 2015/02/12 09:50:55]

Nit: Add DCHECK(thread_checker_.CalledOnValidThread()).

[engedy, 2015/02/12 11:27:03]

Done.

 }
 
 void TestMockTimeTaskRunner::FastForwardUntilNoTasksRemain() {
-  FastForwardBy(TimeDelta::Max());
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ProcessAllTasksNoLaterThan(TimeDelta::Max());
 }
 
-TimeTicks TestMockTimeTaskRunner::GetCurrentMockTime() const {
+Time TestMockTimeTaskRunner::Now() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return now_;
 }
 
+TimeTicks TestMockTimeTaskRunner::NowTicks() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return now_ticks_;
+}
+
+scoped_ptr<Clock> TestMockTimeTaskRunner::GetMockClock() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return make_scoped_ptr(new MockClock(this));
+}
+
 scoped_ptr<TickClock> TestMockTimeTaskRunner::GetMockTickClock() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return make_scoped_ptr(new MockTickClock(this));
@@ -109,7 +139,8 @@ size_t TestMockTimeTaskRunner::GetPendingTaskCount() const {
 
 TimeDelta TestMockTimeTaskRunner::NextPendingTaskDelay() const {
   DCHECK(thread_checker_.CalledOnValidThread());
-  return tasks_.empty() ? TimeDelta::Max() : tasks_.top().GetTimeToRun() - now_;
+  return tasks_.empty() ? TimeDelta::Max()
+                        : tasks_.top().GetTimeToRun() - now_ticks_;
 }
 
 bool TestMockTimeTaskRunner::RunsTasksOnCurrentThread() const {
@@ -120,9 +151,9 @@ bool TestMockTimeTaskRunner::PostDelayedTask(
     const tracked_objects::Location& from_here,
     const Closure& task,
     TimeDelta delay) {
-  base::AutoLock scoped_lock(tasks_lock_);
-  tasks_.push(
-      TestPendingTask(from_here, task, now_, delay, TestPendingTask::NESTABLE));
+  AutoLock scoped_lock(tasks_lock_);
+  tasks_.push(TestPendingTask(from_here, task, now_ticks_, delay,
+                              TestPendingTask::NESTABLE));
   return true;
 }
 
@@ -146,10 +177,37 @@ void TestMockTimeTaskRunner::OnAfterTaskRun() {
   // Empty default implementation.
 }
 
-bool TestMockTimeTaskRunner::DequeueNextTask(const base::TimeTicks& reference,
-                                             const base::TimeDelta& max_delta,
+void TestMockTimeTaskRunner::ProcessAllTasksNoLaterThan(
+    TimeDelta max_delta) {
+  DCHECK_GE(max_delta, TimeDelta());

[bartfab (slow), 2015/02/12 09:50:55]

Expected value first, actual value second.

[engedy, 2015/02/12 11:27:03]

Please see above.

[bartfab (slow), 2015/02/12 12:36:08]

As above :).

[engedy, 2015/02/13 11:08:05]

Please see above, once again. :)

[bartfab (slow), 2015/02/16 13:40:34]

Acknowledged.

+  const TimeTicks original_now_ticks = now_ticks_;
+  for (;;) {

[bartfab (slow), 2015/02/12 09:50:55]

Nit: while (true) is more common.

[engedy, 2015/02/12 11:27:03]

Done.

+    OnBeforeSelectingTask();
+    TestPendingTask task_info;
+    if (!DequeueNextTask(original_now_ticks, max_delta, &task_info))

[bartfab (slow), 2015/02/12 09:50:55]

Why do you not use this as the loop condition?

It used to be:

while (X)  {
 ...
}

Now we have:

for (;;) {
  if (!X)
    break;
 ...
}

They are equivalent but the former uses the language's primitives correctly.

[engedy, 2015/02/12 11:27:03]

I have restructured so that OnBeforeSelectingTask() need not be repeated. Happy
to revert if you feel strongly about it.  

[bartfab (slow), 2015/02/12 12:36:08]

Did you mean that the refactor avoids a spurious call to
OnBeforeSelectingTask()? If so, no, it does not. The number and order of calls
to this method is unchanged. The only difference is that all calls come from one
line instead of two different lines in the code now. I see no harm in the way it
was done before but I am fine with the new version as well.

[engedy, 2015/02/13 11:08:05]

Sorry, I was ambiguous. Yes, I wanted to reduce the call sites from 2 to 1,
without changing the behavior. Ack. 

+      break;
+    // If tasks were posted with a negative delay, task_info.GetTimeToRun() will
+    // be less than |now_ticks_|. ForwardClocksUntilTickTime() takes care of not
+    // moving the clock backwards in this case.

[bartfab (slow), 2015/02/12 09:50:55]

Why not just weed out negative delays in PostDelayedTask()?

[engedy, 2015/02/12 11:27:03]

The contract of TaskRunner does not forbid negative delays, so I would feel bad
about suddenly making such requirements that here.

(I admit, however, that recently TaskRunners seem to have stopped guaranteeing
that delays will be respected.) 

[bartfab (slow), 2015/02/12 12:36:07]

AFAICT, they never guaranteed to respect delays.

[engedy, 2015/02/13 11:08:05]

Hmm, you are right. I must have mixed it up with something else.

+    ForwardClocksUntilTickTime(task_info.GetTimeToRun());
+    task_info.task.Run();
+    OnAfterTaskRun();
+  }
+}
+
+void TestMockTimeTaskRunner::ForwardClocksUntilTickTime(TimeTicks later_ticks) {
+  if (later_ticks <= now_ticks_)
+    return;
+
+  now_ += later_ticks - now_ticks_;
+  now_ticks_ = later_ticks;
+  OnAfterTimePassed();
+}
+
+bool TestMockTimeTaskRunner::DequeueNextTask(const TimeTicks& reference,
+                                             const TimeDelta& max_delta,
                                              TestPendingTask* next_task) {
-  base::AutoLock scoped_lock(tasks_lock_);
+  AutoLock scoped_lock(tasks_lock_);
   if (!tasks_.empty() &&
       (tasks_.top().GetTimeToRun() - reference) <= max_delta) {
     *next_task = tasks_.top();