Currently, CachedResourceLoader only uses CachedResourceRequests to keep them alive (it holds the primary RefPtr) and to cancel them (CachedResourceLoader::cancelRequests()). It appears cancelRequests() isn't strictly necessary (DocumentLoader cancels all its subresources, which accomplishes the same thing), and I can't think of a good reason not to let CachedResource own the CachedResourceRequest (especially since the same CachedResourceRequest may be used in multiple Document). Patch shortly.
Created attachment 96462 [details] patch
Comment on attachment 96462 [details] patch At first glance, this patch looks very cool. I need to study it in more detail.
(In reply to comment #2) > (From update of attachment 96462 [details]) > At first glance, this patch looks very cool. I need to study it in more detail. The main thing I'm concerned about is that I'm removing CachedResourceLoader::m_loadDoneActionTimer. It appears it's not doing anything on trunk (since loadDone() shouldn't be getting called with a null CachedResourceRequest anymore), but it's not 100% clear to me whether or not it should be used.
Comment on attachment 96462 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=96462&action=review > Source/WebCore/loader/cache/CachedResourceLoader.cpp:-604 > - for (unsigned i = 0; i < requestsToCancel.size(); ++i) > - requestsToCancel[i]->didFail(true); Where did this didFail call go? It looks like we're supposed to call didFail on requests when the Document dies. > Source/WebCore/loader/cache/CachedResourceRequest.cpp:131 > - return 0; > + return PassOwnPtr<CachedResourceRequest>(); return nullptr;
(In reply to comment #4) > (From update of attachment 96462 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96462&action=review > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:-604 > > - for (unsigned i = 0; i < requestsToCancel.size(); ++i) > > - requestsToCancel[i]->didFail(true); > > Where did this didFail call go? It looks like we're supposed to call didFail on requests when the Document dies. Currently, we cancel CachedResourceRequests and sever their connection from their SubresourceLoader, but don't actually cancel the SubresourceLoader until later. This merges the two cancellation paths. My attempt at a trac stacktrace (tractrace?): http://trac.webkit.org/browser/trunk/Source/WebCore/loader/SubresourceLoader.cpp#L230 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/ResourceLoader.cpp#L363 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentLoader.cpp#L78 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentLoader.cpp#L756 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/DocumentLoader.cpp#L262 > > > Source/WebCore/loader/cache/CachedResourceRequest.cpp:131 > > - return 0; > > + return PassOwnPtr<CachedResourceRequest>(); > > return nullptr;
Comment on attachment 96462 [details] patch Ok. I'm sold.
http://trac.webkit.org/changeset/88391
I feel that these classes may be getting progressively misnamed. When looking at names alone, of course a loader _should_ know about its request! I'm not commenting on this particular patch here.
(In reply to comment #8) > I feel that these classes may be getting progressively misnamed. When looking at names alone, of course a loader _should_ know about its request! I'm not commenting on this particular patch here. Do you think the problem is that CachedResourceLoader shouldn't be called a "loader" or that CachedResourceRequest shouldn't be called a "request" ?
(In reply to comment #9) > (In reply to comment #8) > > I feel that these classes may be getting progressively misnamed. When looking at names alone, of course a loader _should_ know about its request! I'm not commenting on this particular patch here. > > Do you think the problem is that CachedResourceLoader shouldn't be called a "loader" or that CachedResourceRequest shouldn't be called a "request" ? I've actually been wondering about that too (CachedResourceLoader specifically).