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..acc455f857d0d65ee83760fd196b63760ef52558
--- /dev/null
+++ b/base/callback_list.h
@@ -0,0 +1,283 @@
+// 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/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
+//   or other 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_.RegisterCallback(cb);
+//     }
+//
+//    private:
+//     void NotifyFoo(const Foo& foo) {
+//       callback_list_.Run(foo);
+//     }
+//
+//     CallbackList<Foo> callback_list_;
+//   };
+//
+//
+//  class MyWidgetListener {
+//    public:
+//
+//     MyWidgetListener::MyWidgetListener() {
+//       remove_foo_callback_ = MyWidget::GetCurrent()->RegisterCallback(
+//           base::Bind(&MyWidgetListener::OnFoo, this));
+//     }
+//
+//     MyWidgetListener::~MyWidgetListener() {
+//       remove_foo_callback_.Run();
+//     }
+//
+//     void OnFoo(const Foo& foo) {
+//       // Do something.
+//     }
+//
+//    private:
+//     base::Closure remove_foo_callback_;
+//   };
+//
+///////////////////////////////////////////////////////////////////////////////
+
+// TODO(caitkp): Move to .cc
+class CallbackListBase {

[awong, 2013/08/27 21:23:29]

The declaration will have to be public because CallbackListWithDetails() will
need the full definition here.

I'd just move it into the base::internal namespace and add a scary comment.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+ public:
+  // Enumeration of which callbacks are called.
+  enum NotificationType {
+    // Specifies that any callbacks added during notification are called.
+    // This is the default type if no type is provided to the constructor.
+    NOTIFY_ALL,
+
+    // Specifies that callbacks added while sending out notifications are not
+    // called.
+    NOTIFY_EXISTING_ONLY
+  };
+
+  CallbackListBase()
+      : active_iterator_count_(0),
+        type_(NOTIFY_ALL),
+        weak_factory_(this) {}
+
+  explicit CallbackListBase(NotificationType type)
+      : active_iterator_count_(0),
+        type_(type),
+        weak_factory_(this) {}
+
+  virtual ~CallbackListBase() {
+    Compact();
+    DCHECK_EQ(0U, size());
+    Clear();
+  }
+
+  void Clear() {
+    if (active_iterator_count_) {
+      for (size_t i = 0; i < callbacks_.size(); ++i) {
+        callbacks_[i]->Reset();

[awong, 2013/08/27 21:23:29]

Call delete before setting to NULL?

BTW, do we need to worry about re-entrancy?  What if the Reset() causes this
list to be notified again? Maybe it might be better to do

CallbackBase* cb = callback_[i];
callback_[i] = NULL;
cb->Reset();
delete cb;

I'm not sure if that's sufficient to save us from much though.  Thoughts?

+        callbacks_[i] = NULL;
+      }
+    } else {
+      callbacks_.clear();
+    }
+  }
+
+  bool might_have_callbacks() const {
+    return size() != 0;
+  }
+
+ protected:

[awong, 2013/08/27 21:23:29]

If we put this in the base::internal namespace, it might be okay to make all
this stuff public.  Alternately, I don't think anything other than
NotificationType needs to be public so if we keep protected, maybe just move
everything down?

[Cait (Slow), 2013/08/28 21:50:21]

Seems like we'd want to keep Clear() and might_have_callbacks() as part of the
public API? (ObserverList exposes similar functions).

+  typedef std::vector<base::internal::CallbackBase*> ListType;

[Avi (use Gerrit), 2013/08/28 15:38:58]

Why CallbackBase and not Callback? It's not obvious to me (though to you,
Albert, certainly), and I'd opine comment-worthy.

