CachedResourceLoader should not need to remove fragment identifier
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Oct 2016 12:30:40 +0000 (12:30 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Oct 2016 12:30:40 +0000 (12:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=163015

Patch by Youenn Fablet <youenn@apple.com> on 2016-10-18
Reviewed by Darin Adler.

No expected change for non-window port.
For window port, CachedResourceLoader will strip the fragment identifier of the URL passed to subresourceForURL
before querying the memory cache.

Removing the fragment identifier from the request stored in CachedResourceRequest.
The fragment identifier is stored in a separate field.

This allows CachedResourceLoader to not care about fragment identifier.
CachedResource can then get access to it.

* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::CachedResource):
(WebCore::CachedResource::finishRequestInitialization): Deleted.
* loader/cache/CachedResource.h:
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::cachedResource):
Updated the method taking a const String& to strip the fragment identifier if needed.
Updated the method taking a const URL& to assert if the fragment identifier is present.
(WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
(WebCore::CachedResourceLoader::requestResource):
* loader/cache/CachedResourceRequest.cpp:
(WebCore::CachedResourceRequest::CachedResourceRequest):
(WebCore::CachedResourceRequest::splitFragmentIdentifierFromRequestURL):
* loader/cache/CachedResourceRequest.h:
(WebCore::CachedResourceRequest::releaseFragmentIdentifier):
(WebCore::CachedResourceRequest::clearFragmentIdentifier):
* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::shouldRemoveFragmentIdentifier):
(WebCore::MemoryCache::removeFragmentIdentifierIfNeeded):
(WebCore::MemoryCache::revalidationSucceeded):
(WebCore::MemoryCache::resourceForRequest):
* loader/cache/MemoryCache.h:

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

Source/WebCore/ChangeLog
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/loader/cache/MemoryCache.cpp
Source/WebCore/loader/cache/MemoryCache.h

index 15d116a..1e43ef6 100644 (file)
@@ -1,3 +1,43 @@
+2016-10-18  Youenn Fablet  <youenn@apple.com>
+
+        CachedResourceLoader should not need to remove fragment identifier
+        https://bugs.webkit.org/show_bug.cgi?id=163015
+
+        Reviewed by Darin Adler.
+
+        No expected change for non-window port.
+        For window port, CachedResourceLoader will strip the fragment identifier of the URL passed to subresourceForURL
+        before querying the memory cache.
+
+        Removing the fragment identifier from the request stored in CachedResourceRequest.
+        The fragment identifier is stored in a separate field.
+
+        This allows CachedResourceLoader to not care about fragment identifier.
+        CachedResource can then get access to it.
+
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::CachedResource):
+        (WebCore::CachedResource::finishRequestInitialization): Deleted.
+        * loader/cache/CachedResource.h:
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::cachedResource):
+        Updated the method taking a const String& to strip the fragment identifier if needed.
+        Updated the method taking a const URL& to assert if the fragment identifier is present.
+        (WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
+        (WebCore::CachedResourceLoader::requestResource):
+        * loader/cache/CachedResourceRequest.cpp:
+        (WebCore::CachedResourceRequest::CachedResourceRequest):
+        (WebCore::CachedResourceRequest::splitFragmentIdentifierFromRequestURL):
+        * loader/cache/CachedResourceRequest.h:
+        (WebCore::CachedResourceRequest::releaseFragmentIdentifier):
+        (WebCore::CachedResourceRequest::clearFragmentIdentifier):
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::shouldRemoveFragmentIdentifier):
+        (WebCore::MemoryCache::removeFragmentIdentifierIfNeeded):
+        (WebCore::MemoryCache::revalidationSucceeded):
+        (WebCore::MemoryCache::resourceForRequest):
+        * loader/cache/MemoryCache.h:
+
 2016-10-18  Antti Koivisto  <antti@apple.com>
 
         Rename setNeedsStyleRecalc to invalidateStyle
index 42d7b52..7b28ecf 100644 (file)
@@ -121,13 +121,16 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
     , m_sessionID(sessionID)
     , m_loadPriority(defaultPriorityForResourceType(type))
     , m_responseTimestamp(std::chrono::system_clock::now())
