[webkit-reviews] review granted: [Bug 57055] Add WebCookieManager to WebKit mac port to list hosts with cookies, delete cookies for a host, delete all cookies : [Attachment 86863] Patch

[bugzilla-daemon at webkit.org]

Brian Weinstein <bweinstein at apple.com> has granted Anton D'Auria
<adauria at apple.com>'s request for review:
Bug 57055: Add WebCookieManager to WebKit mac port to list hosts with cookies,
delete cookies for a host, delete all cookies
https://bugs.webkit.org/show_bug.cgi?id=57055

Attachment 86863: Patch
https://bugs.webkit.org/attachment.cgi?id=86863&action=review

------- Additional Comments from Brian Weinstein <bweinstein at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=86863&action=review

If you could link to https://bugs.webkit.org/show_bug.cgi?id=57055 in the
Skipped lists of the platforms you're not implementing this for, that would be
great.

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:821
> +				3A1C2D93133A70FB00A420E5 /* Cookies */,

If these are alphabetical, Cookies should be above Default Delegates and DOM.

> Source/WebKit/mac/WebKit.exp:7
> +.objc_class_name_WebCookieManager

Alphabetical again - WebCookieManager < WebCoreScrollView.

> Source/WebKit/mac/Cookies/WebCookieManager.mm:27
> +#import "WebSecurityOriginInternal.h"

Is this include necessary? Also, I think our style dictates a blank like
between the import of WebCookieManagerPrivate and the other imports.

> Tools/DumpRenderTree/LayoutTestController.cpp:640
> +    ASSERT(!*exception);

Do you also want to assert exception is non-null here? ASSERT(exception);
ASSERT(!*exception). I don't know what our style is, if it's usually just the
ASSERT(!*exception), that's fine.

> Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm:180
> +	   JSRetainPtr<JSStringRef> jsString(Adopt,
JSStringCreateWithCFString((CFStringRef)string));

Can this be a C++ style cast?

> LayoutTests/http/tests/cookies/delete-all-cookies.html:27
> +

You might also want to run logContainsCookies here - to show that we are in a
clean state before the test starts.

> LayoutTests/http/tests/cookies/delete-cookies-for-hostname.html:22
> +    var contains = (hostsWithCookies.indexOf(drtHost) != -1);

I'm not wild about this name. Maybe hasDRTCookies? I don't have a strong
opinion about this though.

> LayoutTests/http/tests/cookies/hostnames-with-cookies.html:6
> +resetCookies();

Don't forget to put resetCookies in runTest here (unless there's a reason not
to).

> LayoutTests/http/tests/cookies/hostnames-with-cookies.html:25
> +    var contains = (hostsWithCookies.indexOf(drtHost) != -1);

Same comment about contains variable name here.