Refactor: Simplify WaitableEventWatcher.

base/synchronization/waitable_event_watcher.h

 // Copyright (c) 2012 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 BASE_SYNCHRONIZATION_WAITABLE_EVENT_WATCHER_H_
 #define BASE_SYNCHRONIZATION_WAITABLE_EVENT_WATCHER_H_
 
 #include "base/base_export.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include "base/win/object_watcher.h"
 #else
 #include "base/callback.h"
 #include "base/message_loop.h"
 #include "base/synchronization/waitable_event.h"
 #endif
 
 namespace base {
 
 class Flag;
 class AsyncWaiter;
 class AsyncCallbackTask;
 class WaitableEvent;
 
+typedef Callback<void(WaitableEvent*)> WaitableEventCallback;

[brettw, 2013/01/28 20:56:59]

I think the typedef is fine. I usually try to keep stuff out of the global (or
base in this case) namespace when possible, so in this case I'd probably do it
inside the class. Maybe we can come up with a shorter name ("EventCallback"?).

[teravest, 2013/01/28 21:25:09]

Done.

+
 // -----------------------------------------------------------------------------
 // This class provides a way to wait on a WaitableEvent asynchronously.
 //
 // Each instance of this object can be waiting on a single WaitableEvent. When
 // the waitable event is signaled, a callback is made in the thread of a given
 // MessageLoop. This callback can be deleted by deleting the waiter.
 //
 // Typical usage:
 //
-//   class MyClass : public base::WaitableEventWatcher::Delegate {
+//   class MyClass {
 //    public:
 //     void DoStuffWhenSignaled(WaitableEvent *waitable_event) {
-//       watcher_.StartWatching(waitable_event, this);
+//       base::WaitableEventCallback watcher_callback =
+//           base::Bind(&MyClass::OnWaitableEventSignaled, this);
+//       watcher_.StartWatching(waitable_event, watcher_callback);
 //     }
-//     virtual void OnWaitableEventSignaled(WaitableEvent* waitable_event) {
+//     void OnWaitableEventSignaled(WaitableEvent* waitable_event) {
 //       // OK, time to do stuff!
 //     }
 //    private:
 //     base::WaitableEventWatcher watcher_;
 //   };
 //
 // In the above example, MyClass wants to "do stuff" when waitable_event
 // becomes signaled. WaitableEventWatcher makes this task easy. When MyClass
 // goes out of scope, the watcher_ will be destroyed, and there is no need to
 // worry about OnWaitableEventSignaled being called on a deleted MyClass
 // pointer.
 //
 // BEWARE: With automatically reset WaitableEvents, a signal may be lost if it
 // occurs just before a WaitableEventWatcher is deleted. There is currently no
 // safe way to stop watching an automatic reset WaitableEvent without possibly
 // missing a signal.
 //
 // NOTE: you /are/ allowed to delete the WaitableEvent while still waiting on
 // it with a Watcher. It will act as if the event was never signaled.
 // -----------------------------------------------------------------------------
 
 class BASE_EXPORT WaitableEventWatcher
-#if !defined(OS_WIN)
-    : public MessageLoop::DestructionObserver
+#if defined(OS_WIN)

[tfarina, 2013/01/28 21:06:10]

for a possible future CL. Just an idea. We should try hard to avoid ifdefs. So
instead of ifdefing here, we could have a WaitableEventWatcherWin and
WaitableEventWatcherLinux and with gyp compile only the class necessary for each
platform.

[dmichael (off chromium), 2013/01/28 21:47:27]

That's what we have, but the header is shared. That seems to be the most common
way to do platform-specific classes, AFAICT. IPC::Channel gets around it by
using a pImpl class that's platform specific, so I could see doing it that way
in a future CL if it's important. But IMHO it's not worth the trouble here.

+    : public win::ObjectWatcher::Delegate {
+#else
+    : public MessageLoop::DestructionObserver {
 #endif
-{
  public:
 
   WaitableEventWatcher();
   virtual ~WaitableEventWatcher();
 
-  class BASE_EXPORT Delegate {
-   public:
-    // -------------------------------------------------------------------------
-    // This is called on the MessageLoop thread when WaitableEvent has been
-    // signaled.
-    //
-    // Note: the event may not be signaled by the time that this function is
-    // called. This indicates only that it has been signaled at some point in
-    // the past.
-    // -------------------------------------------------------------------------
-    virtual void OnWaitableEventSignaled(WaitableEvent* waitable_event) = 0;
-
-   protected:
-    virtual ~Delegate() { }
-  };
-
   // ---------------------------------------------------------------------------

[brettw, 2013/01/28 20:56:59]

Whoever wrote this kind of went crazy with the horizontal lines. Would you mind
removing them while you're here? It doesn't really match our style.

[teravest, 2013/01/28 21:25:09]

Done.

-  // When @event is signaled, the given delegate is called on the thread of the
-  // current message loop when StartWatching is called. The delegate is not
-  // deleted.
+  // When @event is signaled, the given callback is called on the thread of the
+  // current message loop when StartWatching is called.
   // ---------------------------------------------------------------------------
-  bool StartWatching(WaitableEvent* event, Delegate* delegate);
+  bool StartWatching(WaitableEvent* event,
+                     const WaitableEventCallback& callback);
 
   // ---------------------------------------------------------------------------
   // Cancel the current watch. Must be called from the same thread which
   // started the watch.
   //
   // Does nothing if no event is being watched, nor if the watch has completed.
-  // The delegate will *not* be called for the current watch after this
-  // function returns. Since the delegate runs on the same thread as this
+  // The callback will *not* be called for the current watch after this
+  // function returns. Since the callback runs on the same thread as this
   // function, it cannot be called during this function either.
   // ---------------------------------------------------------------------------
   void StopWatching();
 
   // ---------------------------------------------------------------------------
   // Return the currently watched event, or NULL if no object is currently being
   // watched.
   // ---------------------------------------------------------------------------
   WaitableEvent* GetWatchedEvent();
 
   // ---------------------------------------------------------------------------
-  // Return the delegate, or NULL if there is no delegate.
+  // Return the callback that will be invoked when the event is
+  // signaled.
   // ---------------------------------------------------------------------------
-  Delegate* delegate() {
-    return delegate_;
-  }
+  const WaitableEventCallback& callback() const { return callback_; }
 
  private:
 #if defined(OS_WIN)
-  // ---------------------------------------------------------------------------
-  // The helper class exists because, if WaitableEventWatcher were to inherit
-  // from ObjectWatcher::Delegate, then it couldn't also have an inner class
-  // called Delegate (at least on Windows). Thus this object exists to proxy
-  // the callback function
-  // ---------------------------------------------------------------------------
-  class ObjectWatcherHelper : public win::ObjectWatcher::Delegate {
-   public:
-    ObjectWatcherHelper(WaitableEventWatcher* watcher);
-
-    // -------------------------------------------------------------------------
-    // Implementation of ObjectWatcher::Delegate
-    // -------------------------------------------------------------------------
-    void OnObjectSignaled(HANDLE h);
-
-   private:
-    WaitableEventWatcher *const watcher_;
-  };
-
-  void OnObjectSignaled();
-
-  ObjectWatcherHelper helper_;
+  virtual void OnObjectSignaled(HANDLE h) OVERRIDE;
   win::ObjectWatcher watcher_;
 #else
   // ---------------------------------------------------------------------------
   // Implementation of MessageLoop::DestructionObserver
   // ---------------------------------------------------------------------------
   virtual void WillDestroyCurrentMessageLoop() OVERRIDE;
 
   MessageLoop* message_loop_;
   scoped_refptr<Flag> cancel_flag_;
   AsyncWaiter* waiter_;
-  base::Closure callback_;
+  base::Closure internal_callback_;
   scoped_refptr<WaitableEvent::WaitableEventKernel> kernel_;
 #endif
 
   WaitableEvent* event_;
-
-  Delegate* delegate_;
+  WaitableEventCallback callback_;
 };
 
 }  // namespace base
 
 #endif  // BASE_SYNCHRONIZATION_WAITABLE_EVENT_WATCHER_H_