Bug 55435

Summary: WebKit2: Use CFNetwork Sessions API
Product: WebKit Reporter: Jessie Berlin <jberlin>
Component: WebKit2Assignee: 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 Flags
Patch (Part 1)
none
Patch (Part 1) style issues fixed.
none
Patch (Part 1) speculative build fixes.
none
Patch (Part 1) Take 4
none
Patch (Part 2)
none
Patch (Part 3)
none
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards
none
Patch (Part 4)
none
Patch (Part 4) Take 2 sam: review+

Description Jessie Berlin 2011-02-28 18:27:18 PST
<rdar://problem/8272253>
Comment 1 Jessie Berlin 2011-03-01 08:39:21 PST
Created attachment 84233 [details]
Patch (Part 1)
Comment 2 WebKit Review Bot 2011-03-01 08:40:56 PST
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.
Comment 3 Jessie Berlin 2011-03-01 08:47:18 PST
Created attachment 84234 [details]
Patch (Part 1) style issues fixed.
Comment 4 Early Warning System Bot 2011-03-01 09:27:59 PST
Attachment 84233 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8072705
Comment 5 Build Bot 2011-03-01 09:29:58 PST
Attachment 84233 [details] did not build on win:
Build output: http://queues.webkit.org/results/8070755
Comment 6 Early Warning System Bot 2011-03-01 10:11:02 PST
Attachment 84234 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8037013
Comment 7 Jessie Berlin 2011-03-01 10:24:11 PST
Created attachment 84247 [details]
Patch (Part 1) speculative build fixes.
Comment 8 Build Bot 2011-03-01 10:44:48 PST
Attachment 84234 [details] did not build on win:
Build output: http://queues.webkit.org/results/8078101
Comment 9 Maciej Stachowiak 2011-03-01 11:28:21 PST
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.
Comment 10 Build Bot 2011-03-01 11:29:46 PST
Attachment 84234 [details] did not build on win:
Build output: http://queues.webkit.org/results/8077463
Comment 11 Jessie Berlin 2011-03-01 11:50:54 PST
(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 12 Jessie Berlin 2011-03-01 11:51:33 PST
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.
Comment 13 Jessie Berlin 2011-03-01 17:35:31 PST
Created attachment 84336 [details]
Patch (Part 1) Take 4
Comment 14 Brian Weinstein 2011-03-01 17:57:46 PST
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?
Comment 15 Jessie Berlin 2011-03-01 18:01:10 PST
(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.
Comment 16 WebKit Review Bot 2011-03-02 04:35:16 PST
Attachment 84336 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8076879
Comment 17 Adam Roben (:aroben) 2011-03-02 08:27:50 PST
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.
Comment 18 Jessie Berlin 2011-03-02 09:09:43 PST
(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 19 Jessie Berlin 2011-03-02 16:12:15 PST
Comment on attachment 84336 [details]
Patch (Part 1) Take 4

Committed in r80180
http://trac.webkit.org/changeset/80180
Comment 20 WebKit Review Bot 2011-03-02 16:31:57 PST
http://trac.webkit.org/changeset/80180 might have broken Windows Debug (Build)
Comment 21 Jessie Berlin 2011-03-02 17:18:41 PST
(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
Comment 22 Ryosuke Niwa 2011-03-02 19:41:38 PST
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
Comment 23 Jessie Berlin 2011-03-03 12:18:13 PST
(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
Comment 24 Jessie Berlin 2011-03-03 12:25:19 PST
Created attachment 84605 [details]
Patch (Part 2)
Comment 25 Adam Roben (:aroben) 2011-03-03 13:24:06 PST
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.
Comment 26 Jessie Berlin 2011-03-03 14:29:41 PST
(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.
Comment 27 Build Bot 2011-03-03 15:38:45 PST
Attachment 84605 [details] did not build on win:
Build output: http://queues.webkit.org/results/8086360
Comment 28 Jessie Berlin 2011-03-03 15:46:24 PST
Comment on attachment 84605 [details]
Patch (Part 2)

Committed in http://trac.webkit.org/changeset/80294
Comment 29 Jessie Berlin 2011-03-04 08:30:33 PST
Created attachment 84756 [details]
Patch (Part 3)
Comment 30 Brian Weinstein 2011-03-04 10:40:00 PST
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 31 Maciej Stachowiak 2011-03-04 10:48:07 PST
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.
Comment 32 Jessie Berlin 2011-03-04 11:23:09 PST
(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.
Comment 33 Jessie Berlin 2011-03-04 11:23:35 PST
(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 34 Jessie Berlin 2011-03-04 12:48:29 PST
Comment on attachment 84756 [details]
Patch (Part 3)

Committed in r80370
http://trac.webkit.org/changeset/80370
Comment 35 Jessie Berlin 2011-03-04 14:45:59 PST
Created attachment 84801 [details]
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards
Comment 36 Jessie Berlin 2011-03-04 15:12:05 PST
Comment on attachment 84801 [details]
Clean up: Add in more USE(CFURLSTORAGESESSIONS) guards

Committed in r80381
ttp://trac.webkit.org/changeset/80381
Comment 37 Jessie Berlin 2011-03-05 07:49:19 PST
Created attachment 84866 [details]
Patch (Part 4)
Comment 38 Jessie Berlin 2011-03-05 15:06:48 PST
Created attachment 84878 [details]
Patch (Part 4) Take 2
Comment 39 Jessie Berlin 2011-03-05 15:07:39 PST
(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 40 Sam Weinig 2011-03-05 17:25:08 PST
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.
Comment 41 Jessie Berlin 2011-03-05 17:54:07 PST
(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!
Comment 42 Daniel Bates 2011-03-06 16:54:46 PST
"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.