[webkit-reviews] review canceled: [Bug 51836] WebCore should use CFNetwork-based loader on Mac : [Attachment 85776] Patch 1.3 - Update patch to ToT

[bugzilla-daemon at webkit.org]

Pratik Solanki <psolanki at apple.com> has canceled Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 51836: WebCore should use CFNetwork-based loader on Mac
https://bugs.webkit.org/show_bug.cgi?id=51836

Attachment 85776: Patch 1.3 - Update patch to ToT
https://bugs.webkit.org/attachment.cgi?id=85776&action=review

------- Additional Comments from Pratik Solanki <psolanki at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85776&action=review

>> Source/WebCore/loader/ResourceLoader.h:122
>>	    virtual bool shouldCacheResponse(ResourceHandle*,
CFCachedURLResponseRef);
> 
> We should have a bug about unifying these calls (the Windows and Mac
CFNetwork versions).

Filed https://bugs.webkit.org/show_bug.cgi?id=57257

>> Source/WebCore/platform/mac/SoftLinking.h:72
>> +/* callingConvention is unused on Mac but is here to keep the macro
prototype the same between Mac and Windows. */
> 
> I am not sure why having the macro prototype be the same between Mac and
Windows is useful. We can't actually share the code, and the calls to
SOFT_LINK_OPTIONAL will have to be in separate #ifdefs anyways.

This helps keep the SOFT_LINK_OPTIONAL from being in separate ifdefs. This was
based on feedback from Darin on previous patch.

>> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:52
>> +RetainPtr<CFHTTPCookieStorageRef>& privateBrowsingCookieStorage()
> 
> Why did you remove the static modifier here?

Because (I think) it was being called from elsewhere in the code. This part of
the patch has changed.

>> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:33
>> +#include <CFNetwork/CFURLRequestPriv.h>
> 
> This #include will probably fail on non-internal machines (that header is not
public).

Right. I'll have to fix all uses of the headers. Will do that in a separate
patch later.

>> Source/WebCore/platform/network/mac/ResourceResponseMac.mm:54
>> +	// Work around a mistake in the NSURLResponse class.
> 
> Is there a bug / radar for this mistake and something tracking changing our
implementation once it is fixed?

Not that I know of. This was code from ResourceResponse::nsURLResponse that I
refactored.