Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list_internal.cc

+// 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.
+
+#include "base/callback_list_internal.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+
+namespace base {
+namespace internal {
+
+CallbackListImpl::Iterator::Iterator(
+    const base::WeakPtr<CallbackListImpl>& list)
+    : list_(list),
+      index_(0),
+      max_index_(list->type_ == CALLBACKS_NOTIFY_ALL ?
+                 std::numeric_limits<size_t>::max() :
+                 list->callbacks_.size()) {
+  ++list_->active_iterator_count_;
+}
+
+CallbackListImpl::Iterator::~Iterator() {
+  if (list_ && --list_->active_iterator_count_ == 0) {
+    list_->Compact();
+  }
+}
+
+CallbackBase* CallbackListImpl::Iterator::GetNext() {
+  if (!list_)
+    return NULL;
+  size_t max_index = std::min(max_index_, list_->callbacks_.size());
+  while (index_ < max_index && !list_->callbacks_[index_])
+    ++index_;
+  return index_ < max_index ? list_->callbacks_[index_++] : NULL;
+}
+
+CallbackListImpl::CallbackListImpl(CallbackNotificationType type)
+    : active_iterator_count_(0),
+      type_(type),
+      weak_factory_(this) {}
+
+CallbackListImpl::~CallbackListImpl() {
+  Clear();
+}
+
+void CallbackListImpl::Clear() {
+  if (active_iterator_count_) {
+    for (size_t i = 0; i < callbacks_.size(); ++i) {
+      CallbackBase* tmp = callbacks_[i];
+      tmp->Reset();
+      delete tmp;
+      callbacks_[i] = NULL;
+    }
+  } else {
+    for (size_t i = 0; i < callbacks_.size(); ++i)
+      delete callbacks_[i];
+    callbacks_.clear();
+  }
+}
+
+void CallbackListImpl::RemoveCallback(base::internal::CallbackBase* cb) {
+  for (size_t i = 0; i < callbacks_.size(); i++) {
+    if (callbacks_[i] == cb) {
+      CallbackBase* tmp = callbacks_[i];
+      tmp->Reset();
+      delete tmp;
+      if (active_iterator_count_) {
+        callbacks_[i] = NULL;
+      } else {
+        callbacks_.erase(callbacks_.begin() + i);
+      }
+      return;
+    }
+  }
+}
+
+base::Closure CallbackListImpl::AddCallback(base::internal::CallbackBase* cb) {
+  callbacks_.push_back(cb);
+  return base::Bind(&CallbackListImpl::RemoveCallback, Unretained(this), cb);

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

Should this be bound to a weakptr?

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

I think this is what I had originally, but we'd decided that might encourage
people to get a bit sloppy with the lifetimes of callback lists and their
observers.

[awong, 2013/09/04 23:20:07]

Right...hmm...on rereading, I worry that we're going to make it easy to get into
a use-after-free.  How about doing this pattern to get the best of both worlds:

Add a static function like:

// Use a static function to avoid WeakPtr cancellation behavior
// in Bind(). This allows strict enforcement that the caller
// must use the deregister the callback before the CallbackList is
// deleted.
static void CheckedRemove(const WeakPtr<CallbackListImpl>& list,
    base::internal::CallbackBase* cb) {
  list->Remove(cb);
}

Then in Add(), do:

return base::Bind(&CheckedRemove, weak_factory.GetWeakPtr(), cb);


That way the user still has to consider lifetimes, and we don't enter undefined
behavior if they do the wrong thing.  (If you think I'm going too far with
enforcing this, let me know).

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

Done.

+}
+
+base::WeakPtr<CallbackListImpl> CallbackListImpl::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void CallbackListImpl::Compact() {
+  callbacks_.erase(
+      std::remove(callbacks_.begin(), callbacks_.end(),
+                  static_cast<base::internal::CallbackBase*>(NULL)),
+                  callbacks_.end());
+}
+
+}  // namespace internal
+}  // namespace base