Convert CallbackList::Subscription to a standalone class.

Bug: 1103086
AX-Relnotes: n/a.
TBR: pinkerton
Change-Id: I3b241eb7234727f314dd85d1bdbb3a41ceca5938
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2522860
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830015}

ash/app_list/views/search_result_actions_view.h

@@ -79,7 +79,7 @@ class APP_LIST_EXPORT SearchResultActionsView : public views::View {
   base::Optional<int> selected_action_;
 
   SearchResultActionsViewDelegate* const delegate_;  // Not owned.
-  std::list<views::PropertyChangedSubscription> subscriptions_;
+  std::list<base::CallbackListSubscription> subscriptions_;
 
   DISALLOW_COPY_AND_ASSIGN(SearchResultActionsView);
 };

ash/clipboard/views/clipboard_history_item_view.h

@@ -184,7 +184,7 @@ class ClipboardHistoryItemView : public views::View {
   // through `main_button_` or the delete button.
   ClipboardHistoryUtil::Action action_ = ClipboardHistoryUtil::Action::kEmpty;
 
-  views::PropertyChangedSubscription subscription_;
+  base::CallbackListSubscription subscription_;
 };
 
 }  // namespace ash

ash/frame/non_client_frame_view_ash.h

@@ -142,11 +142,10 @@ class ASH_EXPORT NonClientFrameViewAsh : public views::NonClientFrameView {
 
   std::unique_ptr<NonClientFrameViewAshImmersiveHelper> immersive_helper_;
 
-  std::unique_ptr<views::Widget::PaintAsActiveCallbackList::Subscription>
-      paint_as_active_subscription_ =
-          frame_->RegisterPaintAsActiveChangedCallback(
-              base::BindRepeating(&NonClientFrameViewAsh::PaintAsActiveChanged,
-                                  base::Unretained(this)));
+  base::CallbackListSubscription paint_as_active_subscription_ =
+      frame_->RegisterPaintAsActiveChangedCallback(
+          base::BindRepeating(&NonClientFrameViewAsh::PaintAsActiveChanged,
+                              base::Unretained(this)));
 
   base::WeakPtrFactory<NonClientFrameViewAsh> weak_factory_{this};
 

ash/frame/wide_frame_view.h

@@ -87,8 +87,7 @@ class ASH_EXPORT WideFrameView
   // Called when |target_|'s "paint as active" state has changed.
   void PaintAsActiveChanged();
 
-  std::unique_ptr<views::Widget::PaintAsActiveCallbackList::Subscription>
-      paint_as_active_subscription_;
+  base::CallbackListSubscription paint_as_active_subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(WideFrameView);
 };

ash/login/ui/login_pin_view.cc

