Bug 185107

Summary: NetworkResourceLoader::isMainResource() is a misnomer; it is only true for a main frame initiated load
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED INVALID    
Severity: Normal CC: achristensen, aestes, beidson, cdumez, cgarcia, ews-watchlist, japhet, joepeck, youennf
Priority: P2    
Version: WebKit Local Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185105
Attachments:
Description Flags
Patch dbates: review-

Daniel Bates
Reported 2018-04-27 21:29:10 PDT
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()?
Attachments
Patch (26.85 KB, patch)
2018-04-27 21:47 PDT, Daniel Bates 		dbates: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bates
Comment 1 2018-04-27 21:47:22 PDT
Created attachment 339059 [details] Patch
EWS Watchlist
Comment 2 2018-04-27 21:50:22 PDT
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
Comment 3 2018-04-27 21:56:55 PDT
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 4 2018-04-28 15:23:05 PDT
Comment on attachment 339059 [details] Patch Wow, I am not sure what I was thinking yesterday.
Note You need to log in before you can comment on or make changes to this bug.