Update subscribers when a thumbnail is cleared

Ensures subscribers know when a thumbnail is cleared, and avoids
sending them stale thumbnails from previous requests.

Fixed: 1163121
Change-Id: I31d881808fb2a6bb14ae20b7d5b2d67545b585c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628031Reviewed-by: Dana Fried <dfried@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844166}

chrome/browser/ui/thumbnails/thumbnail_image.cc

@@ -61,6 +61,8 @@ std::unique_ptr<ThumbnailImage::Subscription> ThumbnailImage::Subscribe() {
 
 void ThumbnailImage::AssignSkBitmap(SkBitmap bitmap,
                                     base::Optional<uint64_t> frame_id) {
+  thumbnail_id_ = base::Token::CreateRandom();
+
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
       {base::TaskPriority::USER_VISIBLE,
@@ -68,24 +70,29 @@ void ThumbnailImage::AssignSkBitmap(SkBitmap bitmap,
       base::BindOnce(&ThumbnailImage::CompressBitmap, std::move(bitmap),
                      frame_id),
       base::BindOnce(&ThumbnailImage::AssignJPEGData,
-                     weak_ptr_factory_.GetWeakPtr(), base::TimeTicks::Now(),
-                     frame_id));
+                     weak_ptr_factory_.GetWeakPtr(), thumbnail_id_,
+                     base::TimeTicks::Now(), frame_id));
 }
 
 void ThumbnailImage::ClearData() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // TODO(crbug.com/1163121): Update this to notify the observers that the data
-  // has changed. It may be necessary to re-engineer them to accept
-  // empty/invalid data. crbug.com/1152894
-  if (!data_)
+  if (!data_ && thumbnail_id_.is_zero())
     return;
 
-  // TODO(crbug.com/1163121): Cancel existing thumbnail request. If
-  // called after ConvertJPEGDataToImageSkiaAndNotifyObservers() but
-  // before observers get notified, the observers will receive the stale
-  // thumbnail.
+  // If there was stored data we should notify observers that it was
+  // cleared. Otherwise, a bitmap was assigned but never compressed so
+  // the observers still think the thumbnail is blank.
+  const bool should_notify = !!data_;
+
   data_.reset();
