Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list_internal.cc

Index: base/callback_list_internal.cc
diff --git a/base/callback_list_internal.cc b/base/callback_list_internal.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2072f3df9f2daf9a6b5a02512d67fdb68a3f620b
--- /dev/null
+++ b/base/callback_list_internal.cc
@@ -0,0 +1,105 @@
+// 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 {
+
+CallbackListBase::Iterator::Iterator(CallbackListBase* list);
+    : list_(list->GetWeakPtr()),
+      index_(0),
+      max_index_(list->type_ == NOTIFY_ALL ?
+                 std::numeric_limits<size_t>::max() :
+                 list->callbacks_.size()) {
+  ++list_->active_iterator_count_;
+}
+
+CallbackListBase::Iterator::~Iterator(); {
+  if (list_ && --list_->active_iterator_count_ == 0) {
+    list_->Compact();
+  }
+}
+
+CallbackBase* CallbackListBase::Iterator::GetNext(); {
+  if (!list_)
+    return NULL;
+  ListType& callbacks = list_->callbacks_;

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

Normally I would object to the use of a reference type cause they're not really
that common in chromium code, but I also see nothing banning them...hmm...up to
you if you want to keep.

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

Done.

+  size_t max_index = std::min(max_index_, callbacks.size());
+  while (index_ < max_index && !callbacks[index_])
+    ++index_;
+  return index_ < max_index ? callbacks[index_++] : NULL;
+}
+
+CallbackListBase::CallbackListBase(NotificationType type)
+    : active_iterator_count_(0),
+      type_(type),
+      weak_factory_(this) {}
+
+CallbackListBase::~CallbackListBase() {
+  Compact();

[awong, 2013/08/29 20:45:34]

Is this safe if there is an active iterator?

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

Not really...but since we're destroying the list, all active iterators are going
to be inactive very soon. It's not needed if we're not doing the size check in
the dtor though, so I'll remove it.

+  DCHECK_EQ(0U, size());

[awong, 2013/08/29 20:45:34]

Was just thinking about this...in your original CL, sometimes you didn't
actually care if the list was null or not.

What about removing this dcheck, and instead adding a Size() or AssertNull() API
that the person owning the callback list can assert on?

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

So I already sort of have this (with the might_have_callbacks() API), it doesn't
compact the list prior to checking it, but provided there are no active
iterators, the list should already be in a compacted state.

Seems like the most important thing is to make sure that all the callback ptrs
we allocated get freed upon destruction (hence the sanity-check call to
Clear()).

+  Clear();
+}
+
+void CallbackListBase::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 CallbackListBase::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 CallbackListBase::AddCallback(base::internal::CallbackBase* cb) {
+  for (size_t i = 0; i < callbacks_.size(); i++) {
+    if (callbacks_[i] == cb) {
+      NOTREACHED() << "Callbacks can only be added once!";

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

This check doesn't do anything useful beyond a set anymore since we'r only
comparing pointer values.

If you're looking to compare callbacks_[i].Equals(cb) then that's a different
story.

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

Done.

+      return base::Bind(&base::DoNothing);;
+    }
+  }
+  callbacks_.push_back(cb);
+  return base::Bind(&CallbackListBase::RemoveCallback, Unretained(this), cb);
+}
+
+base::WeakPtr<CallbackListBase> CallbackListBase::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void CallbackListBase::Compact() {
+  callbacks_.erase(
+      std::remove(callbacks_.begin(), callbacks_.end(),
+                  static_cast<base::internal::CallbackBase*>(NULL)),
+                  callbacks_.end());
+}
+
+}  // namespace internal
+}  // namespace base