Bug 225773 – Add unit test using WKHTTPCookieStoreObserver and cookies received from HTTP

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add unit test using WKHTTPCookieStoreObserver and cookies received from HTTP

[Alex Christensen]

Created attachment 428534 [details]
Patch

[Chris Dumez]

Comment on attachment 428534 [details]
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.

[Alex Christensen]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 428534 [details]
> 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.
Brilliant.

> > 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.
Parallelism isn't important here.  I'm just removing any leftovers from previous tests.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WKHTTPCookieStore.mm:479
> > +            deleteAllCookies(store, adoptNS([cookies mutableCopy]), ^{
> 
> Why `adoptNS([cookies mutableCopy]` instead of just `cookies` ?
It needs to be mutable because I remove them one by one.

[Alex Christensen]

Created attachment 428552 [details]
Patch

[EWS]

Committed r277451 (237699@main): <https://commits.webkit.org/237699@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428552 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77988762>