Drop ResourceLoadPriorityUnresolved resource load priority and use Optional<> instead
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Feb 2015 01:23:58 +0000 (01:23 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Feb 2015 01:23:58 +0000 (01:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141186

Reviewed by Antti Koivisto.

Source/WebCore:

Drop ResourceLoadPriorityUnresolved resource load priority value and use
Optional<ResourceLoadPriority> when needed instead. If the Optional
doesn't have a value, then it means it is unresolved. Having
ResourceLoadPriorityUnresolved in ResourceLoadPriority was confusing
because this value is only valid in CachedResourceRequest, it is not
a valid value in CachedResource or in ResourceRequest. After this
refactoring, it now becomes more obvious.

Source/WebKit2:

Update code now that ResourceLoadPriorityUnresolved is not longer a
ResourceLoadPriority enum value.

* NetworkProcess/cache/NetworkCache.cpp:
(WebKit::NetworkCache::retrieve):
* WebProcess/Network/WebResourceLoadScheduler.cpp:
(WebKit::WebResourceLoadScheduler::scheduleLoad):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@179584 268f45cc-cd09-0410-ab3c-d52691b4dbfc

19 files changed:
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLLinkElement.cpp
Source/WebCore/loader/LinkLoader.cpp
Source/WebCore/loader/ResourceLoadScheduler.cpp
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp
Source/WebCore/loader/cache/CachedResourceRequest.cpp
Source/WebCore/loader/cache/CachedResourceRequest.h
Source/WebCore/platform/network/ResourceLoadPriority.h
Source/WebCore/platform/network/ResourceRequestBase.cpp
Source/WebCore/platform/network/ResourceRequestBase.h
Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp
Source/WebCore/platform/network/cf/ResourceRequestCFNet.h
Source/WebCore/platform/network/cocoa/ResourceRequestCocoa.mm
Source/WebCore/platform/network/soup/ResourceRequest.h
Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp
Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp

index 5f698ac..9c3f266 100644 (file)
@@ -1,5 +1,20 @@
 2015-02-03  Chris Dumez  <cdumez@apple.com>
 
+        Drop ResourceLoadPriorityUnresolved resource load priority and use Optional<> instead
+        https://bugs.webkit.org/show_bug.cgi?id=141186
+
+        Reviewed by Antti Koivisto.
+
+        Drop ResourceLoadPriorityUnresolved resource load priority value and use
+        Optional<ResourceLoadPriority> when needed instead. If the Optional
+        doesn't have a value, then it means it is unresolved. Having
+        ResourceLoadPriorityUnresolved in ResourceLoadPriority was confusing
+        because this value is only valid in CachedResourceRequest, it is not
+        a valid value in CachedResource or in ResourceRequest. After this
+        refactoring, it now becomes more obvious.
+
+2015-02-03  Chris Dumez  <cdumez@apple.com>
+
         REGRESSION(176609): Very high memory usage in Canvas/reuse.html performance test
         https://bugs.webkit.org/show_bug.cgi?id=139812
 
index 0091a71..0ad1378 100644 (file)
@@ -229,7 +229,9 @@ void HTMLLinkElement::process()
         addPendingSheet(isActive ? ActiveSheet : InactiveSheet);
 
         // Load stylesheets that are not needed for the rendering immediately with low priority.
-        ResourceLoadPriority priority = isActive ? ResourceLoadPriorityUnresolved : ResourceLoadPriorityVeryLow;
+        Optional<ResourceLoadPriority> priority;
+        if (!isActive)
+            priority = ResourceLoadPriorityVeryLow;
         CachedResourceRequest request(ResourceRequest(document().completeURL(url)), charset, priority);
         request.setInitiator(this);
         m_cachedSheet = document().cachedResourceLoader().requestCSSStyleSheet(request);
index 2ffaf60..c6d02f4 100644 (file)
@@ -105,7 +105,7 @@ bool LinkLoader::loadLink(const LinkRelAttribute& relAttribute, const String& ty
     if ((relAttribute.m_isLinkPrefetch || relAttribute.m_isLinkSubresource) && href.isValid() && document->frame()) {
         if (!m_client->shouldLoadLink())
             return false;
-        ResourceLoadPriority priority = ResourceLoadPriorityUnresolved;
+        Optional<ResourceLoadPriority> priority;
         CachedResource::Type type = CachedResource::LinkPrefetch;
         // We only make one request to the cachedresourcelodaer if multiple rel types are
         // specified, 
index 51aeb84..b3eafe8 100644 (file)
@@ -163,7 +163,6 @@ void ResourceLoadScheduler::scheduleLoad(ResourceLoader* resourceLoader)
 #endif
 
     ResourceLoadPriority priority = resourceLoader->request().priority();
-    ASSERT(priority != ResourceLoadPriorityUnresolved);
 
     bool hadRequests = host->hasRequests();
     host->schedule(resourceLoader, priority);
index 971d864..65955e0 100644 (file)
@@ -736,11 +736,12 @@ unsigned CachedResource::overheadSize() const
     return sizeof(CachedResource) + m_response.memoryUsage() + kAverageClientsHashMapSize + m_resourceRequest.url().string().length() * 2;
 }
 
-void CachedResource::setLoadPriority(ResourceLoadPriority loadPriority)
+void CachedResource::setLoadPriority(const Optional<ResourceLoadPriority>& loadPriority)
 {
-    if (loadPriority == ResourceLoadPriorityUnresolved)
-        loadPriority = defaultPriorityForResourceType(type());
-    m_loadPriority = loadPriority;
+    if (loadPriority)
+        m_loadPriority = loadPriority.value();
+    else
+        m_loadPriority = defaultPriorityForResourceType(type());
 }
 
 inline CachedResource::Callback::Callback(CachedResource& resource, CachedResourceClient& client)
index e784d1f..66d29cc 100644 (file)
@@ -117,7 +117,7 @@ public:
     Type type() const { return static_cast<Type>(m_type); }
     
     ResourceLoadPriority loadPriority() const { return m_loadPriority; }
-    void setLoadPriority(ResourceLoadPriority);
+    void setLoadPriority(const Optional<ResourceLoadPriority>&);
 
     WEBCORE_EXPORT void addClient(CachedResourceClient*);
     WEBCORE_EXPORT void removeClient(CachedResourceClient*);
index 10659fd..e6989d5 100644 (file)
@@ -452,7 +452,7 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
 {
     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(), request.forPreload());
+    LOG(ResourceLoading, "CachedResourceLoader::requestResource '%s', charset '%s', priority=%d, forPreload=%u", url.stringCenterEllipsizedToLength().latin1().data(), request.charset().latin1().data(), request.priority() ? request.priority().value() : -1, request.forPreload());
     
     // If only the fragment identifiers differ, it is the same resource.
     url = MemoryCache::removeFragmentIdentifierIfNeeded(url);
index 389e531..53a53e5 100644 (file)
@@ -32,7 +32,7 @@
 
 namespace WebCore {
 
-CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequest, const String& charset, ResourceLoadPriority priority)
+CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequest, const String& charset, Optional<ResourceLoadPriority> priority)
     : m_resourceRequest(resourceRequest)
     , m_charset(charset)
     , m_options(CachedResourceLoader::defaultCachedResourceOptions())
@@ -45,13 +45,12 @@ CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequ
 CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequest, const ResourceLoaderOptions& options)
     : m_resourceRequest(resourceRequest)
     , m_options(options)
-    , m_priority(ResourceLoadPriorityUnresolved)
     , m_forPreload(false)
     , m_defer(NoDefer)
 {
 }
 
-CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequest, ResourceLoadPriority priority)
+CachedResourceRequest::CachedResourceRequest(const ResourceRequest& resourceRequest, Optional<ResourceLoadPriority> priority)
     : m_resourceRequest(resourceRequest)
     , m_options(CachedResourceLoader::defaultCachedResourceOptions())
     , m_priority(priority)
