ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache...

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

Reviewed by Darin Adler.

Source/WebCore:

ResourceRequest deserialization was unnecessarily calling partitionName()
on the decoded cache partition. In the deserialization case, we already
know the cache partition is a valid partition name so we can bypass the
call to partitionName() (which is fairly expensive) for performance.

This patch adds a setDomainForCachePartion() method to ResourceRequest
that calls partitionName() on the domain argument, and moves all the
callers of setCachedPartition() to this new setter, except
ArgumentCoder<ResourceRequest>::decode().

This patch updates the setCachedPartition() to merely set the
m_cachePartition member, without calling partitionName() on the
argument. There is also a new assertion in place to make sure the
argument is a valid partition name.

No new tests, no behavior change.

* html/DOMURL.cpp:
(WebCore::DOMURL::revokeObjectURL):
* inspector/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::cachedResource):
* inspector/InspectorResourceAgent.cpp:
(WebCore::InspectorResourceAgent::replayXHR):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
* loader/archive/cf/LegacyWebArchive.cpp:
(WebCore::LegacyWebArchive::create):
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
(WebCore::CachedResourceLoader::requestResource):
* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::addImageToCache):
(WebCore::MemoryCache::removeImageFromCache):
* loader/cache/MemoryCache.h:
* page/SecurityOrigin.cpp:
(WebCore::SecurityOrigin::domainForCachePartition):
(WebCore::SecurityOrigin::cachePartition): Deleted.
Rename cachePartition() getter to domainForCachePartition() to make it
clear it returns a domain and not a partition name. As such, the caller
should then use ResourceRequest::setDomainForCachePartition(), not
setCachePartition().

* page/SecurityOrigin.h:
* platform/network/cf/ResourceRequest.h:
(WebCore::ResourceRequest::setCachePartition):
(WebCore::ResourceRequest::setDomainForCachePartition):

Source/WebKit/mac:

Call ResourceRequest::setDomainForPartitionName() instead of
setPartitionName() as the argument is a domain, not a valid
partition name.

* Misc/WebCache.mm:
(+[WebCache addImageToCache:forURL:forFrame:]):
(+[WebCache removeImageFromCacheForURL:forFrame:]):

Source/WebKit2:

This patch adds a |needsValidation| argument to
ResourceRequest::setCachePartition() setter so that the caller can
indicate that the partition name is valid (because it was already
processed by partitionName() before). Use this new argument in
ArgumentCoder<ResourceRequest>::decode() to spent a bit less time
deserializing resource requests.

* Shared/WebCoreArgumentCoders.cpp:
(IPC::ArgumentCoder<ResourceRequest>::decode):

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

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c60d5bf..9a58ebe 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,57 @@
+2014-10-20  Chris Dumez  <cdumez@apple.com>
+
+        ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache partition
+        https://bugs.webkit.org/show_bug.cgi?id=137797
+
+        Reviewed by Darin Adler.
+
+        ResourceRequest deserialization was unnecessarily calling partitionName()
+        on the decoded cache partition. In the deserialization case, we already
+        know the cache partition is a valid partition name so we can bypass the
+        call to partitionName() (which is fairly expensive) for performance.
+
+        This patch adds a setDomainForCachePartion() method to ResourceRequest
+        that calls partitionName() on the domain argument, and moves all the
+        callers of setCachedPartition() to this new setter, except
+        ArgumentCoder<ResourceRequest>::decode().
+
+        This patch updates the setCachedPartition() to merely set the
+        m_cachePartition member, without calling partitionName() on the
+        argument. There is also a new assertion in place to make sure the
+        argument is a valid partition name.
+
+        No new tests, no behavior change.
+
+        * html/DOMURL.cpp:
+        (WebCore::DOMURL::revokeObjectURL):
+        * inspector/InspectorPageAgent.cpp:
+        (WebCore::InspectorPageAgent::cachedResource):
+        * inspector/InspectorResourceAgent.cpp:
+        (WebCore::InspectorResourceAgent::replayXHR):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+        * loader/archive/cf/LegacyWebArchive.cpp:
+        (WebCore::LegacyWebArchive::create):
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
+        (WebCore::CachedResourceLoader::requestResource):
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::addImageToCache):
+        (WebCore::MemoryCache::removeImageFromCache):
+        * loader/cache/MemoryCache.h:
+        * page/SecurityOrigin.cpp:
+        (WebCore::SecurityOrigin::domainForCachePartition):
+        (WebCore::SecurityOrigin::cachePartition): Deleted.
+        Rename cachePartition() getter to domainForCachePartition() to make it
+        clear it returns a domain and not a partition name. As such, the caller
+        should then use ResourceRequest::setDomainForCachePartition(), not
+        setCachePartition().
+
+        * page/SecurityOrigin.h:
+        * platform/network/cf/ResourceRequest.h:
+        (WebCore::ResourceRequest::setCachePartition):
+        (WebCore::ResourceRequest::setDomainForCachePartition):
+
 2014-10-20  Youenn Fablet  <youenn.fablet@crf.canon.fr>
 
         Tighten XMLHttpRequest setRequestHeader value check

