Bug 63276 – Add NSURLRequest wrapper in ResourceRequest 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 NSURLRequest objects to WebKit on Mac, we need to have a wrapper NSURLRequest in ResourceRequest.

[Pratik Solanki]

Created attachment 98388 [details]
Patch

[Pratik Solanki]

Created attachment 98942 [details]
Patch v2 - adds null check in updateNSURLRequest

[Jessie Berlin]

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.

[Pratik Solanki]

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.

[David Kilzer (:ddkilzer)]

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)?

[Pratik Solanki]

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.

[Pratik Solanki]

Committed r90807: <http://trac.webkit.org/changeset/90807>