Bug 62308 – CachedResourceLoader doesn't need to know about CachedResourceRequest

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 96462 [details]
patch

[Adam Barth]

Comment on attachment 96462 [details]
patch

At first glance, this patch looks very cool.  I need to study it in more detail.

[Nate Chapin]

(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.

[Adam Barth]

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;

[Nate Chapin]

(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;

[Adam Barth]

Comment on attachment 96462 [details]
patch

Ok.  I'm sold.

[Nate Chapin]

http://trac.webkit.org/changeset/88391

[Alexey Proskuryakov]

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.

[Adam Barth]

(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" ?

[Nate Chapin]

(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).