C++ readability review

components/timers/alarm_timer_unittest.cc

Index: components/timers/alarm_timer_unittest.cc
diff --git a/components/timers/alarm_timer_unittest.cc b/components/timers/alarm_timer_unittest.cc
index efa88d4229e813c28e5b991b915cc4492836a99a..88fa97c6f376c586264861d872f90f3a5c46a972 100644
--- a/components/timers/alarm_timer_unittest.cc
+++ b/components/timers/alarm_timer_unittest.cc
@@ -24,10 +24,10 @@ namespace timers {
 namespace {
 // The message loops on which each timer should be tested.
 const base::MessageLoop::Type testing_message_loops[] = {
-  base::MessageLoop::TYPE_DEFAULT,
-  base::MessageLoop::TYPE_IO,
+    base::MessageLoop::TYPE_DEFAULT,
+    base::MessageLoop::TYPE_IO,
 #if !defined(OS_IOS)  // iOS does not allow direct running of the UI loop.
-  base::MessageLoop::TYPE_UI,
+    base::MessageLoop::TYPE_UI,
 #endif
 };
 
@@ -39,13 +39,10 @@ class OneShotAlarmTimerTester {
   OneShotAlarmTimerTester(bool* did_run, base::TimeDelta delay)
       : did_run_(did_run),
         delay_(delay),
-        timer_(new timers::AlarmTimer(false, false)) {
-  }
+        timer_(new timers::OneShotAlarmTimer()) {}
   void Start() {
-    timer_->Start(FROM_HERE,
-                  delay_,
-                  base::Bind(&OneShotAlarmTimerTester::Run,
-                             base::Unretained(this)));
+    timer_->Start(FROM_HERE, delay_, base::Bind(&OneShotAlarmTimerTester::Run,
+                                                base::Unretained(this)));
   }
 
  private:
@@ -58,7 +55,7 @@ class OneShotAlarmTimerTester {
 
   bool* did_run_;
   const base::TimeDelta delay_;
-  scoped_ptr<timers::AlarmTimer> timer_;
+  scoped_ptr<timers::OneShotAlarmTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(OneShotAlarmTimerTester);
 };
@@ -68,11 +65,9 @@ class OneShotSelfDeletingAlarmTimerTester {
   OneShotSelfDeletingAlarmTimerTester(bool* did_run, base::TimeDelta delay)
       : did_run_(did_run),
         delay_(delay),
-        timer_(new timers::AlarmTimer(false, false)) {
-  }
+        timer_(new timers::OneShotAlarmTimer()) {}
   void Start() {
-    timer_->Start(FROM_HERE,
-                  delay_,
+    timer_->Start(FROM_HERE, delay_,
                   base::Bind(&OneShotSelfDeletingAlarmTimerTester::Run,
                              base::Unretained(this)));
   }
@@ -88,7 +83,7 @@ class OneShotSelfDeletingAlarmTimerTester {
 
   bool* did_run_;
   const base::TimeDelta delay_;
-  scoped_ptr<timers::AlarmTimer> timer_;
+  scoped_ptr<timers::OneShotAlarmTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(OneShotSelfDeletingAlarmTimerTester);
 };
@@ -99,13 +94,10 @@ class RepeatingAlarmTimerTester {
       : did_run_(did_run),
         delay_(delay),
         counter_(10),
-        timer_(new timers::AlarmTimer(true, true)) {
-  }
+        timer_(new timers::RepeatingAlarmTimer()) {}
   void Start() {
-    timer_->Start(FROM_HERE,
-                  delay_,
-                  base::Bind(&RepeatingAlarmTimerTester::Run,
-                             base::Unretained(this)));
+    timer_->Start(FROM_HERE, delay_, base::Bind(&RepeatingAlarmTimerTester::Run,
+                                                base::Unretained(this)));
   }
 
  private:
@@ -122,148 +114,147 @@ class RepeatingAlarmTimerTester {
   bool* did_run_;
   const base::TimeDelta delay_;
   int counter_;
-  scoped_ptr<timers::AlarmTimer> timer_;
+  scoped_ptr<timers::RepeatingAlarmTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(RepeatingAlarmTimerTester);
 };
 
-void RunTest_OneShotAlarmTimer(base::MessageLoop::Type message_loop_type) {
-  base::MessageLoop loop(message_loop_type);
+}  // namespace
 
-  bool did_run = false;
-  OneShotAlarmTimerTester f(&did_run, kTenMilliseconds);
-  f.Start();
+//-----------------------------------------------------------------------------
+// Each test is run against each type of MessageLoop.  That way we are sure
+// that timers work properly in all configurations.
 
-  base::RunLoop().Run();
+TEST(AlarmTimerTest, OneShotAlarmTimer) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
+
+    bool did_run = false;
+    OneShotAlarmTimerTester f(&did_run, kTenMilliseconds);
+    f.Start();
 
-  EXPECT_TRUE(did_run);
+    base::RunLoop().Run();
+
+    EXPECT_TRUE(did_run);
+  }
 }
 
-void RunTest_OneShotAlarmTimer_Cancel(
-    base::MessageLoop::Type message_loop_type) {
-  base::MessageLoop loop(message_loop_type);
+TEST(AlarmTimerTest, OneShotAlarmTimer_Cancel) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  bool did_run_a = false;
-  OneShotAlarmTimerTester* a = new OneShotAlarmTimerTester(&did_run_a,
-                                                           kTenMilliseconds);
+    bool did_run_a = false;
+    OneShotAlarmTimerTester* a =
+        new OneShotAlarmTimerTester(&did_run_a, kTenMilliseconds);
 
-  // This should run before the timer expires.
-  base::MessageLoop::current()->DeleteSoon(FROM_HERE, a);
+    // This should run before the timer expires.
+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, a);
 
-  // Now start the timer.
-  a->Start();
+    // Now start the timer.
+    a->Start();
 
-  bool did_run_b = false;
-  OneShotAlarmTimerTester b(&did_run_b, kTenMilliseconds);
-  b.Start();
+    bool did_run_b = false;
+    OneShotAlarmTimerTester b(&did_run_b, kTenMilliseconds);
+    b.Start();
 
-  base::RunLoop().Run();
+    base::RunLoop().Run();
 
-  EXPECT_FALSE(did_run_a);
-  EXPECT_TRUE(did_run_b);
+    EXPECT_FALSE(did_run_a);
+    EXPECT_TRUE(did_run_b);
+  }
 }
 
-void RunTest_OneShotSelfDeletingAlarmTimer(
-    base::MessageLoop::Type message_loop_type) {
-  base::MessageLoop loop(message_loop_type);
+// If underlying timer does not handle this properly, we will crash or fail
+// in full page heap environment.
+TEST(AlarmTimerTest, OneShotSelfDeletingAlarmTimer) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  bool did_run = false;
-  OneShotSelfDeletingAlarmTimerTester f(&did_run, kTenMilliseconds);
-  f.Start();
+    bool did_run = false;
+    OneShotSelfDeletingAlarmTimerTester f(&did_run, kTenMilliseconds);
+    f.Start();
 
-  base::RunLoop().Run();
+    base::RunLoop().Run();
 
-  EXPECT_TRUE(did_run);
+    EXPECT_TRUE(did_run);
+  }
 }
 
-void RunTest_RepeatingAlarmTimer(base::MessageLoop::Type message_loop_type,
-                                 const base::TimeDelta& delay) {
-  base::MessageLoop loop(message_loop_type);
+TEST(AlarmTimerTest, RepeatingAlarmTimer) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  bool did_run = false;
-  RepeatingAlarmTimerTester f(&did_run, delay);
-  f.Start();
+    bool did_run = false;
+    RepeatingAlarmTimerTester f(&did_run, kTenMilliseconds);
+    f.Start();
 
-  base::RunLoop().Run();
+    base::RunLoop().Run();
 
-  EXPECT_TRUE(did_run);
+    EXPECT_TRUE(did_run);
+  }
 }
 
