Add a CallbackRegistry class to base/ to manage callbacks

base/callback_registry.h

Index: base/callback_registry.h
diff --git a/base/callback_registry.h b/base/callback_registry.h
new file mode 100644
index 0000000000000000000000000000000000000000..c02c97d32f20909d53158a2eaae5f815835ebe96
--- /dev/null
+++ b/base/callback_registry.h
@@ -0,0 +1,230 @@
+// 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_REGISTRY_H_
+#define BASE_CALLBACK_REGISTRY_H_
+
+#include <list>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "base/logging.h"
+#include "base/memory/scoped_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;
+//
+//   scoped_ptr<base::Subscription> RegisterCallback(
+//       const OnFooCallback& cb) {
+//     return callback_registry_.Add(cb);
+//   }
+//
+//  private:
+//   void NotifyFoo(const Foo& foo) {
+//      callback_registry_.Notify(foo);
+//   }
+//
+//   CallbackRegistry<Foo> callback_registry_();

[awong, 2013/09/06 23:49:30]

nit: callback_registry_(); -> callback_registry_;

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

Done.

+// };
+//
+//
+// class MyWidgetListener {
+//  public:
+//   MyWidgetListener::MyWidgetListener() {
+//     foo_callback_handle_ = MyWidget::GetCurrent()->RegisterCallback(
+//             base::Bind(&MyWidgetListener::OnFoo, this)));
+//   }
+//
+//   MyWidgetListener::~MyWidgetListener() {
+//      // Subscription gets deleted automatically and will deregister
+//      // the callback in the process.
+//   }
+//
+//   void OnFoo(const Foo& foo) {
+//     // Do something.
+//   }
+//
+//  private:
+//   scoped_ptr<base::Subscription> foo_callback_handle_;

[awong, 2013/09/06 23:49:30]

foo_callback_handle_ -> foo_subscription_

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

Done.

