Add a CallbackRegistry class to base/ to manage callbacks

base/callback_list_unittest.cc

Index: base/callback_list_unittest.cc
diff --git a/base/callback_list_unittest.cc b/base/callback_list_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e84847dbb81e35059e022eb2f4dacb5884fe83ab
--- /dev/null
+++ b/base/callback_list_unittest.cc
@@ -0,0 +1,362 @@
+// 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.h"
+
+#include <vector>
+
+#include "base/bind_helpers.h"
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace {
+
+class Listener {
+ public:
+  explicit Listener() : total_(0), scaler_(1) {}
+  Listener(int scaler) : total_(0), scaler_(scaler) {}

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

explicit

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

Done.

+ ~Listener() {}

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

No need to declare trivial destructors. Please remove here and elsewhere.

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

Done.

+  void IncrementTotal() { total_++; }
+  void MultiplyTotal(const int& x) { total_ += x * scaler_; }
+
+  int total_;
+ private:
+  int scaler_;
+  DISALLOW_COPY_AND_ASSIGN(Listener);
+};
+
+class Remover {
+ public:
+  explicit Remover() : total_(0), removal_cb_(base::Closure()) {}

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

-explicit

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

Done.

+  ~Remover() {}
+  void IncrementTotalAndRemove() {
+    total_++;
+    removal_cb_.Run();
+  }
+  void SetCallback(const base::Closure& cb) { removal_cb_ = cb; }
+
+  int total_;
+ private:
+  base::Closure removal_cb_;
+  DISALLOW_COPY_AND_ASSIGN(Remover);
+};
+
+class Adder {
+ public:
+  explicit Adder(CallbackList<void>* cb_list)
+      : added_(false),
+        total_(0),
+        cb_list_(cb_list) {}
+  ~Adder() {}
+  void AddCallback() {
+    if (!added_) {
+      added_ = true;
+      cb_list_->Add(
+          base::Bind(&Adder::IncrementTotal, base::Unretained(this)));
+    }
+  }
+  void IncrementTotal() { total_++; }
+
+  bool added_;
+  int total_;
+ private:
+  CallbackList<void>* cb_list_;
+  DISALLOW_COPY_AND_ASSIGN(Adder);
+};
+
+class Clearer {
+ public:
+  explicit Clearer(CallbackList<void>* cb_list)
+      : added_(false),
+        total_(0),
+        cb_list_(cb_list) {}
+  ~Clearer() {}
+  void ClearListAndAddCallback() {
+    cb_list_->Clear();
+    cb_list_->Add(
+        base::Bind(&Clearer::IncrementTotal, base::Unretained(this)));
+    added_ = true;
+  }
+  void IncrementTotal() { total_++; }
+
+  bool added_;
+  int total_;
+ private:
+  CallbackList<void>* cb_list_;
+  DISALLOW_COPY_AND_ASSIGN(Clearer);
+};
+
+class ListDestructor {
+ public:
+  explicit ListDestructor(CallbackList<void>* cb_list)
+      : cb_list_(cb_list) {}
+  ~ListDestructor() {}
+  void DestroyList() {
+    delete cb_list_;
+  }
+
+ private:
+  CallbackList<void>* cb_list_;
+  DISALLOW_COPY_AND_ASSIGN(ListDestructor);
+};
+
+// Sanity check that closures added to the list will be run, and those removed
+// from the list will not be run.
+TEST(CallbackListTest, BasicTest) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_ALL);
+  Listener a, b, c;
+
+  base::Closure remove_a = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&a)));
+  base::Closure remove_b = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&b)));
+
+  EXPECT_FALSE(remove_a.is_null());
+  EXPECT_FALSE(remove_b.is_null());
+
+  cb_list.Run();
+
+  EXPECT_EQ(1, a.total_);
+  EXPECT_EQ(1, b.total_);
+
+  remove_b.Run();
+
+  base::Closure remove_c = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&c)));
+
+  cb_list.Run();
+
+  EXPECT_EQ(2, a.total_);
+  EXPECT_EQ(1, b.total_);
+  EXPECT_EQ(1, c.total_);
+
+  remove_a.Run();
+  remove_b.Run();
+  remove_c.Run();
+
+  EXPECT_FALSE(cb_list.might_have_callbacks());
+}
+
+// Sanity check that callbacks with details added to the list will be run, with
+// the correct details, and those removed from the list will not be run.

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

