Bug 156345

Summary: Write tests for WebCookieManager::addCookie()
Product: WebKit Reporter: Blaze Burg <bburg>
Component: WebKit2Assignee: Blaze Burg <bburg>
Status: NEW    
Severity: Normal CC: achristensen, aestes, andersca, ap, bburg
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 156091, 156319    
Bug Blocks:    
Attachments:
Description Flags
WIP
none
Proposed Fix none

Blaze Burg
Reported 2016-04-07 10:44:31 PDT
Need to expose to C API.
Attachments
WIP (42.55 KB, patch)
2016-04-07 15:01 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Proposed Fix (43.61 KB, patch)
2016-04-07 16:50 PDT, Blaze Burg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Blaze Burg
Comment 1 2016-04-07 15:01:34 PDT
Created attachment 275943 [details] WIP
Blaze Burg
Comment 2 2016-04-07 16:50:52 PDT
Created attachment 275960 [details] Proposed Fix
Alex Christensen
Comment 3 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?
Blaze Burg
Comment 4 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.
Blaze Burg
Comment 5 2016-04-08 14:07:05 PDT
Comment on attachment 275960 [details] Proposed Fix Clear review, new patch coming.
Note You need to log in before you can comment on or make changes to this bug.