+
+  // An iterator class that can be used to access the list of callbacks.
+  class Iterator {
+   public:
+    Iterator(CallbackListBase* list)
+        : list_(list->GetWeakPtr()),
+          index_(0),
+          max_index_(list->type_ == NOTIFY_ALL ?
+                     std::numeric_limits<size_t>::max() :
+                     list->callbacks_.size()) {
+      ++list_->active_iterator_count_;
+    }
+
+    ~Iterator() {
+      if (list_.get() && --list_->active_iterator_count_ == 0)

[awong, 2013/08/27 21:23:29]

You should be able to do
if (list_)

here and elsewhere in the CL.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+        list_->Compact();
+    }
+
+    base::internal::CallbackBase* GetNext() {
+      if (!list_.get())
+        return NULL;
+      ListType& callbacks = list_->callbacks_;
+      size_t max_index = std::min(max_index_, callbacks.size());
+      while (index_ < max_index && !callbacks[index_])
+        ++index_;
+      return index_ < max_index ? callbacks[index_++] :

[awong, 2013/08/27 21:23:29]

This should fit on one line.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+                                  NULL;
+    }
+
+   private:
+    base::WeakPtr<CallbackListBase> list_;
+    size_t index_;
+    size_t max_index_;
+  };
+
+  // Remove an callback from the list if it is in the list.
+  void RemoveCallback(base::internal::CallbackBase* cb) {
+    for (size_t i = 0; i < callbacks_.size(); i++) {
+      if (callbacks_[i] == cb) {
+        if (active_iterator_count_) {
+          callbacks_[i]->Reset();
+          callbacks_[i] = NULL;
+        } else {
+          callbacks_.erase(callbacks_.begin() + i);

[awong, 2013/08/27 21:23:29]

How frequently are lists going to be modified in place?  I'm unsure where the
tipping point for using a vector versus a list or a set is here.

[Cait (Slow), 2013/08/28 21:50:21]

I was wondering that too...

+        }
+        return;
+      }
+    }
+  }
+
+  base::Closure AddCallback(base::internal::CallbackBase* cb) {

[awong, 2013/08/27 21:23:29]

Document that this takes ownership of |cb|

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+    for (size_t i = 0; i < callbacks_.size(); i++) {
+      if (callbacks_[i] == cb) {
+        NOTREACHED() << "Callbacks can only be added once!";
+        return base::Bind(&base::DoNothing);;
+      }
+    }
+    callbacks_.push_back(cb);
+    return base::Bind(&CallbackListBase::RemoveCallback, GetWeakPtr(), cb);
+  }
+
+  bool HasCallback(base::internal::CallbackBase* cb) const {

[awong, 2013/08/27 21:23:29]

I don't think this is used.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+    for (size_t i = 0; i < callbacks_.size(); ++i) {
+      if (callbacks_[i] == cb)
+        return true;
+    }
+    return false;
+  }
+
+  base::WeakPtr<CallbackListBase> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  void Compact() {
+    callbacks_.erase(
+        std::remove(callbacks_.begin(), callbacks_.end(),
+                    static_cast<base::internal::CallbackBase*>(NULL)),
+                    callbacks_.end());
+  }
+
+  size_t size() const { return callbacks_.size(); }
+
+ private:
+

[awong, 2013/08/27 21:23:29]

