Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list.h

+// 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/weak_ptr.h"
+
+///////////////////////////////////////////////////////////////////////////////

[awong, 2013/08/29 19:30:48]

nit: Can we remove the /////// borders?

None of the other code in base seems to do this.

Also, I would unindent the sections.  (I'm basing my pattern off of
base/files/file_path.h)

[Cait (Slow), 2013/08/30 00:28:02]

Done.

+//
+// 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_.Add(cb);
+//     }
+//
+//    private:
+//     void NotifyFoo(const Foo& foo) {
+//       callback_list_.Run(foo);
+//     }
+//
+//     CallbackList<Foo> callback_list_(CallbackList<Foo>::NOTIFY_ALL);
+//   };
+//
+//
+//  class MyWidgetListener {
+//    public:
+//

[awong, 2013/08/29 19:30:48]

nit: spurious newline.

[Cait (Slow), 2013/08/30 00:28:02]

Done.

+//     MyWidgetListener::MyWidgetListener() {
+//       // TODO (caitkp): A less clunky approach here?
+//       remove_foo_callback_.reset(new base::ScopedClosureRunner(

[Avi (use Gerrit), 2013/08/29 16:22:58]

:( It would be nice if you could do what scoped_ptr does, and allow a reset() to
a new value for ScopedClosureRunner.

[awong, 2013/08/29 19:30:48]

Yeah...we shouldn't need to scoped_ptr<> a Scoper...Let's wait for avi's change
to go in first.

[Cait (Slow), 2013/08/30 00:28:02]

Done.

+//           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:
+//     scoped_ptr<base::ScopedClosureRunner> remove_foo_callback_;
+//   };
+//
+///////////////////////////////////////////////////////////////////////////////
+
+namespace base {
+
+template <typename Detail>
+class CallbackListWithDetails : public internal::CallbackListBase {

[awong, 2013/08/29 19:30:48]

Sorry to keep churning this, but it occurs to me we do composition here rather
than inheritance, which would avoid the confusion with the virtual destructor. 
The tradeoff is making more methods in in CallbackListBase public, but since
it's in base::internal, I'm actually okay with that.

We'd also probably want to call CallbackListBase something like CallbackListImpl
or CallbackListInternal or CallbackListFuzzyMonster or something.

Do you mind switching over to that?

[Cait (Slow), 2013/08/30 00:28:02]

Done -- where should the NotificationType enum live in this case? 

+ public:
+  typedef base::Callback<void(const Detail&)> CallbackType;
+  typedef internal::CallbackListBase::NotificationType NotificationType;
+
+  explicit CallbackListWithDetails(NotificationType type)
+      : CallbackListBase(type) {}
+
+  virtual ~CallbackListWithDetails() {}

[awong, 2013/08/29 19:30:48]

Kill off the virtual destructors?

[Cait (Slow), 2013/08/30 00:28:02]

Done.

+
+  // 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) {
+    DCHECK(!cb.is_null());
+    return AddCallback(new CallbackType(cb));
+  }
+
+  // Execute all active (non-null) callbacks with |details| parameter.
+  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);
+      }
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackListWithDetails);
+};
+
+class CallbackList : public internal::CallbackListBase {
+ public:
+  typedef base::Closure CallbackType;
+  typedef CallbackListBase::NotificationType NotificationType;
+
+  explicit CallbackList(NotificationType type)
+      : CallbackListBase(type) {}
+
+  virtual ~CallbackList() {}

[awong, 2013/08/29 19:30:48]

no virtual?

[Cait (Slow), 2013/08/30 00:28:02]

Done.

+
+  // 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 base::Closure& cb) {
+    DCHECK(!cb.is_null());
+    return AddCallback(new CallbackType(cb));
+  }
+
+  // Execute all active (non-null) callbacks.
+  void Run() {
+    if (might_have_callbacks()) {
+      CallbackListBase::Iterator it(this);
+      CallbackType* cb;
+      while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) {
+        cb->Run();
+      }
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackList);
+};
+
+} // namespace base
+
+#endif  // BASE_CALLBACK_LIST_H_