Phase II Step 3: Reload LoFi/placeholder images via new ImageResource

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

Index: third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
diff --git a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
index c91542567a841ce12ec16894141b4a8d9b1fb2b8..72ba9c70ac1f63e289b776581af42449720469bf 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
@@ -41,6 +41,7 @@
 #include "platform/network/HTTPParsers.h"
 #include "platform/weborigin/SecurityViolationReportingPolicy.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebCachePolicy.h"
 #include "v8/include/v8.h"
 #include "wtf/CurrentTime.h"
 #include "wtf/StdLibExtras.h"
@@ -69,9 +70,6 @@ class ImageResource::ImageResourceInfoImpl final
 
  private:
   const KURL& url() const override { return m_resource->url(); }
-  bool isSchedulingReload() const override {
-    return m_resource->m_isSchedulingReload;
-  }
   bool hasDevicePixelRatioHeaderValue() const override {
     return m_resource->m_hasDevicePixelRatioHeaderValue;
   }
@@ -88,10 +86,6 @@ class ImageResource::ImageResourceInfoImpl final
   bool isCacheValidator() const override {
     return m_resource->isCacheValidator();
   }
-  bool schedulingReloadOrShouldReloadBrokenPlaceholder() const override {
-    return m_resource->m_isSchedulingReload ||
-           m_resource->shouldReloadBrokenPlaceholder();
-  }
   bool isAccessAllowed(
       SecurityOrigin* securityOrigin,
       DoesCurrentFrameHaveSingleSecurityOrigin
@@ -175,11 +169,15 @@ ImageResource* ImageResource::fetch(FetchRequest& request,
 }
 
 bool ImageResource::canReuse(const FetchRequest& request) const {
+  if (!getContent())
+    return false;
+
   // If the image is a placeholder, but this fetch doesn't allow a
   // placeholder, then do not reuse this resource.
   if (request.placeholderImageRequestType() != FetchRequest::AllowPlaceholder &&
       m_placeholderOption != PlaceholderOption::DoNotReloadPlaceholder)
     return false;
+
   return true;
 }
 
@@ -196,7 +194,6 @@ ImageResource::ImageResource(const ResourceRequest& resourceRequest,
       m_content(content),
       m_devicePixelRatioHeaderValue(1.0),
       m_hasDevicePixelRatioHeaderValue(false),
-      m_isSchedulingReload(false),
       m_placeholderOption(
           isPlaceholder ? PlaceholderOption::ShowAndReloadPlaceholderAlways
                         : PlaceholderOption::DoNotReloadPlaceholder),
@@ -217,28 +214,14 @@ DEFINE_TRACE(ImageResource) {
   MultipartImageResourceParser::Client::trace(visitor);
 }
 
-void ImageResource::checkNotify() {
-  // Don't notify clients of completion if this ImageResource is
-  // about to be reloaded.
-  if (m_isSchedulingReload || shouldReloadBrokenPlaceholder())
-    return;
-
-  Resource::checkNotify();
-}
-
 bool ImageResource::hasClientsOrObservers() const {
-  return Resource::hasClientsOrObservers() || getContent()->hasObservers();
+  return Resource::hasClientsOrObservers() ||
+         (getContent() && getContent()->hasObservers());
 }
 
 void ImageResource::didAddClient(ResourceClient* client) {
   DCHECK((m_multipartParser && isLoading()) || !data() ||
-         getContent()->hasImage());
-
-  // Don't notify observers and clients of completion if this ImageResource is
-  // about to be reloaded.
-  if (m_isSchedulingReload || shouldReloadBrokenPlaceholder())
-    return;
-
+         (getContent() && getContent()->hasImage()));
   Resource::didAddClient(client);
 }
 
@@ -250,6 +233,8 @@ void ImageResource::destroyDecodedDataForFailedRevalidation() {
 }
 
 void ImageResource::destroyDecodedDataIfPossible() {
+  if (!getContent())
+    return;
   getContent()->destroyDecodedData();
   if (getContent()->hasImage() && !isPreloaded() &&
       getContent()->isRefetchableDataFromDiskCache()) {
@@ -259,16 +244,18 @@ void ImageResource::destroyDecodedDataIfPossible() {
 }
 
 void ImageResource::allClientsAndObserversRemoved() {
-  CHECK(!getContent()->hasImage() || !errorOccurred());
+  CHECK(!getContent() || !getContent()->hasImage() || !errorOccurred());
   // If possible, delay the resetting until back at the event loop. Doing so

[yhirano, 2017/03/17 13:09:35]

This comment should be inside of the outer if statement.

   // after a conservative GC prevents resetAnimation() from upsetting ongoing
   // animation updates (crbug.com/613709)
-  if (!ThreadHeap::willObjectBeLazilySwept(this)) {
-    Platform::current()->currentThread()->getWebTaskRunner()->postTask(
-        BLINK_FROM_HERE, WTF::bind(&ImageResourceContent::doResetAnimation,
-                                   wrapWeakPersistent(getContent())));
-  } else {
-    getContent()->doResetAnimation();
+  if (getContent()) {
+    if (!ThreadHeap::willObjectBeLazilySwept(this)) {
+      Platform::current()->currentThread()->getWebTaskRunner()->postTask(
+          BLINK_FROM_HERE, WTF::bind(&ImageResourceContent::doResetAnimation,
+                                     wrapWeakPersistent(getContent())));
+    } else {
+      getContent()->doResetAnimation();
+    }
   }
   if (m_multipartParser)
     m_multipartParser->cancel();
@@ -278,6 +265,8 @@ void ImageResource::allClientsAndObserversRemoved() {
 PassRefPtr<const SharedBuffer> ImageResource::resourceBuffer() const {
   if (data())

[kouhei (in TOK), 2017/03/15 10:25:28]

Is this when we tried to access ::resourceBuffer while image is being loaded?

[hiroshige, 2017/03/15 19:03:31]

Yes.

(Probably we check getContent() first for consistency; anyway I expect data() is
null when getContent() is null)

     return data();
+  if (!getContent())
+    return nullptr;
   return getContent()->resourceBuffer();
 }
 
@@ -288,6 +277,9 @@ void ImageResource::appendData(const char* data, size_t length) {
   } else {
     Resource::appendData(data, length);
 
+    if (!getContent())
+      return;
+
     // Update the image immediately if needed.
     if (getContent()->shouldUpdateImageImmediately()) {
       updateImage(this->data(), ImageResourceContent::UpdateImage, false);
@@ -330,12 +322,17 @@ void ImageResource::decodeError(bool allDataReceived) {
   if (!errorOccurred())
     setStatus(ResourceStatus::DecodeError);
 
+  // Tries to reload here to avoid imageNotifyFinished()
+  // notification in the case of decode error.
+  if (loader())
+    loader()->reloadIfLoFiOrPlaceholderImage(this);
+
   // Finishes loading if needed, and notifies observers.
   if (!allDataReceived && loader()) {
     // Observers are notified via ImageResource::finish().
     // TODO(hiroshige): Do not call didFinishLoading() directly.
     loader()->didFinishLoading(monotonicallyIncreasingTime(), size, size);
-  } else {
+  } else if (getContent()) {
     auto result = getContent()->updateImage(
         nullptr, ImageResourceContent::ClearImageAndNotifyObservers,
         allDataReceived);
@@ -458,6 +455,17 @@ bool ImageResource::shouldReloadBrokenPlaceholder() const {
   return false;
 }
 
+void ImageResource::detachContent() {
+  bool hasObservers = getContent() && getContent()->hasObservers();

[kouhei (in TOK), 2017/03/15 10:25:28]

Optional: contentHadObservers?

+
+  m_content = nullptr;
+  clearData();
+  memoryCache()->remove(this);
+
+  if (hasObservers)
+    didRemoveClientOrObserver();
+}
+
 static bool isLoFiImage(const ImageResource& resource) {
   if (!(resource.resourceRequest().previewsState() &
         WebURLRequest::ServerLoFiOn)) {
@@ -469,49 +477,51 @@ static bool isLoFiImage(const ImageResource& resource) {
              .contains("empty-image");
 }
 
-void ImageResource::reloadIfLoFiOrPlaceholderImage(
+ImageResource* ImageResource::reloadIfLoFiOrPlaceholderImage(
     ResourceFetcher* fetcher,
     ReloadLoFiOrPlaceholderPolicy policy) {
+  if (!fetcher)

[yhirano, 2017/03/17 13:09:35]

Is passing nullptr valid?

+    return nullptr;
+
   if (policy == kReloadIfNeeded && !shouldReloadBrokenPlaceholder())
-    return;
+    return nullptr;
 
   if (m_placeholderOption == PlaceholderOption::DoNotReloadPlaceholder &&
       !isLoFiImage(*this))
-    return;
-
-  // Prevent clients and observers from being notified of completion while the
-  // reload is being scheduled, so that e.g. canceling an existing load in
-  // progress doesn't cause clients and observers to be notified of completion
-  // prematurely.
-  DCHECK(!m_isSchedulingReload);
-  m_isSchedulingReload = true;
-
-  setCachePolicyBypassingCache();
+    return nullptr;
 
-  setPreviewsStateNoTransform();
+  ImageResourceContent* content = getContent();
+  if (!content)
+    return nullptr;
 
+  // Creates request/options for new ImageResource for reloading.
+  ResourceLoaderOptions reloadingOptions = options();
+  ResourceRequest reloadingRequest = resourceRequest();
+  reloadingRequest.setCachePolicy(WebCachePolicy::BypassingCache);
+  reloadingRequest.setPreviewsState(WebURLRequest::PreviewsNoTransform);
   if (m_placeholderOption != PlaceholderOption::DoNotReloadPlaceholder)
-    clearRangeRequestHeader();
-  m_placeholderOption = PlaceholderOption::DoNotReloadPlaceholder;
+    reloadingRequest.clearHTTPHeaderField("range");
 
-  if (isLoading()) {
-    loader()->cancel();
-    // Canceling the loader causes error() to be called, which in turn calls
-    // clear() and notifyObservers(), so there's no need to call these again
-    // here.
-  } else {
-    clearData();
-    setEncodedSize(0);
-    updateImage(nullptr, ImageResourceContent::ClearImageAndNotifyObservers,
-                false);
-  }
+  // Clear the image before reloading starts to avoid mixing the image from
+  // previous response with the response from the new reloading response.
+  updateImage(nullptr, ImageResourceContent::ClearImageAndNotifyObservers,
+              false);
 
-  setStatus(ResourceStatus::NotStarted);
+  detachContent();
+  // |this->getContent()| is nullptr after this point.
 
-  DCHECK(m_isSchedulingReload);
-  m_isSchedulingReload = false;
+  // TODO(hiroshige): Use ResourceFetcher::requestResource() to unify the
+  // starting points of resource loading into requestResource().
+  // Currently we call ResourceFetcher::startLoad() instead, in order to avoid
+  // re-applying modifications to ResourceRequest in requestResource().
+  ImageResource* reloadingResource =
+      new ImageResource(reloadingRequest, reloadingOptions, content, false);
+  DCHECK(!reloadingResource || content == reloadingResource->getContent());
+  memoryCache()->add(reloadingResource);
+  fetcher->addToDocumentResources(reloadingResource);
+  fetcher->startLoad(reloadingResource);
 
-  fetcher->startLoad(this);
+  return reloadingResource;
 }
 
 void ImageResource::onePartInMultipartReceived(
@@ -570,6 +580,8 @@ const ImageResourceContent* ImageResource::getContent() const {
 }
 
 ResourcePriority ImageResource::priorityFromObservers() {
+  if (!getContent())
+    return ResourcePriority();
   return getContent()->priorityFromObservers();
 }
 
@@ -577,9 +589,12 @@ void ImageResource::updateImage(
     PassRefPtr<SharedBuffer> sharedBuffer,
     ImageResourceContent::UpdateImageOption updateImageOption,
     bool allDataReceived) {
+  if (!getContent())
+    return;
   auto result = getContent()->updateImage(std::move(sharedBuffer),
                                           updateImageOption, allDataReceived);
   if (result == ImageResourceContent::UpdateImageResult::ShouldDecodeError) {
+    // TODO before commit (hiroshige): Update the comment.
     // In case of decode error, we call imageNotifyFinished() iff we don't
     // initiate reloading:
     // [(a): when this is in the middle of loading, or (b): otherwise]