spurious newline.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+  ListType callbacks_;
+  int active_iterator_count_;
+  NotificationType type_;
+  base::WeakPtrFactory<CallbackListBase> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListBase);
+};
+
+
+template <typename Detail>
+class CallbackListWithDetails : public CallbackListBase {
+ public:
+  typedef base::Callback<void(const Detail&)> CallbackType;
+  typedef CallbackListBase::NotificationType NotificationType;
+

[awong, 2013/08/27 21:23:29]

spurious newline.

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+
+  CallbackListWithDetails() {}

[awong, 2013/08/27 21:23:29]

Do we need a default constructor?

Few APIs are nicer :)

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+
+  explicit CallbackListWithDetails(NotificationType type)
+      : CallbackListBase(type) {}
+
+  virtual ~CallbackListWithDetails() {
+  }
+
+  // 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 RegisterCallback(
+    const base::Callback<void(const Detail&)>& cb) {

[awong, 2013/08/27 21:23:29]

Make this "const CallbackType&" as well?  Alternately, don't bother with the
typedef at all (up to you).

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+    DCHECK(!cb.is_null());
+    // TODO(caitkp): This is a leak.

[awong, 2013/08/27 21:23:29]

Can't the CallbackListBase<> just delete the pointer on Remove or destruct?

[Cait (Slow), 2013/08/27 22:14:20]

In the current impl, CallbackListBase keeps a vector of CallbackBase pointers,
and CallbackBase has a protected dtor...

[awong, 2013/08/28 02:02:55]

Geez. What paranoid idiot made that protected...oh...that was me. :(

I suggest we friend CallbackList from CallbackBase. The two are (or should be)
in base::internal, and this class is tightly related enough that it makes sense
to me.

+    return AddCallback(new CallbackType(cb));
+  }
+
+  void Run(const Detail& details) {
+    if (might_have_callbacks()) {
+      CallbackListBase::Iterator it(this);
+      CallbackType* cb;
+      while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) {
+        cb->Run(details);
+      }
+    }
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListWithDetails);

[awong, 2013/08/27 21:23:29]

needs a private:

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+};
+
+class CallbackList : public CallbackListBase {
+ public:
+  typedef base::Closure CallbackType;
+  typedef CallbackListBase::NotificationType NotificationType;
+
+

[awong, 2013/08/27 21:23:29]

spurious newline

[Cait (Slow), 2013/08/28 21:50:21]

Done.

+  CallbackList() {}
+
+  explicit CallbackList(NotificationType type)
+      : CallbackListBase(type) {}
+
+  virtual ~CallbackList() {
+  }
+
+  // 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 RegisterCallback(const base::Closure& cb) {

[awong, 2013/08/27 21:23:29]

One more nit: maybe just call this Add()?  That better parallels the internal
CallbackList API.

Lastly, I am again wondering about returning a "remove callback" here.  Is it
really that annoying for that client classes to keep a reference to the list
around in a way that guarantees the list outlives them?

[Cait (Slow), 2013/08/27 22:14:20]

My thinking for the "remove callback" approach was that in the long run, it
would result in less work for list owners and clients. Without it, each list
owner will have to expose some way of adding and removing a callback (whereas in
the previous approach, they'd only have to expose a way to add a callback, as
removing would be done via the "remove callback"), and each client will need to
hold a ref to the list (or the list owner), and a ref to the callback they added
so it can be removed later. 

I've also encountered cases where things will occasionally disappear out from
under you (see https://codereview.chromium.org/22799016/ for an example), and
then the client ends up needing to hold (and check the validity of) a weak_ptr
to the list owner. 

If the "remove callback" API is cumbersome and/or confusing though, we could
certainly look at other approaches.

[awong, 2013/08/28 02:02:55]

My main concerns are:

(1) Binding a WeakPtr<> allows people to get lazy regarding the lifetime of the
CallbackList and their own object.

(2) returning a Closure means yet another dynamic allocation.

Neither of these are big issues, but I wish we could avoid them.

Given that the implementation internally is using a "new CallbackType(cb)", it
might not be possible to avoid a Callback based approach (what token would we
give back to the client when they want to remove themselves?).

However, do we really have to vend a Closure to a WeakPtr<>? Can we instead
force classes using this to restructure their lifetime guarantees? Also, does
the the previously discussed change to the destruction contract that asserts
size() == 0 make this impossible?

+    DCHECK(!cb.is_null());
+    // TODO(caitkp): This is a leak.
+    return AddCallback(new CallbackType(cb));
+  }
+
+  void Run() {
+    if (might_have_callbacks()) {
+      CallbackListBase::Iterator it(this);
+      CallbackType* cb;
+      while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) {
+        cb->Run();
+      }
+    }
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+#endif  // BASE_CALLBACK_LIST_H_