Thank you for adding these comments! I like them!

+TEST(CallbackListTest, BasicTestWithParams) {
+  CallbackList<int> cb_list(CALLBACKS_NOTIFY_ALL);
+  Listener a(1), b(-1), c(1);
+
+  base::Closure remove_a = cb_list.Add(
+      base::Bind(&Listener::MultiplyTotal, base::Unretained(&a)));
+  base::Closure remove_b = cb_list.Add(
+      base::Bind(&Listener::MultiplyTotal, base::Unretained(&b)));
+
+  EXPECT_FALSE(remove_a.is_null());
+  EXPECT_FALSE(remove_b.is_null());
+
+  cb_list.Run(10);
+
+  EXPECT_EQ(10, a.total_);
+  EXPECT_EQ(-10, b.total_);
+
+  remove_b.Run();
+
+  base::Closure remove_c = cb_list.Add(
+      base::Bind(&Listener::MultiplyTotal, base::Unretained(&c)));
+
+  cb_list.Run(10);
+
+  EXPECT_EQ(20, a.total_);
+  EXPECT_EQ(-10, b.total_);
+  EXPECT_EQ(10, c.total_);
+
+  remove_a.Run();
+  remove_b.Run();
+  remove_c.Run();
+
+  EXPECT_FALSE(cb_list.might_have_callbacks());
+}
+
+// Test the a callback can remove itself or a different callback from the list
+// during iteration without invalidating the iterator.
+TEST(CallbackListTest, RemoveCallbacksDuringIteration) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_ALL);
+  Listener a, b;
+  Remover remover_1, remover_2;
+
+  base::Closure remover_1_cb = cb_list.Add(
+      base::Bind(&Remover::IncrementTotalAndRemove,
+          base::Unretained(&remover_1)));
+  base::Closure remover_2_cb = cb_list.Add(
+      base::Bind(&Remover::IncrementTotalAndRemove,
+          base::Unretained(&remover_2)));
+  base::Closure a_cb = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&a)));
+  base::Closure b_cb = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&b)));
+
+  // |remover_1| will remove itself.
+  remover_1.SetCallback(remover_1_cb);
+  // |remover_2| will remove a.
+  remover_2.SetCallback(a_cb);
+
+  cb_list.Run();
+
+  // |remover_1| runs once (and removes itself), |remover_2| runs once (and
+  // removes a), |a| never runs, and |b| runs once.
+  EXPECT_EQ(1, remover_1.total_);
+  EXPECT_EQ(1, remover_2.total_);
+  EXPECT_EQ(0, a.total_);
+  EXPECT_EQ(1, b.total_);
+
+  cb_list.Run();
+
+  // Only |remover_2| and |b| run this time.
+  EXPECT_EQ(1, remover_1.total_);
+  EXPECT_EQ(2, remover_2.total_);
+  EXPECT_EQ(0, a.total_);
+  EXPECT_EQ(2, b.total_);
+
+  EXPECT_TRUE(cb_list.might_have_callbacks());
+
+  remover_2_cb.Run();
+  b_cb.Run();
+
+  EXPECT_FALSE(cb_list.might_have_callbacks());
+}
+
+// Test that a callback can add another callback to the list durning iteration
+// without invalidating the iterator.
+// - If the CallbackNotificationType is |CALLBACKS_NOTIFY_ALL|, the newly added
+//   callback will be run on the current iteration.
+// - All other callbacks in the list will be run as well.
+TEST(CallbackListTest, AddCallbacksDuringIteration) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_ALL);
+  Adder a(&cb_list);
+  Listener b;
+  cb_list.Add(
+      base::Bind(&Adder::AddCallback, base::Unretained(&a)));
+  cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&b)));
+
+  cb_list.Run();
+
+  EXPECT_EQ(1, a.total_);
+  EXPECT_EQ(1, b.total_);
+  EXPECT_TRUE(a.added_);
+
+  cb_list.Run();
+
+  EXPECT_EQ(2, a.total_);
+  EXPECT_EQ(2, b.total_);
+}
+
+// Test that a callback can add another callback to the list durning iteration
+// without invalidating the iterator.
+// - If the CallbackNotificationType is |CALLBACKS_NOTIFY_EXISTING_ONLY|, the
+//   newly added will *not* be run during the current iteration, but will run on
+//   subsequent iterations.
+// - All other callbacks in the list will be run as well.
+TEST(CallbackListTest, AddCallbacksDuringIteration_Existing) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_EXISTING_ONLY);
+  Adder a(&cb_list);
+  Listener b;
+  cb_list.Add(
+      base::Bind(&Adder::AddCallback, base::Unretained(&a)));
+  cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&b)));
+
+  cb_list.Run();
+
+  // |a| should have been added, but not run yet, as we are in
+  // |CALLBACKS_NOTIFY_EXISTING_ONLY| mode.
+  EXPECT_EQ(0, a.total_);
+  EXPECT_EQ(1, b.total_);
+  EXPECT_TRUE(a.added_);
+
+  cb_list.Run();
+
+  // Now |a| should have run.
+  EXPECT_EQ(1, a.total_);
+  EXPECT_EQ(2, b.total_);
+}
+
+// Test that the list behaves as expected when callbacks are both added and
+// removed during iteration. In particular:
+// - If the CallbackNotificationType is |CALLBACKS_NOTIFY_EXISTING_ONLY|, the
+//   newly added callback should *not* be called during the current iteration.
+// - The callback which was removed should not run again after removal
+TEST(CallbackListTest, AddAndRemoveCallbacksDuringIteration_Existing) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_EXISTING_ONLY);
+  Adder a(&cb_list);
+  Remover b;
+  Listener c;
+
+  base::Closure c_cb = cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&c)));
+
+  // |b| removes |c|.
+  b.SetCallback(c_cb);
+  cb_list.Add(
+      base::Bind(&Remover::IncrementTotalAndRemove, base::Unretained(&b)));
+
+  // |a| adds a new callback.
+  cb_list.Add(base::Bind(&Adder::AddCallback, base::Unretained(&a)));
+
+  cb_list.Run();
+
+  // |c| ran once, new callback (|a|) should have been added but not yet run.
+  EXPECT_EQ(1, c.total_);
+  EXPECT_EQ(0, a.total_);
+  EXPECT_TRUE(a.added_);
+
+  cb_list.Run();
+
+  // Now |a| should have been run.
+  EXPECT_EQ(1, c.total_);
+  EXPECT_EQ(1, a.total_);
+}
+
+// Test that if we clear the list during iteration, no callbacks are run after
+// the one that did the clearing.
+TEST(CallbackListTest, ClearCallbacksDuringIteration) {
+  CallbackList<void> cb_list(CALLBACKS_NOTIFY_ALL);
+  Clearer a(&cb_list);
+  Listener b;
+  cb_list.Add(
+      base::Bind(&Clearer::ClearListAndAddCallback, base::Unretained(&a)));
+  cb_list.Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&b)));
+
+  cb_list.Run();
+
+  EXPECT_EQ(1, a.total_);
+  // |b| never runs because we cleared the list first.
+  EXPECT_EQ(0, b.total_);
+  EXPECT_TRUE(a.added_);
+}
+
+// Test that if the iterator outlives the list (that is, for some reason or
+// we delete the list mid-iteration) then:
+// - no callbacks run after deletion.
+// - the list is cleaned up properly (there will be memory errors if this is not
+//   the case).
+TEST(CallbackListTest, IteratorOutlivesList) {
+  CallbackList<void>* cb_list =
+      new CallbackList<void>(CALLBACKS_NOTIFY_ALL);
+  ListDestructor destructor(cb_list);
+  cb_list->Add(
+      base::Bind(&ListDestructor::DestroyList, base::Unretained(&destructor)));

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

Can this be used?

https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

If so, can we remove ListDestructor?

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

Done.

+  Listener a;
+  cb_list->Add(
+      base::Bind(&Listener::IncrementTotal, base::Unretained(&a)));
+
+  cb_list->Run();
+
+  // |a| never gets called, as |cb_list| got deleted first.

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

Hmm...this is allowing a list to delete itself from a callback.  Do we have the
full reentrancy contract in mind?

Let's say you have:


CallbackList<void>* cl = new CallbackList<void>();

cl->Add(Bind(&DeletePointer<CallbackList<void>*>, cl));
cl->Add(Bind(&CallbackList<void>::Clear, Unretained(&cl)));

cl->Run();

Will that reentrantly call Clear?

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

I think, upon reentrancy, the iterator's weak ptr to the list will be invalid
(as we just deleted the list), so the iterator will return NULL and no more
callbacks will be run.

+  EXPECT_EQ(0, a.total_);
+}
+
+}  // namespace
+}  // namespace base