C++ readability review

components/timers/alarm_timer.cc

Index: components/timers/alarm_timer.cc
diff --git a/components/timers/alarm_timer.cc b/components/timers/alarm_timer.cc
index 8b46020af5496fe191e405218098defaf1166a75..232b779a0a9bfa4813a77f1e551cbf14f60c6b42 100644
--- a/components/timers/alarm_timer.cc
+++ b/components/timers/alarm_timer.cc
@@ -4,22 +4,292 @@
 
 #include "components/timers/alarm_timer.h"
 
+#include <sys/timerfd.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "base/pending_task.h"
-#include "components/timers/rtc_alarm.h"
+#include "base/threading/thread.h"
 
 namespace timers {
+namespace {
+// This class represents the IO thread that the AlarmTimer::Delegate may use for
+// watching file descriptors if it gets called from a thread that does not have
+// a MessageLoopForIO.  It is a lazy global instance because it may not always
+// be necessary.
+class RtcAlarmIOThread : public base::Thread {
+ public:
+  RtcAlarmIOThread() : Thread("RTC Alarm IO Thread") {
+    CHECK(
+        StartWithOptions(base::Thread::Options(base::MessageLoop::TYPE_IO, 0)));
+  }
+  ~RtcAlarmIOThread() override {}
+};
+
+base::LazyInstance<RtcAlarmIOThread> g_io_thread = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+// Watches a MessageLoop and runs a callback if that MessageLoop will be
+// destroyed.
+class AlarmTimer::MessageLoopObserver
+    : public base::MessageLoop::DestructionObserver {
+ public:
+  // Constructs a MessageLoopObserver that will observe |message_loop| and will
+  // call |on_will_be_destroyed_callback| when |message_loop| is about to be
+  // destroyed.
+  MessageLoopObserver(base::MessageLoop* message_loop,
+                      base::Closure on_will_be_destroyed_callback)
+      : message_loop_(message_loop),
+        on_will_be_destroyed_callback_(on_will_be_destroyed_callback) {
+    DCHECK(message_loop_);
+    message_loop_->AddDestructionObserver(this);
+  }
+
+  ~MessageLoopObserver() override {
+    // If |message_loop_| was destroyed, then this class will have already
+    // unregistered itself.  Doing it again will trigger a warning.
+    if (message_loop_)
+      message_loop_->RemoveDestructionObserver(this);
+  }
+
+  // base::MessageLoop::DestructionObserver override.
+  void WillDestroyCurrentMessageLoop() override {
+    message_loop_->RemoveDestructionObserver(this);
+    message_loop_ = NULL;
+
+    on_will_be_destroyed_callback_.Run();
+  }
+
+ private:
+  // The MessageLoop that this class should watch.  Is a weak pointer.
+  base::MessageLoop* message_loop_;
+
+  // The callback to run when |message_loop_| will be destroyed.
+  base::Closure on_will_be_destroyed_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(MessageLoopObserver);
+};
+
+// This class manages a Real Time Clock (RTC) alarm, a feature that is available
+// from linux version 3.11 onwards.  It creates a file descriptor for the RTC
+// alarm timer and then watches that file descriptor to see when it can be read
+// without blocking, indicating that the timer has fired.
+//
+// A major problem for this class is that watching file descriptors is only
+// available on a MessageLoopForIO but there is no guarantee the timer is going
+// to be created on one.  To get around this, the timer has a dedicated thread
+// with a MessageLoopForIO that posts tasks back to the thread that started the
+// timer.
+class AlarmTimer::Delegate
+    : public base::RefCountedThreadSafe<AlarmTimer::Delegate>,
+      public base::MessageLoopForIO::Watcher {
+ public:
+  // Construct a Delegate for the AlarmTimer.  |parent| should be a valid
+  // WeakPtr to the parent AlarmTimer.
+  explicit Delegate(base::Closure on_timer_fired_callback);
+
+  // Returns true if the system timer managed by this delegate is capable of
+  // waking the system from suspend.
+  bool CanWakeFromSuspend();

[gromer, 2015/02/17 22:02:08]

FWIW, I think this would be a reasonable method to keep inline. Either way is
fine, though.

+
+  // Resets the timer to fire after |delay| has passed.  Cancels any
+  // pre-existing delay.
+  void Reset(base::TimeDelta delay);
+
+  // Stops the currently running timer.  It should be safe to call this even if
+  // the timer is not running.
+  void Stop();
+
+  // base::MessageLoopForIO::Watcher overrides.
+  void OnFileCanReadWithoutBlocking(int fd) override;
+  void OnFileCanWriteWithoutBlocking(int fd) override;
+
+ private:
+  friend class base::RefCountedThreadSafe<Delegate>;
+  ~Delegate() override;
+
+  // Actually performs the system calls to set up the timer.  This must be
+  // called on a MessageLoopForIO.
+  void ResetImpl(base::TimeDelta delay, int reset_sequence_number);
+
+  // Callback that is run when the timer fires.  Must be run on
+  // |origin_message_loop_|.
+  void OnTimerFired(int reset_sequence_number);
+
+  // File descriptor associated with the alarm timer.
+  int alarm_fd_;
+
+  // Message loop which initially started the timer.
+  scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
+
+  // Callback that should be run when the timer fires.
+  base::Closure on_timer_fired_callback_;
+
+  // Manages watching file descriptors.
+  scoped_ptr<base::MessageLoopForIO::FileDescriptorWatcher> fd_watcher_;
+
+  // The sequence numbers of the last Reset() call handled respectively on
+  // |origin_message_loop_| and on the MessageLoopForIO used for watching the
+  // timer file descriptor.  Note that these can be the same MessageLoop.
+  // OnTimerFired() runs |on_timer_fired_callback_| only if the sequence number
+  // it receives from the MessageLoopForIO matches
+  // |origin_reset_sequence_number_|.
+  int origin_reset_sequence_number_;
+  int io_reset_sequence_number_;
+
+  DISALLOW_COPY_AND_ASSIGN(Delegate);
+};
+
+// Construct a Delegate for the AlarmTimer.  |parent| should be a valid
+// WeakPtr to the parent AlarmTimer.
+AlarmTimer::Delegate::Delegate(base::Closure on_timer_fired_callback)
+    : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)),
+      on_timer_fired_callback_(on_timer_fired_callback),
+      origin_reset_sequence_number_(0),
+      io_reset_sequence_number_(0) {
+}
+
+AlarmTimer::Delegate::~Delegate() {
+  if (alarm_fd_ != -1)
+    close(alarm_fd_);
+}
+
+bool AlarmTimer::Delegate::CanWakeFromSuspend() {
+  return alarm_fd_ != -1;
+}
+
+void AlarmTimer::Delegate::Reset(base::TimeDelta delay) {
+  // Get a proxy for the current message loop.  When the timer fires, we will
+  // post tasks to this proxy to let the parent timer know.
+  origin_message_loop_ = base::MessageLoopProxy::current();
+
+  // Increment the sequence number.  Used to invalidate any events that have
+  // been queued but not yet run since the last time Reset() was called.
+  origin_reset_sequence_number_++;
+
+  // Calling timerfd_settime with a zero delay actually clears the timer so if
+  // the user has requested a zero delay timer, we need to handle it
+  // differently.  We queue the task here but we still go ahead and call
+  // timerfd_settime with the zero delay anyway to cancel any previous delay
+  // that might have been programmed.
+  if (delay <= base::TimeDelta::FromMicroseconds(0)) {
+    // The timerfd_settime documentation is vague on what happens when it is
+    // passed a negative delay.  We can sidestep the issue by ensuring that
+    // the delay is 0.
+    delay = base::TimeDelta::FromMicroseconds(0);
+    origin_message_loop_->PostTask(
+        FROM_HERE,
+        base::Bind(&Delegate::OnTimerFired, scoped_refptr<Delegate>(this),
+                   origin_reset_sequence_number_));
+  }
+
+  // Run ResetImpl() on a MessageLoopForIO.
+  if (base::MessageLoopForIO::IsCurrent()) {
+    ResetImpl(delay, origin_reset_sequence_number_);
+  } else {
+    g_io_thread.Pointer()->task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&Delegate::ResetImpl, scoped_refptr<Delegate>(this), delay,
+                   origin_reset_sequence_number_));
+  }
+}
+
+void AlarmTimer::Delegate::Stop() {
+  // Stop the RTC from a MessageLoopForIO.
+  if (!base::MessageLoopForIO::IsCurrent()) {
+    g_io_thread.Pointer()->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&Delegate::Stop, scoped_refptr<Delegate>(this)));
+    return;
+  }
+
+  // Stop watching for events.
+  fd_watcher_.reset();
+
+  // Now clear the timer.
+  DCHECK_NE(alarm_fd_, -1);
+  itimerspec blank_time = {};
+  timerfd_settime(alarm_fd_, 0, &blank_time, NULL);
+}
+
+void AlarmTimer::Delegate::OnFileCanReadWithoutBlocking(int fd) {
+  DCHECK_EQ(alarm_fd_, fd);
+
+  // Read from the fd to ack the event.
+  char val[sizeof(uint64_t)];
+  base::ReadFromFD(alarm_fd_, val, sizeof(uint64_t));
+
+  // Make sure that the parent timer is informed on the proper message loop.
+  if (origin_message_loop_->RunsTasksOnCurrentThread()) {
+    OnTimerFired(io_reset_sequence_number_);
+  } else {
+    origin_message_loop_->PostTask(
+        FROM_HERE,
+        base::Bind(&Delegate::OnTimerFired, scoped_refptr<Delegate>(this),
+                   io_reset_sequence_number_));
+  }
+}
+
+void AlarmTimer::Delegate::OnFileCanWriteWithoutBlocking(int fd) {
+  NOTREACHED();
+}
+
+void AlarmTimer::Delegate::ResetImpl(base::TimeDelta delay,
+                                     int reset_sequence_number) {
+  DCHECK(base::MessageLoopForIO::IsCurrent());
+  DCHECK_NE(alarm_fd_, -1);
+
+  // Store the sequence number in the IO thread variable.  When the timer
+  // fires, we will bind this value to the OnTimerFired callback to ensure
+  // that we do the right thing if the timer gets reset.
+  io_reset_sequence_number_ = reset_sequence_number;
+
+  // If we were already watching the fd, this will stop watching it.
+  fd_watcher_.reset(new base::MessageLoopForIO::FileDescriptorWatcher);
+
+  // Start watching the fd to see when the timer fires.
+  if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
+          alarm_fd_, false, base::MessageLoopForIO::WATCH_READ,
+          fd_watcher_.get(), this)) {
+    LOG(ERROR) << "Error while attempting to watch file descriptor for RTC "
+               << "alarm.  Timer will not fire.";
+  }
+
+  // Actually set the timer.  This will also clear the pre-existing timer, if
+  // any.
+  itimerspec alarm_time = {};
+  alarm_time.it_value.tv_sec = delay.InSeconds();
+  alarm_time.it_value.tv_nsec =
+      (delay.InMicroseconds() % base::Time::kMicrosecondsPerSecond) *
+      base::Time::kNanosecondsPerMicrosecond;
+  if (timerfd_settime(alarm_fd_, 0, &alarm_time, NULL) < 0)
+    PLOG(ERROR) << "Error while setting alarm time.  Timer will not fire";
+}
+
+void AlarmTimer::Delegate::OnTimerFired(int reset_sequence_number) {
+  DCHECK(origin_message_loop_->RunsTasksOnCurrentThread());
+
+  // Check to make sure that the timer was not reset in the time between when
+  // this task was queued to run and now.  If it was reset, then don't do
+  // anything.
+  if (reset_sequence_number != origin_reset_sequence_number_)
+    return;
+
+  on_timer_fired_callback_.Run();
+}
 
 AlarmTimer::AlarmTimer(bool retain_user_task, bool is_repeating)
     : base::Timer(retain_user_task, is_repeating),