+    , m_fragmentIdentifierForRequest(request.releaseFragmentIdentifier())
     , m_origin(request.releaseOrigin())
     , m_type(type)
 {
     ASSERT(sessionID.isValid());
 
     setLoadPriority(request.priority());
-    finishRequestInitialization();
+#ifndef NDEBUG
+    cachedResourceLeakCounter.increment();
+#endif
 
     // FIXME: We should have a better way of checking for Navigation loads, maybe FetchMode::Options::Navigate.
     ASSERT(m_origin || m_type == CachedResource::MainResource);
@@ -138,31 +141,20 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
         setCrossOrigin();
 }
 
+// FIXME: For this constructor, we should probably mandate that the URL has no fragment identifier.
 CachedResource::CachedResource(const URL& url, Type type, SessionID sessionID)
     : m_resourceRequest(url)
     , m_decodedDataDeletionTimer(*this, &CachedResource::destroyDecodedData, deadDecodedDataDeletionIntervalForResourceType(type))
     , m_sessionID(sessionID)
     , m_responseTimestamp(std::chrono::system_clock::now())
+    , m_fragmentIdentifierForRequest(CachedResourceRequest::splitFragmentIdentifierFromRequestURL(m_resourceRequest))
     , m_type(type)
     , m_status(Cached)
 {
     ASSERT(sessionID.isValid());
-    finishRequestInitialization();
-}
-
-void CachedResource::finishRequestInitialization()
-{
 #ifndef NDEBUG
     cachedResourceLeakCounter.increment();
 #endif
-
-    if (!m_resourceRequest.url().hasFragmentIdentifier())
-        return;
-    URL urlForCache = MemoryCache::removeFragmentIdentifierIfNeeded(m_resourceRequest.url());
-    if (urlForCache.hasFragmentIdentifier())
-        return;
-    m_fragmentIdentifierForRequest = m_resourceRequest.url().fragmentIdentifier();
-    m_resourceRequest.setURL(urlForCache);
 }
 
 CachedResource::~CachedResource()
index 44f3f74..f04b1d5 100644 (file)
@@ -302,8 +302,6 @@ protected:
 private:
     class Callback;
 
-    void finishRequestInitialization();
-
     bool addClientToSet(CachedResourceClient&);
 
     void decodedDataDeletionTimerFired();
index 9e7922d..c4e3ac3 100644 (file)
@@ -148,17 +148,16 @@ CachedResourceLoader::~CachedResourceLoader()
     ASSERT(m_requestCount == 0);
 }
 
-CachedResource* CachedResourceLoader::cachedResource(const String& resourceURL) const 
+CachedResource* CachedResourceLoader::cachedResource(const String& resourceURL) const
 {
     ASSERT(!resourceURL.isNull());
-    URL url = m_document->completeURL(resourceURL);
-    return cachedResource(url); 
+    return cachedResource(MemoryCache::removeFragmentIdentifierIfNeeded(m_document->completeURL(resourceURL)));
 }
 
-CachedResource* CachedResourceLoader::cachedResource(const URL& resourceURL) const
+CachedResource* CachedResourceLoader::cachedResource(const URL& url) const
 {
-    URL url = MemoryCache::removeFragmentIdentifierIfNeeded(resourceURL);
-    return m_documentResources.get(url).get(); 
+    ASSERT(!MemoryCache::shouldRemoveFragmentIdentifier(url));
+    return m_documentResources.get(url).get();
 }
 
 Frame* CachedResourceLoader::frame() const
@@ -657,9 +656,6 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
 
     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);
-
     if (!url.isValid()) {
         RELEASE_LOG_IF_ALLOWED("requestResource: URL is invalid (frame = %p)", frame());
         return nullptr;
index 1dc2257..ec4fdeb 100644 (file)
@@ -42,9 +42,21 @@ CachedResourceRequest::CachedResourceRequest(ResourceRequest&& resourceRequest,
     , m_charset(WTFMove(charset))
     , m_options(options)
     , m_priority(priority)
+    , m_fragmentIdentifier(splitFragmentIdentifierFromRequestURL(m_resourceRequest))
 {
 }
 
+String CachedResourceRequest::splitFragmentIdentifierFromRequestURL(ResourceRequest& request)
+{
+    if (!MemoryCache::shouldRemoveFragmentIdentifier(request.url()))
+        return { };
+    URL url = request.url();
+    String fragmentIdentifier = url.fragmentIdentifier();
+    url.removeFragmentIdentifier();
+    request.setURL(url);
+    return fragmentIdentifier;
+}
+
 void CachedResourceRequest::setInitiator(PassRefPtr<Element> element)
 {
     ASSERT(!m_initiatorElement && m_initiatorName.isEmpty());
index b28ad03..390376c 100644 (file)
@@ -78,6 +78,11 @@ public:
     RefPtr<SecurityOrigin> releaseOrigin() { return WTFMove(m_origin); }
     SecurityOrigin* origin() const { return m_origin.get(); }
 
+    String&& releaseFragmentIdentifier() { return WTFMove(m_fragmentIdentifier); }
+    void clearFragmentIdentifier() { m_fragmentIdentifier = { }; }
+
+    static String splitFragmentIdentifierFromRequestURL(ResourceRequest&);
+
 private:
     ResourceRequest m_resourceRequest;
     String m_charset;
@@ -86,6 +91,7 @@ private:
     RefPtr<Element> m_initiatorElement;
     AtomicString m_initiatorName;
     RefPtr<SecurityOrigin> m_origin;
+    String m_fragmentIdentifier;
 };
 
 } // namespace WebCore
index 9b42847..066f47e 100644 (file)
@@ -89,14 +89,19 @@ auto MemoryCache::ensureSessionResourceMap(SessionID sessionID) -> CachedResourc
     return *map;
 }
 