index e011d38..9938c08 100644 (file)
@@ -40,9 +40,9 @@ class CachedResourceRequest {
 public:
     enum DeferOption { NoDefer, DeferredByClient };
 
-    explicit CachedResourceRequest(const ResourceRequest&, const String& charset = String(), ResourceLoadPriority = ResourceLoadPriorityUnresolved);
+    explicit CachedResourceRequest(const ResourceRequest&, const String& charset = String(), Optional<ResourceLoadPriority> = Nullopt);
     CachedResourceRequest(const ResourceRequest&, const ResourceLoaderOptions&);
-    CachedResourceRequest(const ResourceRequest&, ResourceLoadPriority);
+    CachedResourceRequest(const ResourceRequest&, Optional<ResourceLoadPriority>);
     ~CachedResourceRequest();
 
     ResourceRequest& mutableResourceRequest() { return m_resourceRequest; }
@@ -51,8 +51,7 @@ public:
     void setCharset(const String& charset) { m_charset = charset; }
     const ResourceLoaderOptions& options() const { return m_options; }
     void setOptions(const ResourceLoaderOptions& options) { m_options = options; }
-    void setPriority(ResourceLoadPriority priority) { m_priority = priority; }
-    ResourceLoadPriority priority() const { return m_priority; }
+    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; }
@@ -65,7 +64,7 @@ private:
     ResourceRequest m_resourceRequest;
     String m_charset;
     ResourceLoaderOptions m_options;