-void RunTest_RepeatingAlarmTimer_Cancel(
-    base::MessageLoop::Type message_loop_type, const base::TimeDelta& delay) {
-  base::MessageLoop loop(message_loop_type);
+TEST(AlarmTimerTest, RepeatingAlarmTimer_Cancel) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  bool did_run_a = false;
-  RepeatingAlarmTimerTester* a = new RepeatingAlarmTimerTester(&did_run_a,
-                                                               delay);
+    bool did_run_a = false;
+    RepeatingAlarmTimerTester* a =
+        new RepeatingAlarmTimerTester(&did_run_a, kTenMilliseconds);
 
-  // This should run before the timer expires.
-  base::MessageLoop::current()->DeleteSoon(FROM_HERE, a);
+    // This should run before the timer expires.
+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, a);
 
-  // Now start the timer.
-  a->Start();
+    // Now start the timer.
+    a->Start();
 
-  bool did_run_b = false;
-  RepeatingAlarmTimerTester b(&did_run_b, delay);
-  b.Start();
+    bool did_run_b = false;
+    RepeatingAlarmTimerTester b(&did_run_b, kTenMilliseconds);
+    b.Start();
 
-  base::RunLoop().Run();
+    base::RunLoop().Run();
 
-  EXPECT_FALSE(did_run_a);
-  EXPECT_TRUE(did_run_b);
+    EXPECT_FALSE(did_run_a);
+    EXPECT_TRUE(did_run_b);
+  }
 }
 
