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..288f839ca54c67ecb647254a3a50d91b6206f3c6 100644
--- a/components/timers/alarm_timer.cc
+++ b/components/timers/alarm_timer.cc
@@ -4,22 +4,228 @@
 
 #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_proxy.h"
 #include "base/pending_task.h"
-#include "components/timers/rtc_alarm.h"
+#include "base/threading/thread.h"
 
 namespace timers {
+namespace {
+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
+
+// 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::WeakPtr<AlarmTimer> parent)
+      : alarm_fd_(timerfd_create(CLOCK_REALTIME_ALARM, 0)),
+        parent_(parent),
+        origin_reset_sequence_number_(0),
+        io_reset_sequence_number_(0) {
+    DCHECK(parent_);
+  }
+
+  // Returns true if the system timer managed by this delegate is capable of
+  // waking the system from suspend.
+  bool CanWakeFromSuspend() { return alarm_fd_ != -1; }
+
+  // Resets the timer to fire after |delay| has passed.  Cancels any
+  // pre-existing delay.
+  void Reset(base::TimeDelta delay) {

[gromer, 2015/02/10 23:47:10]

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

"Define functions inline only when they are small, say, 10 lines or less."

It's not entirely clear whether this style rule is intended to cover member
functions that are defined inside the class (which are technically "inline" even
though they don't use the keyword), but putting such large function definitions
inside the class body also hurts readability, since it's harder to get a
high-level overview of the class. So I recommend moving these large method
definitions out of the class body.

[Chirantan Ekbote, 2015/02/12 01:49:59]

Done.

+    // 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()) {
+      g_io_thread.Pointer()->task_runner()->PostTask(
+          FROM_HERE,
+          base::Bind(&Delegate::ResetImpl, scoped_refptr<Delegate>(this), delay,
+                     origin_reset_sequence_number_));
+    } else {
+      ResetImpl(delay, origin_reset_sequence_number_);
+    }
+  }
+
+  // Stops the currently running timer.  It should be safe to call this even if
+  // the timer is not running.
+  void 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);
+  }
+
+  // base::MessageLoopForIO::Watcher overrides.
+  void OnFileCanReadWithoutBlocking(int fd) override {
+    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 OnFileCanWriteWithoutBlocking(int fd) override { NOTREACHED(); }
+
+ private:
+  friend class base::RefCountedThreadSafe<Delegate>;
+  ~Delegate() override {
+    if (alarm_fd_ != -1)
+      close(alarm_fd_);
+  }
+
+  // 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) {
+    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";
+  }
+
+  // Callback that is run when the timer fires.  Must be run on
+  // |origin_message_loop_|.
+  void 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;
+
+    if (parent_)
+      parent_->OnTimerFired();
+  }
+
+  // File descriptor associated with the alarm timer.
+  int alarm_fd_;
+
+  // Message loop which initially started the timer.
+  scoped_refptr<base::MessageLoopProxy> origin_message_loop_;
+
+  // The parent timer that should be informed when the timer fires.  This class
+  // may end up outliving the parent so it needs to ensure that the reference is
+  // valid before trying to call it.
+  base::WeakPtr<AlarmTimer> parent_;
+
+  // Manages watching file descriptors.
+  scoped_ptr<base::MessageLoopForIO::FileDescriptorWatcher> fd_watcher_;
+
+  // The sequence number of the last Reset() call handled by the origin thread

[gromer, 2015/02/10 23:47:10]

I have to read a long way into this sentence to figure out that it's talking
about both members. Say "numbers" rather than "number", and consider moving
"respectively" to the beginning of the sentence.

[Chirantan Ekbote, 2015/02/12 01:49:59]

Done.

+  // where this class lives and the IO thread used for watching the timer fd,
+  // respectively.  Note that these can be the same thread.  OnTimerFired()
+  // propagates the timer fired event to |parent_| only if the sequence number
+  // it receives from the IO thread matches |origin_reset_sequence_number_|.
+  int origin_reset_sequence_number_;
+  int io_reset_sequence_number_;
+
+  DISALLOW_COPY_AND_ASSIGN(Delegate);
+};
 
 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());
+  delegate_ =
+      make_scoped_refptr(new AlarmTimer::Delegate(weak_factory_.GetWeakPtr()));
+  can_wake_from_suspend_ = delegate_->CanWakeFromSuspend();
 }
 
 AlarmTimer::AlarmTimer(const tracked_objects::Location& posted_from,
@@ -27,11 +233,12 @@ 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());
+  delegate_ =
+      make_scoped_refptr(new AlarmTimer::Delegate(weak_factory_.GetWeakPtr()));
+  can_wake_from_suspend_ = delegate_->CanWakeFromSuspend();
 }
 
 AlarmTimer::~AlarmTimer() {
@@ -101,12 +308,11 @@ void AlarmTimer::Reset() {
 
   // 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(),