CachedResourceRequest should not need to store defer and preload options

https://bugs.webkit.org/show_bug.cgi?id=163004

Patch by Youenn Fablet <youenn@apple.com> on 2016-10-06
Reviewed by Alex Christensen.

No change of behavior.

Removing CachedResourceRequest defer and preload fields.
These fields are computed inside CachedResourceLoader instead.

Updated setting of priority from CachedResourceRequest to CachedResource.
Priority is set for any new resource (this covers all cases where no cached resource can be reused from the memory cache).
Priority is set for a cached resource if the request is not a preload request.

* loader/LinkLoader.cpp:
(WebCore::LinkLoader::preloadIfNeeded):
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::CachedResource):
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::requestImage):
(WebCore::CachedResourceLoader::canRequest):
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::determineRevalidationPolicy):
(WebCore::CachedResourceLoader::requestPreload):
* loader/cache/CachedResourceLoader.h:
* loader/cache/CachedResourceRequest.cpp:
(WebCore::CachedResourceRequest::CachedResourceRequest):
* loader/cache/CachedResourceRequest.h:
(WebCore::CachedResourceRequest::priority):
(WebCore::CachedResourceRequest::forPreload): Deleted.
(WebCore::CachedResourceRequest::setForPreload): Deleted.
(WebCore::CachedResourceRequest::defer): Deleted.
(WebCore::CachedResourceRequest::setDefer): Deleted.

Canonical link: https://commits.webkit.org/180950@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@206900 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,39 @@
+2016-10-06  Youenn Fablet  <youenn@apple.com>
+
+        CachedResourceRequest should not need to store defer and preload options
+        https://bugs.webkit.org/show_bug.cgi?id=163004
+
+        Reviewed by Alex Christensen.
+
+        No change of behavior.
+
+        Removing CachedResourceRequest defer and preload fields.
+        These fields are computed inside CachedResourceLoader instead.
+
+        Updated setting of priority from CachedResourceRequest to CachedResource.
+        Priority is set for any new resource (this covers all cases where no cached resource can be reused from the memory cache).
+        Priority is set for a cached resource if the request is not a preload request.
+
+        * loader/LinkLoader.cpp:
+        (WebCore::LinkLoader::preloadIfNeeded):
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::CachedResource):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::requestImage):
+        (WebCore::CachedResourceLoader::canRequest):
+        (WebCore::CachedResourceLoader::requestResource):
+        (WebCore::CachedResourceLoader::determineRevalidationPolicy):
+        (WebCore::CachedResourceLoader::requestPreload):
+        * loader/cache/CachedResourceLoader.h:
+        * loader/cache/CachedResourceRequest.cpp:
+        (WebCore::CachedResourceRequest::CachedResourceRequest):
+        * loader/cache/CachedResourceRequest.h:
+        (WebCore::CachedResourceRequest::priority):
+        (WebCore::CachedResourceRequest::forPreload): Deleted.
+        (WebCore::CachedResourceRequest::setForPreload): Deleted.
+        (WebCore::CachedResourceRequest::defer): Deleted.
+        (WebCore::CachedResourceRequest::setDefer): Deleted.
+
 2016-10-06  Myles C. Maxfield  <mmaxfield@apple.com>
 
         Variation fonts don't affect glyph advances

Source/WebCore/loader/LinkLoader.cpp

@@ -162,7 +162,6 @@ void LinkLoader::preloadIfNeeded(const LinkRelAttribute& relAttribute, const URL
     linkRequest.setInitiator("link");
 
     linkRequest.setAsPotentiallyCrossOrigin(crossOriginMode, document);
-    linkRequest.setForPreload(true);
     CachedResourceHandle<CachedResource> cachedLinkResource = document.cachedResourceLoader().preload(type.value(), WTFMove(linkRequest), CachedResourceLoader::ExplicitPreload);
 
     if (cachedLinkResource)

Source/WebCore/loader/cache/CachedResource.cpp

@@ -125,6 +125,8 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
     , m_type(type)
 {
     ASSERT(sessionID.isValid());
+
+    setLoadPriority(request.priority());
     finishRequestInitialization();
 
     // FIXME: We should have a better way of checking for Navigation loads, maybe FetchMode::Options::Navigate.

Source/WebCore/loader/cache/CachedResourceLoader.cpp

@@ -183,14 +183,14 @@ CachedResourceHandle<CachedImage> CachedResourceLoader::requestImage(CachedResou
             if (Document* document = frame->document())
                 document->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);
             URL requestURL = request.resourceRequest().url();
-            if (requestURL.isValid() && canRequest(CachedResource::ImageResource, requestURL, request))
+            if (requestURL.isValid() && canRequest(CachedResource::ImageResource, requestURL, request, ForPreload::No))
                 PingLoader::loadImage(*frame, requestURL);
             return nullptr;
         }
     }
 