-}  // namespace
+TEST(AlarmTimerTest, RepeatingAlarmTimerZeroDelay) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-//-----------------------------------------------------------------------------
-// Each test is run against each type of MessageLoop.  That way we are sure
-// that timers work properly in all configurations.
+    bool did_run = false;
+    RepeatingAlarmTimerTester f(&did_run, base::TimeDelta());
+    f.Start();
 
-TEST(AlarmTimerTest, OneShotAlarmTimer) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_OneShotAlarmTimer(testing_message_loops[i]);
-  }
-}
+    base::RunLoop().Run();
 
-TEST(AlarmTimerTest, OneShotAlarmTimer_Cancel) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_OneShotAlarmTimer_Cancel(testing_message_loops[i]);
+    EXPECT_TRUE(did_run);
   }
 }
 
-// If underlying timer does not handle this properly, we will crash or fail
-// in full page heap environment.
-TEST(AlarmTimerTest, OneShotSelfDeletingAlarmTimer) {
+TEST(AlarmTimerTest, RepeatingAlarmTimerZeroDelay_Cancel) {
   for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_OneShotSelfDeletingAlarmTimer(testing_message_loops[i]);
-  }
-}
+    base::MessageLoop loop(testing_message_loops[i]);
 
-TEST(AlarmTimerTest, RepeatingAlarmTimer) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_RepeatingAlarmTimer(testing_message_loops[i],
-                                kTenMilliseconds);
-  }
-}
+    bool did_run_a = false;
+    RepeatingAlarmTimerTester* a =
+        new RepeatingAlarmTimerTester(&did_run_a, base::TimeDelta());
 
-TEST(AlarmTimerTest, RepeatingAlarmTimer_Cancel) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_RepeatingAlarmTimer_Cancel(testing_message_loops[i],
-                                       kTenMilliseconds);
-  }
-}
+    // This should run before the timer expires.
+    base::MessageLoop::current()->DeleteSoon(FROM_HERE, a);
 
-TEST(AlarmTimerTest, RepeatingAlarmTimerZeroDelay) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_RepeatingAlarmTimer(testing_message_loops[i],
-                                base::TimeDelta::FromMilliseconds(0));
-  }
-}
+    // Now start the timer.
+    a->Start();
 
-TEST(AlarmTimerTest, RepeatingAlarmTimerZeroDelay_Cancel) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_RepeatingAlarmTimer_Cancel(testing_message_loops[i],
-                                       base::TimeDelta::FromMilliseconds(0));
+    bool did_run_b = false;
+    RepeatingAlarmTimerTester b(&did_run_b, base::TimeDelta());
+    b.Start();
+
+    base::RunLoop().Run();
+
+    EXPECT_FALSE(did_run_a);
+    EXPECT_TRUE(did_run_b);
   }
 }
 
@@ -283,7 +274,7 @@ TEST(AlarmTimerTest, MessageLoopShutdown) {
       a.Start();
       b.Start();
     }  // MessageLoop destructs by falling out of scope.
-  }  // OneShotTimers destruct.  SHOULD NOT CRASH, of course.
+  }    // OneShotTimers destruct.  SHOULD NOT CRASH, of course.
 
   EXPECT_FALSE(did_run);
 }
