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

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 32fedae9baa97bc96bb98e8f8650d56d8d3cf01e..6093656f243a6c4ceed77e5bbd5a98c47da46e12 100644
--- a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
+++ b/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
@@ -34,16 +34,12 @@ class NullImageResourceInfo final
 
  private:
   const KURL& url() const override { return m_url; }
-  bool isSchedulingReload() const override { return false; }
   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 isPlaceholder() const override { return false; }
   bool isCacheValidator() const override { return false; }
-  bool schedulingReloadOrShouldReloadBrokenPlaceholder() const override {
-    return false;
-  }
   bool isAccessAllowed(
       SecurityOrigin*,
       DoesCurrentFrameHaveSingleSecurityOrigin) const override {
@@ -51,8 +47,13 @@ class NullImageResourceInfo final
   }
   bool hasCacheControlNoStoreHeader() const override { return false; }
   const ResourceError& resourceError() const override { return m_error; }
+  const ImageResource* resourceForTest() const override { return nullptr; }
 
   void decodeError(bool allDataReceived) override {}
+  bool reloadIfLoFiOrPlaceholderIfNeeded(
+      ResourceFetcher* fetcherForReload) override {
+    return false;
+  }
   void setDecodedSize(size_t) override {}
   void willAddClientOrObserver() override {}
   void didRemoveClientOrObserver() override {}
@@ -115,8 +116,7 @@ void ImageResourceContent::addObserver(ImageResourceObserver* observer) {
     observer->imageChanged(this);
   }
 
-  if (isLoaded() && m_observers.contains(observer) &&
-      !m_info->schedulingReloadOrShouldReloadBrokenPlaceholder()) {
+  if (isLoaded() && m_observers.contains(observer)) {
     markObserverFinished(observer);
     observer->imageNotifyFinished(this);
   }
@@ -269,8 +269,7 @@ void ImageResourceContent::notifyObservers(
     if (m_observers.contains(observer)) {
       observer->imageChanged(this, changeRect);
       if (notifyingFinishOption == ShouldNotifyFinish &&
-          m_observers.contains(observer) &&
-          !m_info->schedulingReloadOrShouldReloadBrokenPlaceholder()) {
+          m_observers.contains(observer)) {
         markObserverFinished(observer);
         observer->imageNotifyFinished(this);
       }
@@ -299,7 +298,8 @@ void ImageResourceContent::clearImage() {
 
 void ImageResourceContent::updateImage(PassRefPtr<SharedBuffer> data,
                                        UpdateImageOption updateImageOption,
-                                       bool allDataReceived) {
+                                       bool allDataReceived,
+                                       ResourceFetcher* fetcherForReload) {
   TRACE_EVENT0("blink", "ImageResourceContent::updateImage");
 
   // Clears the existing image, if instructed by |updateImageOption|.
@@ -359,6 +359,9 @@ void ImageResourceContent::updateImage(PassRefPtr<SharedBuffer> data,
       break;
   }
 
+  if (m_info->reloadIfLoFiOrPlaceholderIfNeeded(fetcherForReload))

[Nate Chapin, 2016/12/28 00:14:57]

Is it critical that this happen before notifyObservers? Or in
ImageResourceContent, for that matter? It looks like that wasn't the way it used
to be?

I don't like having to plumb the ResourceFetcher all around to enable it to be
called here if there's a good alternative.

[hiroshige, 2017/01/25 02:01:49]

This is to keep the current behavior of
"ImageResourceObserver::imageNotifyFinished() is not called when Placeholder is
reloaded due to DecodeError".

See the design doc
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...
(Phase II, Step 3, "Cleanup reloading around DecodeError") for more details
about timing dependencies around invoking reloading.

In the latest Patch Set, I moved reloading from here (and thus I removed the
plumbing between ImageResource and ImageResourceContent -- I agree this was not
clean) to ImageResource::decodeError().
Still we have the plumbing ResourceLoader::reloadIfLoFiOrPlaceholderImage() just
to get a reference to ResourceFetcher though.

Any better thoughts for handling this timing dependency are welcome.

+    return;
+
   // 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.
@@ -483,5 +486,8 @@ const ResourceResponse& ImageResourceContent::response() const {
 const ResourceError& ImageResourceContent::resourceError() const {
   return m_info->resourceError();
 }
+const ImageResource* ImageResourceContent::resourceForTest() const {
+  return m_info->resourceForTest();
+}
 
 }  // namespace blink