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..a492179226887e0766eb36ab6f93d5bd522172f9
--- /dev/null
+++ b/base/callback_list.h
@@ -0,0 +1,209 @@
+// 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__

[Avi (use Gerrit), 2013/08/22 16:25:42]

One trailing _. (Yes, I see that ObserverList still has two, but two hasn't been
the style in yeeeears.)

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+
+#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(Foo*)> OnFooCallback;
+//
+//     base::Closure RegisterCallback(const OnFooCallback& cb) {
+//       return callback_list_.RegisterCallback(cb);
+//     }
+//
+//     void NotifyFoo(Foo* foo) {

[Avi (use Gerrit), 2013/08/22 16:25:42]

NotifyFoo really belongs in the private section; it's usually the guts of
MyWidget that are notifying callbacks, not random strangers on the outside.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+//       FOR_EACH_CALLBACK(OnFooCallback, callback_list_, foo);
+//     }
+//
+//    private:
+//     CallbackList<OnFooCallback> callback_list_;
+//   };
+//
+//
+//  class MyWidgetListener {
+//    public:
+//
+//     MyWidgetListener::MyWidgetListener(){

[Avi (use Gerrit), 2013/08/22 16:25:42]

Space between ) {.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+//       remove_foo_callback_  = MyWidget::GetCurrent()->RegisterCallback(

[Avi (use Gerrit), 2013/08/22 16:25:42]

Only one space between _ =.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+//           base::Bind(&MyWidgetListener::OnFoo, this));
+//     }
+//
+//     MyWidgetListener::~MyWidgetListener() {
+//       remove_foo_callback_.Run();
+//     }
+//
+//     void OnFoo(Foo* foo) {
+//       // do something.

[Avi (use Gerrit), 2013/08/22 16:25:42]

Comments are complete sentences. Capitalizing "do" will do.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+//     }
+//
+//    private:
+//     base::Closure remove_foo_callback_;
+//   };
+//
+///////////////////////////////////////////////////////////////////////////////
+
+template <class SIG>
+class CallbackListBase {
+ public:
+  // An iterator class that can be used to access the list of callbacks.  See
+  // also the FOR_EACH_CALLBACK macro defined below.
+  class Iterator {
+   public:
+    Iterator(CallbackListBase<SIG>& list)
+        : list_(list.GetWeakPtr()),
+          index_(0) {
+      ++list_->notify_depth_;
+    }
+
+    ~Iterator() {
+      if (list_.get() && --list_->notify_depth_ == 0)
+        list_->Compact();
+    }
+
+    SIG GetNext() {
+      if (!list_.get())
+        return SIG();
+      ListType& callbacks = list_->callbacks_;
+      return index_ < callbacks.size() ? callbacks[index_++] : SIG();
+    }

[Avi (use Gerrit), 2013/08/22 16:25:42]

This is a very strange iterator class, one that is different than the parallel
one in ObserverList. You might want to note that GetNext might return null
callbacks, and that the caller shouldn't rely on that to stop iteration. Perhaps
we should make the iterator class private. Perhaps we should make
ObserverList::Iterator private too (does anyone external actually use it?).

[Cait (Slow), 2013/08/23 16:33:44]

So I've cleaned up the iterator class a bit. Notably: it is now safe to assume
that if it returns a null callback, then it is safe to stop iteration.
Unfortunately, it can't be made private, because the macro needs to use it (same
with ObserverList::Iterator, which is actually used a fair amount externally
beyond that).

[Avi (use Gerrit), 2013/08/23 16:51:11]

What!? Macros aren't class-scoped?

Yeah... ok... sniff...

+
+    bool HasNext() {
+      if (!list_.get())
+        return false;
+      return index_ < list_->callbacks_.size();
+    }
+
+   private:
+    base::WeakPtr<CallbackListBase<SIG> > list_;
+    size_t index_;
+  };
+
+  CallbackListBase() : notify_depth_(0),
+                       weak_factory_(this) {}
+
+  // 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 SIG& cb) {
+    for (size_t i = 0; i < callbacks_.size(); i++) {
+      if (callbacks_[i].Equals(cb)) {
+        NOTREACHED() << "Callbacks can only be added once!";
+        return base::Bind(&base::DoNothing);
+      }
+    }
+    callbacks_.push_back(cb);
+    base::Closure dereg = base::Bind(
+        &CallbackListBase::RemoveCallback, GetWeakPtr(), cb);
+    return dereg;
+  }
+
+  // Remove an callback from the list if it is in the list.
+  void RemoveCallback(const SIG& cb) {
+    for (size_t i = 0; i < callbacks_.size(); i++) {
+      if (callbacks_[i].Equals(cb)) {
+        if (notify_depth_)
+          callbacks_[i].Reset();
+        else
+          callbacks_.erase(callbacks_.begin() + i);
+        return;
+      }
+    }
+  }
+
+  bool HasCallback(const SIG& cb) const {
+    for (size_t i = 0; i < callbacks_.size(); ++i) {
+      if (callbacks_[i].Equlas(cb))

[Avi (use Gerrit), 2013/08/22 16:25:42]

Can you compile and run your unit test before uploading each patch set? It would
catch typos like this :)

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+        return true;
+    }
+    return false;
+  }
+
+  void Clear() {
+    if (notify_depth_) {
+      for (size_t i = 0; i < callbacks_.size(); ++i)
+        callbacks_[i].Reset();
+    } else {
+      callbacks_.clear();
+    }
+  }
+
+  size_t size() const { return callbacks_.size(); }

