Summary: | Upstream WKHTTPCookiesForURL from WebKitSystemInterface to OpenSource | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Amir Alavi <aalavi> | ||||||
Component: | New Bugs | Assignee: | 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
Amir Alavi
2016-06-20 18:27:18 PDT
Created attachment 281691 [details]
Patch
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 *. *** Bug 158966 has been marked as a duplicate of this bug. *** *** Bug 158964 has been marked as a duplicate of this bug. *** *** Bug 158965 has been marked as a duplicate of this bug. *** 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 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 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? Created attachment 281755 [details]
Patch
(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 on attachment 281755 [details]
Patch
r=me
Comment on attachment 281755 [details] Patch Clearing flags on attachment: 281755 Committed r202279: <http://trac.webkit.org/changeset/202279> All reviewed patches have been landed. Closing bug. |