Handle dynamic updates to holding space images.

This CL also changed from ObserverList to CallbackList since it is
much more lifecycle tolerant. It is safe for a Subscription to outlive
its associated CallbackList and destruction of a Subscription will
automatically un-register it.

Bug: 1113772
Change-Id: Ie1335f617e809b75d7b219aea48968ab54cb8225
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401510
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Ahmed Mehfooz <amehfooz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806219}

ash/public/cpp/holding_space/holding_space_image.cc

@@ -5,7 +5,6 @@
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 
 #include <map>
-#include <memory>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ -70,25 +69,16 @@ HoldingSpaceImage::HoldingSpaceImage(const gfx::ImageSkia& placeholder,
                                                     async_bitmap_resolver),
                   placeholder.size()) {}
 
-HoldingSpaceImage::~HoldingSpaceImage() {
-  NotifyDestroying();
-}
+HoldingSpaceImage::~HoldingSpaceImage() = default;
 
 bool HoldingSpaceImage::operator==(const HoldingSpaceImage& rhs) const {
   return gfx::BitmapsAreEqual(*image_skia_.bitmap(), *rhs.image_skia_.bitmap());
 }
 
-void HoldingSpaceImage::AddObserver(Observer* observer) const {
-  observers_.AddObserver(observer);
-}
-
-void HoldingSpaceImage::RemoveObserver(Observer* observer) const {
-  observers_.RemoveObserver(observer);
-}
-
-void HoldingSpaceImage::NotifyDestroying() {
-  for (auto& observer : observers_)
-    observer.OnHoldingSpaceImageDestroying(this);
+std::unique_ptr<HoldingSpaceImage::Subscription>
+HoldingSpaceImage::AddImageSkiaChangedCallback(
+    CallbackList::CallbackType callback) const {
+  return callback_list_.Add(std::move(callback));
 }
 
 void HoldingSpaceImage::NotifyUpdated(float scale) {
@@ -96,9 +86,7 @@ void HoldingSpaceImage::NotifyUpdated(float scale) {
   // updated `gfx::ImageSkiaRep` at next access.
   image_skia_.RemoveRepresentation(scale);
   image_skia_.RemoveUnsupportedRepresentationsForScale(scale);
-
-  for (auto& observer : observers_)
-    observer.OnHoldingSpaceImageUpdated(this);
+  callback_list_.Notify();
 }
 
 }  // namespace ash

ash/public/cpp/holding_space/holding_space_image.h

@@ -5,28 +5,20 @@
 #ifndef ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_IMAGE_H_
 #define ASH_PUBLIC_CPP_HOLDING_SPACE_HOLDING_SPACE_IMAGE_H_
 
+#include <memory>
+
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/callback_forward.h"
-#include "base/observer_list.h"
+#include "base/callback_list.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace ash {
 
-// A wrapper around a `gfx::ImageSkia` that supports dynamic updates. When
-// updates occur or an instance is being destroyed, observers are notified.
+// A wrapper around a `gfx::ImageSkia` that supports dynamic updates.
 class ASH_PUBLIC_EXPORT HoldingSpaceImage {
  public:
-  // An observer which receives notifications of `HoldingSpaceImage` events.
-  class Observer : public base::CheckedObserver {
-   public:
-    // Invoked when the `HoldingSpaceImage` is updated. UI classes should react
-    // to this event by invalidating any associated views.
-    virtual void OnHoldingSpaceImageUpdated(const HoldingSpaceImage*) {}
-
-    // Invoked when the `HoldingSpaceImage` is being destroyed. Any observers
-    // should react to this event by unregistering from the observer list.
-    virtual void OnHoldingSpaceImageDestroying(const HoldingSpaceImage*) {}
-  };
+  using CallbackList = base::RepeatingClosureList;
+  using Subscription = CallbackList::Subscription;
 
   // Returns a bitmap.
   using BitmapCallback = base::OnceCallback<void(const SkBitmap*)>;
@@ -43,9 +35,9 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
 
   bool operator==(const HoldingSpaceImage& rhs) const;
 
-  // Adds/remove the specified `observer`.
-  void AddObserver(Observer* observer) const;
-  void RemoveObserver(Observer* observer) const;
+  // Adds `callback` to be notified of changes to the underlying `image_skia_`.
+  std::unique_ptr<Subscription> AddImageSkiaChangedCallback(
+      CallbackList::CallbackType callback) const;
 
   // Returns the underlying `gfx::ImageSkia`. Note that the image source may be
   // dynamically updated, so UI classes should observe and react to updates.
@@ -54,13 +46,12 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
  private:
   class ImageSkiaSource;
 
-  void NotifyDestroying();
   void NotifyUpdated(float scale);
 
   gfx::ImageSkia image_skia_;
 
-  // Mutable to allow const access from `AddObserver()`/`RemoveObserver()`.
-  mutable base::ObserverList<HoldingSpaceImage::Observer> observers_;
+  // Mutable to allow const access from `AddImageSkiaChangedCallback()`.
+  mutable CallbackList callback_list_;
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_item_chip_view.cc

@@ -5,7 +5,6 @@
 #include "ash/system/holding_space/holding_space_item_chip_view.h"
 
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
-#include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/shelf_config.h"
 #include "ash/style/ash_color_provider.h"
@@ -66,6 +65,11 @@ HoldingSpaceItemChipView::HoldingSpaceItemChipView(const HoldingSpaceItem* item)
   views::InstallRoundRectHighlightPathGenerator(this, gfx::Insets(),
                                                 kHoldingSpaceChipCornerRadius);
 
+  // Subscribe to be notified of changes to `item_`'s image.
+  image_subscription_ =
+      item_->image().AddImageSkiaChangedCallback(base::BindRepeating(
+          &HoldingSpaceItemChipView::Update, base::Unretained(this)));
+
   Update();
 }
 

ash/system/holding_space/holding_space_item_chip_view.h

@@ -5,7 +5,10 @@
 #ifndef ASH_SYSTEM_HOLDING_SPACE_HOLDING_SPACE_ITEM_CHIP_VIEW_H_
 #define ASH_SYSTEM_HOLDING_SPACE_HOLDING_SPACE_ITEM_CHIP_VIEW_H_
 
+#include <memory>
+
 #include "ash/ash_export.h"
+#include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ui/views/animation/ink_drop_host_view.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/metadata/metadata_header_macros.h"
@@ -52,6 +55,8 @@ class ASH_EXPORT HoldingSpaceItemChipView : public views::InkDropHostView,
   tray::RoundedImageView* image_ = nullptr;
   views::Label* label_ = nullptr;
   views::ToggleImageButton* pin_ = nullptr;
+
+  std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
 };
 
 }  // namespace ash

