Bug 162963

Created attachment 290703 [details] Patch

The name FetchedResourceRequest is pretty long though. Here are some other alternatives: - Request (but maybe too short...) - FetchResourceRequest - FetchRequest (but we would need to rename FetchRequest to something like FetchAPIRequest)

Created attachment 290705 [details] Patch

Are CachedResourceRequests still used in the MemoryCache?

(In reply to comment #4) > Are CachedResourceRequests still used in the MemoryCache? They are currently passed to CachedResource constructor to initialise it, in particular moving the ResourceRequest. The idea behind this patch is that CachedResourceLoader is the entry point of the fetch algorithm. That is probably where we would like to intercept requests and forward them to the service worker, should we implement it. MemoryCache is only a part of CachedResourceLoader duty, which also handles all the fetch options to generate the exact request. CachedResourceLoader should therefore take a FetchRequest as input, return a FetchResource and be renamed to something like ResourceFetcher. The idea would also be to rename most of the CachedXX classes. If we can rename FetchRequest to Request (Request is the name by which it is exposed on JS scripts), we could contemplate renaming FetchedResourceRequest to the simpler FetchRequest.

I think renaming the entire set of Cached* classes to be prefixed by Fetch hurts readability and clarity in the code base. I don't necessarily think "CachedResourceRequest" is a great name, but I'm certain FetchedResourceRequest is a bad one as long as it serves duty for a mechanism that is not the Fetch API. Fetch is an exception to how resources are loaded and stored in memory/on disk; not the rule.

(In reply to comment #6) > I think renaming the entire set of Cached* classes to be prefixed by Fetch > hurts readability and clarity in the code base. I agree there is less incentive to do so for CachedResource and relatives. I disagree for CachedResourceLoader and CachedResourceRequest. > I don't necessarily think "CachedResourceRequest" is a great name, but I'm > certain FetchedResourceRequest is a bad one as long as it serves duty for a > mechanism that is not the Fetch API. We might be talking of two different fetch then :) The general mechanism is called fetch algorithm. Fetch API is one client amongst others (XHR, HTML...) of the fetch algorithm. FetchOptions was introduced in Source/WebCore/loader for the purpose of the fetch algorithm. This allows clients of CachedResourceLoader to start setting fetch options like defined in HTML specs. Let's take XHR as an example. https://xhr.spec.whatwg.org/#the-send()-method step 6 says: Let req be a new request, initialized as follows: ... mode: "cors" credentials mode: If the withCredentials attribute value is true, "include", and "same-origin" otherwise. XMLHttpRequest::createRequest does: options.credentials = m_includeCredentials ? FetchOptions::Credentials::Include : FetchOptions::Credentials::SameOrigin; options.mode = FetchOptions::Mode::Cors; > Fetch is an exception to how resources are loaded and stored in memory/on > disk; not the rule. For loader clients and in the HTML specs, the notion of memory/on disk cache is not very relevant. Whenever a resource needs to be loaded, the term coined is "fetch a resource" or "fetch a request". It would help to mirror this in the code. I would like to rename CachedResourceLoader/CachedResourceRequest to something that: - Has Fetch in it - Does not have Cached in it - Does not have Loader in it For CachedResource and relatives, this is less an issue, although I do not see what the Cached prefix brings.

Two additional pointers: https://fetch.spec.whatwg.org/#requests maps to CachedResourceRequest https://fetch.spec.whatwg.org/#request-class maps to FetchRequest

Can we make progress on this? I am fine renaming CachedResourceRequest to FetchRequest (and then rename FetchRequest to FetchAPIRequest or just Request).

Comment on attachment 290705 [details] Patch Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form. If this patch is still important please rebase it and post it for review again.

Youenn, Brady, Is this still relevant?