diff --git a/Source/WebCore/html/DOMURL.cpp b/Source/WebCore/html/DOMURL.cpp
index dd28ee2..36fc835 100644 (file)
--- a/Source/WebCore/html/DOMURL.cpp
+++ b/Source/WebCore/html/DOMURL.cpp
@@ -116,7 +116,7 @@ void DOMURL::revokeObjectURL(ScriptExecutionContext* scriptExecutionContext, con
     URL url(URL(), urlString);
     ResourceRequest request(url);
 #if ENABLE(CACHE_PARTITIONING)
-    request.setCachePartition(scriptExecutionContext->topOrigin()->cachePartition());
+    request.setDomainForCachePartition(scriptExecutionContext->topOrigin()->domainForCachePartition());
 #endif
     MemoryCache::removeRequestFromSessionCaches(scriptExecutionContext, request);
 

diff --git a/Source/WebCore/inspector/InspectorPageAgent.cpp b/Source/WebCore/inspector/InspectorPageAgent.cpp
index 43ca1ca..5fd2272 100644 (file)
--- a/Source/WebCore/inspector/InspectorPageAgent.cpp
+++ b/Source/WebCore/inspector/InspectorPageAgent.cpp
@@ -266,7 +266,7 @@ CachedResource* InspectorPageAgent::cachedResource(Frame* frame, const URL& url)
     if (!cachedResource) {
         ResourceRequest request(url);
 #if ENABLE(CACHE_PARTITIONING)
-        request.setCachePartition(frame->document()->topOrigin()->cachePartition());
+        request.setDomainForCachePartition(frame->document()->topOrigin()->domainForCachePartition());
 #endif
         cachedResource = memoryCache()->resourceForRequest(request, frame->page()->sessionID());
     }

diff --git a/Source/WebCore/inspector/InspectorResourceAgent.cpp b/Source/WebCore/inspector/InspectorResourceAgent.cpp
index d5b9d29..dc3d9b8 100644 (file)
--- a/Source/WebCore/inspector/InspectorResourceAgent.cpp
+++ b/Source/WebCore/inspector/InspectorResourceAgent.cpp
@@ -698,7 +698,7 @@ void InspectorResourceAgent::replayXHR(ErrorString&, const String& requestId)
 
     ResourceRequest request(xhrReplayData->url());
 #if ENABLE(CACHE_PARTITIONING)
-    request.setCachePartition(m_pageAgent->mainFrame()->document()->topOrigin()->cachePartition());
+    request.setDomainForCachePartition(m_pageAgent->mainFrame()->document()->topOrigin()->domainForCachePartition());
 #endif
 
     CachedResource* cachedResource = memoryCache()->resourceForRequest(request, m_pageAgent->page()->sessionID());

diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
index 24dd28a..fcb524a 100644 (file)
--- a/Source/WebCore/loader/FrameLoader.cpp
+++ b/Source/WebCore/loader/FrameLoader.cpp
@@ -1248,7 +1248,7 @@ void FrameLoader::loadURL(const URL& newURL, const String& referrer, const Strin
     }
 #if ENABLE(CACHE_PARTITIONING)
     if (&m_frame.tree().top() != &m_frame)
-        request.setCachePartition(m_frame.tree().top().document()->securityOrigin()->cachePartition());
+        request.setDomainForCachePartition(m_frame.tree().top().document()->securityOrigin()->domainForCachePartition());
 #endif
     addExtraFieldsToRequest(request, newLoadType, true);
     if (newLoadType == FrameLoadType::Reload || newLoadType == FrameLoadType::ReloadFromOrigin)

diff --git a/Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp b/Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp
index 0802681..24cf4be 100644 (file)
--- a/Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp
+++ b/Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp
@@ -546,7 +546,7 @@ PassRefPtr<LegacyWebArchive> LegacyWebArchive::create(const String& markupString
 
                 ResourceRequest request(subresourceURL);
 #if ENABLE(CACHE_PARTITIONING)
-                request.setCachePartition(frame->document()->topOrigin()->cachePartition());
+                request.setDomainForCachePartition(frame->document()->topOrigin()->domainForCachePartition());
 #endif
                 CachedResource* cachedResource = memoryCache()->resourceForRequest(request, frame->page()->sessionID());
                 if (cachedResource) {

diff --git a/Source/WebCore/loader/cache/CachedResourceLoader.cpp b/Source/WebCore/loader/cache/CachedResourceLoader.cpp
index 5bbf10f..7cf186e 100644 (file)
--- a/Source/WebCore/loader/cache/CachedResourceLoader.cpp
+++ b/Source/WebCore/loader/cache/CachedResourceLoader.cpp
@@ -195,7 +195,7 @@ CachedResourceHandle<CachedCSSStyleSheet> CachedResourceLoader::requestUserCSSSt
     URL url = MemoryCache::removeFragmentIdentifierIfNeeded(request.resourceRequest().url());
 
 #if ENABLE(CACHE_PARTITIONING)
-    request.mutableResourceRequest().setCachePartition(document()->topOrigin()->cachePartition());
+    request.mutableResourceRequest().setDomainForCachePartition(document()->topOrigin()->domainForCachePartition());
 #endif
 
     if (CachedResource* existing = memoryCache()->resourceForRequest(request.resourceRequest(), sessionID())) {
@@ -439,7 +439,7 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
     CachedResourceHandle<CachedResource> resource;
 #if ENABLE(CACHE_PARTITIONING)
     if (document())
-        request.mutableResourceRequest().setCachePartition(document()->topOrigin()->cachePartition());
+        request.mutableResourceRequest().setDomainForCachePartition(document()->topOrigin()->domainForCachePartition());
 #endif
 
     resource = memoryCache()->resourceForRequest(request.resourceRequest(), sessionID());

diff --git a/Source/WebCore/loader/cache/MemoryCache.cpp b/Source/WebCore/loader/cache/MemoryCache.cpp
index 3f2378d..b9fdb2b 100644 (file)
--- a/Source/WebCore/loader/cache/MemoryCache.cpp
+++ b/Source/WebCore/loader/cache/MemoryCache.cpp
@@ -233,11 +233,11 @@ static CachedImageClient& dummyCachedImageClient()
     return client;
 }
 
-bool MemoryCache::addImageToCache(NativeImagePtr image, const URL& url, const String& cachePartition)
+bool MemoryCache::addImageToCache(NativeImagePtr image, const URL& url, const String& domainForCachePartition)
 {
     ASSERT(image);
     SessionID sessionID = SessionID::defaultSessionID();
-    removeImageFromCache(url, cachePartition); // Remove cache entry if it already exists.
+    removeImageFromCache(url, domainForCachePartition); // Remove cache entry if it already exists.
 
     RefPtr<BitmapImage> bitmapImage = BitmapImage::create(image, nullptr);
     if (!bitmapImage)
@@ -250,22 +250,22 @@ bool MemoryCache::addImageToCache(NativeImagePtr image, const URL& url, const St
     cachedImage->addClient(&dummyCachedImageClient());
     cachedImage->setDecodedSize(bitmapImage->decodedSize());
 #if ENABLE(CACHE_PARTITIONING)
-    cachedImage->resourceRequest().setCachePartition(cachePartition);
+    cachedImage->resourceRequest().setDomainForCachePartition(domainForCachePartition);
 #endif
     return add(cachedImage.release());
 }
 
-void MemoryCache::removeImageFromCache(const URL& url, const String& cachePartition)
+void MemoryCache::removeImageFromCache(const URL& url, const String& domainForCachePartition)
 {
     CachedResourceMap& resources = getSessionMap(SessionID::defaultSessionID());
 #if ENABLE(CACHE_PARTITIONING)
     CachedResource* resource;
     if (CachedResourceItem* item = resources.get(url))
-        resource = item->get(ResourceRequest::partitionName(cachePartition));
+        resource = item->get(ResourceRequest::partitionName(domainForCachePartition));
     else
         resource = nullptr;
 #else
-    UNUSED_PARAM(cachePartition);
+    UNUSED_PARAM(domainForCachePartition);
     CachedResource* resource = resources.get(url);
 #endif
     if (!resource)

diff --git a/Source/WebCore/loader/cache/MemoryCache.h b/Source/WebCore/loader/cache/MemoryCache.h
index 4998ced..1bbad1f 100644 (file)
--- a/Source/WebCore/loader/cache/MemoryCache.h
+++ b/Source/WebCore/loader/cache/MemoryCache.h
@@ -174,8 +174,8 @@ public:
 #if USE(CG)
     // FIXME: Remove the USE(CG) once we either make NativeImagePtr a smart pointer on all platforms or
     // remove the usage of CFRetain() in MemoryCache::addImageToCache() so as to make the code platform-independent.
-    WEBCORE_EXPORT bool addImageToCache(NativeImagePtr, const URL&, const String& cachePartition);
-    WEBCORE_EXPORT void removeImageFromCache(const URL&, const String& cachePartition);
+    WEBCORE_EXPORT bool addImageToCache(NativeImagePtr, const URL&, const String& domainForCachePartition);
+    WEBCORE_EXPORT void removeImageFromCache(const URL&, const String& domainForCachePartition);
 #endif
 
     // pruneDead*() - Flush decoded and encoded data from resources not referenced by Web pages.

diff --git a/Source/WebCore/page/SecurityOrigin.cpp b/Source/WebCore/page/SecurityOrigin.cpp
index 9b08e51..2c88b1c 100644 (file)
--- a/Source/WebCore/page/SecurityOrigin.cpp
+++ b/Source/WebCore/page/SecurityOrigin.cpp
@@ -434,7 +434,7 @@ void SecurityOrigin::grantUniversalAccess()
 }
 
 #if ENABLE(CACHE_PARTITIONING)
-String SecurityOrigin::cachePartition() const
+String SecurityOrigin::domainForCachePartition() const
 {
     if (m_storageBlockingPolicy != BlockThirdPartyStorage)
         return String();

diff --git a/Source/WebCore/page/SecurityOrigin.h b/Source/WebCore/page/SecurityOrigin.h
index 744c9d8..aee4f0d 100644 (file)
--- a/Source/WebCore/page/SecurityOrigin.h
+++ b/Source/WebCore/page/SecurityOrigin.h
@@ -147,7 +147,7 @@ public:
     void setStorageBlockingPolicy(StorageBlockingPolicy policy) { m_storageBlockingPolicy = policy; }
 
 #if ENABLE(CACHE_PARTITIONING)
-    WEBCORE_EXPORT String cachePartition() const;
+    WEBCORE_EXPORT String domainForCachePartition() const;
 #endif
 
     bool canAccessDatabase(const SecurityOrigin* topOrigin = 0) const { return canAccessStorage(topOrigin); };

diff --git a/Source/WebCore/platform/network/cf/ResourceRequest.h b/Source/WebCore/platform/network/cf/ResourceRequest.h
index 8c4ae0d..27a6d41 100644 (file)
--- a/Source/WebCore/platform/network/cf/ResourceRequest.h
+++ b/Source/WebCore/platform/network/cf/ResourceRequest.h
@@ -99,7 +99,12 @@ namespace WebCore {
 #if ENABLE(CACHE_PARTITIONING)
         WEBCORE_EXPORT static String partitionName(const String& domain);
         const String& cachePartition() const { return m_cachePartition.isNull() ? emptyString() : m_cachePartition; }
-        void setCachePartition(const String& cachePartition) { m_cachePartition = partitionName(cachePartition); }
+        void setCachePartition(const String& cachePartition)
+        {
+            ASSERT(cachePartition == partitionName(cachePartition));
+            m_cachePartition = cachePartition;
+        }
+        void setDomainForCachePartition(const String& domain) { m_cachePartition = partitionName(domain); }
 #endif
 
 #if PLATFORM(COCOA) || USE(CFNETWORK)

diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog
index e851a08..c7602f2 100644 (file)
--- a/Source/WebKit/mac/ChangeLog
+++ b/Source/WebKit/mac/ChangeLog
@@ -1,3 +1,18 @@
+2014-10-20  Chris Dumez  <cdumez@apple.com>
+
+        ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache partition
+        https://bugs.webkit.org/show_bug.cgi?id=137797
+
+        Reviewed by Darin Adler.
+
+        Call ResourceRequest::setDomainForPartitionName() instead of
+        setPartitionName() as the argument is a domain, not a valid
+        partition name.
+
+        * Misc/WebCache.mm:
+        (+[WebCache addImageToCache:forURL:forFrame:]):
+        (+[WebCache removeImageFromCacheForURL:forFrame:]):
+
 2014-10-20  Andy Estes  <aestes@apple.com>
 
         Fix the iOS build.

diff --git a/Source/WebKit/mac/Misc/WebCache.mm b/Source/WebKit/mac/Misc/WebCache.mm
index 5b3eb03..0221c98 100644 (file)
--- a/Source/WebKit/mac/Misc/WebCache.mm
+++ b/Source/WebKit/mac/Misc/WebCache.mm
@@ -176,7 +176,7 @@
     if (frame)
         topOrigin = core(frame)->document()->topOrigin();
 #endif
-    return WebCore::memoryCache()->addImageToCache(image, url, topOrigin ? topOrigin->cachePartition() : emptyString());
+    return WebCore::memoryCache()->addImageToCache(image, url, topOrigin ? topOrigin->domainForCachePartition() : emptyString());
 }
 
 + (void)removeImageFromCacheForURL:(NSURL *)url
@@ -193,7 +193,7 @@
     if (frame)
         topOrigin = core(frame)->document()->topOrigin();
 #endif
-    WebCore::memoryCache()->removeImageFromCache(url, topOrigin ? topOrigin->cachePartition() : emptyString());
+    WebCore::memoryCache()->removeImageFromCache(url, topOrigin ? topOrigin->domainForCachePartition() : emptyString());
 }
 
 + (CGImageRef)imageForURL:(NSURL *)url

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index 9823517..e98ce1b 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,20 @@
+2014-10-20  Chris Dumez  <cdumez@apple.com>
+
+        ResourceRequest deserialization unnecessarily calls partitionName() on encoded cache partition
+        https://bugs.webkit.org/show_bug.cgi?id=137797
+
+        Reviewed by Darin Adler.
+
+        This patch adds a |needsValidation| argument to
+        ResourceRequest::setCachePartition() setter so that the caller can
+        indicate that the partition name is valid (because it was already
+        processed by partitionName() before). Use this new argument in
+        ArgumentCoder<ResourceRequest>::decode() to spent a bit less time
+        deserializing resource requests.
+
+        * Shared/WebCoreArgumentCoders.cpp:
+        (IPC::ArgumentCoder<ResourceRequest>::decode):
+
 2014-10-20  Beth Dakin  <bdakin@apple.com>
 
         Action menu items should have tags