+// };
+
+namespace base {
+
+class Subscription {

[awong, 2013/09/06 23:49:30]

base::Subscription is too general a name but before bikeshedding that....it also
occurs to me that if you're returning a scoped_ptr<base::Subscription>, I'm not
sure how you're going to store these in a container like you indicated might be
useful in a previous response.

Are you going to call .release() on all the returned values?  Something like:

  vector<base::Subscription*> v;
  v.push_back(registry.Add(...).release());

?

More and more, I'm realizing I don't understand all the usage requirements
enough. Can you a list of those into the CL description?  I've pinged a few
other people for what they think, and it keeps coming up that we don't have a
centralized list of requirements to sanity check the API semantics against.

[erikwright (departed), 2013/09/09 16:24:34]

Anyone managing more than one subscription will, in all likelihood, use an
EventRegistrar (Cait's got a POC for one lying around somewhere). So that will
be the only class that will really worry about grouping these things together.

But in any case, we have ScopedVector, and I would assume that we would do
something like

ScopedVector subscriptions_;

...

Callback<void(int)> cb = base::Bind(...);
subscriptions_.push_back(some_service.AddXCallback(cb).Pass());

Assuming we add a push_back overload taking scoped_ptr (which seems sensible) I
think that makes sense. I forget if Pass() is even required in that case.

[awong, 2013/09/09 17:19:28]

If there's only one expected user, I am weary of adding a general RAII interface
like Subscription into base::.  Let me IM the two of you on this.

(tangent: Pass() isn't required there since the expression is already an
rvalue...however ScopedVector does indeed seem to be missing something that
takes the scoped_ptr<> directly.)

+ public:
+  virtual ~Subscription() {}
+ protected:
+  Subscription() {}
+};
+
+namespace internal {
+// Holds the methods shared by all specializations of CallbackRegistry to avoid
+// code duplication.
+//
+// This class is meant as an implementation detail for CallbackRegistry. Do not

[awong, 2013/09/06 23:49:30]

This sentence is implied by base::internal. Can be dropped if you want.

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

Done.

+// use it directly.
+template <typename CallbackType>
+class CallbackRegistryBase {
+ public:
+  // Add a callback to the list. The callback will remain registered until the
+  // returned Subscription is destroyed, which must occur before the
+  // CallbackRegistry is destroyed.
+  scoped_ptr<Subscription> Add(const CallbackType& cb) WARN_UNUSED_RESULT {
+    DCHECK(!cb.is_null());
+    typename ListType::iterator it = callbacks_.insert(callbacks_.end(), cb);
+    return scoped_ptr<Subscription>(new SubscriptionImpl(this, it));
+  }
+
+ protected:
+  typedef std::list<CallbackType> ListType;
+
+  // An iterator class that can be used to access the list of callbacks.
+  class Iterator {
+   public:
+    explicit Iterator(CallbackRegistryBase<CallbackType>* list)
+        : list_(list),
+          list_iter_(list_->callbacks_.begin()) {
+      ++list_->active_iterator_count_;
+    }
+
+    Iterator(const Iterator& iter)
+        : list_(iter.list_),
+          list_iter_(iter.list_iter_) {
+      ++list_->active_iterator_count_;
+    }
+
+    ~Iterator() {
+      if (list_ && --list_->active_iterator_count_ == 0) {
+        list_->Compact();
+      }
+    }
+
+    CallbackType* GetNext() {
+      while ((list_iter_ != list_->callbacks_.end()) && list_iter_->is_null())
+        list_iter_++;
+
+      CallbackType* cb =
+          list_iter_ != list_->callbacks_.end() ? &(*list_iter_) : NULL;
+      list_iter_++;
+      return cb;
+    }
+
+   private:
+    CallbackRegistryBase<CallbackType>* list_;
+    typename ListType::iterator list_iter_;
+  };
+
+  CallbackRegistryBase()
+      : active_iterator_count_(0) {}
+
+  ~CallbackRegistryBase() {
+    DCHECK(active_iterator_count_ == 0);
+    DCHECK(callbacks_.size() == 0);
+  }
+
+  // Returns an instance of a CallbackRegistryBase::Iterator which can be used
+  // to run callbacks.
+  Iterator GetIterator() {
+    return Iterator(this);
+  }
+
+  // Compact the list: remove any entries which were NULLed out during
+  // iteration.
+  void Compact() {
+    typename ListType::iterator it = callbacks_.begin();
+    while (it != callbacks_.end()) {
+      if ((*it).is_null())
+        it = callbacks_.erase(it);
+      else
+        ++it;
+    }
+  }
+
+ private:
+  class SubscriptionImpl : public Subscription {
+   public:
+    SubscriptionImpl(

[awong, 2013/09/06 23:49:30]

nit: The parameters should be able to align with the (

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

Done.

+        CallbackRegistryBase<CallbackType>* list,
+        typename ListType::iterator iter)
+        : list_(list),
+          iter_(iter) {}
+
+    ~SubscriptionImpl() {
+      if (list_->active_iterator_count_)
+        (*iter_).Reset();
+      else
+        list_->callbacks_.erase(iter_);
+    }
+
+   private:
+    CallbackRegistryBase<CallbackType>* list_;
+    typename ListType::iterator iter_;
+
+    DISALLOW_COPY_AND_ASSIGN(SubscriptionImpl);
+  };
+
+  ListType callbacks_;
+  int active_iterator_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackRegistryBase);
+};
+
+}  // namespace internal
+
+template <typename Details>
+class CallbackRegistry
+    : public base::internal::CallbackRegistryBase<

[awong, 2013/09/09 17:19:28]

base:: is overqualified here.  We're already in base, so you should do
internal::CallbackRegistryBase.

Same comment for all base::internal, and base:: qualifications in this file.

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

Done.

+        Callback<void(const Details&)> > {

[awong, 2013/09/06 23:49:30]

nit: should indent 2 more spaces.

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

Done.

+ public:
+  typedef typename base::internal::CallbackRegistryBase<
+      base::Callback<void(const Details&)> >::Iterator Iterator;

[awong, 2013/09/06 23:49:30]

If removing this doesn't compile, why does it work in CallbackRegistry<void>
below?

[awong, 2013/09/06 23:49:30]

You seemed to be able to remove this for CallbackRegistry<void>. Why is it
necessary here then?

[erikwright (departed), 2013/09/09 16:18:01]

That's because, here, the base class is dependent on a template parameter
whereas in the case below it is a specific concrete class whose declaration is
known.

The compiler, when it parses line 199, needs to know what Iterator is. But
whether or not there is a type (Iterator) defined in this scope cannot be known
at this time. For example, I could declare a specialization of
CallbackRegistryBase<int> that doesn't declare Iterator.

Thus the typedef is necessary in order to tell the compiler "there is a type
base::internal::CallbackRegistryBase<...>::Iterator and I'm going to refer to it
as Iterator henceforth".

[awong, 2013/09/09 17:19:28]

Ah right...it's that stupid 2-phase dependant lookup thing with templates.

See my new comment at line 199.

+  CallbackRegistry() {}
+
+  // Execute all active callbacks with |details| parameter.
+  void Notify(const Details& details) {
+    Iterator it = this->GetIterator();

[awong, 2013/09/09 17:19:28]

There's only one use of the typedef here.  At the very least it should be moved
out of the public API (no one outside this class should need to declare an
Iterator).

However, since it's only a single use, it seems more sensible to just make this:

typename internal::CallbackRegistryBase<void(const Details&)>::Iterator it =
this->GetIterator();

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

Done.

+    base::Callback<void(const Details&)>* cb;
+    while((cb = it.GetNext()) != NULL) {
+      cb->Run(details);
+    }
+  }
+
+private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackRegistry);
+};
+
+template <> class CallbackRegistry<void>
+    : public base::internal::CallbackRegistryBase<Closure> {
+ public:
+  CallbackRegistry() {}
+
+  // Execute all active callbacks.
+  void Notify() {
+    Iterator it = this->GetIterator();
+    Closure* cb;
+    while((cb = it.GetNext()) != NULL) {
+      cb->Run();
+    }
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CallbackRegistry);
+};
+
+}  // namespace base
+
+#endif  // BASE_CALLBACK_REGISTRY_H_