Summary: | CachedResourceLoader doesn't need to know about CachedResourceRequest | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nate Chapin <japhet> | ||||
Component: | WebCore Misc. | Assignee: | Nate Chapin <japhet> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, ap, koivisto | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Nate Chapin
2011-06-08 12:42:45 PDT
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.
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). |