[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