Simplifying finishing a load on Resource

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

Index: third_party/WebKit/Source/core/fetch/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ImageResource.cpp b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
index 67abfc4a4ef71e2e5d06b0d7d1415ca037439650..a29be2ffa4ab1b027af47789b9ba102424349b11 100644
--- a/third_party/WebKit/Source/core/fetch/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/fetch/ImageResource.cpp
@@ -367,7 +367,9 @@ void ImageResource::updateImage(bool allDataReceived)
     // to decode.
     if (sizeAvailable || allDataReceived) {
         if (!m_image || m_image->isNull()) {
-            error(errorOccurred() ? getStatus() : DecodeError);
+            if (!errorOccurred())
+                setStatus(DecodeError);
+            clear();

[hiroshige, 2016/05/02 07:31:23]

checkNotify() and markClientsAndObserversFinished() are not called if we don't
call error() here.
I'm not sure whether those calles are needed, but making isLoading() false
without notification to Clients/Observers looks strange.

[Nate Chapin, 2016/05/02 19:32:24]

I think this just means that ResourceLoader will keep running and isLoading()
will still return true until the ResourceLoader completes?

[yhirano, 2016/05/06 10:06:03]

Is it desirable to cancel loading in such a case?

             if (memoryCache()->contains(this))
                 memoryCache()->remove(this);
             return;
@@ -379,7 +381,7 @@ void ImageResource::updateImage(bool allDataReceived)
     }
 }
 
-void ImageResource::finish()
+void ImageResource::finish(double loadFinishTime)
 {
     if (m_multipartParser) {
         m_multipartParser->finish();
@@ -391,15 +393,15 @@ void ImageResource::finish()
     } else {
         updateImage(true);
     }
-    Resource::finish();
+    Resource::finish(loadFinishTime);
 }
 
-void ImageResource::error(Resource::Status status)
+void ImageResource::error(const ResourceError& error)
 {
     if (m_multipartParser)
         m_multipartParser->cancel();
     clear();
-    Resource::error(status);
+    Resource::error(error);
     notifyObservers();
 }
 
@@ -498,7 +500,13 @@ void ImageResource::reloadIfLoFi(ResourceFetcher* fetcher)
         return;
     m_resourceRequest.setCachePolicy(WebCachePolicy::BypassingCache);
     m_resourceRequest.setLoFiState(WebURLRequest::LoFiOff);
-    error(Resource::LoadError);
+    if (isLoading()) {

[Nate Chapin, 2016/04/28 18:10:27]

I'm not sure if this more complicated if/else block is worthwhile, but I'm
inclined to say it is. The benefit is that it removes the last call to
Resource::error() that isn't associated with a ResourceError, allowing us to
assert a non-null ResourceError in Resource::error(), which seems appropriate :)

[hiroshige, 2016/05/02 07:31:23]

In this case, Resource::error() is called via ResourceLoader::cancel() and
ResourceError is provided by ResourceLoader, right?

[Nate Chapin, 2016/05/02 19:32:24]

Correct.

+        m_loader->cancel();
+    } else {
+        clear();
+        m_data.clear();
+        notifyObservers();

[Nate Chapin, 2016/04/28 18:10:27]

I'm not 100% all of these are necessary, but it emulates that observable things
error() was doing and keeps the expected results of
ImageResourceTest.ReloadIfLofi the same.

[hiroshige, 2016/05/02 07:31:23]

This CL seems to split "something" from error(), and places the "something"
outside error(), e.g. in Lines 506--508 and Line 372 of this file.
It would be nice if we can clearly describe what the "something" is/does, and
split it into a clearly named method (or at least add comments).

My random thoughts:
I feel the sequence of Lines 506--508 is a part of custom Resource state
manipulation (indicated by the |setStatus(NotStarted)| call).
Previously, the error() call is used because the state of Resource after error()
was ready for restarting load (which was not implied by the name of "error()"
though).
At first I thought that the sequence was to "make the Resource to be ready for
start loading again, provided isLoading() is true".
However, the sequence is not executed when starting revalidation, which also
rewinds the Resource's status to NotStarted.

[Nate Chapin, 2016/05/02 19:32:24]

Ok, I modified this block slightly and pulled it out in to a helper called
updateImageAndClearBuffer. This logic now matches what we do for multipart
images when a part completes. This means that a lo fi image will still show the
placeholder while the real image is being loaded, but that seems benign. This
requires a small test change.

+    }
     setStatus(NotStarted);
     load(fetcher);
 }