C++ readability review

components/timers/alarm_timer.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/timers/alarm_timer.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
@@ skipping 27 matching lines @@
   Stop();
 }
 
 void AlarmTimer::Stop() {
   if (!can_wake_from_suspend_) {
     base::Timer::Stop();
     return;
   }
 
   // Clear the running flag, stop the delegate, and delete the pending task.
   base::Timer::set_is_running(false);

[gromer, 2014/12/05 20:13:40]

Why not just "set_is_running(false)"? It's necessary when you call Stop() above,
but here there's no risk of ambiguity.

[Chirantan Ekbote, 2014/12/31 00:58:29]

Yes it's not really necessary but I find that it improves readability for me
personally because it makes it clear that set_is_running() is a method provided
by the parent class.

   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;
   }
@@ skipping 34 matching lines @@
   }
   base::MessageLoop::current()->task_annotator()->DidQueueTask(
       "AlarmTimer::Reset", *pending_task_);
 
   // Now start up the timer.
   delegate_->Reset(base::Timer::GetCurrentDelay());
   base::Timer::set_is_running(true);
 }
 
 void AlarmTimer::WillDestroyCurrentMessageLoop() {
   Stop();

[gromer, 2014/12/05 20:13:40]

Could you avoid all the AddDestructionObserver()/RemoveDestructionObserver()
business by making this something like

if (is_running()) {
  Stop();
}

[Chirantan Ekbote, 2014/12/31 00:58:29]

I'm not sure what you mean by this.  I would still need to call
RemoveDestructionObserver() somehow in the destructor of this class because
otherwise the MessageLoop would hold a pointer to a freed object.

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

Yeah, I'm not sure what I meant by this either.

 }
 
 void AlarmTimer::OnTimerFired() {
   if (!base::Timer::is_running())
     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