[webkit-reviews] review granted: [Bug 207593] Implement in-WebProcess cookie cache to avoid sync IPC for document.cookie in most cases : [Attachment 390575] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 207593: Implement in-WebProcess cookie cache to avoid sync IPC for
document.cookie in most cases
https://bugs.webkit.org/show_bug.cgi?id=207593

Attachment 390575: Patch

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




--- Comment #11 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 390575
  --> https://bugs.webkit.org/attachment.cgi?id=390575
Patch

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

> Source/WTF/wtf/PlatformEnable.h:802
> +#if !defined(ENABLE_COOKIE_CACHE) && PLATFORM(COCOA) &&
HAVE(CFNETWORK_COOKIE_CHANGE_LISTENER_API)
> +#define ENABLE_COOKIE_CACHE 1
> +#endif

Couldn't this just be a runtime no-op without
CFNETWORK_COOKIE_CHANGE_LISTENER_API and avoid most #ifs?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:617
> +    ASSERT(observers.add(&observer).isNewEntry);

Why is observer only added in debug build?

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:131
> +    if (!m_hostsWithCookieListeners.isEmpty()) {
> +	   auto hostsWithCookieListeners =
copyToVector(m_hostsWithCookieListeners);
> +	   unsubscribeFromCookieChangeNotifications(hostsWithCookieListeners);
> +    }

I think it would be nicer to have a separate function or just loop over
m_hostsWithCookieListeners here (avoiding the vector copy). It is just call to
stopListeningForCookieChangeNotifications for each.

> Source/WebKit/WebProcess/WebPage/WebCookieCache.h:44
> +    bool isFunctional();

isEnabled/isSupported?