-    request.setDefer(clientDefersImage(request.resourceRequest().url()) ? CachedResourceRequest::DeferredByClient : CachedResourceRequest::NoDefer);
-    return downcast<CachedImage>(requestResource(CachedResource::ImageResource, WTFMove(request)).get());
+    auto defer = clientDefersImage(request.resourceRequest().url()) ? DeferOption::DeferredByClient : DeferOption::NoDefer;
+    return downcast<CachedImage>(requestResource(CachedResource::ImageResource, WTFMove(request), ForPreload::No, defer).get());
 }
 
 CachedResourceHandle<CachedFont> CachedResourceLoader::requestFont(CachedResourceRequest&& request, bool isSVG)
@@ -437,12 +437,12 @@ static inline bool isSameOriginDataURL(const URL& url, const ResourceLoaderOptio
     return url.protocolIsData() && options.sameOriginDataURLFlag == SameOriginDataURLFlag::Set;
 }
 
-bool CachedResourceLoader::canRequest(CachedResource::Type type, const URL& url, const CachedResourceRequest& request)
+bool CachedResourceLoader::canRequest(CachedResource::Type type, const URL& url, const CachedResourceRequest& request, ForPreload forPreload)
 {
     auto& options = request.options();
 
     if (document() && !document()->securityOrigin()->canDisplay(url)) {
-        if (!request.forPreload())
+        if (forPreload == ForPreload::No)
             FrameLoader::reportLocalLoadFailed(frame(), url.stringCenterEllipsizedToLength());
         LOG(ResourceLoading, "CachedResourceLoader::requestResource URL was not allowed by SecurityOrigin::canDisplay");
         return false;
@@ -634,14 +634,14 @@ void CachedResourceLoader::prepareFetch(CachedResource::Type type, CachedResourc
     // FIXME: Decide whether to support client hints
 }
 
-CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(CachedResource::Type type, CachedResourceRequest&& request)
+CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(CachedResource::Type type, CachedResourceRequest&& request, ForPreload forPreload, DeferOption defer)
 {
     if (Document* document = this->document())
         document->contentSecurityPolicy()->upgradeInsecureRequestIfNeeded(request.mutableResourceRequest(), ContentSecurityPolicy::InsecureRequestType::Load);
 
     URL url = request.resourceRequest().url();
 
-    LOG(ResourceLoading, "CachedResourceLoader::requestResource '%s', charset '%s', priority=%d, forPreload=%u", url.stringCenterEllipsizedToLength().latin1().data(), request.charset().latin1().data(), request.priority() ? static_cast<int>(request.priority().value()) : -1, request.forPreload());
+    LOG(ResourceLoading, "CachedResourceLoader::requestResource '%s', charset '%s', priority=%d, forPreload=%u", url.stringCenterEllipsizedToLength().latin1().data(), request.charset().latin1().data(), request.priority() ? static_cast<int>(request.priority().value()) : -1, forPreload == ForPreload::Yes);
 
     // If only the fragment identifiers differ, it is the same resource.
     url = MemoryCache::removeFragmentIdentifierIfNeeded(url);
@@ -654,7 +654,7 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
     prepareFetch(type, request);
 
     // We are passing url as well as request, as request url may contain a fragment identifier.
-    if (!canRequest(type, url, request)) {
+    if (!canRequest(type, url, request, forPreload)) {
         RELEASE_LOG_IF_ALLOWED("requestResource: Not allowed to request resource (frame = %p)", frame());
         return nullptr;
     }
@@ -712,13 +712,7 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
 
     logMemoryCacheResourceRequest(frame(), resource ? DiagnosticLoggingKeys::inMemoryCacheKey() : DiagnosticLoggingKeys::notInMemoryCacheKey());
 
-    // These 3 fields will be used below after request is moved.
-    // FIXME: We can rearrange the code to not require storing all 3 fields.
-    auto forPreload = request.forPreload();
-    auto defer = request.defer();
-    auto priority = request.priority();
-
-    RevalidationPolicy policy = determineRevalidationPolicy(type, request, resource.get());
+    RevalidationPolicy policy = determineRevalidationPolicy(type, request, resource.get(), forPreload, defer);
     switch (policy) {
     case Reload:
         memoryCache.remove(*resource);
@@ -753,22 +747,21 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
                 m_resourceTimingInfo.addResourceTiming(resource.get(), *document(), loadTiming);
             }
 #endif
+            if (forPreload == ForPreload::No)
+                resource->setLoadPriority(request.priority());
         }
         break;
     }
 
     if (!resource)
         return nullptr;
 
-    if (!forPreload || policy != Use)
-        resource->setLoadPriority(priority);
-
-    if (!forPreload && resource->loader() && resource->resourceRequest().ignoreForRequestCount()) {
+    if (forPreload == ForPreload::No && resource->loader() && resource->resourceRequest().ignoreForRequestCount()) {
         resource->resourceRequest().setIgnoreForRequestCount(false);
         incrementRequestCount(*resource);
     }
 
-    if ((policy != Use || resource->stillNeedsLoad()) && CachedResourceRequest::NoDefer == defer) {
+    if ((policy != Use || resource->stillNeedsLoad()) && defer == DeferOption::NoDefer) {
         resource->load(*this);
 
         // We don't support immediate loads, but we do support immediate failure.
@@ -868,15 +861,15 @@ static void logResourceRevalidationDecision(CachedResource::RevalidationDecision
     }
 }
 
-CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalidationPolicy(CachedResource::Type type, CachedResourceRequest& cachedResourceRequest, CachedResource* existingResource) const
+CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalidationPolicy(CachedResource::Type type, CachedResourceRequest& cachedResourceRequest, CachedResource* existingResource, ForPreload forPreload, DeferOption defer) const
 {
     auto& request = cachedResourceRequest.resourceRequest();
 
     if (!existingResource)
         return Load;
 
     // We already have a preload going for this URL.
-    if (cachedResourceRequest.forPreload() && existingResource->isPreloaded())
+    if (forPreload == ForPreload::Yes && existingResource->isPreloaded())
         return Use;
 
     // If the same URL has been loaded as a different type, we need to reload.
@@ -905,9 +898,8 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
     // Conditional requests should have failed canReuse check.
     ASSERT(!request.isConditional());
 
-    // Do not load from cache if images are not enabled. The load for this image will be blocked
-    // in CachedImage::load.
-    if (cachedResourceRequest.defer() == CachedResourceRequest::DeferredByClient)
+    // Do not load from cache if images are not enabled. The load for this image will be blocked in CachedImage::load.
+    if (defer == DeferOption::DeferredByClient)
         return Reload;
 
     // Don't reload resources while pasting.
@@ -1224,9 +1216,8 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestPreload(Cached
 {
     if (request.charset().isEmpty() && (type == CachedResource::Script || type == CachedResource::CSSStyleSheet))
         request.setCharset(m_document->charset());
-    request.setForPreload(true);
 
-    CachedResourceHandle<CachedResource> resource = requestResource(type, WTFMove(request));
+    CachedResourceHandle<CachedResource> resource = requestResource(type, WTFMove(request), ForPreload::Yes);
     if (!resource || (m_preloads && m_preloads->contains(resource.get())))
         return nullptr;
     // Fonts need special treatment since just creating the resource doesn't trigger a load.

Source/WebCore/loader/cache/CachedResourceLoader.h

@@ -136,7 +136,6 @@ friend class ResourceCacheValidationSuppressor;
     void checkForPendingPreloads();
     void printPreloadStats();
 
-    bool canRequest(CachedResource::Type, const URL&, const CachedResourceRequest&);
     bool canRequestAfterRedirection(CachedResource::Type, const URL&, const ResourceLoaderOptions&);
 
     static const ResourceLoaderOptions& defaultCachedResourceOptions();
@@ -152,14 +151,19 @@ friend class ResourceCacheValidationSuppressor;
 private:
     explicit CachedResourceLoader(DocumentLoader*);
 
-    CachedResourceHandle<CachedResource> requestResource(CachedResource::Type, CachedResourceRequest&&);
+    enum class ForPreload { Yes, No };
+    enum class DeferOption { NoDefer, DeferredByClient };
+
+    CachedResourceHandle<CachedResource> requestResource(CachedResource::Type, CachedResourceRequest&&, ForPreload = ForPreload::No, DeferOption = DeferOption::NoDefer);
     void prepareFetch(CachedResource::Type, CachedResourceRequest&);
     CachedResourceHandle<CachedResource> revalidateResource(CachedResourceRequest&&, CachedResource&);
     CachedResourceHandle<CachedResource> loadResource(CachedResource::Type, CachedResourceRequest&&);
     CachedResourceHandle<CachedResource> requestPreload(CachedResource::Type, CachedResourceRequest&&);
 
+    bool canRequest(CachedResource::Type, const URL&, const CachedResourceRequest&, ForPreload);
+
     enum RevalidationPolicy { Use, Revalidate, Reload, Load };
-    RevalidationPolicy determineRevalidationPolicy(CachedResource::Type, CachedResourceRequest&, CachedResource* existingResource) const;
+    RevalidationPolicy determineRevalidationPolicy(CachedResource::Type, CachedResourceRequest&, CachedResource* existingResource, ForPreload, DeferOption) const;
 
     bool shouldUpdateCachedResourceWithCurrentRequest(const CachedResource&, const CachedResourceRequest&);
     CachedResourceHandle<CachedResource> updateCachedResourceWithCurrentRequest(const CachedResource&, CachedResourceRequest&&);

Source/WebCore/loader/cache/CachedResourceRequest.cpp

@@ -39,17 +39,13 @@ CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequ
     , m_charset(charset)
     , m_options(CachedResourceLoader::defaultCachedResourceOptions())
     , m_priority(priority)
-    , m_forPreload(false)
-    , m_defer(NoDefer)
 {
 }
 
 CachedResourceRequest::CachedResourceRequest(ResourceRequest&& resourceRequest, const ResourceLoaderOptions& options, Optional<ResourceLoadPriority> priority)
     : m_resourceRequest(WTFMove(resourceRequest))
     , m_options(options)
     , m_priority(priority)
-    , m_forPreload(false)
-    , m_defer(NoDefer)
 {
 }
 

Source/WebCore/loader/cache/CachedResourceRequest.h

@@ -40,8 +40,6 @@ class Document;
 
 class CachedResourceRequest {
 public:
-    enum DeferOption { NoDefer, DeferredByClient };
-
     explicit CachedResourceRequest(const ResourceRequest&, const String& charset = String(), Optional<ResourceLoadPriority> = Nullopt);
     CachedResourceRequest(ResourceRequest&&, const ResourceLoaderOptions&, Optional<ResourceLoadPriority> = Nullopt);
 
@@ -52,10 +50,6 @@ class CachedResourceRequest {
     const ResourceLoaderOptions& options() const { return m_options; }
     void setOptions(const ResourceLoaderOptions& options) { m_options = options; }
     const Optional<ResourceLoadPriority>& priority() const { return m_priority; }
-    bool forPreload() const { return m_forPreload; }
-    void setForPreload(bool forPreload) { m_forPreload = forPreload; }
-    DeferOption defer() const { return m_defer; }
-    void setDefer(DeferOption defer) { m_defer = defer; }
     void setInitiator(PassRefPtr<Element>);
     void setInitiator(const AtomicString& name);
     const AtomicString& initiatorName() const;
@@ -72,8 +66,6 @@ class CachedResourceRequest {
     String m_charset;
     ResourceLoaderOptions m_options;
     Optional<ResourceLoadPriority> m_priority;
-    bool m_forPreload;
-    DeferOption m_defer;
     RefPtr<Element> m_initiatorElement;
     AtomicString m_initiatorName;
     RefPtr<SecurityOrigin> m_origin;