C++ readability review

components/timers/alarm_timer.h

Index: components/timers/alarm_timer.h
diff --git a/components/timers/alarm_timer.h b/components/timers/alarm_timer.h
index d2f93335d61c518c4516b523b94058df949a1577..c7bc3f4f4ef3cb73299cbc5cb3b2ef18779f0fa8 100644
--- a/components/timers/alarm_timer.h
+++ b/components/timers/alarm_timer.h
@@ -9,11 +9,11 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "base/message_loop/message_loop.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 
 namespace base {
+class MessageLoop;
 struct PendingTask;
 }
 
@@ -24,61 +24,40 @@ namespace timers {
 // Currently, this feature is only available on Chrome OS systems running linux
 // version 3.11 or higher.  On all other platforms, the AlarmTimer behaves
 // exactly the same way as a regular Timer.
-class AlarmTimer : public base::Timer,
-                   public base::MessageLoop::DestructionObserver {
+class AlarmTimer : public base::Timer {
  public:
-  // The delegate is responsible for managing the system level details for
-  // actually setting up and monitoring a timer that is capable of waking the
-  // system from suspend.  This class is reference counted because it may need
-  // to outlive the timer in order to clean up after itself.
-  class Delegate : public base::RefCountedThreadSafe<Delegate> {
-   public:
-    // Initializes the timer.  Should return true iff the system has timers that
-    // can wake it up from suspend.  Will only be called once.
-    virtual bool Init(base::WeakPtr<AlarmTimer> timer) = 0;
-
-    // Stops the currently running timer.  It should be safe to call this more
-    // than once.
-    virtual void Stop() = 0;
-
-    // Resets the timer to fire after |delay| has passed.  Cancels any
-    // pre-existing delay.
-    virtual void Reset(base::TimeDelta delay) = 0;
-
-   protected:
-    virtual ~Delegate() {}
-
-   private:
-    friend class base::RefCountedThreadSafe<Delegate>;
-  };
-
-  AlarmTimer(bool retain_user_task, bool is_repeating);
-
-  AlarmTimer(const tracked_objects::Location& posted_from,
-             base::TimeDelta delay,
-             const base::Closure& user_task,
-             bool is_repeating);
-
   ~AlarmTimer() override;
 
   bool can_wake_from_suspend() const { return can_wake_from_suspend_; }
 
-  // Must be called by the delegate to indicate that the timer has fired and
-  // that the user task should be run.
-  void OnTimerFired();
-
   // Timer overrides.
   void Stop() override;
   void Reset() override;
 
-  // MessageLoop::DestructionObserver overrides.
-  void WillDestroyCurrentMessageLoop() override;
+ protected:
+  // The constructors for this class are protected because consumers should
+  // instantiate one of the specialized sub-classes defined below instead.
+  AlarmTimer(bool retain_user_task, bool is_repeating);
+  AlarmTimer(const tracked_objects::Location& posted_from,
+             base::TimeDelta delay,
+             const base::Closure& user_task,
+             bool is_repeating);
 
  private:
-  // Initializes the timer with the appropriate delegate.
+  // Common initialization that must be performed by both constructors.  This
+  // really should live in a delegated constructor but the way base::Timer's
+  // constructors are written makes it really hard to do so.
   void Init();
 
+  // Will be called by the delegate to indicate that the timer has fired and
+  // that the user task should be run.
+  void OnTimerFired();
+
+  // Called when |origin_message_loop_| will be destroyed.
+  void WillDestroyCurrentMessageLoop();
+
   // Delegate that will manage actually setting the timer.
+  class Delegate;
   scoped_refptr<Delegate> delegate_;
 
   // Keeps track of the user task we want to run.  A new one is constructed
@@ -95,11 +74,65 @@ class AlarmTimer : public base::Timer,
   // destruction of that message loop.
   base::MessageLoop* origin_message_loop_;
 
+  // Observes |origin_message_loop_| and informs this class if it will be
+  // destroyed.
+  class MessageLoopObserver;
+  scoped_ptr<MessageLoopObserver> message_loop_observer_;
+
   base::WeakPtrFactory<AlarmTimer> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AlarmTimer);
 };
 
+// As its name suggests, a OneShotAlarmTimer runs a given task once.  It does
+// not remember the task that was given to it after it has fired and does not
+// repeat.  Useful for fire-and-forget tasks.
+class OneShotAlarmTimer : public AlarmTimer {
+ public:
+  // Constructs a basic OneShotAlarmTimer.  An AlarmTimer constructed this way
+  // requires that Start() is called before Reset() is called.
+  OneShotAlarmTimer() : AlarmTimer(false, false) {}
+};
+
+// A RepeatingAlarmTimer takes a task and delay and repeatedly runs the task
+// using the specified delay as an interval between the runs until it is
+// explicitly stopped.  It remembers both the task and the delay it was given
+// after it fires.
+class RepeatingAlarmTimer : public AlarmTimer {
+ public:
+  // Constructs a basic RepeatingAlarmTimer.  An AlarmTimer constructed this way
+  // requires that Start() is called before Reset() is called.
+  RepeatingAlarmTimer() : AlarmTimer(true, true) {}

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

i think that you need to define these c'tors (and a d'tor) in the .cc file
instead of inlining them here:
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....

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

Done.

+
+  // Constructs a RepeatingAlarmTimer with pre-populated parameters but does not
+  // start it.  Useful if |user_task| or |delay| are not going to change.
+  // Reset() can be called immediately after constructing an AlarmTimer in this
+  // way.
+  RepeatingAlarmTimer(const tracked_objects::Location& posted_from,
+                      base::TimeDelta delay,
+                      const base::Closure& user_task)
+      : AlarmTimer(posted_from, delay, user_task, true) {}
+};
+
+// A SimpleAlarmTimer only fires once but remembers the task that it was given
+// even after it has fired.  Useful if you want to run the same task multiple
+// times but not at a regular interval.
+class SimpleAlarmTimer : public AlarmTimer {
+ public:
+  // Constructs a basic SimpleAlarmTimer.  An AlarmTimer constructed this way
+  // requires that Start() is called before Reset() is called.
+  SimpleAlarmTimer() : AlarmTimer(true, false) {}
+
+  // Constructs a SimpleAlarmTimer with pre-populated parameters but does not
+  // start it.  Useful if |user_task| or |delay| are not going to change.
+  // Reset() can be called immediately after constructing an AlarmTimer in this
+  // way.
+  SimpleAlarmTimer(const tracked_objects::Location& posted_from,
+                   base::TimeDelta delay,
+                   const base::Closure& user_task)
+      : AlarmTimer(posted_from, delay, user_task, false) {}
+};
+
 }  // namespace timers
 
 #endif  // COMPONENTS_TIMER_ALARM_TIMER_H_