Unify Resource loading status tracking

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

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index 515a577be25bb1d2839280469410f3f3872e8c22..fad574fc174501fc0deb124f65582ffb91349656 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -262,6 +262,8 @@ bool ResourceFetcher::resourceNeedsLoad(Resource* resource, const FetchRequest&
 {
     if (FetchRequest::DeferredByClient == request.defer())
         return false;
+    if (resource->isImage() && shouldDeferImageLoad(resource->url()))
+        return false;
     return policy != Use || resource->stillNeedsLoad();
 }
 
@@ -662,10 +664,11 @@ ResourceFetcher::RevalidationPolicy ResourceFetcher::determineRevalidationPolicy
         return Reload;
     }
 
-    // Do not load from cache if images are not enabled. The load for this image will be blocked
-    // in ImageResource::load.
+    // Do not load from cache if images are not enabled.
     if (FetchRequest::DeferredByClient == fetchRequest.defer())
         return Reload;
+    if (existingResource->isImage() && !context().allowImage(m_imagesEnabled, existingResource->url()))

[Nate Chapin, 2016/03/18 21:24:06]

shouldDeferImageLoad(), if used here, blocks SVGImages from loading inline data:
images. It'd be nice make it shouldDeferImageLoad() here, too, for consistency.

[hiroshige, 2016/03/18 23:21:20]

Offline discussion memo: can we |m_autoLoadImages| to true only for
ResourceFetcher for SVGs and use shouldDeferImageLoad() here?

[Nate Chapin, 2016/03/21 22:04:01]

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/La...
fails with this change.

It appears that m_imagesEnabled and m_autoLoadImages are intentionally subtly
different. m_imagesEnabled being false blocks *everything*, while
m_autoLoadImages being false blocks network requests only (but not data: urls or
cache hits). It would be nice if we could unify this behavior, but I don't think
this CL is the place to do it.

[hiroshige, 2016/03/22 18:21:33]

I see.
So, if allowImage() is false,
(1) we reload Resource here, just to make all Resource to be deferred by
shouldDeferImageLoad() in resourceNeedsLoad(), and
(2) we apply this intentionally to data URLs (right?), and thus this if
statement has higher priority than |if(isStaticData)| at Line 679.
Please add comments here to describe these two points.

[Nate Chapin, 2016/03/22 19:51:23]

Done.

+        return Reload;
 
     // Never use cache entries for downloadToFile / useStreamOnResponse
     // requests. The data will be delivered through other paths.
@@ -816,14 +819,9 @@ void ResourceFetcher::setImagesEnabled(bool enable)
     reloadImagesIfNotDeferred();
 }
 
-bool ResourceFetcher::clientDefersImage(const KURL& url) const
-{
-    return !context().allowImage(m_imagesEnabled, url);
-}
-
 bool ResourceFetcher::shouldDeferImageLoad(const KURL& url) const
 {
-    return clientDefersImage(url) || !m_autoLoadImages;
+    return !context().allowImage(m_imagesEnabled, url) || !m_autoLoadImages;
 }
 
 void ResourceFetcher::reloadImagesIfNotDeferred()
@@ -833,7 +831,7 @@ void ResourceFetcher::reloadImagesIfNotDeferred()
     // the resource probably won't be necesssary.
     for (const auto& documentResource : m_documentResources) {
         Resource* resource = documentResource.value.get();
-        if (resource && resource->getType() == Resource::Image && resource->stillNeedsLoad() && !clientDefersImage(resource->url()))
+        if (resource && resource->getType() == Resource::Image && resource->stillNeedsLoad() && !shouldDeferImageLoad(resource->url()))
             const_cast<Resource*>(resource)->load(this, defaultResourceOptions());
     }
 }
@@ -915,7 +913,7 @@ bool ResourceFetcher::scheduleArchiveLoad(Resource* resource, const ResourceRequ
         return false;
     }
 
-    resource->setLoading(true);
+    resource->setStatus(Resource::Pending);
     resource->responseReceived(archiveResource->response(), nullptr);
     SharedBuffer* data = archiveResource->data();
     if (data)