Bug 158967

Summary: Upstream WKHTTPCookiesForURL from WebKitSystemInterface to OpenSource
Product: WebKit Reporter: Amir Alavi <aalavi>
Component: New BugsAssignee: Amir Alavi <aalavi>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, commit-queue
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Amir Alavi 2016-06-20 18:27:18 PDT
Upstream WKHTTPCookiesForURL from WebKitSystemInterface to OpenSource
Comment 1 Amir Alavi 2016-06-20 18:40:14 PDT
Created attachment 281691 [details]
Patch
Comment 2 Andy Estes 2016-06-20 18:47:59 PDT
Comment on attachment 281691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281691&action=review

r=me with a few comments.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:60
> +    CFArrayRef cookies = _CFHTTPCookieStorageCopyCookiesForURLWithMainDocumentURL(cookieStorage, static_cast<CFURLRef>(url), static_cast<CFURLRef>(firstParty), secure);
> +
> +    NSArray *nsCookies = [NSHTTPCookie _cf2nsCookies:cookies];
> +    CFRelease(cookies);

While we're here, let's change this to use a RetainPtr:

    auto cookies = adoptCF(_CFHTTPCookieStorageCopyCookiesForURLWithMainDocumentURL(cookieStorage, static_cast<CFURLRef>(url), static_cast<CFURLRef>(firstParty), secure));
    NSArray *nsCookies = [NSHTTPCookie _cf2nsCookies:cookies.get()];

Then get rid of the call to CFRelease().

> Source/WebCore/platform/network/mac/CookieJarMac.mm:253
> -    NSArray *cookies = wkHTTPCookiesForURL(cookieStorage.get(), 0, url);
> +    NSArray *cookies = httpCookiesForURL(cookieStorage.get(), 0, url);

Also while we're here, we should pass nil as the second argument to httpCookiesForURL() instead of 0.

> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:224
> ++ (NSArray*)_cf2nsCookies:(CFArrayRef)cfCookies;

There should be a space between the NSArray and the *.
Comment 3 Andy Estes 2016-06-20 20:02:37 PDT
rdar://problem/26630073
Comment 4 Alexey Proskuryakov 2016-06-20 23:59:06 PDT
*** Bug 158966 has been marked as a duplicate of this bug. ***
Comment 5 Alexey Proskuryakov 2016-06-20 23:59:09 PDT
*** Bug 158964 has been marked as a duplicate of this bug. ***
Comment 6 Alexey Proskuryakov 2016-06-20 23:59:12 PDT
*** Bug 158965 has been marked as a duplicate of this bug. ***
Comment 7 Alexey Proskuryakov 2016-06-21 00:03:06 PDT
Comment on attachment 281691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281691&action=review

> Source/WebCore/platform/network/mac/CookieJarMac.mm:51
> +        // FIXME: The fallback to NSHTTPCookieStorage should not be present when USE(CFNETWORK) is defined in WebKit.

Now that this code is in WebKit, " in WebKit" should be removed.
Comment 8 Brent Fulgham 2016-06-21 09:42:35 PDT
Comment on attachment 281691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281691&action=review

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:51
>> +        // FIXME: The fallback to NSHTTPCookieStorage should not be present when USE(CFNETWORK) is defined in WebKit.
> 
> Now that this code is in WebKit, " in WebKit" should be removed.

Fixed while landing.

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:60
>> +    CFRelease(cookies);
> 
> While we're here, let's change this to use a RetainPtr:
> 
>     auto cookies = adoptCF(_CFHTTPCookieStorageCopyCookiesForURLWithMainDocumentURL(cookieStorage, static_cast<CFURLRef>(url), static_cast<CFURLRef>(firstParty), secure));
>     NSArray *nsCookies = [NSHTTPCookie _cf2nsCookies:cookies.get()];
> 
> Then get rid of the call to CFRelease().

Fixed while landing.

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:253
>> +    NSArray *cookies = httpCookiesForURL(cookieStorage.get(), 0, url);
> 
> Also while we're here, we should pass nil as the second argument to httpCookiesForURL() instead of 0.

Fixed while landing.
Comment 9 Brent Fulgham 2016-06-21 09:43:27 PDT
Comment on attachment 281691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281691&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

What tests did we run that confirm that this change didn't break anything? Obviously it shouldn't since we are just moving code around -- but what cookie tests actually exercise this code?
Comment 10 Amir Alavi 2016-06-21 09:49:40 PDT
Created attachment 281755 [details]
Patch
Comment 11 Brent Fulgham 2016-06-21 09:51:11 PDT
(In reply to comment #9)
> Comment on attachment 281691 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281691&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> What tests did we run that confirm that this change didn't break anything?
> Obviously it shouldn't since we are just moving code around -- but what
> cookie tests actually exercise this code?

EWS shows all tests continue to pass, but it would be nice to mention one or two tests that are known to exercise the 'filterCookies' and 'deleteCookies' methods.
Comment 12 Brent Fulgham 2016-06-21 09:51:47 PDT
Comment on attachment 281755 [details]
Patch

r=me
Comment 13 WebKit Commit Bot 2016-06-21 10:20:18 PDT
Comment on attachment 281755 [details]
Patch

Clearing flags on attachment: 281755

Committed r202279: <http://trac.webkit.org/changeset/202279>
Comment 14 WebKit Commit Bot 2016-06-21 10:20:22 PDT
All reviewed patches have been landed.  Closing bug.