[webkit-reviews] review granted: [Bug 105676] Add a separate class for networking related storage : [Attachment 180609] patch for review

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 105676: Add a separate class for networking related storage
https://bugs.webkit.org/show_bug.cgi?id=105676

Attachment 180609: patch for review
https://bugs.webkit.org/attachment.cgi?id=180609&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180609&action=review


> Source/WebCore/platform/network/NetworkStorageSession.h:41
> +class NetworkStorageSession {

Given how platform dependent this looking, do you think it might make sense to
follow the idiom we use in ResourceRequest/ResourceResponse, where we have a
base that is platform agnostic and concrete implementations that are platform
specific?

Another though is I wonder if this really needs to be storage specific?  It
seems like there might be other facets of a networking session that might be
useful to keep.

> Source/WebCore/platform/network/PlatformCookieJar.h:49
> -String cookiesForDOM(NetworkingContext*, const KURL& firstParty, const
KURL&);
> -void setCookiesFromDOM(NetworkingContext*, const KURL& firstParty, const
KURL&, const String&);
> -bool cookiesEnabled(NetworkingContext*, const KURL& firstParty, const
KURL&);
> -String cookieRequestHeaderFieldValue(NetworkingContext*, const KURL&
firstParty, const KURL&);
> -bool getRawCookies(NetworkingContext*, const KURL& firstParty, const KURL&,
Vector<Cookie>&);
> -void deleteCookie(NetworkingContext*, const KURL&, const String&);
> -void getHostnamesWithCookies(NetworkingContext*, HashSet<String>&
hostnames);
> -void deleteCookiesForHostname(NetworkingContext*, const String& hostname);
> -void deleteAllCookies(NetworkingContext*);
> +String cookiesForDOM(const NetworkStorageSession&, const KURL& firstParty,
const KURL&);
> +void setCookiesFromDOM(const NetworkStorageSession&, const KURL& firstParty,
const KURL&, const String&);
> +bool cookiesEnabled(const NetworkStorageSession&, const KURL& firstParty,
const KURL&);
> +String cookieRequestHeaderFieldValue(const NetworkStorageSession&, const
KURL& firstParty, const KURL&);
> +bool getRawCookies(const NetworkStorageSession&, const KURL& firstParty,
const KURL&, Vector<Cookie>&);
> +void deleteCookie(const NetworkStorageSession&, const KURL&, const String&);

> +void getHostnamesWithCookies(const NetworkStorageSession&, HashSet<String>&
hostnames);
> +void deleteCookiesForHostname(const NetworkStorageSession&, const String&
hostname);
> +void deleteAllCookies(const NetworkStorageSession&);

Since you are passing by const reference now, the comment doesn't make as much
sense.	It also seems like we should eventually move these functions to be
member functions on NetworkStorageSession. If so, a FIXME should be added.