-    ResourceLoadPriority m_priority;
+    Optional<ResourceLoadPriority> m_priority;
     bool m_forPreload;
     DeferOption m_defer;
     RefPtr<Element> m_initiatorElement;
index 5a7104f..6d1dc5b 100644 (file)
@@ -29,9 +29,7 @@
 namespace WebCore {
 
 enum ResourceLoadPriority {
-    // The unresolved priority is here for the convenience of the clients. It should not be passed to the ResourceLoadScheduler.
-    ResourceLoadPriorityUnresolved = -1,
-    ResourceLoadPriorityVeryLow = 0,
+    ResourceLoadPriorityVeryLow,
     ResourceLoadPriorityLow,
     ResourceLoadPriorityMedium,
     ResourceLoadPriorityHigh,
index 09d8fb1..94dccb9 100644 (file)
@@ -437,7 +437,7 @@ ResourceLoadPriority ResourceRequestBase::priority() const
 {
     updateResourceRequest();
 
-    return m_priority;
+    return static_cast<ResourceLoadPriority>(m_priority);
 }
 
 void ResourceRequestBase::setPriority(ResourceLoadPriority priority)
@@ -448,6 +448,7 @@ void ResourceRequestBase::setPriority(ResourceLoadPriority priority)
         return;
 
     m_priority = priority;
+    ASSERT(static_cast<ResourceLoadPriority>(m_priority) == priority); // Make sure it fits in the bitfield.
 
     if (url().protocolIsInHTTPFamily())
         m_platformRequestUpdated = false;
index 9b5e797..3920d2b 100644 (file)
@@ -214,16 +214,16 @@ namespace WebCore {
         Vector<String> m_responseContentDispositionEncodingFallbackArray;
         RefPtr<FormData> m_httpBody;
         unsigned m_cachePolicy : 3;
-        bool m_allowCookies : 1;
-        mutable bool m_resourceRequestUpdated : 1;
-        mutable bool m_platformRequestUpdated : 1;
-        mutable bool m_resourceRequestBodyUpdated : 1;
-        mutable bool m_platformRequestBodyUpdated : 1;
-        bool m_reportUploadProgress : 1;
-        bool m_reportLoadTiming : 1;
-        bool m_reportRawHeaders : 1;
-        bool m_hiddenFromInspector : 1;
-        ResourceLoadPriority m_priority : 4; // not unsigned because ResourceLoadPriority has negative values
+        unsigned m_allowCookies : 1;
+        mutable unsigned m_resourceRequestUpdated : 1;
+        mutable unsigned m_platformRequestUpdated : 1;
+        mutable unsigned m_resourceRequestBodyUpdated : 1;
+        mutable unsigned m_platformRequestBodyUpdated : 1;
+        unsigned m_reportUploadProgress : 1;
+        unsigned m_reportLoadTiming : 1;
+        unsigned m_reportRawHeaders : 1;
+        unsigned m_hiddenFromInspector : 1;
+        unsigned m_priority : 4;
 
     private:
         const ResourceRequest& asResourceRequest() const;
index c6475df..f6f2bc0 100644 (file)
@@ -158,7 +158,7 @@ void ResourceRequest::doUpdatePlatformRequest()
         wkHTTPRequestEnablePipelining(cfRequest);
 
     if (resourcePrioritiesEnabled())
-        wkSetHTTPRequestPriority(cfRequest, toPlatformRequestPriority(m_priority));
+        wkSetHTTPRequestPriority(cfRequest, toPlatformRequestPriority(priority()));
 
 #if !PLATFORM(WIN)
     wkCFURLRequestAllowAllPostCaching(cfRequest);
@@ -274,11 +274,8 @@ void ResourceRequest::doUpdateResourceRequest()
     }
     m_allowCookies = CFURLRequestShouldHandleHTTPCookies(m_cfRequest.get());
 
-    if (resourcePrioritiesEnabled()) {
-        auto priority = toResourceLoadPriority(wkGetHTTPRequestPriority(m_cfRequest.get()));
-        if (priority > ResourceLoadPriorityUnresolved)
-            m_priority = priority;
-    }
+    if (resourcePrioritiesEnabled())
+        m_priority = toResourceLoadPriority(wkGetHTTPRequestPriority(m_cfRequest.get()));
 
     m_httpHeaderFields.clear();
     if (CFDictionaryRef headers = CFURLRequestCopyAllHTTPHeaderFields(m_cfRequest.get())) {
index af5d38a..769c8e5 100644 (file)
@@ -44,8 +44,6 @@ CFURLRequestRef cfURLRequest(const ResourceRequest&);
 inline ResourceLoadPriority toResourceLoadPriority(int priority)
 {
     switch (priority) {
-    case -1:
-        return ResourceLoadPriorityUnresolved;
     case 0:
         return ResourceLoadPriorityVeryLow;
     case 1:
@@ -65,8 +63,6 @@ inline ResourceLoadPriority toResourceLoadPriority(int priority)
 inline int toPlatformRequestPriority(ResourceLoadPriority priority)
 {
     switch (priority) {
-    case ResourceLoadPriorityUnresolved:
-        return -1;
     case ResourceLoadPriorityVeryLow:
         return 0;
     case ResourceLoadPriorityLow:
index 0a609fc..d777a03 100644 (file)
@@ -78,11 +78,8 @@ void ResourceRequest::doUpdateResourceRequest()
         m_httpMethod = method;
     m_allowCookies = [m_nsRequest.get() HTTPShouldHandleCookies];
 
-    if (resourcePrioritiesEnabled()) {
-        auto priority = toResourceLoadPriority(wkGetHTTPRequestPriority([m_nsRequest.get() _CFURLRequest]));
-        if (priority > ResourceLoadPriorityUnresolved)
-            m_priority = priority;
-    }
+    if (resourcePrioritiesEnabled())
+        m_priority = toResourceLoadPriority(wkGetHTTPRequestPriority([m_nsRequest.get() _CFURLRequest]));
 
     m_httpHeaderFields.clear();
     [[m_nsRequest allHTTPHeaderFields] enumerateKeysAndObjectsUsingBlock: ^(NSString *name, NSString *value, BOOL *) {
@@ -145,7 +142,7 @@ void ResourceRequest::doUpdatePlatformRequest()
         wkHTTPRequestEnablePipelining([nsRequest _CFURLRequest]);
 
     if (ResourceRequest::resourcePrioritiesEnabled())
-        wkSetHTTPRequestPriority([nsRequest _CFURLRequest], toPlatformRequestPriority(m_priority));
+        wkSetHTTPRequestPriority([nsRequest _CFURLRequest], toPlatformRequestPriority(priority()));
 
     [nsRequest setCachePolicy:(NSURLRequestCachePolicy)cachePolicy()];
     wkCFURLRequestAllowAllPostCaching([nsRequest _CFURLRequest]);
index 44c8345..3fa0934 100644 (file)
@@ -132,8 +132,6 @@ namespace WebCore {
 inline SoupMessagePriority toSoupMessagePriority(ResourceLoadPriority priority)
 {
     switch (priority) {
-    case ResourceLoadPriorityUnresolved:
-        return SOUP_MESSAGE_PRIORITY_NORMAL;
     case ResourceLoadPriorityVeryLow:
         return SOUP_MESSAGE_PRIORITY_VERY_LOW;
     case ResourceLoadPriorityLow:
index e055d97..0c98d1f 100644 (file)
@@ -1,3 +1,18 @@
+2015-02-03  Chris Dumez  <cdumez@apple.com>
+
+        Drop ResourceLoadPriorityUnresolved resource load priority and use Optional<> instead
+        https://bugs.webkit.org/show_bug.cgi?id=141186
+
+        Reviewed by Antti Koivisto.
+
+        Update code now that ResourceLoadPriorityUnresolved is not longer a
+        ResourceLoadPriority enum value.
+
+        * NetworkProcess/cache/NetworkCache.cpp:
+        (WebKit::NetworkCache::retrieve):
+        * WebProcess/Network/WebResourceLoadScheduler.cpp:
+        (WebKit::WebResourceLoadScheduler::scheduleLoad):
+
 2015-02-03  Enrica Casucci  <enrica@apple.com>
 
         [iOS] Add support for deleteFromInputWithFlags.
index 7e4e6c4..81f24a4 100644 (file)
@@ -218,7 +218,7 @@ void NetworkCache::retrieve(const WebCore::ResourceRequest& originalRequest, std
 {
     ASSERT(isEnabled());
 
-    LOG(NetworkCache, "(NetworkProcess) retrieving %s priority %d", originalRequest.url().string().ascii().data(), originalRequest.priority());
+    LOG(NetworkCache, "(NetworkProcess) retrieving %s priority %u", originalRequest.url().string().ascii().data(), originalRequest.priority());
 
     if (!canRetrieve(originalRequest)) {
         completionHandler(nullptr);
@@ -249,7 +249,7 @@ void NetworkCache::retrieve(const WebCore::ResourceRequest& originalRequest, std
 #if !LOG_DISABLED
         auto elapsedMS = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::system_clock::now() - startTime).count();
 #endif
-        LOG(NetworkCache, "(NetworkProcess) retrieve complete success=%d priority=%d time=%lldms", success, capture->originalRequest.priority(), elapsedMS);
+        LOG(NetworkCache, "(NetworkProcess) retrieve complete success=%d priority=%u time=%lldms", success, capture->originalRequest.priority(), elapsedMS);
         capture->completionHandler(WTF::move(decodedEntry));
         return success;
     });
index 80245fb..925279e 100644 (file)
@@ -150,7 +150,7 @@ void WebResourceLoadScheduler::scheduleLoad(ResourceLoader* resourceLoader, Cach
     }
 #endif
 
-    LOG(NetworkScheduling, "(WebProcess) WebResourceLoadScheduler::scheduleLoad, url '%s' will be scheduled with the NetworkProcess with priority %i", resourceLoader->url().string().utf8().data(), resourceLoader->request().priority());
+    LOG(NetworkScheduling, "(WebProcess) WebResourceLoadScheduler::scheduleLoad, url '%s' will be scheduled with the NetworkProcess with priority %u", resourceLoader->url().string().utf8().data(), resourceLoader->request().priority());
 
     ContentSniffingPolicy contentSniffingPolicy = resourceLoader->shouldSniffContent() ? SniffContent : DoNotSniffContent;
     StoredCredentials allowStoredCredentials = resourceLoader->shouldUseCredentialStorage() ? AllowStoredCredentials : DoNotAllowStoredCredentials;