[Avi (use Gerrit), 2013/08/22 16:25:42]

This is misleading, as it counts deleted items. I'd make it private or at least
protected. I'd make the ObserverList one private too.

[Cait (Slow), 2013/08/23 16:33:44]

Done. (NB: Parallel observerList changes coming in a separate CL).

+
+ protected:
+  base::WeakPtr<CallbackListBase<SIG>> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+  void Compact() {
+    for (size_t i = 0; i < callbacks_.size(); i++) {
+      if (callbacks_[i].is_null()) {
+        callbacks_.erase(callbacks_.begin() + i);
+      }
+    }
+  }
+
+ private:
+  friend class CallbackListBase::Iterator;
+  typedef std::vector<SIG> ListType;
+  ListType callbacks_;
+  int notify_depth_;

[Avi (use Gerrit), 2013/08/22 16:25:42]

IMO this should be named "active_iterator_count_" or the like. Yes, it's
misnamed in ObserverList too.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+  base::WeakPtrFactory<CallbackListBase<SIG>> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListBase);
+};
+
+template <class SIG, bool check_empty = false>
+class CallbackList : public CallbackListBase<SIG> {
+ public:
+  CallbackList() {}
+
+  ~CallbackList() {
+    // When check_empty is true, assert that the list is empty on destruction.
+    if (check_empty) {
+      CallbackListBase<SIG>::Compact();
+      DCHECK_EQ(CallbackListBase<SIG>::size(), 0U);

[Avi (use Gerrit), 2013/08/22 16:25:42]

In DCHECKs the constant is the first parameter. Yes, broken in ObserverList too.

[Cait (Slow), 2013/08/23 16:33:44]

Done.

+    }
+  }
+
+  bool might_have_callbacks() const {
+    return CallbackListBase<SIG>::size() != 0;
+  }
+};
+
+#define FOR_EACH_CALLBACK(CallbackType, callback_list, ...)          \
+  do {                                                               \
+    if ((callback_list).might_have_callbacks()) {                    \
+      CallbackListBase<CallbackType>::Iterator it(callback_list);    \
+      CallbackType cb;                                               \
+      while(it.HasNext()) {                                          \
+        if(!(cb = it.GetNext()).is_null())                           \
+          cb.Run(__VA_ARGS__ ) ;                                     \

[Avi (use Gerrit), 2013/08/22 16:25:42]

Are those two odd spaces required? Eew.

[Avi (use Gerrit), 2013/08/22 16:25:42]

Variadic macros are new in C99. Does this work in all our compilers (especially
concerned here about VS)? Does this work in C++ code?

[Cait (Slow), 2013/08/23 16:33:44]

Done.

[Cait (Slow), 2013/08/23 16:33:44]

Good question -- to the trybots!

+      }                                                              \
+    }                                                                \
+  } while (0)
+
+#endif  // BASE_CALLBACK_LIST_H__