[webkit-reviews] review granted: [Bug 174580] WKHTTPCookieStore observing only works on the default cookie store : [Attachment 315660] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 174580: WKHTTPCookieStore observing only works on the default cookie store
https://bugs.webkit.org/show_bug.cgi?id=174580

Attachment 315660: Patch

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




--- Comment #5 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 315660
  --> https://bugs.webkit.org/attachment.cgi?id=315660
Patch

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

r=me but please consider the changes proposed.

> Source/WebCore/platform/network/NetworkStorageSession.h:153
> +
> +#if PLATFORM(COCOA)
> +public:
> +    CookieStorageObserver& cookieStorageObserver() const;
> +
> +private:
> +    mutable RefPtr<CookieStorageObserver> m_cookieStorageObserver;
> +#endif

It makes me sad how this ostensibly cross-platform file, NetworkStorageSession,
has so much that is platform dependent.  Something like CookieStorageObserver
seems like it could be a cross-platform abstraction that everyone would want
(see cookiesDidChange in the SOUP section above).  Can we make it that way, and
just stub things out for SOUP for now?

> Source/WebCore/platform/network/cocoa/CookieStorageObserver.h:25
> + */
> +#pragma once

Missing newline above pragma.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKHTTPCookieStore.mm:203
> +    NSURL *cookieStorageFile = [NSURL
fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/CustomWebsiteData/CookieStora
ge/Cookie.File" stringByExpandingTildeInPath] isDirectory:NO];
> +    NSURL *idbPath = [NSURL
fileURLWithPath:[@"~/Library/WebKit/TestWebKitAPI/CustomWebsiteData/IndexedDB/"
stringByExpandingTildeInPath] isDirectory:YES];

Do we really need to use ~/Library/ for this stuff? Can we make it point to
something in the temp directory (_CS_DARWIN_USER_TEMP_DIR and all that jazz)?