-      delegate_(new RtcAlarm()),
       can_wake_from_suspend_(false),
       origin_message_loop_(NULL),
       weak_factory_(this) {
-  can_wake_from_suspend_ = delegate_->Init(weak_factory_.GetWeakPtr());
+  Init();
 }
 
 AlarmTimer::AlarmTimer(const tracked_objects::Location& posted_from,
@@ -27,36 +297,20 @@ AlarmTimer::AlarmTimer(const tracked_objects::Location& posted_from,
                        const base::Closure& user_task,
                        bool is_repeating)
     : base::Timer(posted_from, delay, user_task, is_repeating),
-      delegate_(new RtcAlarm()),
       can_wake_from_suspend_(false),
       origin_message_loop_(NULL),
       weak_factory_(this) {
-  can_wake_from_suspend_ = delegate_->Init(weak_factory_.GetWeakPtr());
+  Init();
 }
 
 AlarmTimer::~AlarmTimer() {
   Stop();
 }
 
-void AlarmTimer::OnTimerFired() {
-  if (!base::Timer::IsRunning())
-    return;
-
-  DCHECK(pending_task_.get());
-
-  // Take ownership of the pending user task, which is going to be cleared by
-  // the Stop() or Reset() functions below.
-  scoped_ptr<base::PendingTask> pending_user_task(pending_task_.Pass());
-
-  // Re-schedule or stop the timer as requested.
-  if (base::Timer::is_repeating())
-    Reset();
-  else
-    Stop();
-
-  // Now run the user task.
-  base::MessageLoop::current()->task_annotator()->RunTask(
-      "AlarmTimer::Reset", "AlarmTimer::OnTimerFired", *pending_user_task);
+void AlarmTimer::Init() {
+  delegate_ = make_scoped_refptr(new AlarmTimer::Delegate(
+      base::Bind(&AlarmTimer::OnTimerFired, weak_factory_.GetWeakPtr())));
+  can_wake_from_suspend_ = delegate_->CanWakeFromSuspend();
 }
 
 void AlarmTimer::Stop() {
@@ -70,13 +324,9 @@ void AlarmTimer::Stop() {
   delegate_->Stop();
   pending_task_.reset();
 
-  // Stop is called when the AlarmTimer is destroyed so we need to remove
-  // ourselves as a MessageLoop::DestructionObserver to prevent a segfault
-  // later.
-  if (origin_message_loop_) {
-    origin_message_loop_->RemoveDestructionObserver(this);
-    origin_message_loop_ = NULL;
-  }
+  // Stop watching |origin_message_loop_|.
+  origin_message_loop_ = NULL;
+  message_loop_observer_.reset();
 
   if (!base::Timer::retain_user_task())
     base::Timer::set_user_task(base::Closure());
@@ -96,17 +346,19 @@ void AlarmTimer::Reset() {
   // destroyed.
   if (!origin_message_loop_) {
     origin_message_loop_ = base::MessageLoop::current();
-    origin_message_loop_->AddDestructionObserver(this);
+    message_loop_observer_.reset(new MessageLoopObserver(
+        origin_message_loop_,
+        base::Bind(&AlarmTimer::WillDestroyCurrentMessageLoop,
+                   weak_factory_.GetWeakPtr())));
   }
 
   // Set up the pending task.
   if (base::Timer::GetCurrentDelay() > base::TimeDelta::FromMicroseconds(0)) {
-    base::Timer::set_desired_run_time(
-        base::TimeTicks::Now() + base::Timer::GetCurrentDelay());
-    pending_task_.reset(new base::PendingTask(base::Timer::posted_from(),
-                                              base::Timer::user_task(),
-                                              base::Timer::desired_run_time(),
-                                              true  /* nestable */));
+    base::Timer::set_desired_run_time(base::TimeTicks::Now() +
+                                      base::Timer::GetCurrentDelay());
+    pending_task_.reset(new base::PendingTask(
+        base::Timer::posted_from(), base::Timer::user_task(),
+        base::Timer::desired_run_time(), true /* nestable */));
   } else {
     base::Timer::set_desired_run_time(base::TimeTicks());
     pending_task_.reset(new base::PendingTask(base::Timer::posted_from(),
@@ -124,4 +376,25 @@ void AlarmTimer::WillDestroyCurrentMessageLoop() {
   Stop();
 }
 
+void AlarmTimer::OnTimerFired() {
+  if (!base::Timer::IsRunning())
+    return;
+
+  DCHECK(pending_task_.get());
+
+  // Take ownership of the pending user task, which is going to be cleared by
+  // the Stop() or Reset() functions below.
+  scoped_ptr<base::PendingTask> pending_user_task(pending_task_.Pass());
+
+  // Re-schedule or stop the timer as requested.
+  if (base::Timer::is_repeating())
+    Reset();
+  else
+    Stop();
+
+  // Now run the user task.
+  base::MessageLoop::current()->task_annotator()->RunTask(
+      "AlarmTimer::Reset", "AlarmTimer::OnTimerFired", *pending_user_task);
+}
+
 }  // namespace timers