Bug 156345 – Write tests for WebCookieManager::addCookie()

• Status: NEW
• Component: WebKit2

[Blaze Burg]

Need to expose to C API.

[Blaze Burg]

Created attachment 275943 [details]
WIP

[Blaze Burg]

Created attachment 275960 [details]
Proposed Fix

[Alex Christensen]

Comment on attachment 275960 [details]
Proposed Fix

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

Looks good to me.  I'd prefer Anders glance at this and make sure he has no opposition to adding the new WKCookieRef type.

> Source/WebKit2/UIProcess/WebCookie.cpp:40
> +const WebCore::Cookie& WebCookie::cookie()

Add this file to CMakeLists.txt.

> Tools/TestWebKitAPI/Tests/WebKit2/CookieManager.cpp:-73
> -TEST(WebKit2, CookieManager)

I'm not sure it's necessary to rename this file just to add another test.

> Tools/TestWebKitAPI/Tests/WebKit2/WKCookieManagerSetCookie.cpp:42
> +static void cookiesDidChange(WKCookieManagerRef cookieManager, const void*)

Can we get the cookie here and make sure that it has the expected name and value?

[Blaze Burg]

(In reply to comment #3)
> Comment on attachment 275960 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275960&action=review
> 
> Looks good to me.  I'd prefer Anders glance at this and make sure he has no
> opposition to adding the new WKCookieRef type.
> 
> > Source/WebKit2/UIProcess/WebCookie.cpp:40
> > +const WebCore::Cookie& WebCookie::cookie()
> 
> Add this file to CMakeLists.txt

Oops! Will do.

> 
> > Tools/TestWebKitAPI/Tests/WebKit2/CookieManager.cpp:-73
> > -TEST(WebKit2, CookieManager)
> 
> I'm not sure it's necessary to rename this file just to add another test.

With all the static methods needed for clients, I thought it would be better to put one test per file and try to cover as much as possible in one test. (The name CookieManager.cpp is kinda prime real estate and the existing test only covers two APIs.)

> > Tools/TestWebKitAPI/Tests/WebKit2/WKCookieManagerSetCookie.cpp:42
> > +static void cookiesDidChange(WKCookieManagerRef cookieManager, const void*)
> 
> Can we get the cookie here and make sure that it has the expected name and
> value?

Would need to add new WebCookieManagerProxy stuff, but it's doable.

I had this working before but I removed it in favor of using the Automation proxy, since for automation it needs to get the first party URL from WebProcess.

[Blaze Burg]

Comment on attachment 275960 [details]
Proposed Fix

Clear review, new patch coming.