ash/system/holding_space/holding_space_screenshot_view.cc

@@ -5,7 +5,6 @@
 #include "ash/system/holding_space/holding_space_screenshot_view.h"
 
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
-#include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/user/rounded_image_view.h"
@@ -30,6 +29,11 @@ HoldingSpaceScreenshotView::HoldingSpaceScreenshotView(
   image_ =
       AddChildView(std::make_unique<tray::RoundedImageView>(kTrayItemSize / 2));
 
+  // Subscribe to be notified of changes to `item_`'s image.
+  image_subscription_ =
+      item_->image().AddImageSkiaChangedCallback(base::BindRepeating(
+          &HoldingSpaceScreenshotView::Update, base::Unretained(this)));
+
   Update();
 }
 

ash/system/holding_space/holding_space_screenshot_view.h

@@ -5,7 +5,10 @@
 #ifndef ASH_SYSTEM_HOLDING_SPACE_HOLDING_SPACE_SCREENSHOT_VIEW_H_
 #define ASH_SYSTEM_HOLDING_SPACE_HOLDING_SPACE_SCREENSHOT_VIEW_H_
 
+#include <memory>
+
 #include "ash/ash_export.h"
+#include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ui/views/metadata/metadata_header_macros.h"
 #include "ui/views/view.h"
 
@@ -36,6 +39,8 @@ class ASH_EXPORT HoldingSpaceScreenshotView : public views::View {
 
   const HoldingSpaceItem* const item_;
   tray::RoundedImageView* image_ = nullptr;
+
+  std::unique_ptr<HoldingSpaceImage::Subscription> image_subscription_;
 };
 
 }  // namespace ash