Bug 158967 – Upstream WKHTTPCookiesForURL from WebKitSystemInterface to OpenSource

• Component: New Bugs
• Status: RESOLVED FIXED

[Amir Alavi]

Upstream WKHTTPCookiesForURL from WebKitSystemInterface to OpenSource

[Amir Alavi]

Created attachment 281691 [details]
Patch

[Andy Estes]

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 *.

[Andy Estes]

rdar://problem/26630073

[Alexey Proskuryakov]

*** Bug 158966 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 158964 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

*** Bug 158965 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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?

[Amir Alavi]

Created attachment 281755 [details]
Patch

[Brent Fulgham]

(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.

[Brent Fulgham]

Comment on attachment 281755 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 281755 [details]
Patch

Clearing flags on attachment: 281755

Committed r202279: <http://trac.webkit.org/changeset/202279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.