185105 2018-04-27 20:55:43 -0700 ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse() has comment that states we should avoid sniffing for HTTP 304, but we no longer do 2018-04-27 21:57:59 -0700 1 1 1 Unclassified WebKit Platform WebKit Nightly Build PC Windows 10 NEW https://bugs.webkit.org/show_bug.cgi?id=160677 https://bugs.webkit.org/show_bug.cgi?id=179688 https://bugs.webkit.org/show_bug.cgi?id=185107 InRadar, PlatformOnly P2 Normal --- 1 dbates webkit-unassigned achristensen ap beidson bfulgham dbates koivisto pvollan webkit-bug-importer oldest_to_newest 1418750 0 dbates 2018-04-27 20:55:43 -0700 In ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse() we have the following confusing code snippet: [[ ... // Avoid MIME type sniffing if the response comes back as 304 Not Modified. auto msg = CFURLResponseGetHTTPResponse(cfResponse.get()); int statusCode = msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0; if (statusCode != 304) { bool isMainResourceLoad = m_handle->firstRequest().requester() == ResourceRequest::Requester::MainFrame; } ... ]] <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp?rev=224846#L191> This code no longer matches the comment. What should we do with this code? 1418751 1 dbates 2018-04-27 21:06:36 -0700 Q. How did this happen? A. Before <https://trac.webkit.org/changeset/224267/> (bug #160677) the code snippet read: [[ ... // Avoid MIME type sniffing if the response comes back as 304 Not Modified. auto msg = CFURLResponseGetHTTPResponse(cfResponse.get()); int statusCode = msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0; if (statusCode != 304) { bool isMainResourceLoad = handle->firstRequest().requester() == ResourceRequest::Requester::Main; adjustMIMETypeIfNecessary(cfResponse.get(), isMainResourceLoad); } ... ]] <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp?rev=224266#L158> After <https://trac.webkit.org/changeset/224267/> (bug #160677) the code changed to read: [[ ... // Avoid MIME type sniffing if the response comes back as 304 Not Modified. auto msg = CFURLResponseGetHTTPResponse(cfResponse.get()); int statusCode = msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0; if (statusCode != 304) { bool isMainResourceLoad = handle->firstRequest().requester() == ResourceRequest::Requester::Main; #if !PLATFORM(WIN) adjustMIMETypeIfNecessary(cfResponse.get(), isMainResourceLoad); #endif } ... ]] <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp?rev=224267#L193> I am unclear why the !PLATFORM(WIN)-guard was added at all as this code does not seem specific to Cocoa platforms. Moreover, I am unclear why the !PLATFORM(WIN)-guard was added only around adjustMIMETypeIfNecessary() and not this entire snippet as the locals msg, statusCode, and isMainResourceLoad exist solely so that we can write the control flow logic to conditionally call adjustMIMETypeIfNecessary(). I suspect the existence of the !PLATFORM(WIN)-guard caused confusion and hence we removed the guarded code, but did not remove all the aforementioned locals, in <http://trac.webkit.org/changeset/224846> (bug #179688) and hence this code snippet became what it is today, repeating the snippet from comment 0: [[ ... // Avoid MIME type sniffing if the response comes back as 304 Not Modified. auto msg = CFURLResponseGetHTTPResponse(cfResponse.get()); int statusCode = msg ? CFHTTPMessageGetResponseStatusCode(msg) : 0; if (statusCode != 304) { bool isMainResourceLoad = m_handle->firstRequest().requester() == ResourceRequest::Requester::MainFrame; } ... ]] <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/network/cf/ResourceHandleCFURLConnectionDelegateWithOperationQueue.cpp?rev=224846#L191> 1418754 2 webkit-bug-importer 2018-04-27 21:10:33 -0700 <rdar://problem/39813370>