Refactor reloading tests in ImageResourceTest.cpp

third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Index: third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
diff --git a/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp b/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
index 9d7b489b81e83aaaee0c3ea9af8b41e0377397be..6a3a7a94ba465c65878febb8893b5c241a5bb7d2 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
@@ -140,8 +140,6 @@ const unsigned char kJpegImage2[] = {
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x03, 0xff, 0xd9};
 
-constexpr int kJpegImage2Width = 50;
-
 const char kSvgImage[] =
     "<svg width=\"200\" height=\"200\" xmlns=\"http://www.w3.org/2000/svg\" "
     "xmlns:xlink=\"http://www.w3.org/1999/xlink\">"
@@ -168,6 +166,67 @@ void receiveResponse(ImageResource* imageResource,
   imageResource->finish();
 }
 
+void testThatReloadIsStartedThenServeReload(
+    const KURL& testURL,
+    ImageResource* imageResource,
+    ImageResourceContent* content,
+    MockImageResourceObserver* observer,
+    bool alreadyNotifiedFinish,
+    WebCachePolicy policyForReload = WebCachePolicy::BypassingCache) {

[yhirano, 2017/02/08 02:11:07]

Why do you use a default parameter? It makes things unclear in general, and in
this particular case, I'm not sure which should be the default value as you are
using the value only for one assertion.

[hiroshige, 2017/02/10 23:56:02]

Done.

+  const char* data = reinterpret_cast<const char*>(kJpegImage2);
+  const size_t dataLength = sizeof(kJpegImage2);
+  const int imageWidth = 50;

[yhirano, 2017/02/08 02:11:07]

constexpr

[hiroshige, 2017/02/10 23:56:02]

Done.

+  const int imageHeight = 50;

[yhirano, 2017/02/08 02:11:07]

ditto

[hiroshige, 2017/02/10 23:56:02]

Done.

+
+  // Checks that |imageResource| and |content| are ready for non-placeholder
+  // reloading.
+  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
+  EXPECT_FALSE(imageResource->resourceBuffer());
+  EXPECT_FALSE(imageResource->isPlaceholder());
+  EXPECT_EQ(nullAtom,
+            imageResource->resourceRequest().httpHeaderField("range"));
+  EXPECT_EQ(
+      static_cast<int>(policyForReload),
+      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));

[yhirano, 2017/02/08 02:11:07]

I think we can remove these static_casts.

[hiroshige, 2017/02/10 23:56:02]

Done.

+  EXPECT_EQ(content, imageResource->getContent());
+  EXPECT_FALSE(content->hasImage());
+
+  // Checks |observer| before reloading.
+  const int originalImageChangedCount = observer->imageChangedCount();
+  EXPECT_EQ(alreadyNotifiedFinish, observer->imageNotifyFinishedCalled());

[yhirano, 2017/02/08 02:11:07]

You leave EXPECT_EQ(t/f, observer->imageNotifyFinishedCalled()) in some tests
and you don't in other tests. Can we be consistent on the point? I prefer
putting the assertion out of this function, stop passing the boolean to this
function and initializing alreadyNotifiedFinish here, but it's not a strong
opinion.

[hiroshige, 2017/02/10 23:56:01]

Done.
Agree, because I noticed we anyway have to put (and actually put) assertion of
EXPECT_TRUE(observer->imageNotifyFinishedCalled()) in the caller, BEFORE
reloading is started.

+  const int imageWidthOnImageNotifyFinished =
+      observer->imageWidthOnImageNotifyFinished();

[yhirano, 2017/02/08 02:11:08]

ASSERT_NE(imageWidth, imageWidthOnImageNotifyFinished);

[hiroshige, 2017/02/10 23:56:02]

Done.

