Summary: | Add NSURLResponse wrapper in ResourceResponse when USE(CFNETWORK) is enabled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pratik Solanki <psolanki> | ||||||||
Component: | Platform | Assignee: | Pratik Solanki <psolanki> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, beidson, darin, ddkilzer, jberlin, koivisto, psolanki | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 51836 | ||||||||||
Attachments: |
|
Description
Pratik Solanki
2011-06-23 14:01:38 PDT
Created attachment 98396 [details]
Patch
Created attachment 98399 [details]
Patch
Created attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses
Comment on attachment 98943 [details]
Patch v3 - fixes some layout tests by creating correct NSURLResponses
Unofficial r=me!
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 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. Committed r90825: <http://trac.webkit.org/changeset/90825> |