-URL MemoryCache::removeFragmentIdentifierIfNeeded(const URL& originalURL)
+bool MemoryCache::shouldRemoveFragmentIdentifier(const URL& originalURL)
 {
     if (!originalURL.hasFragmentIdentifier())
-        return originalURL;
+        return false;
     // Strip away fragment identifier from HTTP URLs.
-    // Data URLs must be unmodified. For file and custom URLs clients may expect resources 
+    // Data URLs must be unmodified. For file and custom URLs clients may expect resources
     // to be unique even when they differ by the fragment identifier only.
-    if (!originalURL.protocolIsInHTTPFamily())
+    return originalURL.protocolIsInHTTPFamily();
+}
+
+URL MemoryCache::removeFragmentIdentifierIfNeeded(const URL& originalURL)
+{
+    if (!shouldRemoveFragmentIdentifier(originalURL))
         return originalURL;
     URL url = originalURL;
     url.removeFragmentIdentifier();
@@ -154,7 +159,7 @@ void MemoryCache::revalidationSucceeded(CachedResource& revalidatingResource, co
         insertInLiveDecodedResourcesList(resource);
     if (delta)
         adjustSize(resource.hasClients(), delta);
-    
+
     revalidatingResource.switchClientsToRevalidatedResource();
     ASSERT(!revalidatingResource.m_deleted);
     // this deletes the revalidating resource
@@ -171,6 +176,8 @@ void MemoryCache::revalidationFailed(CachedResource& revalidatingResource)
 
 CachedResource* MemoryCache::resourceForRequest(const ResourceRequest& request, SessionID sessionID)
 {
+    // FIXME: Change all clients to make sure HTTP(s) URLs have no fragment identifiers before calling here.
+    // CachedResourceLoader is now doing this. Add an assertion once all other clients are doing it too.
     auto* resources = sessionResourceMap(sessionID);
     if (!resources)
         return nullptr;
index 502972a..60415e7 100644 (file)
@@ -97,16 +97,17 @@ public:
     bool add(CachedResource&);
     void remove(CachedResource&);
 
-    static URL removeFragmentIdentifierIfNeeded(const URL& originalURL);
-    
+    static bool shouldRemoveFragmentIdentifier(const URL&);
+    static URL removeFragmentIdentifierIfNeeded(const URL&);
+
     void revalidationSucceeded(CachedResource& revalidatingResource, const ResourceResponse&);
     void revalidationFailed(CachedResource& revalidatingResource);
 
     void forEachResource(const std::function<void(CachedResource&)>&);
     void forEachSessionResource(SessionID, const std::function<void(CachedResource&)>&);
     void destroyDecodedDataForAllImages();
-    
-    // Sets the cache's memory capacities, in bytes. These will hold only approximately, 
+
+    // Sets the cache's memory capacities, in bytes. These will hold only approximately,
     // since the decoded cost of resources like scripts and stylesheets is not known.
     //  - minDeadBytes: The maximum number of bytes that dead resources should consume when the cache is under pressure.
     //  - maxDeadBytes: The maximum number of bytes that dead resources should consume when the cache is not under pressure.
@@ -120,7 +121,7 @@ public:
 
     WEBCORE_EXPORT void evictResources();
     WEBCORE_EXPORT void evictResources(SessionID);
-    
+
     void prune();
     void pruneSoon();
     unsigned size() const { return m_liveSize + m_deadSize; }