Summary: | Write tests for WebCookieManager::addCookie() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||
Component: | WebKit2 | Assignee: | BJ 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
BJ Burg
2016-04-07 10:44:31 PDT
Created attachment 275943 [details]
WIP
Created attachment 275960 [details]
Proposed Fix
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? (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 on attachment 275960 [details]
Proposed Fix
Clear review, new patch coming.
|