[webkit-reviews] review denied: [Bug 101300] Store the initiating CachedResourceRequest : [Attachment 172487] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied James Simonsen
<simonjam at chromium.org>'s request for review:
Bug 101300: Store the initiating CachedResourceRequest
https://bugs.webkit.org/show_bug.cgi?id=101300

Attachment 172487: Patch
https://bugs.webkit.org/attachment.cgi?id=172487&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172487&action=review


> Source/WebCore/loader/SubresourceLoader.cpp:206
> -	   // 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" 
> +	   // 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"

The only changes to this file appear to be whitespace changes.

> Source/WebCore/loader/cache/CachedResource.cpp:360
> -    
> +

Can you post a version of this patch that doesn't contain all these whitespace
changes.  They're really distracting.

> Source/WebCore/loader/cache/CachedResource.h:235
> -    
> +

More noise.  :(

> Source/WebCore/loader/cache/CachedResource.h:365
> +    OwnPtr<CachedResourceRequest> m_initialRequest;

How can the CachedResource hold the request?  I thought the whole point of this
approach is that there were many CachedResourceRequests that would lead to a
single CachedResource so we decided to store the initiator state on the request
instead of the resource.

> Source/WebCore/loader/cache/CachedResourceRequest.h:74
> +    RefPtr<Element> m_initiatorElement;
> +    RefPtr<Document> m_initiatorDocument;

This seems very likely to create a reference cycle.  Certainly the way this
patch has CachedResource holding the CachedResourceRequest, that means the
MemoryCache (with its static lifetime) is transitively holding the entire
document.....  I don't understand how we're managing the lifetimes correctly in
this patch.  Perhaps it's worth explaining the design in the ChangeLog?