Phase III Step 1: Make ImageResourceContent manage its own ResourceStatus

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

Index: third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
diff --git a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp b/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
index 55e30a9458c597141ed503e0c09865edb4d96b3c..db851d2367c627f346455ca32ebf160a56daf3ad 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
@@ -38,7 +38,6 @@ class NullImageResourceInfo final
   bool hasDevicePixelRatioHeaderValue() const override { return false; }
   float devicePixelRatioHeaderValue() const override { return 1.0; }
   const ResourceResponse& response() const override { return m_response; }
-  ResourceStatus getStatus() const override { return ResourceStatus::Cached; }
   bool shouldShowPlaceholder() const override { return false; }
   bool isCacheValidator() const override { return false; }
   bool isAccessAllowed(
@@ -291,8 +290,36 @@ void ImageResourceContent::clearImage() {
   m_sizeAvailable = Image::SizeUnavailable;
 }
 
+// |status| is the status of corresponding ImageResource.
+void ImageResourceContent::updateStatus(ResourceStatus status,
+                                        NotifyFinishOption notifyFinishOption) {
+  switch (notifyFinishOption) {
+    case ShouldNotifyFinish:
+      // When |ShouldNotifyFinish|, we set |m_status| to a loaded

[kouhei (in TOK), 2017/03/15 11:18:53]

Can we have switch(status) here to ensure all status is covered? (w/
NOTREACHED() where needed)

[hiroshige, 2017/05/04 20:21:57]

Done.

+      // ResourceStatus.
+      if (status == ResourceStatus::LoadError ||
+          status == ResourceStatus::DecodeError) {
+        // In case of error, Resource's status is set to an error status before
+        // updateImage() and thus we use the error status also as
+        // ImageResourceContent's status.
+        m_status = status;
+      } else {
+        // In case of successful load, Resource's status might not be set to
+        // Cached here. Therefore we set |m_status| to Cached, regardless of
+        // |status|.
+        m_status = ResourceStatus::Cached;
+      }
+      break;
+    case DoNotNotifyFinish:
+      if (m_status == ResourceStatus::NotStarted)

[kouhei (in TOK), 2017/03/15 11:18:53]

Ditto

[yhirano, 2017/03/17 13:47:25]

Can you tell me why this is needed? Setting the status to Pending is basically
done by notifyStartLoad(), right?

[yhirano, 2017/03/17 13:47:25]

Is |status| arbitrary here? Can we put some DCHECKs?

[hiroshige, 2017/05/04 20:21:57]

Right. I removed the transition in DoNotNotifyFinish.

[hiroshige, 2017/05/04 20:21:57]

|status| shouldn't be kNotStarted.

Also, |status| should be kPending, except for one callsite of UpdateImage() in
ReloadIfLoFiOrPlaceholderImage().
We might consider adding DCHECK_EQ(status, kPending) if it is not called from
ReloadIfLoFiOrPlaceholderImage(), but this complicates the concept (and code) of
state transition a little bit, and I don't have a clear reason to forbid
updating
the image data after the state changed to e.g. kCached.

Therefore, I only added a CHECK() for kNotStarted (in UpdateImage()) in the
latest Patch Set.

[hiroshige, 2017/05/04 20:21:57]

Removed.

+        m_status = ResourceStatus::Pending;
+      break;
+  }
+}
+
 ImageResourceContent::UpdateImageResult ImageResourceContent::updateImage(
     PassRefPtr<SharedBuffer> data,
+    ResourceStatus status,
     UpdateImageOption updateImageOption,
     bool allDataReceived) {
   TRACE_EVENT0("blink", "ImageResourceContent::updateImage");
@@ -333,8 +360,10 @@ ImageResourceContent::UpdateImageResult ImageResourceContent::updateImage(
       // Go ahead and tell our observers to try to draw if we have either
       // received all the data or the size is known. Each chunk from the network
       // causes observers to repaint, which will force that chunk to decode.
-      if (m_sizeAvailable == Image::SizeUnavailable && !allDataReceived)
+      if (m_sizeAvailable == Image::SizeUnavailable && !allDataReceived) {
+        updateStatus(status, DoNotNotifyFinish);
         return UpdateImageResult::NoDecodeError;
+      }
 
       if (m_info->shouldShowPlaceholder() && allDataReceived) {
         if (m_image && !m_image->isNull()) {
@@ -346,6 +375,7 @@ ImageResourceContent::UpdateImageResult ImageResourceContent::updateImage(
 
       if (!m_image || m_image->isNull()) {
         clearImage();
+        updateStatus(status, DoNotNotifyFinish);
         return UpdateImageResult::ShouldDecodeError;
       }
       break;
@@ -354,10 +384,18 @@ ImageResourceContent::UpdateImageResult ImageResourceContent::updateImage(
   // Notifies the observers.
   // It would be nice to only redraw the decoded band of the image, but with the
   // current design (decoding delayed until painting) that seems hard.
-  notifyObservers(allDataReceived ? ShouldNotifyFinish : DoNotNotifyFinish);
+
+  NotifyFinishOption notifyFinishOption =
+      allDataReceived ? ShouldNotifyFinish : DoNotNotifyFinish;
+  updateStatus(status, notifyFinishOption);
+  notifyObservers(notifyFinishOption);
   return UpdateImageResult::NoDecodeError;
 }
 
+void ImageResourceContent::notifyStartLoad() {

[yhirano, 2017/03/17 13:47:25]

DCHECK_EQ(m_status, NotStarted)?

[hiroshige, 2017/05/04 20:21:57]

Not necessarily NotStarted, e.g. in the case of revalidation.

+  m_status = ResourceStatus::Pending;

[kouhei (in TOK), 2017/03/15 11:18:53]

Can we always update status via updateStatus?

[hiroshige, 2017/05/04 20:21:57]

I removed UpdateStatus(kDoNotNotifyFinish) case and made NotifyStartLoad() here
the
single point of the state transision to kPending.

+}
+
 void ImageResourceContent::decodedSizeChangedTo(const blink::Image* image,
                                                 size_t newSize) {
   if (!image || image != m_image)
@@ -450,7 +488,7 @@ bool ImageResourceContent::loadFailedOrCanceled() const {
 }
 
 ResourceStatus ImageResourceContent::getStatus() const {
-  return m_info->getStatus();
+  return m_status;
 }
 
 const KURL& ImageResourceContent::url() const {