[webkit-reviews] review granted: [Bug 63276] Add NSURLRequest wrapper in ResourceRequest when USE(CFNETWORK) is enabled : [Attachment 98942] Patch v2 - adds null check in updateNSURLRequest

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 63276: Add NSURLRequest wrapper in ResourceRequest when USE(CFNETWORK) is
enabled
https://bugs.webkit.org/show_bug.cgi?id=63276

Attachment 98942: Patch v2 - adds null check in updateNSURLRequest
https://bugs.webkit.org/attachment.cgi?id=98942&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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)?