[webkit-reviews] review granted: [Bug 55435] WebKit2: Use CFNetwork Sessions API : [Attachment 84756] Patch (Part 3)

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has granted Jessie Berlin
<jberlin at webkit.org>'s request for review:
Bug 55435: WebKit2: Use CFNetwork Sessions API
https://bugs.webkit.org/show_bug.cgi?id=55435

Attachment 84756: Patch (Part 3)
https://bugs.webkit.org/attachment.cgi?id=84756&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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.