[webkit-reviews] review denied: [Bug 76564] Let MemoryCache reuse cached XHRs : [Attachment 122979] First draft

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 76564: Let MemoryCache reuse cached XHRs
https://bugs.webkit.org/show_bug.cgi?id=76564

Attachment 122979: First draft
https://bugs.webkit.org/attachment.cgi?id=122979&action=review

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


This is looking really close.  I think we should just polish up some of the
memory management idioms.

>>> Source/WebCore/loader/cache/CachedRawResource.cpp:92
>>> +	 CachedResourceClient* m_client;
>> 
>> What keeps the client alive?
> 
> That part is a little subtle, and I haven't figured out a clear way to
document it. The first thing CachedRawResource::sendCallbacks() does is see if
the CachedResourceClient* is still in the resource's list of clients. If it's
in the client list, it should be guaranteed to be alive. If it's not, we
shouldn't be sending callbacks to it, regardless of whether it's alive.

I'm not sure that's exactly right.  If m_client gets deallocated and another
CachedResourceClient gets allocated in its place, we could be in trouble,
right?

> Source/WebCore/loader/cache/CachedRawResource.cpp:100
> +    static_cast<CachedRawResourceClient*>(c)->responseReceived(this,
m_response);

I would make a local variable of type CachedRawResourceClient to avoid having
to cast this variable so many times.