From f126cb2007e7d20764aeb4d4a41f3061010d4ee4 Mon Sep 17 00:00:00 2001
From: davidben <davidben@chromium.org>
Date: Fri, 30 Oct 2015 13:42:07 -0700
Subject: [PATCH] Only signal the removal callback once in
 CallbackList::Compact.

Noticed this typo on a whim. Right now it signals it once for every registered
callback (removed or not) after the first removed one.

BUG=none

Review URL: https://codereview.chromium.org/1420053007

Cr-Commit-Position: refs/heads/master@{#357188}
---
 base/callback_list.h           |  6 ++---
 base/callback_list_unittest.cc | 45 ++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/base/callback_list.h b/base/callback_list.h
index aeed5f1e221621..5f263f899a37a7 100644
--- a/base/callback_list.h
+++ b/base/callback_list.h
@@ -185,10 +185,10 @@ class CallbackListBase {
       } else {
         ++it;
       }
-
-      if (updated && !removal_callback_.is_null())
-        removal_callback_.Run();
     }
+
+    if (updated && !removal_callback_.is_null())
+      removal_callback_.Run();
   }
 
  private:
diff --git a/base/callback_list_unittest.cc b/base/callback_list_unittest.cc
index 9adbabb0931cf8..1cae827bfcc8ef 100644
--- a/base/callback_list_unittest.cc
+++ b/base/callback_list_unittest.cc
@@ -98,6 +98,19 @@ class Summer {
   DISALLOW_COPY_AND_ASSIGN(Summer);
 };
 
+class Counter {
+ public:
+  Counter() : value_(0) {}
+
+  void Increment() { value_++; }
+
+  int value() const { return value_; }
+
+ private:
+  int value_;
+  DISALLOW_COPY_AND_ASSIGN(Counter);
+};
+
 // Sanity check that we can instantiate a CallbackList for each arity.
 TEST(CallbackListTest, ArityTest) {
   Summer s;
@@ -287,5 +300,37 @@ TEST(CallbackListTest, EmptyList) {
   cb_reg.Notify();
 }
 
+TEST(CallbackList, RemovalCallback) {
+  Counter remove_count;
+  CallbackList<void(void)> cb_reg;
+  cb_reg.set_removal_callback(
+      Bind(&Counter::Increment, Unretained(&remove_count)));
+
+  scoped_ptr<CallbackList<void(void)>::Subscription> subscription =
+      cb_reg.Add(Bind(&DoNothing));
+
+  // Removing a subscription outside of iteration signals the callback.
+  EXPECT_EQ(0, remove_count.value());
+  subscription.reset();
+  EXPECT_EQ(1, remove_count.value());
+
+  // Configure two subscriptions to remove themselves.
+  Remover remover_1, remover_2;
+  scoped_ptr<CallbackList<void(void)>::Subscription> remover_1_sub =
+      cb_reg.Add(Bind(&Remover::IncrementTotalAndRemove,
+          Unretained(&remover_1)));
+  scoped_ptr<CallbackList<void(void)>::Subscription> remover_2_sub =
+      cb_reg.Add(Bind(&Remover::IncrementTotalAndRemove,
+          Unretained(&remover_2)));
+  remover_1.SetSubscriptionToRemove(remover_1_sub.Pass());
+  remover_2.SetSubscriptionToRemove(remover_2_sub.Pass());
+
+  // The callback should be signaled exactly once.
+  EXPECT_EQ(1, remove_count.value());
+  cb_reg.Notify();
+  EXPECT_EQ(2, remove_count.value());
+  EXPECT_TRUE(cb_reg.empty());
+}
+
 }  // namespace
 }  // namespace base