Blink: Throttle progressively loaded images.

third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp

Index: third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp b/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
index 4389f64bcc35631a24c1d8bced656addeb8cbd59..33f93f7b7be72cb851d948de29533d0e3229994d 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
@@ -369,7 +369,7 @@ TEST(ImageResourceTest, UpdateBitmapImages) {
   EXPECT_FALSE(cachedImage->errorOccurred());
   ASSERT_TRUE(cachedImage->hasImage());
   EXPECT_FALSE(cachedImage->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(cachedImage->getImage()->isBitmapImage());
 }
@@ -401,7 +401,7 @@ TEST(ImageResourceTest, ReloadIfLoFi) {
   EXPECT_FALSE(cachedImage->errorOccurred());
   ASSERT_TRUE(cachedImage->hasImage());
   EXPECT_FALSE(cachedImage->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(cachedImage->getImage()->isBitmapImage());
   EXPECT_EQ(1, cachedImage->getImage()->width());
@@ -411,7 +411,7 @@ TEST(ImageResourceTest, ReloadIfLoFi) {
   EXPECT_FALSE(cachedImage->errorOccurred());
   EXPECT_FALSE(cachedImage->resourceBuffer());
   EXPECT_FALSE(cachedImage->hasImage());
-  EXPECT_EQ(3, client->imageChangedCount());
+  EXPECT_EQ(2, client->imageChangedCount());
 
   Vector<unsigned char> jpeg2 = jpegImage2();
   cachedImage->loader()->didReceiveResponse(
@@ -456,7 +456,7 @@ TEST(ImageResourceTest, SuccessfulRevalidationJpeg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(1, imageResource->getImage()->width());
@@ -472,7 +472,7 @@ TEST(ImageResourceTest, SuccessfulRevalidationJpeg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(1, imageResource->getImage()->width());
@@ -523,7 +523,7 @@ TEST(ImageResourceTest, FailedRevalidationJpegToJpeg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(1, imageResource->getImage()->width());
@@ -535,7 +535,7 @@ TEST(ImageResourceTest, FailedRevalidationJpegToJpeg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(4, client->imageChangedCount());
+  EXPECT_EQ(2, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(50, imageResource->getImage()->width());
@@ -553,7 +553,7 @@ TEST(ImageResourceTest, FailedRevalidationJpegToSvg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(2, client->imageChangedCount());
+  EXPECT_EQ(1, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(1, imageResource->getImage()->width());
@@ -565,7 +565,7 @@ TEST(ImageResourceTest, FailedRevalidationJpegToSvg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(3, client->imageChangedCount());
+  EXPECT_EQ(2, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_FALSE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(200, imageResource->getImage()->width());
@@ -595,7 +595,7 @@ TEST(ImageResourceTest, FailedRevalidationSvgToJpeg) {
   EXPECT_FALSE(imageResource->errorOccurred());
   ASSERT_TRUE(imageResource->hasImage());
   EXPECT_FALSE(imageResource->getImage()->isNull());
-  EXPECT_EQ(3, client->imageChangedCount());
+  EXPECT_EQ(2, client->imageChangedCount());
   EXPECT_TRUE(client->notifyFinishedCalled());
   EXPECT_TRUE(imageResource->getImage()->isBitmapImage());
   EXPECT_EQ(1, imageResource->getImage()->width());
@@ -687,8 +687,105 @@ TEST(ImageResourceTest, CancelOnDecodeError) {
       nullptr);
   cachedImage->loader()->didReceiveData(nullptr, "notactuallyanimage", 18, 18,
                                         18);
+  cachedImage->finish();

[pdr., 2016/10/05 23:24:25]

Why is this needed?

[vmpstr, 2016/10/05 23:48:17]

Without this, we don't try to decode the image, so we get a status of
Resource::Pending, instead of Resource::DecodeError.

   EXPECT_EQ(Resource::DecodeError, cachedImage->getStatus());
   EXPECT_FALSE(cachedImage->isLoading());
 }
 
+namespace {
+struct MockTime {
+  static double getTime() { return time; }
+  static double time;
+};
+
+double MockTime::time = 10.;
+}  // namespace
+
+TEST(ImageResourceTest, PeriodicFlushTest) {
+  KURL testURL(ParsedURLString, "http://www.test.com/cancelTest.html");
+  URLTestHelpers::registerMockedURLLoad(testURL, "cancelTest.html",
+                                        "text/html");
+  ResourceRequest request = ResourceRequest(testURL);
+  request.setLoFiState(WebURLRequest::LoFiOn);

[pdr., 2016/10/05 23:24:26]

Is this needed?

[vmpstr, 2016/10/05 23:48:17]

Nope :)

+  ImageResource* cachedImage = ImageResource::create(request);
+  cachedImage->setStatus(Resource::Pending);
+
+  Persistent<MockImageResourceClient> client =
+      new MockImageResourceClient(cachedImage);
+
+  // Send the image response.
+  Vector<unsigned char> jpeg = jpegImage2();
+  ResourceResponse resourceResponse(KURL(), "image/jpeg", jpeg.size(), nullAtom,
+                                    String());
+  resourceResponse.addHTTPHeaderField("chrome-proxy", "q=low");
+
+  cachedImage->responseReceived(resourceResponse, nullptr);
+
+  WTF::setTimeFunctionsForTesting(MockTime::getTime);
+
+  size_t meaningfulImageSize = 280;

[pdr., 2016/10/05 23:24:26]

Can you add a comment about where this number came from?

[vmpstr, 2016/10/05 23:48:17]

Done.

+  cachedImage->appendData(reinterpret_cast<const char*>(jpeg.data()),
+                          meaningfulImageSize);
+  size_t bytesSent = meaningfulImageSize;
+
+  EXPECT_FALSE(cachedImage->errorOccurred());
+  EXPECT_FALSE(cachedImage->hasImage());
+  EXPECT_EQ(0, client->imageChangedCount());
+
+  MockTime::time += 1.1;
+  cachedImage->appendData(
+      reinterpret_cast<const char*>(jpeg.data()) + bytesSent, 1);
+  ++bytesSent;
+
+  // Sanity check that we created an image after appending
+  // |meaningfulImageSize| + 1 bytes.
+  EXPECT_FALSE(cachedImage->errorOccurred());
+  ASSERT_TRUE(cachedImage->hasImage());
+  EXPECT_EQ(1, client->imageChangedCount());
+
+  for (int flushCount = 1; flushCount <= 3; ++flushCount) {
+    // For each of the iteration that appends data, we don't expect
+    // |imageChangeCount()| to change, since the time is adjusted by 0.2001
+    // seconds (it's greater than 0.2 to avoid double precision problems).
+    // After 5 appends, we breach the flush interval and the flush count
+    // increases.
+    for (int i = 0; i < 5; ++i) {
+      cachedImage->appendData(
+          reinterpret_cast<const char*>(jpeg.data()) + bytesSent, 1);
+
+      EXPECT_FALSE(cachedImage->errorOccurred());
+      ASSERT_TRUE(cachedImage->hasImage());
+      EXPECT_EQ(flushCount, client->imageChangedCount());
+
+      ++bytesSent;
+      MockTime::time += 0.2001;

[pdr., 2016/10/05 23:24:26]

If this ends up being flaky, we could just increment time by 0.1 within this
loop, and then increment it by a full second after the } (still inside the
flushCount loop).

[vmpstr, 2016/10/05 23:48:17]

I don't think it's flaky, it's just if I start with 10 and add 0.2 5 times, then
it still evaluates as < 11. But yeah, there are other things we could try.

+    }
+  }
+
+  // Increasing time by a large number only causes one extra flush.
+  MockTime::time += 10.f;
+  cachedImage->appendData(
+      reinterpret_cast<const char*>(jpeg.data()) + bytesSent, 1);
+  EXPECT_FALSE(cachedImage->errorOccurred());
+  ASSERT_TRUE(cachedImage->hasImage());
+  EXPECT_FALSE(cachedImage->getImage()->isNull());
+  EXPECT_EQ(4, client->imageChangedCount());
+
+  ++bytesSent;
+  // Append the rest of the data and finish (which causes another flush).
+  cachedImage->appendData(
+      reinterpret_cast<const char*>(jpeg.data()) + bytesSent,
+      jpeg.size() - bytesSent);
+  cachedImage->finish();
+
+  EXPECT_FALSE(cachedImage->errorOccurred());
+  ASSERT_TRUE(cachedImage->hasImage());
+  EXPECT_FALSE(cachedImage->getImage()->isNull());
+  EXPECT_EQ(5, client->imageChangedCount());
+  EXPECT_TRUE(client->notifyFinishedCalled());
+  EXPECT_TRUE(cachedImage->getImage()->isBitmapImage());
+  EXPECT_EQ(50, cachedImage->getImage()->width());
+  EXPECT_EQ(50, cachedImage->getImage()->height());
+}
+
 }  // namespace blink