@@ -356,7 +356,7 @@ class LoginPinView::BackspacePinButton : public BasePinButton {
       std::make_unique<base::RepeatingTimer>();
 
   views::ImageView* image_ = nullptr;
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(base::BindRepeating(
           &LoginPinView::BackspacePinButton::OnEnabledChanged,
           base::Unretained(this)));
@@ -399,7 +399,7 @@ class LoginPinView::SubmitPinButton : public BasePinButton {
 
  private:
   views::ImageView* image_ = nullptr;
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(
           base::BindRepeating(&LoginPinView::SubmitPinButton::OnEnabledChanged,
                               base::Unretained(this)));

ash/public/cpp/holding_space/holding_space_image.cc

@@ -75,8 +75,7 @@ bool HoldingSpaceImage::operator==(const HoldingSpaceImage& rhs) const {
   return gfx::BitmapsAreEqual(*image_skia_.bitmap(), *rhs.image_skia_.bitmap());
 }
 
-std::unique_ptr<HoldingSpaceImage::Subscription>
-HoldingSpaceImage::AddImageSkiaChangedCallback(
+base::CallbackListSubscription HoldingSpaceImage::AddImageSkiaChangedCallback(
     CallbackList::CallbackType callback) const {
   return callback_list_.Add(std::move(callback));
 }

ash/public/cpp/holding_space/holding_space_image.h

@@ -18,7 +18,6 @@ namespace ash {
 class ASH_PUBLIC_EXPORT HoldingSpaceImage {
  public:
   using CallbackList = base::RepeatingClosureList;
-  using Subscription = CallbackList::Subscription;
 
   // Returns a bitmap.
   using BitmapCallback = base::OnceCallback<void(const SkBitmap*)>;
@@ -36,7 +35,7 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
   bool operator==(const HoldingSpaceImage& rhs) const;
 
   // Adds `callback` to be notified of changes to the underlying `image_skia_`.
-  std::unique_ptr<Subscription> AddImageSkiaChangedCallback(
+  base::CallbackListSubscription AddImageSkiaChangedCallback(
       CallbackList::CallbackType callback) const;
 
   // Returns the underlying `gfx::ImageSkia`. Note that the image source may be

ash/search_box/search_box_view_base.h

@@ -199,7 +199,7 @@ class SearchBoxViewBase : public views::WidgetDelegateView,
   // Whether to show assistant button.
   bool show_assistant_button_ = false;
 
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(
           base::BindRepeating(&SearchBoxViewBase::OnEnabledChanged,
                               base::Unretained(this)));

ash/system/holding_space/holding_space_item_chip_view.h

@@ -48,7 +48,7 @@ class ASH_EXPORT HoldingSpaceItemChipView : public HoldingSpaceItemView {
   views::Label* label_ = nullptr;
   views::View* label_and_pin_button_container_ = nullptr;
 
-  std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
+  base::CallbackListSubscription image_subscription_;
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_item_screen_capture_view.h

@@ -34,7 +34,7 @@ class ASH_EXPORT HoldingSpaceItemScreenCaptureView
 
   RoundedImageView* image_ = nullptr;
 
-  std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
+  base::CallbackListSubscription image_subscription_;
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_tray_icon_preview.cc

@@ -146,7 +146,7 @@ class ContentsImage : public gfx::ImageSkia {
   }
 
   base::RepeatingClosure image_invalidated_closure_;
-  std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
+  base::CallbackListSubscription image_subscription_;
 };
 
 }  // namespace

ash/system/tray/hover_highlight_view.h

@@ -120,7 +120,7 @@ class HoverHighlightView : public ActionableView {
   TriView* tri_view_ = nullptr;
   bool expandable_ = false;
   AccessibilityState accessibility_state_ = AccessibilityState::DEFAULT;
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(
           base::BindRepeating(&HoverHighlightView::OnEnabledChanged,
                               base::Unretained(this)));

ash/system/unified/feature_pod_button.h

@@ -99,7 +99,7 @@ class FeaturePodLabelButton : public views::Button {
   views::Label* const label_;
   views::Label* const sub_label_;
   views::ImageView* const detailed_view_arrow_;
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(
           base::BindRepeating(&FeaturePodLabelButton::OnEnabledChanged,
                               base::Unretained(this)));
@@ -191,7 +191,7 @@ class ASH_EXPORT FeaturePodButton : public views::View {
   // expanded.
   bool visible_preferred_ = true;
 
-  views::PropertyChangedSubscription enabled_changed_subscription_ =
+  base::CallbackListSubscription enabled_changed_subscription_ =
       AddEnabledChangedCallback(
           base::BindRepeating(&FeaturePodButton::OnEnabledChanged,
                               base::Unretained(this)));

base/BUILD.gn

@@ -166,6 +166,7 @@ component("base") {
     "callback_helpers.h",
     "callback_internal.cc",
     "callback_internal.h",
+    "callback_list.cc",
     "callback_list.h",
     "cancelable_callback.h",
     "check.cc",

base/callback_list.cc

@@ -0,0 +1,39 @@
+// Copyright 2020 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 <utility>
+
+#include "base/callback.h"
+
+namespace base {
+
+CallbackListSubscription::CallbackListSubscription() = default;
+
+CallbackListSubscription::CallbackListSubscription(base::OnceClosure closure)
+    : closure_(std::move(closure)) {}
+
+CallbackListSubscription::CallbackListSubscription(
+    CallbackListSubscription&& subscription)
+    : closure_(std::move(subscription.closure_)) {}
+
+CallbackListSubscription& CallbackListSubscription::operator=(
+    CallbackListSubscription&& subscription) {
+  // Note: This still works properly for self-assignment.
+  Run();
+  closure_ = std::move(subscription.closure_);
+  return *this;
+}
+
+CallbackListSubscription::~CallbackListSubscription() {
+  Run();
+}
+
+void CallbackListSubscription::Run() {
+  if (closure_)
+    std::move(closure_).Run();
+}
+
+}  // namespace base

base/callback_list.h

@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "base/auto_reset.h"
+#include "base/base_export.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
@@ -32,8 +33,7 @@
 //   using CallbackList = base::RepeatingCallbackList<void(const Foo&)>;
 //
 //   // Registers |cb| to be called whenever NotifyFoo() is executed.
-//   std::unique_ptr<CallbackList::Subscription>
-//   RegisterCallback(CallbackList::CallbackType cb) {
+//   CallbackListSubscription RegisterCallback(CallbackList::CallbackType cb) {
 //     return callback_list_.Add(std::move(cb));
 //   }
 //
@@ -51,13 +51,13 @@
 //  private:
 //   void OnFoo(const Foo& foo) {
 //     // Called whenever MyWidget::NotifyFoo() is executed, unless
-//     // |foo_subscription_| has been reset().
+//     // |foo_subscription_| has been destroyed.
 //   }
 //
 //   // Automatically deregisters the callback when deleted (e.g. in
 //   // ~MyWidgetListener()).  Unretained(this) is safe here since the
-//   // Subscription does not outlive |this|.
-//   std::unique_ptr<MyWidget::CallbackList::Subscription> foo_subscription_ =
+//   // ScopedClosureRunner does not outlive |this|.
+//   CallbackListSubscription foo_subscription_ =
 //       MyWidget::Get()->RegisterCallback(
 //           base::BindRepeating(&MyWidgetListener::OnFoo,
 //                               base::Unretained(this)));
@@ -70,13 +70,44 @@
 // This is possible to support, but not currently necessary.
 
 namespace base {
+namespace internal {
+template <typename CallbackListImpl>
+class CallbackListBase;
+}  // namespace internal
 
 template <typename Signature>
 class OnceCallbackList;
 
 template <typename Signature>
 class RepeatingCallbackList;
 
+// A trimmed-down version of ScopedClosureRunner that can be used to guarantee a
+// closure is run on destruction. This is designed to be used by
+// CallbackListBase to run CancelCallback() when this subscription dies;
+// consumers can avoid callbacks on dead objects by ensuring the subscription
+// returned by CallbackListBase::Add() does not outlive the bound object in the
+// callback. A typical way to do this is to bind a callback to a member function
+// on `this` and store the returned subscription as a member variable.
+class BASE_EXPORT CallbackListSubscription {
+ public:
+  CallbackListSubscription();
+  CallbackListSubscription(CallbackListSubscription&& subscription);
+  CallbackListSubscription& operator=(CallbackListSubscription&& subscription);
+  ~CallbackListSubscription();
+
+  explicit operator bool() const { return !!closure_; }
+
+ private:
+  template <typename T>
+  friend class internal::CallbackListBase;
+
+  explicit CallbackListSubscription(base::OnceClosure closure);
+
+  void Run();
+
+  OnceClosure closure_;
+};
+
 namespace internal {
 
 // A traits class to break circular type dependencies between CallbackListBase
@@ -104,26 +135,9 @@ class CallbackListBase {
       typename CallbackListTraits<CallbackListImpl>::CallbackType;
   static_assert(IsBaseCallback<CallbackType>::value, "");
 
-  // A cancellation handle for callers who register callbacks. Subscription
-  // destruction cancels the associated callback and is legal any time,
-  // including after the destruction of the CallbackList that vends it.
-  class Subscription {
-   public:
-    explicit Subscription(base::OnceClosure destruction_closure)
-        : destruction_closure_(std::move(destruction_closure)) {}
-
-    Subscription(Subscription&&) = default;
-    Subscription& operator=(Subscription&&) = default;
-
-    ~Subscription() { std::move(destruction_closure_).Run(); }
-
-   private:
-    // Run when |this| is destroyed to notify the CallbackList the associated
-    // callback should be canceled. Since this is bound using a WeakPtr to the
-    // CallbackList, it will automatically no-op if the CallbackList no longer
-    // exists.
-    base::OnceClosure destruction_closure_;
-  };
+  // TODO(crbug.com/1103086): Update references to use this directly and by
+  // value, then remove.
+  using Subscription = CallbackListSubscription;
 
   CallbackListBase() = default;
   CallbackListBase(const CallbackListBase&) = delete;
@@ -134,11 +148,11 @@ class CallbackListBase {
     CHECK(!iterating_);
   }
 
-  // Registers |cb| for future notifications. Returns a Subscription that can be
-  // used to cancel |cb|.
-  std::unique_ptr<Subscription> Add(CallbackType cb) WARN_UNUSED_RESULT {
+  // Registers |cb| for future notifications. Returns a CallbackListSubscription
+  // whose destruction will cancel |cb|.
+  CallbackListSubscription Add(CallbackType cb) WARN_UNUSED_RESULT {
     DCHECK(!cb.is_null());
-    return std::make_unique<Subscription>(base::BindOnce(
+    return CallbackListSubscription(base::BindOnce(
         &CallbackListBase::CancelCallback, weak_ptr_factory_.GetWeakPtr(),
         callbacks_.insert(callbacks_.end(), std::move(cb))));
   }