Bug 185105 – ResourceHandleCFURLConnectionDelegateWithOperationQueue::didReceiveResponse() has comment that states we should avoid sniffing for HTTP 304, but we no longer do

• Component: Platform
• Status: NEW

[Daniel Bates]

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?

[Daniel Bates]

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>

[Radar WebKit Bug Importer]

<rdar://problem/39813370>