Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list.h

Index: base/callback_list.h
diff --git a/base/callback_list.h b/base/callback_list.h
new file mode 100644
index 0000000000000000000000000000000000000000..647c779efe52031ad49e20f70159f8a773fca838
--- /dev/null
+++ b/base/callback_list.h
@@ -0,0 +1,175 @@
+// Copyright 2013 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_CALLBACK_LIST_H_
+#define BASE_CALLBACK_LIST_H_
+
+#include <algorithm>
+#include <limits>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/callback_list_internal.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+
+// OVERVIEW:
+//
+// A container for a list of callbacks.  Unlike a normal STL vector or list,
+// this container can be modified during iteration without invalidating the
+// iterator.  So, it safely handles the case of an callback removing itself

[erikwright (departed), 2013/09/04 19:35:55]

an->a

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+// or other callback from the list while callbacks are being run.

[erikwright (departed), 2013/09/04 19:35:55]

either 'other'->'another' or 'callback'->'callbacks'.

I think the 'while callbacks are being run' is redundant. If the actor is a
callback, it is by definition while callbacks are being run.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+//
+// TYPICAL USAGE:
+//
+// class MyWidget {
+//  public:
+//   ...
+//
+//   typedef base::Callback<void(const Foo&)> OnFooCallback;
+//
+//   base::Closure RegisterCallback(const OnFooCallback& cb) {
+//     return callback_list_.Add(cb);
+//   }
+//
+//  private:
+//   void NotifyFoo(const Foo& foo) {
+//      callback_list_.Run(foo);
+//   }
+//
+//   CallbackList<Foo> callback_list_(CallbackList<Foo>::NOTIFY_ALL);
+// };
+//
+//
+// class MyWidgetListener {
+//  public:
+//   MyWidgetListener::MyWidgetListener() {
+//     remove_foo_callback_.Reset(
+//         MyWidget::GetCurrent()->RegisterCallback(
+//             base::Bind(&MyWidgetListener::OnFoo, this)));
+//   }
+//
+//   MyWidgetListener::~MyWidgetListener() {
+//      // ScopedClosureRunner runs its closure automatically on deletion.
+//   }
+//
+//   void OnFoo(const Foo& foo) {
+//     // Do something.
+//   }
+//
+//  private:
+//   base::ScopedClosureRunner remove_foo_callback_;
+// };
+
+namespace base {
+
+template <typename T>
+struct CallbackHelper {

[awong, 2013/09/04 18:48:21]

Anything that isn't in the public API should be in base::internal.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+  typedef base::Callback<void(const T&)> Type;
+};
+
+template <>
+struct CallbackHelper<void> {
+  typedef base::Closure Type;
+};
+
+template <typename Details>
+class CallbackListBase {

[awong, 2013/09/04 18:48:21]

As discussed on IM, this feels like it should be specialized on CallbackType
explicitly.

In the CallbackList<> declaration, you know enough to statically specify the
signature, so I think you can avoid the indirection through a Helper.

So something like
// Declare, but not define to get the arity right.
template <typename CallbackType> class CallbackListBase;

// Only provide 1 specialization to whitelist only  Callback<void(const T&)>
types.
template <typename Details> class CallbackListBase<Callback<void(const
Details&)> > { /* All the stuff currently there */ };

followed by
template <typename Details> class CallbackList : public
CallbackListBase<void(const Details&)> { /* implementation of void Run(const
Details&); */ };

template <> class CallbackList<void> : public CallbackListBase<Closure> { /*
implementation of void Run(void); */ };

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+ public:
+  typedef typename CallbackHelper<Details>::Type CallbackType;

[awong, 2013/09/04 18:48:21]

I think you may need to typedef CallbackNotificationType here if you move the
enum declaration into CallbackListImpl.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+  explicit CallbackListBase(CallbackNotificationType type)

[erikwright (departed), 2013/09/04 19:35:55]

Should be protected.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+      : list_impl_(new base::internal::CallbackListImpl(type)) {}
+
+  // Add a callback to the list. A callback should not be added to
+  // the same list more than once. The returned closure (used to remove the
+  // callback from the list) is guaranteed to be safe to run.
+  base::Closure Add(const CallbackType& cb) {

[erikwright (departed), 2013/09/04 19:35:55]

It seems like, in almost all cases, it would probably be a bug to not invoke the
removal closure. Yet it would be easy to overlook.

I suggest at a minimum making this method WARN_UNUSED_RESULT. The caller should
at a minimum be copying the closure somewhere.

I would also propose returning a scoped_ptr<Handle> where Handle is some object
that, when it is destroyed, removes the registration. This at least switchs the
failure mode from a use-after-free to never receiving the expected notification.

WARN_UNUSED_RESULT would still be useful and catch cases where the client
forgets to store the handle.

[Cait (Slow), 2013/09/04 22:09:25]

Going with WARN_UNUSED_RESULT for now. Will look into moving to handles.

+    DCHECK(!cb.is_null());
+    return list_impl_->AddCallback(new CallbackType(cb));
+  }
+
+  // Delete (or nullify, if called during iteration), all callbacks in the list.

[erikwright (departed), 2013/09/04 19:35:55]

I don't think that the parenthetical is relevant clients (i.e., it's not part of
the public contract). It is better placed inside the implementation of
CallbackListImpl::Clear.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+  void Clear() {
+    list_impl_->Clear();
+  }
+
+  bool might_have_callbacks() {

[awong, 2013/09/04 18:48:21]

One more thought...if this is necessary to expose, would it be better to
actually just store an int that has the exact number of registered callbacks? 
You could decrement on each remove even if you're just tombstoning things with a
NULL.

That feels like a cleaner API since, as a user, I'm not certain what I would do
if this returns true...

Either that, or just remove might_have_callbacks() from the public API. 

[erikwright (departed), 2013/09/04 19:35:55]

What client would call this?

[awong, 2013/09/04 19:50:41]

IIRC, the discussion with Cait is that some clients want to assert the list is
empty on destruction. Perhaps we just want an AssertEmpty() or something.

[Cait (Slow), 2013/09/04 22:09:25]

Switched to an AssertEmpty, which checks 1. no active iterators, 2. no
callbacks. I think this is more clear.

+    return list_impl_->might_have_callbacks();
+  }
+
+ protected:
+  // Execute all active (non-null) callbacks with |details| parameter.

[erikwright (departed), 2013/09/04 19:35:55]

ditto on the parenthetical. And the rest (about |details|) is also now
incorrect.

// Visits each callback, respecting the CallbackNotificationType with regards to
callbacks added during iteration.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+  template <typename Visitor>
+  void VisitCallbacks(Visitor visitor) {

[awong, 2013/09/04 18:48:21]

The visitor is a nifty way to reuse code, but since we only have 2
implementations, and it's just a simple Iterator() loop, duplicating the logic
seems easiest.

BTW, is it worth having this might_have_callbacks() check?  It seems more common
with containers to just construct the iterator always and have the conditional
immediately fail.

That seems more analogous to

for(vector<Foo>::const_iterator it = v.begin(); it != v.end(); ++it) {
  ...
}

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+    if (might_have_callbacks()) {
+      internal::CallbackListImpl::Iterator it(list_impl_->GetWeakPtr());
+      CallbackType* cb;
+      while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) {
+        visitor.Visit(*cb);
+      }
+    }
+  }
+
+ private:
+  scoped_ptr<base::internal::CallbackListImpl> list_impl_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListBase);
+};
+
+template <typename Details>
+class CallbackList : public CallbackListBase<Details> {
+ public:
+  typedef base::Callback<void(const Details&)> CallbackType;
+
+  class Visitor {

[erikwright (departed), 2013/09/04 19:35:55]

can be private

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+   public:
+    explicit Visitor(const Details& details) : details_(details) {}
+    void Visit(const CallbackType& callback) {
+      callback.Run(details_);
+    }
+   private:
+    const Details& details_;
+  };
+
+  explicit CallbackList(CallbackNotificationType type)
+      : CallbackListBase<Details>(type) {}
+
+  // Execute all active (non-null) callbacks with |details| parameter.

[erikwright (departed), 2013/09/04 19:35:55]

ditto on the parenthetical.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+  void Run(const Details& details) {
+    this->template VisitCallbacks<Visitor>(Visitor(details));
+  }
+
+private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+template <> class CallbackList<void> : public CallbackListBase<void> {
+ public:
+  typedef Closure CallbackType;
+
+  class Visitor {

[erikwright (departed), 2013/09/04 19:35:55]

private

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+   public:
+    void Visit(const CallbackType& callback) {
+      callback.Run();
+    }
+  };
+
+  explicit CallbackList(CallbackNotificationType type)
+      : CallbackListBase<void>(type) {}
+
+  // Execute all active (non-null) callbacks.

[erikwright (departed), 2013/09/04 19:35:55]

parenthetical

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+  void Run() {
+    CallbackListBase<void>::VisitCallbacks<Visitor>(Visitor());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+} // namespace base

[awong, 2013/09/04 18:48:21]

nit:
}  // namespace base

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+#endif  // BASE_CALLBACK_LIST_H_