[webkit-reviews] review granted: [Bug 55435] WebKit2: Use CFNetwork Sessions API : [Attachment 84605] Patch (Part 2)

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Jessie Berlin
<jberlin at webkit.org>'s request for review:
Bug 55435: WebKit2: Use CFNetwork Sessions API
https://bugs.webkit.org/show_bug.cgi?id=55435

Attachment 84605: Patch (Part 2)
https://bugs.webkit.org/attachment.cgi?id=84605&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84605&action=review

r=me, but you need to fix a leak before committing.

> Source/WebCore/platform/network/cf/ResourceRequestCFNet.cpp:202
> +    CFMutableURLRequestRef cfRequest = CFURLRequestCreateMutableCopy(0,
m_cfRequest.get());
> +    wkSetStorageSessionOnRequest(storageSession, cfRequest);
> +    m_cfRequest.adoptCF(cfRequest);

Why not do the adoption on the first line?
m_cfRequest.adoptCF(CFURLRequestCreateMutableCopy(...))

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:243
> +    if (CFURLStorageSessionRef storageSession =
privateBrowsingStorageSession())
> +	   nsRequest = [wkRequestWithStorageSessionSet(storageSession,
nsRequest) retain];

Looks like you're leaking the new request.

> Source/WebCore/platform/network/mac/ResourceRequestMac.mm:179
> +    m_nsRequest.adoptNS([wkRequestWithStorageSessionSet(storageSession,
m_nsRequest.get()) retain]);

No need for the adoptNS+retain. You can just use assignment.