Changeset 104756 in webkit

Timestamp:

Jan 11, 2012 3:54:28 PM (12 years ago)

Author:

Nate Chapin

Message:

SubresourceLoader cleanup post r100311.

1. Simplify matching incrementRequestCount()/decrementRequestCount() calls.
2. Remove CachedImage custom code from SubresourceLoader.
3. Add a bunch of ASSERTs.
4. Remove the multipart-only call to didReceiveData() from didReceiveResponse(), since didReceiveData() would get called immediately after anyway.

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

Reviewed by Adam Barth.

No new tests, refactor only.

• loader/SubresourceLoader.cpp:

(WebCore::SubresourceLoader::didReceiveResponse): Remove multipart special case, handle it in didReceiveData().
(WebCore::SubresourceLoader::didReceiveData): Handle multipart state here, since we will receive only one

didReceiveData() call per multipart segment, but no didFinishLoading() call.

• loader/SubresourceLoader.h: Add a RequestCountTracker subclass to reduce complexity of ensuring we don't

decrement request count twice on CachedResourceLoader.

• loader/cache/CachedImage.cpp:

(WebCore::CachedImage::setResponse): Move CachedImage::clear() call out of SubresourceLoader, since it's

kind of a layering violation as is.

• loader/cache/CachedImage.h:
• loader/cache/CachedResource.cpp:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/SubresourceLoader.cpp (modified) (11 diffs)
• loader/SubresourceLoader.h (modified) (3 diffs)
• loader/cache/CachedImage.cpp (modified) (1 diff)
• loader/cache/CachedImage.h (modified) (3 diffs)
• loader/cache/CachedResource.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-01-11  Nate Chapin  <japhet@chromium.org>
+
+        SubresourceLoader cleanup post r100311.
+        1. Simplify matching incrementRequestCount()/decrementRequestCount() calls.
+        2. Remove CachedImage custom code from SubresourceLoader.
+        3. Add a bunch of ASSERTs.
+        4. Remove the multipart-only call to didReceiveData() from didReceiveResponse(),
+           since didReceiveData() would get called immediately after anyway.
+        https://bugs.webkit.org/show_bug.cgi?id=75887
+
+        Reviewed by Adam Barth.
+
+        No new tests, refactor only.
+
+        * loader/SubresourceLoader.cpp:
+        (WebCore::SubresourceLoader::didReceiveResponse): Remove multipart special case, handle it in didReceiveData().
+        (WebCore::SubresourceLoader::didReceiveData): Handle multipart state here, since we will receive only one
+             didReceiveData() call per multipart segment, but no didFinishLoading() call.
+        * loader/SubresourceLoader.h: Add a RequestCountTracker subclass to reduce complexity of ensuring we don't
+            decrement request count twice on CachedResourceLoader.
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::setResponse): Move CachedImage::clear() call out of SubresourceLoader, since it's
+            kind of a layering violation as is.
+        * loader/cache/CachedImage.h:
+        * loader/cache/CachedResource.cpp:
+
 2012-01-11  Joshua Bell  <jsbell@chromium.org>
 

trunk/Source/WebCore/loader/SubresourceLoader.cpp

@@ -30 +30 @@
 #include "SubresourceLoader.h"
 
-#include "CachedImage.h"
 #include "CachedResourceLoader.h"
 #include "Document.h"
@@ -48 +47 @@
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, subresourceLoaderCounter, ("SubresourceLoader"));
 
+SubresourceLoader::RequestCountTracker::RequestCountTracker(CachedResourceLoader* cachedResourceLoader, CachedResource* resource)
+    : m_cachedResourceLoader(cachedResourceLoader)
+    , m_resource(resource)
+{
+    m_cachedResourceLoader->incrementRequestCount(m_resource);
+}
+
+SubresourceLoader::RequestCountTracker::~RequestCountTracker()
+{
+    m_cachedResourceLoader->decrementRequestCount(m_resource);
+}
+
 SubresourceLoader::SubresourceLoader(Frame* frame, CachedResource* resource, const ResourceLoaderOptions& options)
     : ResourceLoader(frame, options)
@@ -54 +65 @@
     , m_loadingMultipartContent(false)
     , m_state(Uninitialized)
