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..4b3bda25c382abddff572a93486da593a969ec9b
--- /dev/null
+++ b/base/callback_list.h
@@ -0,0 +1,149 @@
+// 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. It safely handles the case of a callback removing itself
+// or another callback from the list while callbacks are being run.
+//
+// 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 CallbackType>
+class CallbackListBase {

[awong, 2013/09/04 23:20:07]

CallbackListBase should be in base::internal.

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+ public:
+  // 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) WARN_UNUSED_RESULT {

[awong, 2013/09/04 23:20:07]

I'm going to slightly disagree with erikwright here...do we want the user to
always handle this return?

If users of this class don't care to AssertEmpty() before the CallbackList
destruction, it seems odd to force them to explicitly ignore the removal
callback.  I think not having the WARN_UNUSED_RESULT is better.

[erikwright (departed), 2013/09/05 14:14:31]

AIUI, the general model is publisher lifetime exceeds the listener's lifetime.
In that case, when an event is published after a listener is destroyed we will
have a use-after-free.

Ergo, the client has an obligation to unregister itself upon its destruction.

Developers who are not Cait or Albert will not be familiar with this nuance of
the API and could easily overlook the necessity of storing and correctly
invoking the Closure returned from Add - lovely file comments notwithstanding.

As I've mentioned, I actually prefer not returning a Closure but returning a
scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, with
the result that negligence will lead to not receiving events. That outcome is
much more likely to be noted by developers and tests than a racey
use-after-free. WARN_UNUSED_RESULT could and probably should still be applied in
that case.

The EventRegistrar that Cait is working on would hide these details in any case.
Only clients that did their own unsubscription would need to use this.

[Avi (use Gerrit), 2013/09/05 15:03:48]

Or a ScopedClosureRunner? Same basic philosophy as returning a scoped_ptr to
make ownership clear.

[awong, 2013/09/05 17:42:17]

I see what you're saying but I'm not completely convinced.  Assume we have a
class X. In the case where X is refcounted or we grab a WeakPtr<X>, then this is
a non issue.  What we're worried about is this case:

  cl.Add(Bind(&X::Foo, base::Unretained(this));

The "Unretained" is already a very strong indicator that lifetimes are a
problem.  If we have code structures that ensure CallbackList exists the
listener, then we're just adding clutter to the call site.

I think this code-smell is well known in Chrome (or if it's not...then we have
some serious problems). 
 
Hmm...I think this might be where our difference in opinion is coming in.  I'm
expecting this to be used beyond just replacing Notifications and the question
we're deciding is whether it's okay to make it harder for a code pattern where
you *know* the CallbackList is destroyed before the listeners.

Erik, I can't tell over text so I'm going to ask directly...how strongly do
think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause long term
harm in code maintenance?  Low, med, high?

On this evaluation, I'm in the "low" category...

+    DCHECK(!cb.is_null());
+    return list_impl_.Add(new CallbackType(cb));
+  }
+
+  // Delete all callbacks in the list.
+  void Clear() {
+    list_impl_.Clear();
+  }
+
+  // Assert that the list is empty and no iterators are active.
+  void AssertEmpty() {
+    list_impl_.AssertEmpty();
+  }
+
+ protected:
+  CallbackListBase() {}
+
+  // Get an iterator to the CallbackList.
+  scoped_ptr<internal::CallbackListImpl::Iterator> GetIterator() {
+    return list_impl_.GetIterator();
+  }
+
+ private:
+  base::internal::CallbackListImpl list_impl_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListBase);
+};
+
+template <typename Details>
+class CallbackList : public CallbackListBase<Callback<void(const Details&)> > {
+ public:
+  typedef base::Callback<void(const Details&)> CallbackType;
+
+  CallbackList() : CallbackListBase<CallbackType>() {}

[awong, 2013/09/04 23:20:07]

There's no need to explicitly call the default constructor of a base class. 
This is just a no-op.

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+
+  // Execute all active callbacks with |details| parameter.
+  void Run(const Details& details) {
+  scoped_ptr<internal::CallbackListImpl::Iterator> it = this->GetIterator();

[awong, 2013/09/04 23:20:07]

indentation.

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+    CallbackType* cb;
+    // static_cast is safe here, all callbacks were added via

[awong, 2013/09/04 23:20:07]

here, all -> here. All

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+    // CallbackListBase::Add and so must be of type

[awong, 2013/09/04 23:20:07]

::Add -> ::Add()

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+    // Callback<void(const Details&)>.
+    while((cb = static_cast<CallbackType*>(it->GetNext())) !=

[awong, 2013/09/04 23:20:07]

This can fit on one line.

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+         NULL) {
+      cb->Run(details);
+    }
+  }
+
+private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+template <> class CallbackList<void> : public CallbackListBase<Closure> {
+ public:
+  CallbackList() : CallbackListBase<Closure>() {}

[awong, 2013/09/04 23:20:07]

Remove explicit invocation of base class's default constructor.

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+
+  // Execute all active callbacks.
+  void Run() {
+    scoped_ptr<internal::CallbackListImpl::Iterator> it = GetIterator();
+    Closure* cb;
+    // static_cast is safe here, all callbacks were added via

[awong, 2013/09/04 23:20:07]

here, all -> here. All

[Cait (Slow), 2013/09/06 18:41:46]

Done.

+    // CallbackListBase::Add and so must be Closures.
+    while((cb = static_cast<Closure*>(it->GetNext())) != NULL) {
+      cb->Run();
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+}  // namespace base
+
+#endif  // BASE_CALLBACK_LIST_H_