Throttle holding space image refresh requests

The goal is to avoid excessive image load requests if the backing file
gets modified multiple times in quick succession.

BUG=1139115

Change-Id: I5ecf0171d28c5c2882f660039a6923e0951ed68c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2606383Reviewed-by: David Black <dmblack@google.com>
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839658}

ash/public/cpp/holding_space/holding_space_image.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/location.h"
 #include "ui/gfx/image/image_skia_source.h"
 #include "ui/gfx/skia_util.h"
 
@@ -77,7 +78,7 @@ void HoldingSpaceImage::OnBitmapLoaded(float scale, const SkBitmap* bitmap) {
 
   // Update the placeholder image, so the newly loaded representation becomes
   // the default for any `ImageSkia` instances created when the holding space
-  // image gets refreshed.
+  // image gets invalidated.
   placeholder_.RemoveRepresentation(scale);
   placeholder_.AddRepresentation(gfx::ImageSkiaRep(*bitmap, scale));
   placeholder_.RemoveUnsupportedRepresentationsForScale(scale);
@@ -86,6 +87,26 @@ void HoldingSpaceImage::OnBitmapLoaded(float scale, const SkBitmap* bitmap) {
 }
 
 void HoldingSpaceImage::Invalidate() {
+  if (invalidate_timer_.IsRunning())
+    return;
+
+  // Schedule an invalidation task with a delay to reduce number of image loads
+  // when multiple image invalidations are requested in quick succession. The
+  // delay is selected somewhat arbitrarily to be non trivial but still not
+  // easily noticable by the user.
+  invalidate_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(250),
+                          base::BindOnce(&HoldingSpaceImage::OnInvalidateTimer,
+                                         base::Unretained(this)));
+}
+
+bool HoldingSpaceImage::FireInvalidateTimerForTesting() {
+  if (!invalidate_timer_.IsRunning())
+    return false;
+  invalidate_timer_.FireNow();
+  return true;
+}
+
+void HoldingSpaceImage::OnInvalidateTimer() {
   // Invalidate the existing pointers to:
   // *   Invalidate previous `image_skia_`'s host pointer, and prevent it from
   //     requesting bitmap loads.

ash/public/cpp/holding_space/holding_space_image.h

@@ -10,6 +10,7 @@
 #include "ash/public/cpp/ash_public_export.h"
 #include "base/callback_forward.h"
 #include "base/callback_list.h"
+#include "base/timer/timer.h"
 #include "ui/gfx/image/image_skia.h"
 
 namespace ash {
@@ -47,6 +48,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
   // representations will be reloaded.
   void Invalidate();
 
+  bool FireInvalidateTimerForTesting();
+
  private:
   class ImageSkiaSource;
 
@@ -61,11 +64,18 @@ class ASH_PUBLIC_EXPORT HoldingSpaceImage {
   // source.
   void CreateImageSkia();
 
+  // `Invalidate()` requests are handled with a delay to reduce number of image
+  // loads if the backing file gets updated multiple times in quick succession.
+  void OnInvalidateTimer();
+
   gfx::ImageSkia placeholder_;
   AsyncBitmapResolver async_bitmap_resolver_;
 
   gfx::ImageSkia image_skia_;
 
+  // Timer used to throttle image invalidate requests.
+  base::OneShotTimer invalidate_timer_;
+
   // Mutable to allow const access from `AddImageSkiaChangedCallback()`.
   mutable CallbackList callback_list_;
 

ash/public/cpp/holding_space/holding_space_image_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/test/bind.h"
+#include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -140,8 +141,19 @@ std::unique_ptr<HoldingSpaceItem> CreateTestItem(
 
 }  // namespace
 
+class HoldingSpaceImageTest : public ::testing::Test {
+ public:
+  HoldingSpaceImageTest() = default;
+  HoldingSpaceImageTest(const HoldingSpaceImageTest&) = delete;
+  HoldingSpaceImageTest& operator=(const HoldingSpaceImageTest&) = delete;
+  ~HoldingSpaceImageTest() override = default;
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+};
+
 // Tests the basic flow for generating holding space image bitmaps.
-TEST(HoldingSpaceImageTest, ImageGeneration) {
+TEST_F(HoldingSpaceImageTest, ImageGeneration) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -174,7 +186,7 @@ TEST(HoldingSpaceImageTest, ImageGeneration) {
 
 // Tests the basic flow for generating holding space image bitmaps where 2x
 // bitmap gets requested.
-TEST(HoldingSpaceImageTest, ImageGenerationWith2xScale) {
+TEST_F(HoldingSpaceImageTest, ImageGenerationWith2xScale) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -215,7 +227,7 @@ TEST(HoldingSpaceImageTest, ImageGenerationWith2xScale) {
 
 // Verifies that the holding space image handles failed holding space image
 // requests.
-TEST(HoldingSpaceImageTest, ImageLoadFailure) {
+TEST_F(HoldingSpaceImageTest, ImageLoadFailure) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -244,7 +256,7 @@ TEST(HoldingSpaceImageTest, ImageLoadFailure) {
 
 // Verifies that the holding space image can be updated using
 // `HoldingSpaceItem::InvalidateImage()`.
-TEST(HoldingSpaceImageTest, ImageRefresh) {
+TEST_F(HoldingSpaceImageTest, ImageRefresh) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -262,6 +274,12 @@ TEST(HoldingSpaceImageTest, ImageRefresh) {
 
   // Request image refresh, and verify another image gets requested.
   holding_space_item->InvalidateImage();
+  EXPECT_EQ(0u, image_client.GetAndResetImageChangeCount());
+  EXPECT_EQ(0u, image_generator.NumberOfPendingRequests());
+
+  ASSERT_TRUE(
+      holding_space_item->image_for_testing().FireInvalidateTimerForTesting());
+  EXPECT_EQ(1u, image_generator.NumberOfPendingRequests());
 
   // While image load request is in progress, use the previously loaded icon.
   image = holding_space_item->image().image_skia();
@@ -282,9 +300,53 @@ TEST(HoldingSpaceImageTest, ImageRefresh) {
   EXPECT_EQ(0u, image_client.GetAndResetImageChangeCount());
 }
 
+// Verifies that image refresh requests issued in quick succession do not result
+// in multiple image load requests.
+TEST_F(HoldingSpaceImageTest, ImageRefreshThrottling) {
+  ImageGenerator image_generator;
+  std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
+      &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
+
+  // Finish loading the initial image.
+  TestImageClient image_client(&holding_space_item->image());
+  EXPECT_EQ(1u, image_generator.NumberOfPendingRequests());
+  image_generator.FulfillRequest(0, SK_ColorBLUE);
+  EXPECT_EQ(1u, image_client.GetAndResetImageChangeCount());
+
+  gfx::ImageSkia image = holding_space_item->image().image_skia();
+  EXPECT_EQ(gfx::Size(10, 10), image.size());
+  EXPECT_EQ(SK_ColorBLUE, image.bitmap()->getColor(5, 5));
+  EXPECT_EQ(0u, image_client.GetAndResetImageChangeCount());
+
+  // Request image refresh multiple times, and verify that image load gets
+  // requested only once.
+  holding_space_item->InvalidateImage();
+  holding_space_item->InvalidateImage();
+  holding_space_item->InvalidateImage();
+  EXPECT_EQ(0u, image_client.GetAndResetImageChangeCount());
+  EXPECT_EQ(0u, image_generator.NumberOfPendingRequests());
+
+  ASSERT_TRUE(
+      holding_space_item->image_for_testing().FireInvalidateTimerForTesting());
+  EXPECT_EQ(1u, image_generator.NumberOfPendingRequests());
+  EXPECT_EQ(1u, image_client.GetAndResetImageChangeCount());
+
+  // Verify that image gets updated once the image load request completes.
+  EXPECT_EQ(1u, image_generator.NumberOfPendingRequests());
+  image_generator.FulfillRequest(0, SK_ColorGREEN);
+  EXPECT_EQ(1u, image_client.GetAndResetImageChangeCount());
+
+  image = holding_space_item->image().image_skia();
+  EXPECT_EQ(gfx::Size(10, 10), image.size());
+  EXPECT_EQ(SK_ColorGREEN, image.bitmap()->getColor(5, 5));
+
+  EXPECT_EQ(0u, image_generator.NumberOfPendingRequests());
+  EXPECT_EQ(0u, image_client.GetAndResetImageChangeCount());
+}
+
 // Verifies that holding space image can be refreshed while the initial image
 // load is in progress.
-TEST(HoldingSpaceImageTest, ImageRefreshDuringInitialLoad) {
+TEST_F(HoldingSpaceImageTest, ImageRefreshDuringInitialLoad) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -300,6 +362,8 @@ TEST(HoldingSpaceImageTest, ImageRefreshDuringInitialLoad) {
   EXPECT_EQ(SK_ColorRED, image.bitmap()->getColor(5, 5));
 
   holding_space_item->InvalidateImage();
+  ASSERT_TRUE(
+      holding_space_item->image_for_testing().FireInvalidateTimerForTesting());
 
   // Verify that placeholder image remains to be used.
   image = holding_space_item->image().image_skia();
@@ -333,8 +397,8 @@ TEST(HoldingSpaceImageTest, ImageRefreshDuringInitialLoad) {
 // Verifies that holding space image can be refreshed while the initial image
 // load is in progress - test the case where the initial load request finishes
 // after the request for refreshed image.
-TEST(HoldingSpaceImageTest,
-     ImageRefreshDuringInitialLoadWithOutOfOrderResponses) {
+TEST_F(HoldingSpaceImageTest,
+       ImageRefreshDuringInitialLoadWithOutOfOrderResponses) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -350,6 +414,8 @@ TEST(HoldingSpaceImageTest,
   EXPECT_EQ(SK_ColorRED, image.bitmap()->getColor(5, 5));
 
   holding_space_item->InvalidateImage();
+  ASSERT_TRUE(
+      holding_space_item->image_for_testing().FireInvalidateTimerForTesting());
   EXPECT_EQ(2u, image_generator.NumberOfPendingRequests());
   EXPECT_EQ(1u, image_client.GetAndResetImageChangeCount());
 
@@ -377,7 +443,7 @@ TEST(HoldingSpaceImageTest,
 // Verifies that holding space image representation can be requested even after
 // the holding space item gets deleted (in which case the image will continue
 // using the image placeholder).
-TEST(HoldingSpaceImageTest, ImageRequestsAfterItemDestruction) {
+TEST_F(HoldingSpaceImageTest, ImageRequestsAfterItemDestruction) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -405,7 +471,7 @@ TEST(HoldingSpaceImageTest, ImageRequestsAfterItemDestruction) {
 
 // Tests that HoldingSpaceImage can handle holding space item destruction while
 // image load is still in progress.
-TEST(HoldingSpaceImageTest, ItemDestructionDuringImageLoad) {
+TEST_F(HoldingSpaceImageTest, ItemDestructionDuringImageLoad) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -432,7 +498,7 @@ TEST(HoldingSpaceImageTest, ItemDestructionDuringImageLoad) {
 
 // Tests that HoldingSpaceImage can handle holding space item destruction while
 // image load is still in progress, in case the pending image load fails.
-TEST(HoldingSpaceImageTest, ItemDestructionDuringFailedImageLoad) {
+TEST_F(HoldingSpaceImageTest, ItemDestructionDuringFailedImageLoad) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -461,7 +527,7 @@ TEST(HoldingSpaceImageTest, ItemDestructionDuringFailedImageLoad) {
 
 // Verifies that HoldingSpaceImage can handle holding space item destruction
 // while image refresh is in progress.
-TEST(HoldingSpaceImageTest, ItemDestructionDuringImageRefresh) {
+TEST_F(HoldingSpaceImageTest, ItemDestructionDuringImageRefresh) {
   ImageGenerator image_generator;
   std::unique_ptr<HoldingSpaceItem> holding_space_item = CreateTestItem(
       &image_generator, /*image_size=*/10, /*image_color=*/SK_ColorRED);
@@ -477,6 +543,8 @@ TEST(HoldingSpaceImageTest, ItemDestructionDuringImageRefresh) {
   EXPECT_EQ(SK_ColorBLUE, image.bitmap()->getColor(5, 5));
 
   holding_space_item->InvalidateImage();
+  ASSERT_TRUE(
+      holding_space_item->image_for_testing().FireInvalidateTimerForTesting());
   EXPECT_EQ(1u, image_generator.NumberOfPendingRequests());
   EXPECT_EQ(1u, image_client.GetAndResetImageChangeCount());
 

ash/public/cpp/holding_space/holding_space_item.h

@@ -102,6 +102,8 @@ class ASH_PUBLIC_EXPORT HoldingSpaceItem {
 
   const GURL& file_system_url() const { return file_system_url_; }
 
+  HoldingSpaceImage& image_for_testing() { return *image_; }
+
  private:
   // Constructor for file backed items.
   HoldingSpaceItem(Type type,