@@ -291,7 +282,7 @@ TEST(AlarmTimerTest, MessageLoopShutdown) {
 TEST(AlarmTimerTest, NonRepeatIsRunning) {
   {
     base::MessageLoop loop;
-    timers::AlarmTimer timer(false, false);
+    timers::OneShotAlarmTimer timer;
     EXPECT_FALSE(timer.IsRunning());
     timer.Start(FROM_HERE, base::TimeDelta::FromDays(1),
                 base::Bind(&base::DoNothing));
@@ -302,7 +293,7 @@ TEST(AlarmTimerTest, NonRepeatIsRunning) {
   }
 
   {
-    timers::AlarmTimer timer(true, false);
+    timers::SimpleAlarmTimer timer;
     base::MessageLoop loop;
     EXPECT_FALSE(timer.IsRunning());
     timer.Start(FROM_HERE, base::TimeDelta::FromDays(1),
@@ -317,7 +308,7 @@ TEST(AlarmTimerTest, NonRepeatIsRunning) {
 }
 
 TEST(AlarmTimerTest, NonRepeatMessageLoopDeath) {
-  timers::AlarmTimer timer(false, false);
+  timers::OneShotAlarmTimer timer;
   {
     base::MessageLoop loop;
     EXPECT_FALSE(timer.IsRunning());
@@ -331,8 +322,8 @@ TEST(AlarmTimerTest, NonRepeatMessageLoopDeath) {
 
 TEST(AlarmTimerTest, RetainRepeatIsRunning) {
   base::MessageLoop loop;
-  timers::AlarmTimer timer(FROM_HERE, base::TimeDelta::FromDays(1),
-                          base::Bind(&base::DoNothing), true);
+  timers::RepeatingAlarmTimer timer(FROM_HERE, base::TimeDelta::FromDays(1),
+                                    base::Bind(&base::DoNothing));
   EXPECT_FALSE(timer.IsRunning());
   timer.Reset();
   EXPECT_TRUE(timer.IsRunning());
@@ -344,8 +335,8 @@ TEST(AlarmTimerTest, RetainRepeatIsRunning) {
 
 TEST(AlarmTimerTest, RetainNonRepeatIsRunning) {
   base::MessageLoop loop;
-  timers::AlarmTimer timer(FROM_HERE, base::TimeDelta::FromDays(1),
-                          base::Bind(&base::DoNothing), false);
+  timers::SimpleAlarmTimer timer(FROM_HERE, base::TimeDelta::FromDays(1),
+                                 base::Bind(&base::DoNothing));
   EXPECT_FALSE(timer.IsRunning());
   timer.Reset();
   EXPECT_TRUE(timer.IsRunning());
@@ -381,7 +372,7 @@ TEST(AlarmTimerTest, ContinuationStopStart) {
   {
     ClearAllCallbackHappened();
     base::MessageLoop loop;
-    timers::AlarmTimer timer(false, false);
+    timers::OneShotAlarmTimer timer;
     timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
                 base::Bind(&SetCallbackHappened1));
     timer.Stop();
@@ -397,7 +388,7 @@ TEST(AlarmTimerTest, ContinuationReset) {
   {
     ClearAllCallbackHappened();
     base::MessageLoop loop;
-    timers::AlarmTimer timer(false, false);
+    timers::OneShotAlarmTimer timer;
     timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
                 base::Bind(&SetCallbackHappened1));
     timer.Reset();
@@ -410,7 +401,6 @@ TEST(AlarmTimerTest, ContinuationReset) {
 
 }  // namespace
 
-
 namespace {
 void TimerRanCallback(bool* did_run) {
   *did_run = true;
@@ -419,75 +409,65 @@ void TimerRanCallback(bool* did_run) {
       FROM_HERE, base::MessageLoop::QuitWhenIdleClosure());
 }
 
-void RunTest_OneShotTimerConcurrentResetAndTimerFired(
-    base::MessageLoop::Type message_loop_type) {
-  base::MessageLoop loop(message_loop_type);
+TEST(AlarmTimerTest, OneShotTimerConcurrentResetAndTimerFired) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  timers::AlarmTimer timer(false, false);
-  bool did_run = false;
+    timers::OneShotAlarmTimer timer;
+    bool did_run = false;
 
-  timer.Start(
-      FROM_HERE, kTenMilliseconds, base::Bind(&TimerRanCallback, &did_run));
+    timer.Start(FROM_HERE, kTenMilliseconds,
+                base::Bind(&TimerRanCallback, &did_run));
 
-  // Sleep twice as long as the timer to ensure that the timer task gets queued.
-  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20));
+    // Sleep twice as long as the timer to ensure that the timer task gets
+    // queued.
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(20));

[Daniel Erat, 2015/03/09 16:09:47]

code like this makes me uneasy. it's both potentially-flaky and slower than it
needs to be. can you instead increase the delay to something like 10 seconds but
also stop the message loop as soon as the timer runs?

[Chirantan Ekbote, 2015/03/13 20:43:44]

In this test, and the one below, we don't actually want the timer to fire.  Here
is the situation it is trying to test:

    base::MessageLoopForUI          base::MessageLoopForIO
T1             |                               |
T2             |                               |
T3             |                               |
T4    AlarmTimer::Reset      Delegate::OnFileCanReadWithoutBlocking
T5             |                         Delegate::Reset
T6 AlarmTimer::OnTimerFired                    |

While it's hard to reproduce this situation, we know what the state will be
after T4:  AlarmTimer::OnTimerFired will be queued in the base::MessageLoopForUI
even though the timer has just been reset.  That's what the Sleep() call is
waiting for.  The call to RunLoop::Run() in line 437 waits for the timer to fire
after the reset  and stops the message loop once it does.

[Daniel Erat, 2015/03/14 14:02:33]

thanks for the explanation. could you instead add a testing-only hook to the
class to register a callback that gets invoked when the timer's task is queued?
then you could stop the event loop there, and you'd know that the test is both
a) waiting long enough for the task to be queued, and b) not waiting any longer
than it needs to.

[Chirantan Ekbote, 2015/04/07 01:16:06]

Done.  This actually took a little long because my new test uncovered a bug in
MessagePumpLibevent so I had to fix that first before I could upload a new patch
set.

 
-  // Now reset the timer.  This is attempting to simulate the timer firing and
-  // being reset at the same time.  The previously queued task should be
-  // removed.
-  timer.Reset();
+    // Now reset the timer.  This is attempting to simulate the timer firing and
+    // being reset at the same time.  The previously queued task should be
+    // removed.
+    timer.Reset();
 
-  base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(did_run);
+    base::RunLoop().RunUntilIdle();
+    EXPECT_FALSE(did_run);
 
-  // If the previous check failed, running the message loop again will hang the
-  // test so we only do it if the callback has not run yet.
-  if (!did_run) {
-    base::RunLoop().Run();
-    EXPECT_TRUE(did_run);
+    // If the previous check failed, running the message loop again will hang
+    // the test so we only do it if the callback has not run yet.
+    if (!did_run) {
+      base::RunLoop().Run();
+      EXPECT_TRUE(did_run);
+    }
   }
 }
 
-void RunTest_RepeatingTimerConcurrentResetAndTimerFired(
-    base::MessageLoop::Type message_loop_type) {
-  base::MessageLoop loop(message_loop_type);
-
-  timers::AlarmTimer timer(true, true);
-  bool did_run = false;
-
-  timer.Start(
-      FROM_HERE, kTenMilliseconds, base::Bind(&TimerRanCallback, &did_run));
+TEST(AlarmTimerTest, RepeatingTimerConcurrentResetAndTimerFired) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    base::MessageLoop loop(testing_message_loops[i]);
 
-  // Sleep more that three times as long as the timer duration to ensure that
-  // multiple tasks get queued.
-  base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(35));
+    timers::RepeatingAlarmTimer timer;
+    bool did_run = false;
 
-  // Now reset the timer.  This is attempting to simulate a very busy message
-  // loop where multiple tasks get queued but the timer gets reset before any of
-  // them have a chance to run.
-  timer.Reset();
+    timer.Start(FROM_HERE, kTenMilliseconds,
+                base::Bind(&TimerRanCallback, &did_run));
 
-  base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(did_run);
+    // Sleep more that three times as long as the timer duration to ensure that
+    // multiple tasks get queued.
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(35));

[Daniel Erat, 2015/03/09 16:09:47]

same here

 
-  // If the previous check failed, running the message loop again will hang the
-  // test so we only do it if the callback has not run yet.
-  if (!did_run) {
-    base::RunLoop().Run();
-    EXPECT_TRUE(did_run);
-  }
-}
+    // Now reset the timer.  This is attempting to simulate a very busy message
+    // loop where multiple tasks get queued but the timer gets reset before any
+    // of them have a chance to run.
+    timer.Reset();
 
-TEST(AlarmTimerTest, OneShotTimerConcurrentResetAndTimerFired) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_OneShotTimerConcurrentResetAndTimerFired(testing_message_loops[i]);
-  }
-}
+    base::RunLoop().RunUntilIdle();
+    EXPECT_FALSE(did_run);
 
-TEST(AlarmTimerTest, RepeatingTimerConcurrentResetAndTimerFired) {
-  for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_RepeatingTimerConcurrentResetAndTimerFired(
-        testing_message_loops[i]);
+    // If the previous check failed, running the message loop again will hang
+    // the test so we only do it if the callback has not run yet.
+    if (!did_run) {
+      base::RunLoop().Run();
+      EXPECT_TRUE(did_run);
+    }
   }
 }