Add a CallbackRegistry class to base/ to manage callbacks

base/callback_registry_internal.h

Index: base/callback_registry_internal.h
diff --git a/base/callback_registry_internal.h b/base/callback_registry_internal.h
new file mode 100644
index 0000000000000000000000000000000000000000..0b87943ffc94d1634e53601328d4706e3f3c3df2
--- /dev/null
+++ b/base/callback_registry_internal.h
@@ -0,0 +1,138 @@
+// 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_INTERNAL_H_
+#define BASE_CALLBACK_REGISTRY_INTERNAL_H_
+
+#include <limits>
+#include <list>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "base/callback_handle.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+
+namespace base {
+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

[erikwright (departed), 2013/09/06 21:01:07]

"do" should start a new sentence (be preceded by a ".").

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+// use it directly.
+template <typename CallbackType>
+class CallbackRegistryBase {
+ public:
+  virtual ~CallbackRegistryBase() {

[awong, 2013/09/06 19:03:24]

No need for virtual since no one should have a pointer to this.  Make the
destructor protected?

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+    AssertEmpty();
+  }
+
+  // Add a callback to the list. A callback should not be added to
+  // the same list more than once. The returned handle should be retained until

[erikwright (departed), 2013/09/06 21:01:07]

The warning against adding a callback more than once can be removed.

If the client does that, they'll get what they asked for (double notification).

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+  // the listener wishes to deregister the callback, at which point, deleting

[erikwright (departed), 2013/09/06 21:01:07]

The bit about the returned handle is a bit repetitive.

How about "The callback will remain registered until the returned CallbackHandle
is destroyed, which must occur before the CallbackList is destroyed."

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+  // the handle will automatically deregister the callback.
+  scoped_ptr<CallbackHandle> Add(const CallbackType& cb) WARN_UNUSED_RESULT {
+    DCHECK(!cb.is_null());
+    typename ListType::iterator it = callbacks_.insert(callbacks_.end(), cb);
+    return scoped_ptr<CallbackHandle>(new HandleImpl(this, it));
+  }
+
+  // Assert that the list is empty and no iterators are active.
+  void AssertEmpty() {

[awong, 2013/09/06 19:03:24]

Make this protected?

[erikwright (departed), 2013/09/06 21:01:07]

I would actually just inline it in the destructor. In fact, the name is
inaccurate and the checks redundant:

1) It asserts both that the list is both empty AND there are no active
iterators.
2) The check against iterators is redundant since there may not be any active
iterators if the list is empty.

I think both checks are useful (to catch internal bugs) but I would create two
distinct DCHECKs so that you can actually tell which one fired. And I would
check active_iterator_count_ first because, again, it should only be > 0 if
callbacks_.size() is also > 0, so we only get additional information if it is
checked first.

[Cait (Slow), 2013/09/06 22:17:11]

Done.

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+    DCHECK(active_iterator_count_ == 0 && callbacks_.size() == 0);
+  }
+
+ protected:
+  typedef std::list<CallbackType> ListType;

[awong, 2013/09/06 19:03:24]

nit: maybe use std::slist?

[erikwright (departed), 2013/09/06 21:01:07]

That would make removal O(N).

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

[erikwright (departed), 2013/09/06 21:01:07]

also here, list_ is never null

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+        list_->Compact();
+      }
+    }
+
+    CallbackType* GetNext() {
+      if (!list_)

[awong, 2013/09/06 19:03:24]

I don't think it's possible for list_ to be NULL.

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+        return NULL;
+      while ((list_iter_ != list_->callbacks_.end()) && (*list_iter_).is_null())

[erikwright (departed), 2013/09/06 21:01:07]

'(*list_iter_).' -> 'list_iter_->'

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+        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_;
+

[erikwright (departed), 2013/09/06 21:01:07]

remove blank line

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+  };
+
+  CallbackRegistryBase()
+      : active_iterator_count_(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 HandleImpl : public CallbackHandle {
+   public:
+    HandleImpl(CallbackRegistryBase<CallbackType>* list,
+               typename ListType::iterator iter)
+      : list_(list),
+        iter_(iter) {}
+
+    ~HandleImpl() {
+      if (list_->active_iterator_count_)
+        (*iter_).Reset();
+      else
+        list_->callbacks_.erase(iter_);
+    }
+
+   private:
+    CallbackRegistryBase<CallbackType>* list_;
+    typename ListType::iterator iter_;
+  };

[erikwright (departed), 2013/09/06 21:01:07]

DISALLOW...

[Cait (Slow), 2013/09/06 22:17:11]

Done.

+
+  ListType callbacks_;
+  int active_iterator_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackRegistryBase);
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_CALLBACK_REGISTRY_INTERNAL_H_