[webkit-reviews] review granted: [Bug 225773] Add unit test using WKHTTPCookieStoreObserver and cookies received from HTTP : [Attachment 428534] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 225773: Add unit test using WKHTTPCookieStoreObserver and cookies received
from HTTP
https://bugs.webkit.org/show_bug.cgi?id=225773

Attachment 428534: Patch

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




--- Comment #2 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 428534
  --> https://bugs.webkit.org/attachment.cgi?id=428534
Patch

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

r=me with suggestions.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:462
> +static void deleteAllCookies(WKHTTPCookieStore *store,
RetainPtr<NSMutableArray> array, BlockPtr<void(void)> completionBlock)

Maybe we should call this deleteCookies() instead of deleteAllCookies() given
that it only deletes the cookies in the provided array.

Maybe we could rename array to cookies too for clarity.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:466
> +    [store deleteCookie:[array lastObject] completionHandler:^(void) {

We're not really testing the parallelism here and it is slower than could be. I
wish we'd use WTF's CallbackAggregator and do the removals in parallel.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:479
> +	       deleteAllCookies(store, adoptNS([cookies mutableCopy]), ^{

Why `adoptNS([cookies mutableCopy]` instead of just `cookies` ?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:501
> +	       EXPECT_EQ(cookies.count, 1u);

Should probably be ASSERT_EQ() in case we get 0 cookies.