Bug 55435 – WebKit2: Use CFNetwork Sessions API

• Component: WebKit2
• Status: RESOLVED FIXED

[Jessie Berlin]

<rdar://problem/8272253>

[Jessie Berlin]

Created attachment 84233 [details]
Patch (Part 1)

[WebKit Review Bot]

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.

[Jessie Berlin]

Created attachment 84234 [details]
Patch (Part 1) style issues fixed.

[Early Warning System Bot]

Attachment 84233 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8072705

[Build Bot]

Attachment 84233 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070755

[Early Warning System Bot]

Attachment 84234 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8037013

[Jessie Berlin]

Created attachment 84247 [details]
Patch (Part 1) speculative build fixes.

[Build Bot]

Attachment 84234 [details] did not build on win:
Build output: http://queues.webkit.org/results/8078101

[Maciej Stachowiak]

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.

[Build Bot]

Attachment 84234 [details] did not build on win:
Build output: http://queues.webkit.org/results/8077463

[Jessie Berlin]

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

[Jessie Berlin]

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.

[Jessie Berlin]

Created attachment 84336 [details]
Patch (Part 1) Take 4

[Brian Weinstein]

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?

[Jessie Berlin]

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

[WebKit Review Bot]

Attachment 84336 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8076879

[Adam Roben (:aroben)]

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.

[Jessie Berlin]

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

[Jessie Berlin]

Comment on attachment 84336 [details]
Patch (Part 1) Take 4

Committed in r80180
http://trac.webkit.org/changeset/80180

[WebKit Review Bot]

http://trac.webkit.org/changeset/80180 might have broken Windows Debug (Build)

[Jessie Berlin]

(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

[Ryosuke Niwa]

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

[Jessie Berlin]

(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

[Jessie Berlin]

Created attachment 84605 [details]
Patch (Part 2)

[Adam Roben (:aroben)]

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.

[Jessie Berlin]

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

[Build Bot]

Attachment 84605 [details] did not build on win:
Build output: http://queues.webkit.org/results/8086360

[Jessie Berlin]

Comment on attachment 84605 [details]
Patch (Part 2)

Committed in http://trac.webkit.org/changeset/80294

[Jessie Berlin]

Created attachment 84756 [details]
Patch (Part 3)

[Brian Weinstein]

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.

[Maciej Stachowiak]

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.

[Jessie Berlin]

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

[Jessie Berlin]

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

[Jessie Berlin]

Comment on attachment 84756 [details]
Patch (Part 3)

Committed in r80370
http://trac.webkit.org/changeset/80370

[Jessie Berlin]

Created attachment 84801 [details]
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards

[Jessie Berlin]

Comment on attachment 84801 [details]
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards

Committed in r80381
ttp://trac.webkit.org/changeset/80381

[Jessie Berlin]

Created attachment 84866 [details]
Patch (Part 4)

[Jessie Berlin]

Created attachment 84878 [details]
Patch (Part 4) Take 2

[Jessie Berlin]

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

[Sam Weinig]

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.

[Jessie Berlin]

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

[Daniel Bates]

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