Summary: | WebKit2: Use CFNetwork Sessions API | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jessie Berlin <jberlin> | ||||||||||||||||||||
Component: | WebKit2 | Assignee: | Jessie Berlin <jberlin> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | abarth, buildbot, dbates, eric, jberlin, rniwa, webkit-ews, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Attachments: |
|
Description
Jessie Berlin
2011-02-28 18:27:18 PST
Created attachment 84233 [details]
Patch (Part 1)
Attachment 84233 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/WebCore/platform/network/ResourceHandle.h:191: The parameter name "storageSession" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 84234 [details]
Patch (Part 1) style issues fixed.
Attachment 84233 [details] did not build on qt: Build output: http://queues.webkit.org/results/8072705 Attachment 84233 [details] did not build on win: Build output: http://queues.webkit.org/results/8070755 Attachment 84234 [details] did not build on qt: Build output: http://queues.webkit.org/results/8037013 Created attachment 84247 [details]
Patch (Part 1) speculative build fixes.
Attachment 84234 [details] did not build on win: Build output: http://queues.webkit.org/results/8078101 Comment on attachment 84247 [details] Patch (Part 1) speculative build fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=84247&action=review > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:496 > +void ResourceHandle::setPrivateBrowsingStorageSession(CFURLStorageSessionRef storageSession) > +{ > + privateStorageSession().adoptCF(storageSession); > +} It seems a little strange that storing the private browsing session is stored here. > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:143 > +static RetainPtr<CFURLStorageSessionRef>& privateStorageSession() > +{ > + DEFINE_STATIC_LOCAL(RetainPtr<CFURLStorageSessionRef>, storageSession, ()); > + return storageSession; > +} This is duplicated from the CFNet version, it would be nice if we could share this code. > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:572 > +void ResourceHandle::setPrivateBrowsingStorageSession(CFURLStorageSessionRef storageSession) > +{ > + privateStorageSession().adoptCF(storageSession); > +} > + > +CFURLStorageSessionRef ResourceHandle::privateBrowsingStorageSession() > +{ > + return privateStorageSession().get(); > +} This is duplicated from the CFNet version, it would be nice if we could share this code. Perhaps a new file for stuff shared between CFNet and mac implementations? > Source/WebKit2/UIProcess/API/C/WKContext.cpp:214 > +void WKContextSetPrivateBrowsingStorageSessionIdentifier(WKContextRef contextRef, WKStringRef identifier) > +{ > + toImpl(contextRef)->setPrivateBrowsingStorageSessionIdentifier(toImpl(identifier)->string()); > +} Why is it necessary to set an explicit string identifier for the private browsing storage session, and then pass it around from the UI process to the Web process? Can't WebProcess just invent its own arbitrary string identifier? That would simplify the code to manage the private browsing storage session a lot, as it could be mostly encapsulated at the ResourceHandle level. Attachment 84234 [details] did not build on win: Build output: http://queues.webkit.org/results/8077463 (In reply to comment #9) > (From update of attachment 84247 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84247&action=review > > > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:496 > > +void ResourceHandle::setPrivateBrowsingStorageSession(CFURLStorageSessionRef storageSession) > > +{ > > + privateStorageSession().adoptCF(storageSession); > > +} > > It seems a little strange that storing the private browsing session is stored here. Where else would you suggest? > > > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:143 > > +static RetainPtr<CFURLStorageSessionRef>& privateStorageSession() > > +{ > > + DEFINE_STATIC_LOCAL(RetainPtr<CFURLStorageSessionRef>, storageSession, ()); > > + return storageSession; > > +} > > This is duplicated from the CFNet version, it would be nice if we could share this code. I could put them within a #if USE(CFSTORAGESESSION) in ResourceHandle.cpp > > > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:572 > > +void ResourceHandle::setPrivateBrowsingStorageSession(CFURLStorageSessionRef storageSession) > > +{ > > + privateStorageSession().adoptCF(storageSession); > > +} > > + > > +CFURLStorageSessionRef ResourceHandle::privateBrowsingStorageSession() > > +{ > > + return privateStorageSession().get(); > > +} > > This is duplicated from the CFNet version, it would be nice if we could share this code. Perhaps a new file for stuff shared between CFNet and mac implementations? What would that new file be called? I think it is probably better to just put them inside a #if USE(CFSTORAGESESSION) in ResourceHandle.cpp. > > > Source/WebKit2/UIProcess/API/C/WKContext.cpp:214 > > +void WKContextSetPrivateBrowsingStorageSessionIdentifier(WKContextRef contextRef, WKStringRef identifier) > > +{ > > + toImpl(contextRef)->setPrivateBrowsingStorageSessionIdentifier(toImpl(identifier)->string()); > > +} > > Why is it necessary to set an explicit string identifier for the private browsing storage session, and then pass it around from the UI process to the Web process? Can't WebProcess just invent its own arbitrary string identifier? Yes, I will change it so that the Web Process invents its own arbitrary string identifier here. > That would simplify the code to manage the private browsing storage session a lot, as it could be mostly encapsulated at the ResourceHandle level. Maciej pointed out that it would be best to also make the create calls in ResourceHandle. I will work on another patch that makes these changes. Comment on attachment 84247 [details]
Patch (Part 1) speculative build fixes.
I will work on a new patch that takes into account Maciej's feedback.
Created attachment 84336 [details]
Patch (Part 1) Take 4
Comment on attachment 84336 [details] Patch (Part 1) Take 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84336&action=review Looks great though. > Source/WebCore/WebCore.exp.in:1336 > +_wkCreatePrivateStorageSession This might not build when CFSTORAGESESSIONS is off. > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:729 > +CFURLStorageSessionRef ResourceHandle::createPrivateBrowsingStorageSession(CFStringRef identifier) > +{ > + return wkCreatePrivateStorageSession(identifier); > +} > + > +String ResourceHandle::defaultBaseForPrivateBrowsingStorageSessionIdentifier() > +{ > + RetainPtr<CFStringRef> base(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > + return String(base.get()); > +} > + Should these be guarded? (In reply to comment #14) > (From update of attachment 84336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84336&action=review > > Looks great though. > > > Source/WebCore/WebCore.exp.in:1336 > > +_wkCreatePrivateStorageSession > > This might not build when CFSTORAGESESSIONS is off. This line shouldn't actually be problematic because it is soft-linked against in WKSI and declared in WebCoreSystemInterface. The line that should be problematic is the one for setBasePrivateBrowsingStorageSession. I added guards around that. > > > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:729 > > +CFURLStorageSessionRef ResourceHandle::createPrivateBrowsingStorageSession(CFStringRef identifier) > > +{ > > + return wkCreatePrivateStorageSession(identifier); > > +} > > + > > +String ResourceHandle::defaultBaseForPrivateBrowsingStorageSessionIdentifier() > > +{ > > + RetainPtr<CFStringRef> base(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > > + return String(base.get()); > > +} > > + > > Should these be guarded? These don't need to be guarded because USE(CFSTORAGESESSIONS) is defined when USE(CFNETWORK) is defined. Attachment 84336 [details] did not build on mac: Build output: http://queues.webkit.org/results/8076879 Comment on attachment 84336 [details] Patch (Part 1) Take 4 View in context: https://bugs.webkit.org/attachment.cgi?id=84336&action=review > Source/JavaScriptCore/wtf/Platform.h:695 > +#define WTF_USE_CFSTORAGESESSIONS 1 I think CFURLSTORAGESESSIONS would be clearer. No need to conflate CF and CFNetwork more than we already do. ;-) > Source/WebCore/ChangeLog:15 > + Propogate the private browsing state to the ResourceHandle. Typo: Propogate > Source/WebCore/platform/network/ResourceHandle.cpp:197 > +static String& baseForPrivateBrowsingStorageSessionIdentifier() I think "privateBrowserStorageSessionIdentifierBase" would be a slightly clearer term. (You'd want to propagate this to all the functions/variables that use this terminology.) > Source/WebCore/platform/network/ResourceHandle.cpp:205 > + if (enabled && privateStorageSession().get()) No need for .get() here. > Source/WebCore/platform/network/ResourceHandle.cpp:209 > + privateStorageSession().clear(); We're using "= nullptr" instead of .clear() these days. You could perhaps clean this all up a little bit like this: if (!enabled) { privateStorageSession() = nullptr; return; } if (privateStorageSession()) return; > Source/WebCore/platform/network/ResourceHandle.cpp:213 > + String base = baseForPrivateBrowsingStorageSessionIdentifier().isNull() ? defaultBaseForPrivateBrowsingStorageSessionIdentifier() : baseForPrivateBrowsingStorageSessionIdentifier(); What about an empty base? > Source/WebCore/platform/network/ResourceHandle.h:228 > + static CFURLStorageSessionRef createPrivateBrowsingStorageSession(CFStringRef identifier); It would be better if this returned a RetainPtr. Maybe it should take a const String&? > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:728 > +String ResourceHandle::defaultBaseForPrivateBrowsingStorageSessionIdentifier() > +{ > + RetainPtr<CFStringRef> base(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > + return String(base.get()); > +} You're overreleasing the CFStringRef here. You don't need a RetainPtr at all. (In fact, this whole function can become a single return statement.) > Source/WebKit2/UIProcess/mac/WebContextMac.mm:96 > // FIXME: This should really be configurable; we shouldn't just blindly allow read access to the UI process bundle. > parameters.uiProcessBundleResourcePath = fileSystemRepresentation([[NSBundle mainBundle] resourcePath]); > + parameters.uiProcessBundleIdentifier = String([[NSBundle mainBundle] bundleIdentifier]); The FIXME here only applies to the first line, not the second. You should probably separate them. > Source/WebKit2/UIProcess/win/WebContextWin.cpp:73 > + RetainPtr<CFStringRef> bundleIdentifier(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > + parameters.uiProcessBundleIdentifier = String(bundleIdentifier.get()); Again, you're overreleasing the CFStringRef and don't need a RetainPtr at all. (In reply to comment #17) > (From update of attachment 84336 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84336&action=review > > > Source/JavaScriptCore/wtf/Platform.h:695 > > +#define WTF_USE_CFSTORAGESESSIONS 1 > > I think CFURLSTORAGESESSIONS would be clearer. No need to conflate CF and CFNetwork more than we already do. ;-) Done. > > > Source/WebCore/ChangeLog:15 > > + Propogate the private browsing state to the ResourceHandle. > > Typo: Propogate Whoops! Fixed. > > > Source/WebCore/platform/network/ResourceHandle.cpp:197 > > +static String& baseForPrivateBrowsingStorageSessionIdentifier() > > I think "privateBrowserStorageSessionIdentifierBase" would be a slightly clearer term. (You'd want to propagate this to all the functions/variables that use this terminology.) Done. > > > Source/WebCore/platform/network/ResourceHandle.cpp:205 > > + if (enabled && privateStorageSession().get()) > > No need for .get() here. Went with your cleaner version below. > > > Source/WebCore/platform/network/ResourceHandle.cpp:209 > > + privateStorageSession().clear(); > > We're using "= nullptr" instead of .clear() these days. > > You could perhaps clean this all up a little bit like this: > > if (!enabled) { > privateStorageSession() = nullptr; > return; > } > > if (privateStorageSession()) > return; Done. > > > Source/WebCore/platform/network/ResourceHandle.cpp:213 > > + String base = baseForPrivateBrowsingStorageSessionIdentifier().isNull() ? defaultBaseForPrivateBrowsingStorageSessionIdentifier() : baseForPrivateBrowsingStorageSessionIdentifier(); > > What about an empty base? I went with a default base so that when WebKit1 calls this in a single-process model, it will get the same identifier (which will get it the same session). > > > Source/WebCore/platform/network/ResourceHandle.h:228 > > + static CFURLStorageSessionRef createPrivateBrowsingStorageSession(CFStringRef identifier); > > It would be better if this returned a RetainPtr. Maybe it should take a const String&? I was trying to share as much of the code as possible (including getting the CFStringRef from String). I made the function return a RetainPtr. > > > Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:728 > > +String ResourceHandle::defaultBaseForPrivateBrowsingStorageSessionIdentifier() > > +{ > > + RetainPtr<CFStringRef> base(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > > + return String(base.get()); > > +} > > You're overreleasing the CFStringRef here. You don't need a RetainPtr at all. (In fact, this whole function can become a single return statement.) Done. > > > Source/WebKit2/UIProcess/mac/WebContextMac.mm:96 > > // FIXME: This should really be configurable; we shouldn't just blindly allow read access to the UI process bundle. > > parameters.uiProcessBundleResourcePath = fileSystemRepresentation([[NSBundle mainBundle] resourcePath]); > > + parameters.uiProcessBundleIdentifier = String([[NSBundle mainBundle] bundleIdentifier]); > > The FIXME here only applies to the first line, not the second. You should probably separate them. Done. > > > Source/WebKit2/UIProcess/win/WebContextWin.cpp:73 > > + RetainPtr<CFStringRef> bundleIdentifier(AdoptCF, reinterpret_cast<CFStringRef>(CFBundleGetValueForInfoDictionaryKey(CFBundleGetMainBundle(), kCFBundleIdentifierKey))); > > + parameters.uiProcessBundleIdentifier = String(bundleIdentifier.get()); > > Again, you're overreleasing the CFStringRef and don't need a RetainPtr at all. Fixed. Comment on attachment 84336 [details] Patch (Part 1) Take 4 Committed in r80180 http://trac.webkit.org/changeset/80180 http://trac.webkit.org/changeset/80180 might have broken Windows Debug (Build) (In reply to comment #20) > http://trac.webkit.org/changeset/80180 might have broken Windows Debug (Build) Fixed in http://trac.webkit.org/changeset/80187 It seems like 4 tests are crashing on Snow Leopard (not WebKit2) after this patch is landed: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r80188%20(26214)/results.html Blame list: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(Tests)/builds/26213 (In reply to comment #22) > It seems like 4 tests are crashing on Snow Leopard (not WebKit2) after this patch is landed: > http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r80188%20(26214)/results.html > > Blame list: > http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20(Tests)/builds/26213 This was fixed in http://trac.webkit.org/changeset/80205 Created attachment 84605 [details]
Patch (Part 2)
Comment on attachment 84605 [details] Patch (Part 2) 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. (In reply to comment #25) > (From update of attachment 84605 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84605&action=review > > r=me, but you need to fix a leak before committing. Thanks! > > > 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(...)) Because wkSetRequestStorageSession takes a MutableURLRequest, and m_cfRequest is non-mutable. I would rather not do a const-cast, so I think I should stick with 3 lines as is. > > > 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. Fixed. Since I changed to wkCopyRequestWithStorageSession, I this line is now: nsRequest = [wkCopyRequestWithStorageSession(storageSession, nsRequest) autorelease]; > > > 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. Done. Attachment 84605 [details] did not build on win: Build output: http://queues.webkit.org/results/8086360 Comment on attachment 84605 [details] Patch (Part 2) Committed in http://trac.webkit.org/changeset/80294 Created attachment 84756 [details]
Patch (Part 3)
Comment on attachment 84756 [details] Patch (Part 3) View in context: https://bugs.webkit.org/attachment.cgi?id=84756&action=review Patch looks good - just make sure all the calls to privateBrowsingStorageSession are guarded. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:448 > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) Does this need to be guarded? (It is on the Windows side). > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:463 > + cachedResponse = WKCachedResponseForRequest(storageSession, request); Ditto. > Source/WebKit/mac/WebView/WebView.mm:1876 > + cachedResponse = WKCachedResponseForRequest(storageSession, request); Same question about being guarded. Comment on attachment 84756 [details] Patch (Part 3) View in context: https://bugs.webkit.org/attachment.cgi?id=84756&action=review Two minor comments, but r=me. > Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:261 > RetainPtr<CFURLCacheRef> cache(AdoptCF, CFURLCacheCopySharedURLCache()); > +#if USE(CFURLSTORAGESESSIONS) > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) > + cache.adoptCF(wkCopyURLCache(storageSession)); > +#endif This is a little unfortunate because it ends up assigning twice in the session case, needlessly thrashing the shared cache refcount. This probably isn't a hot enough function to matter, but it would maybe be nicer to write this as an else/if, so that CFURLCacheCopySharedURLCache is not even called in the shared case. > Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:283 > RetainPtr<CFURLCacheRef> cache(AdoptCF, CFURLCacheCopySharedURLCache()); > +#if USE(CFURLSTORAGESESSIONS) > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) > + cache.adoptCF(wkCopyURLCache(storageSession)); > +#endif ditto previous comment. (In reply to comment #30) > (From update of attachment 84756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84756&action=review > > Patch looks good - just make sure all the calls to privateBrowsingStorageSession are guarded. > > > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:448 > > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) > > Does this need to be guarded? (It is on the Windows side). > > > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:463 > > + cachedResponse = WKCachedResponseForRequest(storageSession, request); > > Ditto. > > > Source/WebKit/mac/WebView/WebView.mm:1876 > > + cachedResponse = WKCachedResponseForRequest(storageSession, request); > > Same question about being guarded. I will add guards in this patch, and then do a clean-up patch to add guards everywhere I should have in Part #1 and Part #2. (In reply to comment #31) > (From update of attachment 84756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84756&action=review > > Two minor comments, but r=me. Thanks! > > > Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:261 > > RetainPtr<CFURLCacheRef> cache(AdoptCF, CFURLCacheCopySharedURLCache()); > > +#if USE(CFURLSTORAGESESSIONS) > > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) > > + cache.adoptCF(wkCopyURLCache(storageSession)); > > +#endif > > This is a little unfortunate because it ends up assigning twice in the session case, needlessly thrashing the shared cache refcount. This probably isn't a hot enough function to matter, but it would maybe be nicer to write this as an else/if, so that CFURLCacheCopySharedURLCache is not even called in the shared case. Done. > > > Source/WebKit2/WebProcess/WebPage/win/WebPageWin.cpp:283 > > RetainPtr<CFURLCacheRef> cache(AdoptCF, CFURLCacheCopySharedURLCache()); > > +#if USE(CFURLSTORAGESESSIONS) > > + if (CFURLStorageSessionRef storageSession = ResourceHandle::privateBrowsingStorageSession()) > > + cache.adoptCF(wkCopyURLCache(storageSession)); > > +#endif > > ditto previous comment. Done. Comment on attachment 84756 [details] Patch (Part 3) Committed in r80370 http://trac.webkit.org/changeset/80370 Created attachment 84801 [details]
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards
Comment on attachment 84801 [details] Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards Committed in r80381 ttp://trac.webkit.org/changeset/80381 Created attachment 84866 [details]
Patch (Part 4)
Created attachment 84878 [details]
Patch (Part 4) Take 2
(In reply to comment #38) > Created an attachment (id=84878) [details] > Patch (Part 4) Take 2 This patch updates some WKSI function names, but is otherwise identical to the original Part 4 patch. Comment on attachment 84878 [details] Patch (Part 4) Take 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84878&action=review > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:74 > +#if USE(CFURLSTORAGESESSIONS) > + privateBrowsingCookieStorage().adoptCF(enabled ? wkCreatePrivateInMemoryHTTPCookieStorage(ResourceHandle::privateBrowsingStorageSession()) : 0); > +#else > + privateBrowsingCookieStorage().adoptCF(enabled ? wkCreatePrivateInMemoryHTTPCookieStorage(0) : 0); > I think the use of the ternary operator here makes things less clear and a simple if statement would work better. > Source/WebCore/platform/network/mac/CookieStorageMac.mm:103 > + privateBrowsingCookieStorage().adoptCF(0); This would read clearer if you just used .clear() instead of adoptCF(0). > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:542 > + if (shouldRelaxThirdPartyCookiePolicy(firstRequest URL])) { This probably doesn't compile. It is missing a [, or maybe the ] is wrong. (In reply to comment #40) > (From update of attachment 84878 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84878&action=review > > > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:74 > > +#if USE(CFURLSTORAGESESSIONS) > > + privateBrowsingCookieStorage().adoptCF(enabled ? wkCreatePrivateInMemoryHTTPCookieStorage(ResourceHandle::privateBrowsingStorageSession()) : 0); > > +#else > > + privateBrowsingCookieStorage().adoptCF(enabled ? wkCreatePrivateInMemoryHTTPCookieStorage(0) : 0); > > > > I think the use of the ternary operator here makes things less clear and a simple if statement would work better. Done. > > > Source/WebCore/platform/network/mac/CookieStorageMac.mm:103 > > + privateBrowsingCookieStorage().adoptCF(0); > > This would read clearer if you just used .clear() instead of adoptCF(0). I think Adam pointed out that we are using = nullptr instead of .clear() nowadays. I will do that. > > > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:542 > > + if (shouldRelaxThirdPartyCookiePolicy(firstRequest URL])) { > > This probably doesn't compile. It is missing a [, or maybe the ] is wrong. Whoops! Thanks for spotting it! (it happens to be within a #if BUILDING_ON_TIGER block, which is why the EWS bots are not complaining). Thanks for the review! "Patch (Part 4) Take 2" was committed in WebKit changeset 80435 <http://trac.webkit.org/changeset/80435>. All patches for this bug have been landed. So, I am marking this bug Resolved Fixed. |