Bug 63286 – Add NSURLResponse wrapper in ResourceResponse when USE(CFNETWORK) is enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

This is part of the move to the CFNetwork based loader on Mac - bug 51836. In order to send back NSURLResponse objects to WebKit on Mac, we need to have a wrapper NSURLResponse in ResourceResponse.

[Pratik Solanki]

Created attachment 98396 [details]
Patch

[Pratik Solanki]

Created attachment 98399 [details]
Patch

[Pratik Solanki]

Created attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses

[Jessie Berlin]

Comment on attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses

Unofficial r=me!

[David Kilzer (:ddkilzer)]

Comment on attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses

View in context: https://bugs.webkit.org/attachment.cgi?id=98943&action=review

r=me

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:56
> +    // Work around a mistake in the NSURLResponse class.

Would be nice to document this as <rdar://problem/3346574> (per our discussion on IRC).

> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:77
> +    if (!m_cfResponse)
> +        return nil;

Is there ever a case where you have an m_nsResponse but not a m_cfResponse (in which case you could make the m_cfResponse from the m_nsResponse)?  Or is this ASSERT-ed elsewhere during construction so it's not possible to get into this state?

Could some of this logic be simplified by doing an early return if m_isNull is true:

    if (m_isNull)
        return nil;

[Pratik Solanki]

Comment on attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses

View in context: https://bugs.webkit.org/attachment.cgi?id=98943&action=review

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:77
>> +        return nil;
> 
> Is there ever a case where you have an m_nsResponse but not a m_cfResponse (in which case you could make the m_cfResponse from the m_nsResponse)?  Or is this ASSERT-ed elsewhere during construction so it's not possible to get into this state?
> 
> Could some of this logic be simplified by doing an early return if m_isNull is true:
> 
>     if (m_isNull)
>         return nil;

Hmm. I can't recall the exact reasons for the way the code is. I know I had updated this part of the patch because of some failures in layout tests. But this does feel a bit more complicated than it needs to be. I should be able to simplify this I think. I'll check this in right now and look into simplification in bug 64342.

[Pratik Solanki]

Committed r90825: <http://trac.webkit.org/changeset/90825>