Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list_internal.h

Index: base/callback_list_internal.h
diff --git a/base/callback_list_internal.h b/base/callback_list_internal.h
new file mode 100644
index 0000000000000000000000000000000000000000..b3c1834498281ae200c8803b05bd9679600e5fd2
--- /dev/null
+++ b/base/callback_list_internal.h
@@ -0,0 +1,94 @@
+// 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_INTERNAL_H_
+#define BASE_CALLBACK_LIST_INTERNAL_H_
+
+#include <limits>
+#include <vector>
+
+#include "base/base_export.h"
+#include "base/basictypes.h"
+#include "base/callback_internal.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+
+namespace base {
+namespace internal {
+
+// Holds the CallbackList methods that don't require specialization to reduce
+// template bloat.
+//
+// DO NOT USE THIS CLASS DIRECTLY YOURSELF.

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

Probably better to say

"This class is meant as an implementation detail for CallbackList and
CallbackListDetails. Do not use it directly."

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

Done.

+
+class BASE_EXPORT CallbackListBase {
+ public:
+  // Enumeration of which callbacks are called.
+  enum NotificationType {
+    // Specifies that any callbacks added during notification are called.
+    NOTIFY_ALL,
+
+    // Specifies that callbacks added while sending out notifications are not
+    // called.
+    NOTIFY_EXISTING_ONLY
+  };
+
+  explicit CallbackListBase(NotificationType type);
+  virtual ~CallbackListBase();

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

I don't think this needs to be virtual.  No one should ever use this type...

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

Done.

+
+  // Delete (or nullify, if called during iteration), all callbacks in the list.
+  void Clear();
+
+  bool might_have_callbacks() const { return size() != 0; }
+
+  protected:
+  typedef std::vector<base::internal::CallbackBase*> ListType;

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

It does seem that set or hash_set is a better choice here since the current code
largely seems to be checking that a bunch of pointers are unique.

Do you know if calling erase() in a hash_set or set invalidates iterators? If it
does not invalidate, you might be able to simplify your iterator implementation.

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

I think the issue is for any type of collection, erase() will invalidate all
iterators pointing to the element that was erased. So if I have two callbacks,
cb1 and cb2, and cb1 erases cb2, then cb1 will run fine, but when I come back to
run cb2, I'll have an iterator pointing to an element I just erased (and setting
the element to NULL instead of erasing is a no-go since set/hash_set iterators
are read-only).

I agree that some sort of container that enforces uniqueness would be nice here,
but that would prevent us from inserting NULL elements in as placeholders, and
I've yet to find a better way of dealing with the (somewhat esoteric) case of
callbacks removing other callbacks...
 

+
+  // An iterator class that can be used to access the list of callbacks.
+  class Iterator {
+   public:
+    Iterator(CallbackListBase* list);

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

explicit

Also, this should jsut take a const base::WeakPtr<CallbackListBase>&.

Then you don't need the GetWeakPtr() method in CallbackListBase.

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

I think GetWeakPtr is still needed, as CallbackList and CallbackListWithDetails
will still need a way to get a weak_ptr when they instantiate an iterator.

[awong, 2013/09/04 17:44:35]

How about replacing it with a GetIterator()?

+
+    ~Iterator();
+
+    base::internal::CallbackBase* GetNext();
+
+   private:
+    base::WeakPtr<CallbackListBase> list_;
+    size_t index_;
+    size_t max_index_;
+  };
+
+  // Remove a callback from the list if it is in the list.
+  void RemoveCallback(base::internal::CallbackBase* cb);
+
+  // Add a callback to the list, if it is not already in there. This method
+  // takes ownership of |cb|, and will handle deleting it upon removal from the
+  // list.
+  base::Closure AddCallback(base::internal::CallbackBase* cb);
+
+  // Get a weak pointer to the CallbackList.
+  base::WeakPtr<CallbackListBase> GetWeakPtr();
+
+  // Compact the list (remove any elements which were nulled out during
+  // iteration).
+  void Compact();
+
+  // Returns the number of callbacks in the list. Note that if the list has not
+  // been compacted, some of these callbacks may be NULL.
+  size_t size() const { return callbacks_.size(); }
+
+ private:
+  ListType callbacks_;
+  int active_iterator_count_;
+  NotificationType type_;
+  base::WeakPtrFactory<CallbackListBase> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListBase);
+};
+
+} // namespace internal
+} // namespace base
+
+#endif  // BASE_CALLBACK_LIST_INTERNAL_H_