[webkit-reviews] review granted: [Bug 55435] WebKit2: Use CFNetwork Sessions API : [Attachment 84336] Patch (Part 1) Take 4

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben 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 84336: Patch (Part 1) Take 4
https://bugs.webkit.org/attachment.cgi?id=84336&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
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(CFBundleGetM
ainBundle(), 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(CFBundleGetM
ainBundle(), kCFBundleIdentifierKey)));
> +    parameters.uiProcessBundleIdentifier = String(bundleIdentifier.get());

Again, you're overreleasing the CFStringRef and don't need a RetainPtr at all.