Bug 185107 – NetworkResourceLoader::isMainResource() is a misnomer; it is only true for a main frame initiated load

• Component: WebKit Misc.
• Status: RESOLVED INVALID

[Daniel Bates]

NetworkResourceLoader::isMainResource() is a misnomer. It only returns true if the request is for the main frame. Throughout the code base the term "main resource" is known to be the document that is loaded in a frame. Another way to say this, every frame has exactly one main resource, including the top-most frame ("main frame"). However, NetworkResourceLoader::isMainResource() is currently defined to be:

[[
    bool isMainResource() const { return m_parameters.request.requester() == WebCore::ResourceRequest::Requester::Main; }
]]
<https://trac.webkit.org/browser/trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.h?rev=231110#L107>

So, NetworkResourceLoader::isMainResource() is only true for a main resource of the main frame. This is confusing. We should rename NetworkResourceLoader::isMainResource() to better descibre its purpose. Maybe NetworkResourceLoader::isMainFrameLoader()? NetworkResourceLoader::isMainFrameMainResource()? or NetworkResourceLoader::initiatedByMainFrame()?

[Daniel Bates]

Created attachment 339059 [details]
Patch

[EWS Watchlist]

Attachment 339059 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:166:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

I am open to naming suggestions. The proposed patch renames NetworkResourceLoader::isMainResource() to NetworkResourceLoader::isMainFrameLoader(). I also renamed ResourceRequest::Requester::Main to ResourceRequest::Requester::MainFrame to help clarify that the requester is the "main frame" as opposed to an arbitrary main resource (that may or may not be for the main frame).

While I was performing the renames I came across code that looks incorrect as the intention of the code looks like it would be applicable to all main resources. However it was coded with respect to ResourceRequest::Requester::Main, which is only set for the main frame's main resource. I did not fix up such code. Further investigation is required. The following are the most suspicious functions that have such code:

WebCore::adjustMIMETypeIfNecessary():
    <https://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/ios/WebCoreURLResponseIOS.mm?rev=228531#L52>

WebKit::NetworkCache::SpeculativeLoadManager::registerLoad():
    <https://trac.webkit.org/browser/trunk/Source/WebKit/NetworkProcess/cache/NetworkCacheSpeculativeLoadManager.cpp?rev=230944#L373>

WebKit::ServiceWorkerClientFetch::didReceiveResponse():
    <https://trac.webkit.org/browser/trunk/Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp?rev=229774#L143>

We should consider auditing all the functions in this patch that were affected by the renames for correctness.

[Daniel Bates]

Comment on attachment 339059 [details]
Patch

Wow, I am not sure what I was thinking yesterday.