+
+  // Does Reload.
+  imageResource->loader()->didReceiveResponse(WrappedResourceResponse(
+      ResourceResponse(testURL, "image/jpeg", dataLength, nullAtom, String())));
+  imageResource->loader()->didReceiveData(data, dataLength);
+  imageResource->loader()->didFinishLoading(0.0, dataLength, dataLength);
+
+  // Checks |imageResource|'s status after reloading.
+  EXPECT_EQ(ResourceStatus::Cached, imageResource->getStatus());
+  EXPECT_FALSE(imageResource->errorOccurred());
+  EXPECT_EQ(dataLength, imageResource->encodedSize());
+
+  // Checks |observer| after reloading that it is notified of updates/finish.
+  EXPECT_LT(originalImageChangedCount, observer->imageChangedCount());
+  EXPECT_EQ(imageWidth, observer->imageWidthOnLastImageChanged());
+  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
+  if (alreadyNotifiedFinish) {
+    EXPECT_EQ(imageWidthOnImageNotifyFinished,

[yhirano, 2017/02/08 02:11:08]

Putting a comment explaining that you are using |imageWidth| to check if
notifiyFinished is called again would be helpful.

[hiroshige, 2017/02/10 23:56:01]

I removed this then-clause, because the mock observer has ASSERT_EQ() to check
imageNotifyFinished() is not called twice, and thus checking
imageWidthOnImageNotifyFinished() again is not helpful.
Also added a comment.

+              observer->imageWidthOnImageNotifyFinished());
+  } else {
+    EXPECT_EQ(imageWidth, observer->imageWidthOnImageNotifyFinished());
+  }
+
+  // Checks |content| receives the correct image.
+  EXPECT_TRUE(content->hasImage());
+  EXPECT_FALSE(content->getImage()->isNull());
+  EXPECT_EQ(imageWidth, content->getImage()->width());
+  EXPECT_EQ(imageHeight, content->getImage()->height());
+  EXPECT_TRUE(content->getImage()->isBitmapImage());
+}
+
 class ImageResourceTestMockFetchContext : public FetchContext {
  public:
   static ImageResourceTestMockFetchContext* create() {
@@ -445,30 +504,11 @@ TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderAfterFinished) {
   // Call reloadIfLoFiOrPlaceholderImage() after the image has finished loading.
   imageResource->reloadIfLoFiOrPlaceholderImage(fetcher,
                                                 Resource::kReloadAlways);
-  EXPECT_FALSE(imageResource->errorOccurred());
-  EXPECT_FALSE(imageResource->resourceBuffer());
-  EXPECT_FALSE(imageResource->getContent()->hasImage());
   EXPECT_EQ(3, observer->imageChangedCount());
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
 
-  imageResource->loader()->didReceiveResponse(
-      WrappedResourceResponse(resourceResponse), nullptr);
-  imageResource->loader()->didReceiveData(
-      reinterpret_cast<const char*>(kJpegImage2), sizeof(kJpegImage2));
-  imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage2),
-                                            sizeof(kJpegImage2));
-  EXPECT_FALSE(imageResource->errorOccurred());
-  ASSERT_TRUE(imageResource->getContent()->hasImage());
-  EXPECT_FALSE(imageResource->getContent()->getImage()->isNull());
-  EXPECT_EQ(kJpegImage2Width, observer->imageWidthOnLastImageChanged());
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-
-  // The observer should not have been notified of completion again.
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnImageNotifyFinished());
-
-  EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->width());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->height());
+  testThatReloadIsStartedThenServeReload(testURL, imageResource,
+                                         imageResource->getContent(),
+                                         observer.get(), true);
 }
 
 TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderViaResourceFetcher) {
@@ -504,30 +544,8 @@ TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderViaResourceFetcher) {
 
   fetcher->reloadLoFiImages();
 
-  EXPECT_FALSE(imageResource->errorOccurred());
-  EXPECT_FALSE(imageResource->resourceBuffer());
-  EXPECT_FALSE(imageResource->getContent()->hasImage());
-  EXPECT_EQ(3, observer->imageChangedCount());

[yhirano, 2017/02/08 02:11:07]

Shouldn't this assertion be preserved?

[hiroshige, 2017/02/10 23:56:02]

Done.

-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-
-  imageResource->loader()->didReceiveResponse(
-      WrappedResourceResponse(resourceResponse), nullptr);
-  imageResource->loader()->didReceiveData(
-      reinterpret_cast<const char*>(kJpegImage2), sizeof(kJpegImage2));
-  imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage2),
-                                            sizeof(kJpegImage2));
-  EXPECT_FALSE(imageResource->errorOccurred());
-  ASSERT_TRUE(imageResource->getContent()->hasImage());
-  EXPECT_FALSE(imageResource->getContent()->getImage()->isNull());
-  EXPECT_EQ(kJpegImage2Width, observer->imageWidthOnLastImageChanged());
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-
-  // The observer should not have been notified of completion again.
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnImageNotifyFinished());
-
-  EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->width());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->height());
+  testThatReloadIsStartedThenServeReload(testURL, imageResource, content,
+                                         observer.get(), true);
 }
 
 TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderDuringFetch) {
@@ -567,36 +585,16 @@ TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderDuringFetch) {
   // Call reloadIfLoFiOrPlaceholderImage() while the image is still loading.
   imageResource->reloadIfLoFiOrPlaceholderImage(fetcher,
                                                 Resource::kReloadAlways);
-  EXPECT_FALSE(imageResource->errorOccurred());
-  EXPECT_FALSE(imageResource->resourceBuffer());
-  EXPECT_FALSE(imageResource->getContent()->hasImage());
+
   EXPECT_EQ(2, observer->imageChangedCount());
   EXPECT_EQ(0, observer->imageWidthOnLastImageChanged());
   // The observer should not have been notified of completion yet, since the
-  // image
-  // is still loading.
+  // image is still loading.
   EXPECT_FALSE(observer->imageNotifyFinishedCalled());
 
-  imageResource->loader()->didReceiveResponse(
-      WrappedResourceResponse(ResourceResponse(
-          testURL, "image/jpeg", sizeof(kJpegImage2), nullAtom, String())),
-      nullptr);
-  imageResource->loader()->didReceiveData(
-      reinterpret_cast<const char*>(kJpegImage2), sizeof(kJpegImage2));
-  imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage2),
-                                            sizeof(kJpegImage2));
-
-  EXPECT_FALSE(imageResource->errorOccurred());
-  ASSERT_TRUE(imageResource->getContent()->hasImage());
-  EXPECT_FALSE(imageResource->getContent()->getImage()->isNull());
-  EXPECT_EQ(kJpegImage2Width, observer->imageWidthOnLastImageChanged());
-  // The observer should have been notified of completion only after the reload
-  // completed.
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-  EXPECT_EQ(kJpegImage2Width, observer->imageWidthOnImageNotifyFinished());
-  EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->width());
-  EXPECT_EQ(50, imageResource->getContent()->getImage()->height());
+  testThatReloadIsStartedThenServeReload(testURL, imageResource,
+                                         imageResource->getContent(),
+                                         observer.get(), false);
 }
 
 TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderForPlaceholder) {
@@ -633,19 +631,14 @@ TEST(ImageResourceTest, ReloadIfLoFiOrPlaceholderForPlaceholder) {
 
   EXPECT_EQ(ResourceStatus::Cached, imageResource->getStatus());
   EXPECT_TRUE(imageResource->isPlaceholder());
+  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
 
   imageResource->reloadIfLoFiOrPlaceholderImage(fetcher,
                                                 Resource::kReloadAlways);
 
-  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_EQ(nullAtom,
-            imageResource->resourceRequest().httpHeaderField("range"));
-  EXPECT_EQ(
-      static_cast<int>(WebCachePolicy::BypassingCache),
-      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
-
-  imageResource->loader()->cancel();
+  testThatReloadIsStartedThenServeReload(testURL, imageResource,

[yhirano, 2017/02/08 02:11:08]

Can you tell me why you use testThatReload... function here?

[hiroshige, 2017/02/10 23:56:01]

Some tests test that reloading is started and serve the reloaded response, while
some tests (like this) only test that reloading is started and just cancel the
reloading.
I think there are no fundamental differences between the two types of tests, and
thus I make these tests to always server the reloaded response, and thus used
testThat...() function here.

+                                         imageResource->getContent(),
+                                         observer.get(), true);
 }
 
 TEST(ImageResourceTest, SVGImage) {
@@ -1185,36 +1178,12 @@ TEST(ImageResourceTest, FetchAllowPlaceholderUnsuccessful) {
 
   // The dimensions could not be extracted, so the full original image should be
   // loading.
-  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_EQ(nullAtom,
-            imageResource->resourceRequest().httpHeaderField("range"));
-  EXPECT_EQ(
-      static_cast<int>(WebCachePolicy::BypassingCache),
-      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
   EXPECT_FALSE(observer->imageNotifyFinishedCalled());
   EXPECT_EQ(3, observer->imageChangedCount());
 
-  imageResource->loader()->didReceiveResponse(
-      WrappedResourceResponse(ResourceResponse(
-          testURL, "image/jpeg", sizeof(kJpegImage), nullAtom, String())));
-  imageResource->loader()->didReceiveData(
-      reinterpret_cast<const char*>(kJpegImage), sizeof(kJpegImage));
-  imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage),
-                                            sizeof(kJpegImage));
-
-  EXPECT_EQ(ResourceStatus::Cached, imageResource->getStatus());
-  EXPECT_EQ(sizeof(kJpegImage), imageResource->encodedSize());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_LT(3, observer->imageChangedCount());
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnLastImageChanged());
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnImageNotifyFinished());
-
-  ASSERT_TRUE(imageResource->getContent()->hasImage());
-  EXPECT_EQ(1, imageResource->getContent()->getImage()->width());
-  EXPECT_EQ(1, imageResource->getContent()->getImage()->height());
-  EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
+  testThatReloadIsStartedThenServeReload(testURL, imageResource,
+                                         imageResource->getContent(),
+                                         observer.get(), false);
 }
 
 TEST(ImageResourceTest, FetchAllowPlaceholderPartialContentWithoutDimensions) {
@@ -1255,38 +1224,11 @@ TEST(ImageResourceTest, FetchAllowPlaceholderPartialContentWithoutDimensions) {
       0.0, kJpegImageSubrangeWithoutDimensionsLength,
       kJpegImageSubrangeWithoutDimensionsLength);
 
-  // Decode error didn't occur but the dimensions could not be extracted,
-  // so the full original image should be loading.
-  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_EQ(nullAtom,
-            imageResource->resourceRequest().httpHeaderField("range"));
-  EXPECT_EQ(
-      static_cast<int>(WebCachePolicy::BypassingCache),
-      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
-  EXPECT_FALSE(observer->imageNotifyFinishedCalled());
   EXPECT_EQ(2, observer->imageChangedCount());
 
-  imageResource->loader()->didReceiveResponse(
-      WrappedResourceResponse(ResourceResponse(
-          testURL, "image/jpeg", sizeof(kJpegImage), nullAtom, String())));
-  imageResource->loader()->didReceiveData(
-      reinterpret_cast<const char*>(kJpegImage), sizeof(kJpegImage));
-  imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage),
-                                            sizeof(kJpegImage));
-
-  EXPECT_EQ(ResourceStatus::Cached, imageResource->getStatus());
-  EXPECT_EQ(sizeof(kJpegImage), imageResource->encodedSize());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_LT(0, observer->imageChangedCount());
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnLastImageChanged());
-  EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-  EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnImageNotifyFinished());
-
-  ASSERT_TRUE(imageResource->getContent()->hasImage());
-  EXPECT_EQ(1, imageResource->getContent()->getImage()->width());
-  EXPECT_EQ(1, imageResource->getContent()->getImage()->height());
-  EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
+  testThatReloadIsStartedThenServeReload(testURL, imageResource,
+                                         imageResource->getContent(),
+                                         observer.get(), false);
 }
 
 TEST(ImageResourceTest, FetchAllowPlaceholderThenDisallowPlaceholder) {
@@ -1306,16 +1248,10 @@ TEST(ImageResourceTest, FetchAllowPlaceholderThenDisallowPlaceholder) {
   ImageResource* secondImageResource =
       ImageResource::fetch(nonPlaceholderRequest, fetcher);
   EXPECT_EQ(imageResource, secondImageResource);
-  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_EQ(nullAtom,
-            imageResource->resourceRequest().httpHeaderField("range"));
-  EXPECT_EQ(
-      static_cast<int>(WebCachePolicy::UseProtocolCachePolicy),
-      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
-  EXPECT_FALSE(observer->imageNotifyFinishedCalled());
 
-  imageResource->loader()->cancel();
+  testThatReloadIsStartedThenServeReload(
+      testURL, imageResource, imageResource->getContent(), observer.get(),
+      false, WebCachePolicy::UseProtocolCachePolicy);
 }
 
 TEST(ImageResourceTest,
@@ -1359,15 +1295,10 @@ TEST(ImageResourceTest,
   ImageResource* secondImageResource =
       ImageResource::fetch(nonPlaceholderRequest, fetcher);
   EXPECT_EQ(imageResource, secondImageResource);
-  EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-  EXPECT_FALSE(imageResource->isPlaceholder());
-  EXPECT_EQ(nullAtom,
-            imageResource->resourceRequest().httpHeaderField("range"));
-  EXPECT_EQ(
-      static_cast<int>(WebCachePolicy::UseProtocolCachePolicy),
-      static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
 
-  imageResource->loader()->cancel();
+  testThatReloadIsStartedThenServeReload(
+      testURL, imageResource, imageResource->getContent(), observer.get(), true,
+      WebCachePolicy::UseProtocolCachePolicy);
 }
 
 TEST(ImageResourceTest, FetchAllowPlaceholderFullResponseDecodeSuccess) {
@@ -1500,36 +1431,9 @@ TEST(ImageResourceTest,
 
     // The dimensions could not be extracted, and the response code was a 4xx
     // error, so the full original image should be loading.
-    EXPECT_EQ(ResourceStatus::Pending, imageResource->getStatus());
-    EXPECT_FALSE(imageResource->isPlaceholder());
-    EXPECT_EQ(nullAtom,
-              imageResource->resourceRequest().httpHeaderField("range"));
-    EXPECT_EQ(
-        static_cast<int>(WebCachePolicy::BypassingCache),
-        static_cast<int>(imageResource->resourceRequest().getCachePolicy()));
-    EXPECT_FALSE(observer->imageNotifyFinishedCalled());
-
-    imageResource->loader()->didReceiveResponse(
-
-        WrappedResourceResponse(ResourceResponse(
-            testURL, "image/jpeg", sizeof(kJpegImage), nullAtom, String())));
-    imageResource->loader()->didReceiveData(
-        reinterpret_cast<const char*>(kJpegImage), sizeof(kJpegImage));
-    imageResource->loader()->didFinishLoading(0.0, sizeof(kJpegImage),
-                                              sizeof(kJpegImage));
-
-    EXPECT_EQ(ResourceStatus::Cached, imageResource->getStatus());
-    EXPECT_EQ(sizeof(kJpegImage), imageResource->encodedSize());
-    EXPECT_FALSE(imageResource->isPlaceholder());
-    EXPECT_LT(0, observer->imageChangedCount());
-    EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnLastImageChanged());
-    EXPECT_TRUE(observer->imageNotifyFinishedCalled());
-    EXPECT_EQ(kJpegImageWidth, observer->imageWidthOnImageNotifyFinished());
-
-    ASSERT_TRUE(imageResource->getContent()->hasImage());
-    EXPECT_EQ(1, imageResource->getContent()->getImage()->width());
-    EXPECT_EQ(1, imageResource->getContent()->getImage()->height());
-    EXPECT_TRUE(imageResource->getContent()->getImage()->isBitmapImage());
+    testThatReloadIsStartedThenServeReload(testURL, imageResource,
+                                           imageResource->getContent(),
+                                           observer.get(), false);
   }
 }