[webkit-reviews] review granted: [Bug 55086] WebKit2: Need a way to manage cookies from the web process : [Attachment 83553] [PATCH] Fix

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 55086: WebKit2: Need a way to manage cookies from the web process
https://bugs.webkit.org/show_bug.cgi?id=55086

Attachment 83553: [PATCH] Fix
https://bugs.webkit.org/attachment.cgi?id=83553&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83553&action=review

You should probably let the EWS chew on this a bit. Looks nice!

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:63
> +    if (!m_webContext->hasValidProcess()) {

Seems like you need to null-check m_webContext here and everywhere else you use
it, since clearContext could have been called. Or, if it couldn't have been
called, you should assert that m_webContext is non-null.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:79
> +void WebCookieManagerProxy::deleteCookiesForOrigin(WebSecurityOrigin*
origin)

Can that be a const WebSecurityOrigin*?

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:35
> +#include "APIObject.h"
> +#include "GenericCallback.h"
> +#include "ImmutableArray.h"
> +
> +#include <wtf/PassRefPtr.h>
> +#include <wtf/RefPtr.h>
> +#include <wtf/Vector.h>

We don't normally separate #includes like this.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.h:47
> +struct SecurityOriginData;
> +class WebContext;
> +class WebSecurityOrigin;

I personally like to put struct and class declarations in separate paragraphs,
but I don't know if that's our prevailing style.

> Source/WebKit2/UIProcess/API/C/WKCookieManager.cpp:52
> +void WKCookieManagerGetCookieOrigins(WKCookieManagerRef cookieManagerRef,
void* context, WKCookieManagerGetCookieOriginsFunction callback)
> +{
> +   
toImpl(cookieManagerRef)->getCookieOrigins(ArrayCallback::create(context,
callback));
> +}
> +
> +void WKCookieManagerDeleteCookiesForOrigin(WKCookieManagerRef
cookieManagerRef, WKSecurityOriginRef originRef)
> +{
> +    toImpl(cookieManagerRef)->deleteCookiesForOrigin(toImpl(originRef));
> +}
> +
> +void WKCookieManagerDeleteAllCookies(WKCookieManagerRef cookieManagerRef)
> +{
> +    toImpl(cookieManagerRef)->deleteAllCookies();
> +}

I don't really see the point of all the "Ref" suffixes, though I know that's
what we do elsewhere.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:42
> +    static WebCookieManager& shared = *new WebCookieManager;

DEFINE_STATIC_LOCAL would be better.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:59
> +    // FIXME <rdar://problem/8971738>: Implement.

Huh? This function looks pretty implemented to the uninitiated. You should say
what is still missing here.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:65
> +    HashSet<RefPtr<SecurityOrigin>, SecurityOriginHash>::iterator i =
origins.begin();

It's weird to use "i" as the name of an iterator. We normally use "it".

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:67
> +	   RefPtr<SecurityOrigin> origin = *i;

This could be a const RefPtr& to save an extra ref/deref.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:74
> +	   SecurityOriginData originData;
> +	   originData.protocol = origin->protocol();
> +	   originData.host = origin->host();
> +	   originData.port = origin->port();
> +
> +	   identifiers.uncheckedAppend(originData);

If we had a SecurityOriginData constructor that took a single const
RefPtr<SecurityOrigin>& argument, you could just use copyKeysToVector instead
of writing this loop manually. Such a constructor would need to be marked
"explicit", and would probably warrant a comment about why we have it (since we
would normally just take a bare SecurityOrigin*). You might want to post a new
patch if you do this, or do it separately.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:82
> +    // FIXME <rdar://problem/8971738>: Implement.

A notImplemented() call would be nice. It would also be nice if we had a
Bugzilla bug to reference instead of a silly private Radar.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.cpp:87
> +    // FIXME <rdar://problem/8971738>: Implement.

Ditto.

> Source/WebKit2/WebProcess/Cookies/WebCookieManager.h:45
> +    WTF_MAKE_NONCOPYABLE(WebCookieManager);
> +
> +public:

No need for the blank line here.