[webkit-reviews] review granted: [Bug 206450] Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC when browsing reddit.com : [Attachment 388359] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 21 17:11:37 PST 2020


Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 206450: Frequent NetworkConnectionToWebProcess::CookiesEnabled sync IPC
when browsing reddit.com
https://bugs.webkit.org/show_bug.cgi?id=206450

Attachment 388359: Patch

https://bugs.webkit.org/attachment.cgi?id=388359&action=review




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 388359
  --> https://bugs.webkit.org/attachment.cgi?id=388359
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388359&action=review

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:272
> +    if (policy == CFHTTPCookieStorageAcceptPolicyAlways)
> +	   return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (policy == CFHTTPCookieStorageAcceptPolicyOnlyFromMainDocumentDomain)
> +	   return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    if (policy ==
CFHTTPCookieStorageAcceptPolicyExclusivelyFromMainDocumentDomain)
> +	   return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain;
> +
> +    return HTTPCookieAcceptPolicy::Never;

Why not use a switch? I guess because it’s not an enum we don’t get that extra
compiler checking, but I’d still be tempted to write it that way.

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:464
> +    if (cookieAcceptPolicy == NSHTTPCookieAcceptPolicyAlways)
> +	   return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (cookieAcceptPolicy ==
NSHTTPCookieAcceptPolicyOnlyFromMainDocumentDomain)
> +	   return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    if (cookieAcceptPolicy ==
NSHTTPCookieAcceptPolicyExclusivelyFromMainDocumentDomain)
> +	   return HTTPCookieAcceptPolicy::ExclusivelyFromMainDocumentDomain;
>      END_BLOCK_OBJC_EXCEPTIONS;
> -    return false;
> +
> +    return HTTPCookieAcceptPolicy::Never;

Ditto.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:252
> +    if (policy == SOUP_COOKIE_JAR_ACCEPT_ALWAYS)
> +	   return HTTPCookieAcceptPolicy::AlwaysAccept;
> +    if (policy == SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY)
> +	   return HTTPCookieAcceptPolicy::OnlyFromMainDocumentDomain;
> +    return HTTPCookieAcceptPolicy::Never;

Ditto.

> Source/WebKit/NetworkProcess/Cookies/WebCookieManager.cpp:158
> +#if PLATFORM(COCOA)
> +   
completionHandler(WebCookieManager::platformGetHTTPCookieAcceptPolicy());
> +#else
> +   
completionHandler(m_process.defaultStorageSession().cookieAcceptPolicy());
> +#endif

I don’t understand why Cocoa is different here. Maybe it should be obvious but
somehow it’s not.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:390
> +    HTTPCookieAcceptPolicy cookieAcceptPolicy =
HTTPCookieAcceptPolicy::Never;
> +    if (auto* storage = storageSession(sessionID))
> +	   cookieAcceptPolicy = storage->cookieAcceptPolicy();
> +
> +    completionHandler(WTFMove(ipcConnection->second), cookieAcceptPolicy);

Consider:

    auto* storage = storageSession(sessionID);
    completionHandler(WTFMove(ipcConnection->second),
	storage ? storage->cookieAcceptPolicy() :
HTTPCookieAcceptPolicy::Never);

Maybe you still like the if better?

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm:203
> +static WebCore::HTTPCookieAcceptPolicy
toHTTPCookieAcceptPolicy(NSHTTPCookieAcceptPolicy policy)

Seems to duplicate code in NetworkStorageSession::cookieAcceptPolicy. Might be
nice to find a way to share these conversion functions.

> Source/WebKit/WebProcess/Network/NetworkProcessConnection.h:84
> +    bool cookiesEnabled();

Maybe const.


More information about the webkit-reviews mailing list