Bug 156345 - Write tests for WebCookieManager::addCookie()
Summary: Write tests for WebCookieManager::addCookie()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on: 156091 156319
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-07 10:44 PDT by BJ Burg
Modified: 2016-04-08 14:07 PDT (History)
5 users (show)

See Also:


Attachments
WIP (42.55 KB, patch)
2016-04-07 15:01 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (43.61 KB, patch)
2016-04-07 16:50 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-04-07 10:44:31 PDT
Need to expose to C API.
Comment 1 BJ Burg 2016-04-07 15:01:34 PDT
Created attachment 275943 [details]
WIP
Comment 2 BJ Burg 2016-04-07 16:50:52 PDT
Created attachment 275960 [details]
Proposed Fix
Comment 3 Alex Christensen 2016-04-08 13:26:30 PDT
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?
Comment 4 BJ Burg 2016-04-08 14:06:48 PDT
(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.
Comment 5 BJ Burg 2016-04-08 14:07:05 PDT
Comment on attachment 275960 [details]
Proposed Fix

Clear review, new patch coming.