Bug 63286 - Add NSURLResponse wrapper in ResourceResponse when USE(CFNETWORK) is enabled
: Add NSURLResponse wrapper in ResourceResponse when USE(CFNETWORK) is enabled
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 51836
  Show dependency treegraph
 
Reported: 2011-06-23 14:01 PST by
Modified: 2011-07-12 10:51 PST (History)


Attachments
Patch (6.45 KB, patch)
2011-06-23 14:05 PST, Pratik Solanki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.45 KB, patch)
2011-06-23 14:14 PST, Pratik Solanki
no flags Review Patch | Details | Formatted Diff | Diff
Patch v3 - fixes some layout tests by creating correct NSURLResponses (6.52 KB, patch)
2011-06-28 11:08 PST, Pratik Solanki
ddkilzer: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-06-23 14:01:38 PST
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.
------- Comment #1 From 2011-06-23 14:05:10 PST -------
Created an attachment (id=98396) [details]
Patch
------- Comment #2 From 2011-06-23 14:14:03 PST -------
Created an attachment (id=98399) [details]
Patch
------- Comment #3 From 2011-06-28 11:08:13 PST -------
Created an attachment (id=98943) [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses
------- Comment #4 From 2011-07-11 11:25:16 PST -------
(From update of attachment 98943 [details])
Unofficial r=me!
------- Comment #5 From 2011-07-11 11:42:10 PST -------
(From update of attachment 98943 [details])
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;
------- Comment #6 From 2011-07-11 22:31:28 PST -------
(From update of attachment 98943 [details])
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.
------- Comment #7 From 2011-07-12 10:51:39 PST -------
Committed r90825: <http://trac.webkit.org/changeset/90825>