+    , m_requestCountTracker(adoptPtr(new RequestCountTracker(frame->document()->cachedResourceLoader(), resource)))
 {
 #ifndef NDEBUG
@@ -62 +74 @@
 SubresourceLoader::~SubresourceLoader()
 {
+    ASSERT(m_state != Initialized);
+    ASSERT(!m_document);
+    ASSERT(reachedTerminalState());
 #ifndef NDEBUG
     subresourceLoaderCounter.decrement();
@@ -120 +135 @@
         return false;
 
+    ASSERT(!reachedTerminalState());
     m_state = Initialized;
     m_documentLoader->addSubresourceLoader(this);
@@ -142 +158 @@
 void SubresourceLoader::didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
+    ASSERT(m_state == Initialized);
     RefPtr<SubresourceLoader> protect(this);
     m_resource->didSendData(bytesSent, totalBytesToBeSent);
@@ -149 +166 @@
 {
     ASSERT(!response.isNull());
+    ASSERT(m_state == Initialized);
 
     // Reference the object in this method since the additional processing can do
@@ -175 +193 @@
     ResourceLoader::didReceiveResponse(response);
 
-    if (m_loadingMultipartContent) {
-        ASSERT(m_resource->isImage());
-        static_cast<CachedImage*>(m_resource)->clear();
-    } else if (response.isMultipart()) {
+    if (response.isMultipart()) {
         m_loadingMultipartContent = true;
 
         // We don't count multiParts in a CachedResourceLoader's request count
-        m_document->cachedResourceLoader()->decrementRequestCount(m_resource);
+        m_requestCountTracker.clear();
+        setShouldBufferData(DoNotBufferData);
         if (!m_resource->isImage()) {
             cancel();
@@ -188 +204 @@
         }
     }
-
-    RefPtr<SharedBuffer> buffer = resourceData();
-    if (m_loadingMultipartContent && buffer && buffer->size()) {
-        // Since a subresource loader does not load multipart sections progressively,
-        // deliver the previously received data to the loader all at once now.
-        // Then clear the data to make way for the next multipart section.
-        sendDataToResource(buffer->data(), buffer->size());
-        clearResourceData();
-        
+}
+
+void SubresourceLoader::didReceiveData(const char* data, int length, long long encodedDataLength, bool allAtOnce)
+{
+    ASSERT(!m_resource->resourceToRevalidate());
+    ASSERT(!m_resource->errorOccurred());
+    ASSERT(m_state == Initialized);
+    // Reference the object in this method since the additional processing can do
+    // anything including removing the last reference to this object; one example of this is 3266216.
+    RefPtr<SubresourceLoader> protect(this);
+    ResourceLoader::didReceiveData(data, length, encodedDataLength, allAtOnce);
+
+    if (errorLoadingResource())
+        return;
+
+    sendDataToResource(data, length);
+    
+    if (m_loadingMultipartContent) {
+        // Since a subresource loader does not load multipart sections progressively, data was delivered to the loader all at once.        
         // After the first multipart section is complete, signal to delegates that this load is "finished" 
         m_documentLoader->subresourceLoaderFinishedLoadingOnePart(this);
         didFinishLoadingOnePart(0);
     }
-}
-
-void SubresourceLoader::didReceiveData(const char* data, int length, long long encodedDataLength, bool allAtOnce)
-{
-    ASSERT(!m_resource->resourceToRevalidate());
-    ASSERT(!m_resource->errorOccurred());
-    // Reference the object in this method since the additional processing can do
-    // anything including removing the last reference to this object; one example of this is 3266216.
-    RefPtr<SubresourceLoader> protect(this);
-    ResourceLoader::didReceiveData(data, length, encodedDataLength, allAtOnce);
-
-    if (errorLoadingResource())
-        return;
-
-    if (!m_loadingMultipartContent)
-        sendDataToResource(data, length);
 }
 
@@ -232 +242 @@
 void SubresourceLoader::sendDataToResource(const char* data, int length)
 {
-    // There are two cases where we might need to create our own SharedBuffer instead of copying the one in ResourceLoader.
-    // (1) Multipart content: The loader delivers the data in a multipart section all at once, then sends eof.
-    //     The resource data will change as the next part is loaded, so we need to make a copy.
-    // (2) Our client requested that the data not be buffered at the ResourceLoader level via ResourceLoaderOptions. In this case,
-    //     ResourceLoader::resourceData() will be null. However, unlike the multipart case, we don't want to tell the CachedResource
-    //     that all data has been received yet.
-    if (m_loadingMultipartContent || !resourceData()) {
-        RefPtr<SharedBuffer> copiedData = SharedBuffer::create(data, length);
-        m_resource->data(copiedData.release(), m_loadingMultipartContent);
-    } else
-        m_resource->data(resourceData(), false);
+    RefPtr<SharedBuffer> buffer = resourceData() ? resourceData() : SharedBuffer::create(data, length);
+    m_resource->data(buffer.release(), m_loadingMultipartContent);
 }
 
 void SubresourceLoader::didReceiveCachedMetadata(const char* data, int length)
 {
+    ASSERT(m_state == Initialized);
     ASSERT(!m_resource->resourceToRevalidate());
     m_resource->setSerializedCachedMetadata(data, length);
@@ -304 +306 @@
     ASSERT(!reachedTerminalState());
     if (m_state != Uninitialized) {
-        if (!m_loadingMultipartContent && m_state != Releasing)
-            m_document->cachedResourceLoader()->decrementRequestCount(m_resource);
-        m_state = Releasing;
+        m_requestCountTracker.clear();
         m_document->cachedResourceLoader()->loadDone();
         if (reachedTerminalState())

trunk/Source/WebCore/loader/SubresourceLoader.h

@@ -38 +38 @@
 
 class CachedResource;
+class CachedResourceLoader;
 class Document;
 class ResourceRequest;
@@ -79 +80 @@
         Initialized,
         Revalidating,
-        Finishing,
-        Releasing
+        Finishing
+    };
+
+    class RequestCountTracker {
+    public:
+        RequestCountTracker(CachedResourceLoader*, CachedResource*);
+        ~RequestCountTracker();
+    private:
+        CachedResourceLoader* m_cachedResourceLoader;
+        CachedResource* m_resource;
     };
 
@@ -87 +96 @@
     bool m_loadingMultipartContent;
     SubresourceLoaderState m_state;
+    OwnPtr<RequestCountTracker> m_requestCountTracker;
 };
 

trunk/Source/WebCore/loader/cache/CachedImage.cpp

@@ -388 +388 @@
 }
 
+void CachedImage::setResponse(const ResourceResponse& response)
+{
+    if (!m_response.isNull())
+        clear();
+    CachedResource::setResponse(response);
+}
+
 void CachedImage::destroyDecodedData()
 {

trunk/Source/WebCore/loader/cache/CachedImage.h

@@ -76 +76 @@
     virtual void data(PassRefPtr<SharedBuffer> data, bool allDataReceived);
     virtual void error(CachedResource::Status);
+    virtual void setResponse(const ResourceResponse&);
     
     // For compatibility, images keep loading even if there are HTTP errors.
@@ -81 +82 @@
 
     virtual bool isImage() const { return true; }
-
-    void clear();
-    
     bool stillNeedsLoad() const { return !errorOccurred() && status() == Unknown && !isLoading(); }
     void load();
@@ -97 +95 @@
 private:
     Image* lookupOrCreateImageForRenderer(const RenderObject*);
+
+    void clear();
 
     void createImage();

trunk/Source/WebCore/loader/cache/CachedResource.cpp

@@ -215 +215 @@
     
     m_loader = resourceLoadScheduler()->scheduleSubresourceLoad(cachedResourceLoader->document()->frame(), this, m_resourceRequest, m_resourceRequest.priority(), options);
-    if (!m_loader || m_loader->reachedTerminalState()) {
+    if (!m_loader) {
         // FIXME: What if resources in other frames were waiting for this revalidation?
         LOG(ResourceLoading, "Cannot start loading '%s'", url().string().latin1().data());
@@ -225 +225 @@
 
     m_status = Pending;
-    cachedResourceLoader->incrementRequestCount(this);
 }