Reland "Replace ThumbnailImage::Observer with Subscriber"

This is a reland of e4a4b7a3

Fixes null dereference bug

Original change's description:
> Replace ThumbnailImage::Observer with Subscriber
>
> Previously, clients interested in seeing thumbnail updates had to
> derive from ThumbnailImage::Observer and register themselves with
> ThumbnailImage.
>
> Removing ThumbnailTabHelper and ThumbnailImage from the public API and
> instead exposing a global ThumbnailService is a design
> goal. ThumbnailImage::Observer makes this refactor difficult.
>
> This CL uses a callback-based subscriber model instead.
> base::Callbacks replace observer methods. Hints are supplied
> directly on the subscription object. The subscription models loosens
> the coupling between clients and ThumbnailImage.
>
> Clients subscribe by ThumbnailImage::Subscribe(). Subscription can
> later be pushed up easily to the ThumbnailService level by a method
> like ThumbnailService::Subscribe(content::WebContents*).
>
> Bug: 1090038
> Change-Id: I020d848e00331d5f5bbb562f0c53aec5f8ccbc62
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593158
> Reviewed-by: Dana Fried <dfried@chromium.org>
> Reviewed-by: John Lee <johntlee@chromium.org>
> Commit-Queue: John Lee <johntlee@chromium.org>
> Auto-Submit: Collin Baker <collinbaker@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#837818}

Bug: 1090038
Change-Id: I3d93fc36d9725c088ba684188265df1ad1fd0905
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612046
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Reviewed-by: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842260}

chrome/browser/ui/thumbnails/thumbnail_image.cc

@@ -4,8 +4,10 @@
 
 #include "chrome/browser/ui/thumbnails/thumbnail_image.h"
 
+#include <algorithm>
 #include <utility>
 
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
@@ -15,15 +17,12 @@
 #include "ui/gfx/codec/jpeg_codec.h"
 #include "ui/gfx/skia_util.h"
 
-void ThumbnailImage::Observer::OnThumbnailImageAvailable(
-    gfx::ImageSkia thumbnail_image) {}
+ThumbnailImage::Subscription::Subscription(
+    scoped_refptr<ThumbnailImage> thumbnail)
+    : thumbnail_(std::move(thumbnail)) {}
 
