C++ readability review

components/timers/alarm_timer.h

 // 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.
 
 #ifndef COMPONENTS_TIMER_ALARM_TIMER_H_
 #define COMPONENTS_TIMER_ALARM_TIMER_H_
 
 #include "base/callback.h"
 #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 {
 struct PendingTask;
 }
 
 namespace timers {
 // The class implements a timer that is capable of waking the system up from a
 // suspended state.  For example, this is useful for running tasks that are
 // needed for maintaining network connectivity, like sending heartbeat messages.
 // 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 {

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

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

"Multiple inheritance is allowed only when all superclasses, with the possible
exception of the first one, are pure interfaces. In order to ensure that they
remain pure interfaces, they must end with the Interface suffix."

One way of fixing this might be to create a helper class which inherits from
DestructionObserver, and forwards the calls to the AlarmTimer.

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

MessageLoop::DestructionObserver _is_ a pure interface so this should be ok
right?

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

"In order to ensure that they remain pure interfaces, they must end with the
Interface suffix."

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

I had no say when this interface was created and it appears that a lot of code
in chrome ignores this particular rule of the style guide.  Regardless, I've
moved this into a separate class.

  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> {

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

Why not use shared_ptr for thread-safe refcounting?

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

Why is this in the public section? It seems like an implementation detail.

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

RtcAlarm is the only implementation of Delegate that I can find. If that's the
case, why factor out the abstract base class? Could AlarmTimer just use RtcAlarm
directly?

[Lei Zhang, 2014/12/05 20:58:12]

We don't use shared_ptrs or unique_ptrs in Chromium. (yet)

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

As I mentioned above, it's abstracted out to make it easier to add delegates for
Windows and Mac.  It's unlikely that this will ever happen though, so I'll make
the RtcAlarm a private nested class.

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

The Delegate provides all the platform-specific functionality to make this
feature work.  It's in the public section so that mac and windows delegates can
be written later if needed.  But you're right, this is currently just an
implementation detail and can be moved back into the public section later if we
decide that we actually need this functionality on platforms other than Chrome
OS.

    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;

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

Init() methods are bad news; see http://go/totw:42 for more. Why not just use a
constructor for this?

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

So this Init function exists to break a dependency cycle.  The Delegate needs a
base::WeakPtr<AlarmTimer> because it might end up outliving the AlarmTimer and
should not try to call the OnTimerFired() function after the AlarmTimer has been
destroyed.

WeakPtrs are created using a base::WeakPtrFactory<AlarmTimer>.  This needs to be
the final member of the AlarmTimer class so that it gets destroyed before any
other member, invalidating any existing WeakPtrs that those members might have
to the AlarmTimer.

Since the WeakPtrFactory has to be the final member, the Delegate appears before
it in the initialization list and the WeakPtrFactory cannot be used at that time
to create a WeakPtr that can be passed to the constructor for the Delegate.

Using raw pointers is out because then we would still need some way to
invalidate it in the AlarmTimer destructor to prevent a use-after-free.  I
suppose the simplest way to get rid of the Init function would be to construct
the Delegate in the body of the AlarmTimer constructor instead of the
initialization list.  That seems straightforward enough so I think I'll do that.

 
     // 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>;

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

Why is this needed?

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

It's recommended by the docstring for base::RefCountedThreadSafe:

// If you're using the default trait, then you should add compile time
// asserts that no one else is deleting your object.  i.e.
//    private:
//     friend class base::RefCountedThreadSafe<MyFoo>;
//     ~MyFoo();

   };
 
   AlarmTimer(bool retain_user_task, bool is_repeating);

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

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

"Every function declaration should have comments immediately preceding it that
describe what the function does and how to use it."

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

bool parameters are a readability hazard, especially when you have more than
one. The issue is that a callsite like

AlarmTimer my_timer(true, false);

gives the user almost no clues about the meanings of those arguments. Even if
you know that one controls retention of the user task, and the other controls
repetition, you may not know which is which, and you may not know e.g. whether
"true" means that the timer is single-use or that it's repeating.

See also the "Function Argument Comments" section of
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

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

I agree wholeheartedly.  I was already bit by this once when refactoring some
code to use this timer.  This class is meant to be a drop in replacement for
base::Timer, which is why I made these constructors mirror those of base::Timer.
 It's probably not necessary for the constructors to match though so I will
clean this up.

 
   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() { return can_wake_from_suspend_; }

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

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

"Use const whenever it makes sense."

This method looks like it should be const.

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

Done.  This was cleaned up in a separate CL.

 
   // Timer overrides.
   void Stop() override;
   void Reset() override;
 
   // MessageLoop::DestructionObserver overrides.
   void WillDestroyCurrentMessageLoop() override;
 
   // Must be called by the delegate to indicate that the timer has fired and
   // that the user task should be run.
   void OnTimerFired();
 
  private:
   // Initializes the timer with the appropriate delegate.
   void Init();
 
   // Delegate that will manage actually setting the timer.
   scoped_refptr<Delegate> delegate_;
 
   // Keeps track of the user task we want to run.  A new one is constructed
   // every time Reset() is called.
   scoped_ptr<base::PendingTask> pending_task_;

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

Why not unique_ptr?

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

As Lei mentioned, chromium doesn't use unique_ptr yet.  However, chromium's
scoped_ptr provides move semantics so it is essentially equivalent to a
unique_ptr.

 
   // Tracks whether the timer has the ability to wake the system up from
   // suspend.  This is a runtime check because we won't know if the system
   // supports being woken up from suspend until the delegate actually tries to
   // set it up.
   bool can_wake_from_suspend_;
 
   // Pointer to the message loop that started the timer.  Used to track the
   // destruction of that message loop.
   base::MessageLoop* origin_message_loop_;
 
   base::WeakPtrFactory<AlarmTimer> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(AlarmTimer);
 };
 
 }  // namespace timers
 
+
 #endif  // COMPONENTS_TIMER_ALARM_TIMER_H_