+  thumbnail_id_ = base::Token();
+
+  // Notify observers of the new, blank thumbnail.
+  if (should_notify) {
+    NotifyCompressedDataObservers(data_);
+    NotifyUncompressedDataObservers(thumbnail_id_, gfx::ImageSkia());
+  }
 }
 
 void ThumbnailImage::RequestThumbnailImage() {
@@ -104,9 +111,18 @@ size_t ThumbnailImage::GetCompressedDataSizeInBytes() const {
   return data_->data.size();
 }
 
-void ThumbnailImage::AssignJPEGData(base::TimeTicks assign_sk_bitmap_time,
-                                    base::Optional<uint64_t> frame_id,
+void ThumbnailImage::AssignJPEGData(base::Token thumbnail_id,
+                                    base::TimeTicks assign_sk_bitmap_time,
+                                    base::Optional<uint64_t> frame_id_for_trace,
                                     std::vector<uint8_t> data) {
+  // If the image is stale (a new thumbnail was assigned or the
+  // thumbnail was cleared after AssignSkBitmap), ignore it.
+  if (thumbnail_id != thumbnail_id_) {
+    if (async_operation_finished_callback_)
+      async_operation_finished_callback_.Run();
+    return;
+  }
+
   data_ = base::MakeRefCounted<base::RefCountedData<std::vector<uint8_t>>>(
       std::move(data));
 
@@ -125,9 +141,9 @@ void ThumbnailImage::AssignJPEGData(base::TimeTicks assign_sk_bitmap_time,
     ConvertJPEGDataToImageSkiaAndNotifyObservers();
   };
 
-  if (frame_id) {
+  if (frame_id_for_trace) {
     TRACE_EVENT_WITH_FLOW0("ui", "Tab.Preview.JPEGReceivedOnUIThreadWithFlow",
-                           *frame_id, TRACE_EVENT_FLAG_FLOW_IN);
+                           *frame_id_for_trace, TRACE_EVENT_FLAG_FLOW_IN);
     notify();
   } else {
     TRACE_EVENT0("ui", "Tab.Preview.JPEGReceivedOnUIThread");
@@ -149,14 +165,20 @@ bool ThumbnailImage::ConvertJPEGDataToImageSkiaAndNotifyObservers() {
        base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
       base::BindOnce(&ThumbnailImage::UncompressImage, data_),
       base::BindOnce(&ThumbnailImage::NotifyUncompressedDataObservers,
-                     weak_ptr_factory_.GetWeakPtr()));
+                     weak_ptr_factory_.GetWeakPtr(), thumbnail_id_));
 }
 
-void ThumbnailImage::NotifyUncompressedDataObservers(gfx::ImageSkia image) {
+void ThumbnailImage::NotifyUncompressedDataObservers(base::Token thumbnail_id,
+                                                     gfx::ImageSkia image) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (async_operation_finished_callback_)
     async_operation_finished_callback_.Run();
 
+  // If the image is stale (a new thumbnail was assigned or the
+  // thumbnail was cleared after AssignSkBitmap), ignore it.
+  if (thumbnail_id != thumbnail_id_)
+    return;
+
   for (Subscription* subscription : subscribers_) {
     auto size_hint = subscription->size_hint_;
     if (subscription->uncompressed_image_callback_)

chrome/browser/ui/thumbnails/thumbnail_image.h

@@ -17,6 +17,7 @@
 #include "base/observer_list.h"
 #include "base/optional.h"
 #include "base/sequence_checker.h"
+#include "base/token.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace base {
@@ -149,11 +150,13 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
 
   virtual ~ThumbnailImage();
 
-  void AssignJPEGData(base::TimeTicks assign_sk_bitmap_time,
-                      base::Optional<uint64_t> frame_id,
+  void AssignJPEGData(base::Token thumbnail_id,
+                      base::TimeTicks assign_sk_bitmap_time,
+                      base::Optional<uint64_t> frame_id_for_trace,
                       std::vector<uint8_t> data);
   bool ConvertJPEGDataToImageSkiaAndNotifyObservers();
-  void NotifyUncompressedDataObservers(gfx::ImageSkia image);
+  void NotifyUncompressedDataObservers(base::Token thumbnail_id,
+                                       gfx::ImageSkia image);
   void NotifyCompressedDataObservers(CompressedThumbnailData data);
 
   static std::vector<uint8_t> CompressBitmap(SkBitmap bitmap,
@@ -175,6 +178,10 @@ class ThumbnailImage : public base::RefCounted<ThumbnailImage> {
   // the old data.
   CompressedThumbnailData data_;
 
+  // A randomly generated ID associated with each image assigned by
+  // AssignSkBitmap().
+  base::Token thumbnail_id_;
+
   // 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

chrome/browser/ui/thumbnails/thumbnail_image_unittest.cc

@@ -295,25 +295,63 @@ TEST_F(ThumbnailImageTest, RequestCompressedThumbnailData) {
   EXPECT_TRUE(waiter.called());
 }
 
-TEST_F(ThumbnailImageTest, ClearThumbnailWhileNotifyingObservers) {
+TEST_F(ThumbnailImageTest, ClearThumbnailAfterAssignBitmap) {
   auto image = base::MakeRefCounted<ThumbnailImage>(this);
 
   std::unique_ptr<Subscription> subscription = image->Subscribe();
 
-  CallbackWaiter waiter;
+  CallbackWaiter uncompressed_image_waiter;
   subscription->SetUncompressedImageCallback(
-      IgnoreArgs<gfx::ImageSkia>(waiter.callback()));
+      IgnoreArgs<gfx::ImageSkia>(uncompressed_image_waiter.callback()));
+
+  CallbackWaiter compressed_image_waiter;
+  subscription->SetCompressedImageCallback(
+      IgnoreArgs<ThumbnailImage::CompressedThumbnailData>(
+          compressed_image_waiter.callback()));
 
+  CallbackWaiter async_operation_finished_waiter;
+  image->set_async_operation_finished_callback_for_testing(
+      async_operation_finished_waiter.callback());
+
+  // No observers should be notified if the thumbnail is cleared just
+  // after assigning a bitmap.
   SkBitmap bitmap = CreateBitmap(kTestBitmapWidth, kTestBitmapHeight);
   image->AssignSkBitmap(bitmap, base::nullopt);
-  waiter.Wait();
-  EXPECT_TRUE(waiter.called());
-  waiter.Reset();
+  image->ClearData();
+  async_operation_finished_waiter.Wait();
+  EXPECT_TRUE(async_operation_finished_waiter.called());
+  EXPECT_FALSE(uncompressed_image_waiter.called());
+  EXPECT_FALSE(compressed_image_waiter.called());
+}
+
+TEST_F(ThumbnailImageTest, ClearExistingThumbnailNotifiesObservers) {
+  auto image = base::MakeRefCounted<ThumbnailImage>(this);
+
+  std::unique_ptr<Subscription> subscription = image->Subscribe();
+
+  CallbackWaiter uncompressed_image_waiter;
+  subscription->SetUncompressedImageCallback(
+      IgnoreArgs<gfx::ImageSkia>(uncompressed_image_waiter.callback()));
+
+  CallbackWaiter compressed_image_waiter;
+  subscription->SetCompressedImageCallback(
+      IgnoreArgs<ThumbnailImage::CompressedThumbnailData>(
+          compressed_image_waiter.callback()));
+
+  SkBitmap bitmap = CreateBitmap(kTestBitmapWidth, kTestBitmapHeight);
+  image->AssignSkBitmap(bitmap, base::nullopt);
+  compressed_image_waiter.Wait();
+  uncompressed_image_waiter.Wait();
+  EXPECT_TRUE(compressed_image_waiter.called());
+  EXPECT_TRUE(uncompressed_image_waiter.called());
+  compressed_image_waiter.Reset();
+  uncompressed_image_waiter.Reset();
 
-  image->RequestThumbnailImage();
   image->ClearData();
-  waiter.Wait();
-  EXPECT_TRUE(waiter.called());
+  compressed_image_waiter.Wait();
+  uncompressed_image_waiter.Wait();
+  EXPECT_TRUE(compressed_image_waiter.called());
+  EXPECT_TRUE(uncompressed_image_waiter.called());
 }
 
 // Makes sure a null dereference does not happen. Regression test for