-void ThumbnailImage::Observer::OnCompressedThumbnailDataAvailable(
-    CompressedThumbnailData thumbnail_data) {}
-
-base::Optional<gfx::Size> ThumbnailImage::Observer::GetThumbnailSizeHint()
-    const {
-  return base::nullopt;
+ThumbnailImage::Subscription::~Subscription() {
+  thumbnail_->HandleSubscriptionDestroyed(this);
 }
 
 ThumbnailImage::Delegate::~Delegate() {
@@ -46,29 +45,17 @@ ThumbnailImage::~ThumbnailImage() {
     delegate_->thumbnail_ = nullptr;
 }
 
-void ThumbnailImage::AddObserver(Observer* observer) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(observer);
-  if (!observers_.HasObserver(observer)) {
-    const bool is_first_observer = !observers_.might_have_observers();
-    observers_.AddObserver(observer);
-    if (is_first_observer && delegate_)
-      delegate_->ThumbnailImageBeingObservedChanged(true);
-  }
-}
+std::unique_ptr<ThumbnailImage::Subscription> ThumbnailImage::Subscribe() {
+  // Use explicit new since Subscription constructor is private.
+  auto subscription =
+      base::WrapUnique(new Subscription(base::WrapRefCounted(this)));
+  subscribers_.insert(subscribers_.end(), subscription.get());
 
-void ThumbnailImage::RemoveObserver(Observer* observer) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(observer);
-  if (observers_.HasObserver(observer)) {
-    observers_.RemoveObserver(observer);
-    if (delegate_ && !observers_.might_have_observers())
-      delegate_->ThumbnailImageBeingObservedChanged(false);
-  }
-}
+  // Notify |delegate_| if this is the first subscriber.
+  if (subscribers_.size() == 1)
+    delegate_->ThumbnailImageBeingObservedChanged(true);
 
-bool ThumbnailImage::HasObserver(const Observer* observer) const {
-  return observers_.HasObserver(observer);
+  return subscription;
 }
 
 void ThumbnailImage::AssignSkBitmap(SkBitmap bitmap) {
@@ -147,18 +134,22 @@ void ThumbnailImage::NotifyUncompressedDataObservers(gfx::ImageSkia image) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (async_operation_finished_callback_)
     async_operation_finished_callback_.Run();
-  for (auto& observer : observers_) {
-    auto size_hint = observer.GetThumbnailSizeHint();
-    observer.OnThumbnailImageAvailable(
-        size_hint ? CropPreviewImage(image, *size_hint) : image);
+
+  for (Subscription* subscription : subscribers_) {
+    auto size_hint = subscription->size_hint_;
+    if (subscription->uncompressed_image_callback_)
+      subscription->uncompressed_image_callback_.Run(
+          size_hint ? CropPreviewImage(image, *size_hint) : image);
   }
 }
 
 void ThumbnailImage::NotifyCompressedDataObservers(
     CompressedThumbnailData data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  for (auto& observer : observers_)
-    observer.OnCompressedThumbnailDataAvailable(data);
+  for (Subscription* subscription : subscribers_) {
+    if (subscription->compressed_image_callback_)
+      subscription->compressed_image_callback_.Run(data);
+  }
 }
 
 // static
@@ -216,3 +207,17 @@ gfx::ImageSkia ThumbnailImage::CropPreviewImage(
   source_image.bitmap()->extractSubset(&cropped, gfx::RectToSkIRect(clip_rect));
   return gfx::ImageSkia::CreateFrom1xBitmap(cropped);
 }
+
+void ThumbnailImage::HandleSubscriptionDestroyed(Subscription* subscription) {
+  // The order of |subscribers_| does not matter. We can simply swap
+  // |subscription| in |subscribers_| with the last element, then pop it
+  // off the back.
+  auto it = std::find(subscribers_.begin(), subscribers_.end(), subscription);
+  DCHECK(it != subscribers_.end());
+  std::swap(*it, *(subscribers_.end() - 1));
+  subscribers_.pop_back();
+
+  // If that was the last subscriber, tell |delegate_| (if it still exists).
+  if (delegate_ && subscribers_.empty())
+    delegate_->ThumbnailImageBeingObservedChanged(false);
+}

chrome/browser/ui/thumbnails/thumbnail_image.h

@@ -5,10 +5,12 @@
 #ifndef CHROME_BROWSER_UI_THUMBNAILS_THUMBNAIL_IMAGE_H_
 #define CHROME_BROWSER_UI_THUMBNAILS_THUMBNAIL_IMAGE_H_
 
+#include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/callback.h"
+#include "base/containers/flat_set.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
@@ -29,16 +31,27 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
   using CompressedThumbnailData =
       scoped_refptr<base::RefCountedData<std::vector<uint8_t>>>;
 
-  // Observes uncompressed and/or compressed versions of the thumbnail image as
-  // they are available.
-  class Observer : public base::CheckedObserver {
+  class Subscription {
    public:
-    // Receives uncompressed thumbnail image data. Default is no-op.
-    virtual void OnThumbnailImageAvailable(gfx::ImageSkia thumbnail_image);
+    Subscription() = delete;
+    ~Subscription();
 
-    // Receives compressed thumbnail image data. Default is no-op.
-    virtual void OnCompressedThumbnailDataAvailable(
-        CompressedThumbnailData thumbnail_data);
+    using UncompressedImageCallback =
+        base::RepeatingCallback<void(gfx::ImageSkia)>;
+    using CompressedImageCallback =
+        base::RepeatingCallback<void(CompressedThumbnailData)>;
+
+    // Set callbacks to receive image data. Subscribers are not allowed
+    // to unsubscribe (by destroying |this|) from the callback. If
+    // necessary, post a task to destroy it soon after.
+
+    void SetUncompressedImageCallback(UncompressedImageCallback callback) {
+      uncompressed_image_callback_ = std::move(callback);
+    }
+
+    void SetCompressedImageCallback(CompressedImageCallback callback) {
+      compressed_image_callback_ = std::move(callback);
+    }
 
     // Provides a desired aspect ratio and minimum size that the observer will
     // accept. If not specified, or if available thumbnail data is smaller in
@@ -54,7 +67,20 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
     // image passed to OnThumbnailImageAvailable fits the needs of the observer
     // for display purposes, without the observer having to further crop the
     // image. The default is unspecified.
-    virtual base::Optional<gfx::Size> GetThumbnailSizeHint() const;
+    void SetSizeHint(const base::Optional<gfx::Size>& size_hint) {
+      size_hint_ = size_hint;
+    }
+
+   private:
+    friend class ThumbnailImage;
+
+    explicit Subscription(scoped_refptr<ThumbnailImage> thumbnail);
+
+    scoped_refptr<ThumbnailImage> thumbnail_;
+    base::Optional<gfx::Size> size_hint_;
+
+    UncompressedImageCallback uncompressed_image_callback_;
+    CompressedImageCallback compressed_image_callback_;
   };
 
   // Represents the endpoint
@@ -77,9 +103,14 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
 
   bool has_data() const { return data_.get(); }
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
-  bool HasObserver(const Observer* observer) const;
+  // Subscribe to thumbnail updates. See |Subscription| to set a
+  // callback and conigure additional options.
+  //
+  // Even if a callback is not set, the subscription influences
+  // thumbnail capture. It should be destroyed when updates are not
+  // needed. It is designed to be stored in base::Optional, created and
+  // destroyed as needed.
+  std::unique_ptr<Subscription> Subscribe();
 
   // Sets the SkBitmap data and notifies observers with the resulting image.
   void AssignSkBitmap(SkBitmap bitmap);
@@ -130,6 +161,8 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
   static gfx::ImageSkia CropPreviewImage(const gfx::ImageSkia& source_image,
                                          const gfx::Size& minimum_size);
 
+  void HandleSubscriptionDestroyed(Subscription* subscription);
+
   Delegate* delegate_;
 
   // This is a scoped_refptr to immutable data. Once set, the wrapped
@@ -138,7 +171,12 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
   // the old data.
   CompressedThumbnailData data_;
 
-  base::ObserverList<Observer> observers_;
+  // Subscriptions are inserted on |Subscribe()| calls and removed when
+  // they are destroyed via callback. The order of subscriber
+  // notification doesn't matter, so don't maintain any ordering. Since
+  // the number of subscribers for a given thumbnail is expected to be
+  // small, doing a linear search to remove a subscriber is fine.
+  std::vector<Subscription*> subscribers_;
 
   // Called when an asynchronous operation (such as encoding image data upon
   // assignment or decoding image data for observers) finishes or fails.

chrome/browser/ui/thumbnails/thumbnail_tab_helper_browsertest.cc

@@ -29,37 +29,30 @@
 
 namespace {
 
-class ThumbnailWaiter : public ThumbnailImage::Observer {
+class ThumbnailWaiter {
  public:
   ThumbnailWaiter() = default;
-  ~ThumbnailWaiter() override = default;
+  ~ThumbnailWaiter() = default;
 
   base::Optional<gfx::ImageSkia> WaitForThumbnail(ThumbnailImage* thumbnail) {
-    DCHECK(!thumbnail_);
-    thumbnail_ = thumbnail;
-    scoped_observer_.Add(thumbnail);
-    thumbnail_->RequestThumbnailImage();
+    std::unique_ptr<ThumbnailImage::Subscription> subscription =
+        thumbnail->Subscribe();
+    subscription->SetUncompressedImageCallback(base::BindRepeating(
+        &ThumbnailWaiter::ThumbnailImageCallback, base::Unretained(this)));
+    thumbnail->RequestThumbnailImage();
     run_loop_.Run();
     return image_;
   }
 
  protected:
-  // ThumbnailImage::Observer:
-  void OnThumbnailImageAvailable(gfx::ImageSkia thumbnail_image) override {
-    if (thumbnail_) {
-      scoped_observer_.Remove(thumbnail_);
-      thumbnail_ = nullptr;
-      image_ = thumbnail_image;
-      run_loop_.Quit();
-    }
+  void ThumbnailImageCallback(gfx::ImageSkia thumbnail_image) {
+    image_ = std::move(thumbnail_image);
+    run_loop_.Quit();
   }
 
  private:
   base::RunLoop run_loop_;
-  ThumbnailImage* thumbnail_ = nullptr;
   base::Optional<gfx::ImageSkia> image_;
-  ScopedObserver<ThumbnailImage, ThumbnailImage::Observer> scoped_observer_{
-      this};
 };
 
 }  // anonymous namespace

chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc

@@ -396,12 +396,11 @@ class TabHoverCardBubbleView::FadeLabel : public views::Label {
 // Maintains a set of thumbnails to watch, ensuring the capture count on the
 // associated WebContents stays nonzero until a valid thumbnail has been
 // captured.
-class TabHoverCardBubbleView::ThumbnailObserver
-    : public ThumbnailImage::Observer {
+class TabHoverCardBubbleView::ThumbnailObserver {
  public:
   explicit ThumbnailObserver(TabHoverCardBubbleView* hover_card)
       : hover_card_(hover_card) {}
-  ~ThumbnailObserver() override = default;
+  ~ThumbnailObserver() = default;
 
   // Begin watching the specified thumbnail image for updates. Ideally, should
   // trigger the associated WebContents to load (if not loaded already) and
@@ -411,13 +410,17 @@ class TabHoverCardBubbleView::ThumbnailObserver
     if (current_image_ == thumbnail_image)
       return;
 
-    scoped_observation_.Reset();
+    subscription_.reset();
     current_image_ = std::move(thumbnail_image);
+    if (!current_image_)
+      return;
 
-    if (current_image_) {
-      scoped_observation_.Observe(current_image_.get());
-      current_image_->RequestThumbnailImage();
-    }
+    subscription_ = current_image_->Subscribe();
+    subscription_->SetSizeHint(TabStyle::GetPreviewImageSize());
+    subscription_->SetUncompressedImageCallback(base::BindRepeating(
+        &ThumbnailObserver::ThumbnailImageCallback, base::Unretained(this)));
+
+    current_image_->RequestThumbnailImage();
   }
 
   // Returns the current (most recent) thumbnail being watched.
@@ -425,18 +428,13 @@ class TabHoverCardBubbleView::ThumbnailObserver
     return current_image_;
   }
 
-  base::Optional<gfx::Size> GetThumbnailSizeHint() const override {
-    return TabStyle::GetPreviewImageSize();
-  }
-
-  void OnThumbnailImageAvailable(gfx::ImageSkia preview_image) override {
+  void ThumbnailImageCallback(gfx::ImageSkia preview_image) {
     hover_card_->OnThumbnailImageAvailable(std::move(preview_image));
   }
 
   scoped_refptr<ThumbnailImage> current_image_;
+  std::unique_ptr<ThumbnailImage::Subscription> subscription_;
   TabHoverCardBubbleView* const hover_card_;
-  base::ScopedObservation<ThumbnailImage, ThumbnailImage::Observer>
-      scoped_observation_{this};
 };
 
 TabHoverCardBubbleView::TabHoverCardBubbleView(Tab* tab)

chrome/browser/ui/webui/tab_strip/thumbnail_tracker.cc

@@ -15,14 +15,17 @@
 
 // Handles requests for a given tab's thumbnail and watches for thumbnail
 // updates for the lifetime of the tab.
-class ThumbnailTracker::ContentsData : public content::WebContentsObserver,
-                                       public ThumbnailImage::Observer {
+class ThumbnailTracker::ContentsData : public content::WebContentsObserver {
  public:
   ContentsData(ThumbnailTracker* parent, content::WebContents* contents)
       : content::WebContentsObserver(contents), parent_(parent) {
     thumbnail_ = parent_->thumbnail_getter_.Run(contents);
-    if (thumbnail_)
-      observation_.Observe(thumbnail_.get());
+    if (!thumbnail_)
+      return;
+
+    subscription_ = thumbnail_->Subscribe();
+    subscription_->SetCompressedImageCallback(base::BindRepeating(
+        &ContentsData::ThumbnailImageCallback, base::Unretained(this)));
   }
 
   void RequestThumbnail() {
@@ -35,8 +38,7 @@ class ThumbnailTracker::ContentsData : public content::WebContentsObserver,
     // We must un-observe each ThumbnailImage when the WebContents it came from
     // closes.
     if (thumbnail_) {
-      DCHECK(observation_.IsObservingSource(thumbnail_.get()));
-      observation_.Reset();
+      subscription_.reset();
       thumbnail_.reset();
     }
 
@@ -44,17 +46,14 @@ class ThumbnailTracker::ContentsData : public content::WebContentsObserver,
     parent_->ContentsClosed(web_contents());
   }
 
-  // ThumbnailImage::Observer:
-  void OnCompressedThumbnailDataAvailable(
-      CompressedThumbnailData thumbnail_image) override {
-    parent_->ThumbnailUpdated(web_contents(), thumbnail_image);
+ private:
+  void ThumbnailImageCallback(CompressedThumbnailData image) {
+    parent_->ThumbnailUpdated(web_contents(), image);
   }
 
- private:
   ThumbnailTracker* parent_;
   scoped_refptr<ThumbnailImage> thumbnail_;
-  base::ScopedObservation<ThumbnailImage, ThumbnailImage::Observer>
-      observation_{this};
+  std::unique_ptr<ThumbnailImage::Subscription> subscription_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentsData);
 };