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..32ede83ff31b52ece7e80cc2a66fcb0aa6368716
--- /dev/null
+++ b/base/callback_list_internal.h
@@ -0,0 +1,96 @@
+// 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 {
+  // Enumeration of which callbacks are called.
+  enum CallbackNotificationType {

[erikwright (departed), 2013/09/04 19:35:55]

I would strongly prefer that we get opinionated and select one or the other of
these modes to implement.

[Cait (Slow), 2013/09/04 22:09:25]

Agree. By default, all of the current NotificationSystem uses NOTIFY_ALL. How
about we choose that one?

+    // Specifies that any callbacks added during notification are called.
+    CALLBACKS_NOTIFY_ALL,
+
+    // Specifies that callbacks added while sending out notifications are not
+    // called.
+    CALLBACKS_NOTIFY_EXISTING_ONLY
+  };
+
+namespace internal {
+
+// Holds the CallbackList methods that don't require specialization to reduce
+// template bloat.
+//
+// This class is meant as an implementation detail for CallbackList and
+// CallbackListWithDetails. Do not use it directly.
+
+class BASE_EXPORT CallbackListImpl {
+ public:
+  typedef std::vector<base::internal::CallbackBase*> ListType;
+
+  // An iterator class that can be used to access the list of callbacks.
+  class Iterator {
+   public:
+    explicit Iterator(const base::WeakPtr<CallbackListImpl>& list);

[erikwright (departed), 2013/09/04 19:35:55]

Instead of exposing GetWeakPtr and allowing clients to construct their own
Iterator, how about exposing a GetIterator() and then making GetWeakPtr private?

Either make it copyable (consistent with STL iterators) so that clients can
assign it to a local variable or return a scoped_ptr<Iterator>.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+    ~Iterator();
+
+    base::internal::CallbackBase* GetNext();
+
+   private:
+    base::WeakPtr<CallbackListImpl> list_;
+    size_t index_;
+    size_t max_index_;
+  };
+
+  explicit CallbackListImpl(CallbackNotificationType type);
+  ~CallbackListImpl();
+
+  // 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);

[awong, 2013/09/04 18:48:21]

Rename AddCallback/RemoveCallback to Add/Remove to match the public API?

[erikwright (departed), 2013/09/04 19:35:55]

Take a scoped_ptr. I think that's the new standard for documenting ownership
transfer.

[erikwright (departed), 2013/09/04 19:35:55]

Document the purpose of the return value.

[Cait (Slow), 2013/09/04 22:09:25]

CallbackBase has a protected dtor, so a scoped_ptr won't work here.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+  // Delete (or nullify, if called during iteration), all callbacks in the list.
+  void Clear();
+
+  // Returns true if there is a chance that the list may still contain active
+  // callbacks.
+  bool might_have_callbacks() const { return size() != 0; }

[erikwright (departed), 2013/09/04 19:35:55]

Why would a client call this?

[Cait (Slow), 2013/09/04 22:09:25]

removing it

+
+  // Remove a callback from the list if it is in the list.
+  void RemoveCallback(base::internal::CallbackBase* cb);

[erikwright (departed), 2013/09/04 19:35:55]

Seems this could be private (no reason not to insist that clients use the
'remove closure').

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+  // Get a weak pointer to the CallbackList.
+  base::WeakPtr<CallbackListImpl> GetWeakPtr();
+
+  // Compact the list (remove any elements which were nulled out during
+  // iteration).
+  void Compact();

[erikwright (departed), 2013/09/04 19:35:55]

Any reason for a client to call this (as opposed to the Iterator)?

[Cait (Slow), 2013/09/04 22:09:25]

No -- it should only be called by the iterator.

+
+  // 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(); }

[awong, 2013/09/04 18:48:21]

This doesn't seem necessary.  Remove?

[Cait (Slow), 2013/09/04 22:09:25]

Done.

+
+ private:
+  ListType callbacks_;
+  int active_iterator_count_;
+  CallbackNotificationType type_;
+  base::WeakPtrFactory<CallbackListImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallbackListImpl);
+};
+
+} // namespace internal
+} // namespace base
+
+#endif  // BASE_CALLBACK_LIST_INTERNAL_H_