Summary: | Add NSURLRequest wrapper in ResourceRequest 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 13:09:32 PDT
Created attachment 98388 [details]
Patch
Created attachment 98942 [details]
Patch v2 - adds null check in updateNSURLRequest
Comment on attachment 98942 [details] Patch v2 - adds null check in updateNSURLRequest View in context: https://bugs.webkit.org/attachment.cgi?id=98942&action=review Unofficial r=me! > Source/WebCore/platform/network/cf/ResourceRequest.h:124 > + mutable RetainPtr<CFURLRequestRef> m_cfRequest; Why does this need to be mutable? > Source/WebCore/platform/network/cf/ResourceRequest.h:127 > + mutable RetainPtr<NSURLRequest> m_nsRequest; Same question here > Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:77 > +#if PLATFORM(MAC) This would be cleaner as a #elif PLATFORM(MAC) > Source/WebCore/platform/network/mac/ResourceRequestMac.mm:69 > + m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:m_cfRequest.get()]); It is unfortunate that we have to allocate a new NSURLRequest every time we update it, since doUpdatePlatformRequest is called quite often. We are already doing something similar in ResourceRequestCFNet.cpp. However, I don't think that issue needs to be addressed in this patch. Comment on attachment 98942 [details] Patch v2 - adds null check in updateNSURLRequest View in context: https://bugs.webkit.org/attachment.cgi?id=98942&action=review >> Source/WebCore/platform/network/cf/ResourceRequest.h:124 >> + mutable RetainPtr<CFURLRequestRef> m_cfRequest; > > Why does this need to be mutable? Hmm. doesn't look like its needed anymore. I'll remove it. >> Source/WebCore/platform/network/cf/ResourceRequest.h:127 >> + mutable RetainPtr<NSURLRequest> m_nsRequest; > > Same question here I'll remove the mutable here as well. >> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:77 >> +#if PLATFORM(MAC) > > This would be cleaner as a #elif PLATFORM(MAC) ok >> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:69 >> + m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:m_cfRequest.get()]); > > It is unfortunate that we have to allocate a new NSURLRequest every time we update it, since doUpdatePlatformRequest is called quite often. We are already doing something similar in ResourceRequestCFNet.cpp. However, I don't think that issue needs to be addressed in this patch. Well, we create a CFURLRequestRef in doUpdatePlatformRequest() so we have to create an NSURLRequest as well. Comment on attachment 98942 [details] Patch v2 - adds null check in updateNSURLRequest View in context: https://bugs.webkit.org/attachment.cgi?id=98942&action=review r=me > Source/WebCore/platform/network/cf/ResourceRequest.h:79 > + void updateNSURLRequest() { } In one part of the patch, the call to updateNSURLRequest() is protected by #if PLATFORM(MAC)/#endif, and in another place it's not. I don't think this method should be defined, then just add #if PLATFORM(MAC)/#endif where it's called (which is one more place below). > Source/WebCore/platform/network/cf/ResourceRequest.h:86 > + updateNSURLRequest(); Add #if PLATFORM(MAC)/#endif here. >>> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:69 >>> + m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:m_cfRequest.get()]); >> >> It is unfortunate that we have to allocate a new NSURLRequest every time we update it, since doUpdatePlatformRequest is called quite often. We are already doing something similar in ResourceRequestCFNet.cpp. However, I don't think that issue needs to be addressed in this patch. > > Well, we create a CFURLRequestRef in doUpdatePlatformRequest() so we have to create an NSURLRequest as well. Will the NSURLRequest go away completely if PLATFORM(MAC) switches to USE(CFNETWORK)? Comment on attachment 98942 [details] Patch v2 - adds null check in updateNSURLRequest View in context: https://bugs.webkit.org/attachment.cgi?id=98942&action=review >> Source/WebCore/platform/network/cf/ResourceRequest.h:79 >> + void updateNSURLRequest() { } > > In one part of the patch, the call to updateNSURLRequest() is protected by #if PLATFORM(MAC)/#endif, and in another place it's not. > > I don't think this method should be defined, then just add #if PLATFORM(MAC)/#endif where it's called (which is one more place below). Ok. >>>> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:69 >>>> + m_nsRequest.adoptNS([[NSURLRequest alloc] _initWithCFURLRequest:m_cfRequest.get()]); >>> >>> It is unfortunate that we have to allocate a new NSURLRequest every time we update it, since doUpdatePlatformRequest is called quite often. We are already doing something similar in ResourceRequestCFNet.cpp. However, I don't think that issue needs to be addressed in this patch. >> >> Well, we create a CFURLRequestRef in doUpdatePlatformRequest() so we have to create an NSURLRequest as well. > > Will the NSURLRequest go away completely if PLATFORM(MAC) switches to USE(CFNETWORK)? No, WebKit needs the NSURLRequest on Mac. The way these patches work is that they expose the obj-c classes to WebKit while using CF code for the actual networking in WebCore. So we'll need NSURLRequest. Committed r90807: <http://trac.webkit.org/changeset/90807> |