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
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Pratik Solanki
:
Depends on:
Blocks: 51836
  Show dependency treegraph
 
Reported: 2011-06-23 14:01 PDT by Pratik Solanki
Modified: 2011-07-12 10:51 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-06-23 14:01:38 PDT
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 Pratik Solanki 2011-06-23 14:05:10 PDT
Created attachment 98396 [details]
Patch
Comment 2 Pratik Solanki 2011-06-23 14:14:03 PDT
Created attachment 98399 [details]
Patch
Comment 3 Pratik Solanki 2011-06-28 11:08:13 PDT
Created attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses
Comment 4 Jessie Berlin 2011-07-11 11:25:16 PDT
Comment on attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses

Unofficial r=me!
Comment 5 David Kilzer (:ddkilzer) 2011-07-11 11:42:10 PDT
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;
Comment 6 Pratik Solanki 2011-07-11 22:31:28 PDT
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.
Comment 7 Pratik Solanki 2011-07-12 10:51:39 PDT
Committed r90825: <http://trac.webkit.org/changeset/90825>