http/tests/cookies/document-cookie-multiple-cookies.html is failing on Windows: e.g.: https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean-tree/results.html -PASS document.cookie is "samesite-unspecified=0" +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1; samesite-strict=2; samesite-unspecified=0.
Created attachment 405368 [details] Patch
Created attachment 405384 [details] Patch
Comment on attachment 405384 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405384&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:201 > + RetainPtr<CFStringRef> cookieStringCF = cookieString.createCFString(); Could be auto > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:231 > + auto cookies = adoptCF(CFArrayCreate(kCFAllocatorDefault, 0, 0, &kCFTypeArrayCallBacks)); Don't you need CFArrayCreateMutable if you want to append? Seems like a bug. Can we avoid my question about whether append is valid, and the cost of reallocation in append, by passing in { &cookie, 1 } (or syntactically valid equivalent) instead of { 0, 0 } here?
Created attachment 405390 [details] Patch
Comment on attachment 405390 [details] Patch r=me
Created attachment 405398 [details] Patch
Comment on attachment 405398 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405398&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:216 > + return WTFMove(cookie); Windows really does not like this return statement it seems. I tried without WTFMove() first and now with it. It is still failing. Not sure what's going on...
Created attachment 405417 [details] Patch
Created attachment 405420 [details] Patch
I have marked these tests as failing while this issue is investigated. https://trac.webkit.org/changeset/265021/webkit
Created attachment 405429 [details] Patch
(In reply to Karl Rackler from comment #10) > I have marked these tests as failing while this issue is investigated. > https://trac.webkit.org/changeset/265021/webkit Thanks, I updated my patch to unskip it.
NetworkStorageSessionCFNetWin.obj : error LNK2019: unresolved external symbol __imp__CFHTTPCookieCreateWithStringAndPartition referenced in function "class WTF::RetainPtr<struct OpaqueCFHTTPCookie const *> __cdecl WebCore::parseDOMCookie(class WTF::String,struct __CFURL const *)" (?parseDOMCookie@WebCore@@YA?AV?$RetainPtr@PEBUOpaqueCFHTTPCookie@@@WTF@@VString@3@PEBU__CFURL@@@Z) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj] C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\bin64\TestWebCoreLib.dll : fatal error LNK1120: 1 unresolved externals [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\TestWebKitAPI\TestWebCoreLib.vcxproj] Looks like I may not be able to use __CFHTTPCookieCreateWithStringAndPartition() on Windows... I guess I'll keep using the old API but only keep the first cookie..
Created attachment 405433 [details] Patch
Comment on attachment 405433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405433&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:223 > + return cookie; Getting a type mismatch on this line; maybe you need a cast somewhere to CFHTTPCookieRef?
Created attachment 405438 [details] Patch
Created attachment 405465 [details] Patch
Comment on attachment 405465 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405465&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:211 > + auto cookie = static_cast<CFHTTPCookieRef>(CFArrayGetValueAtIndex(parsedCookies.get(), 0)); Idle unimportant thought: I wonder if we can use checked_cf_cast instead of static_cast. I think itβs designed for uses like this one.
Created attachment 405467 [details] Patch
Comment on attachment 405467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405467&action=review > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:229 > + UNUSED_PARAM(frameID); > + UNUSED_PARAM(pageID); Could use "omit the name" style for unused arguments instead of UNUSED_PARAM style. > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNetWin.cpp:239 > + auto cookies = adoptCF(CFArrayCreateMutable(0, 1, &kCFTypeArrayCallBacks)); > + CFArrayAppendValue(cookies.get(), cookie.get()); Not important to optimize, but this could be done more efficiently with CFArrayCreate. Like we do above for CFDictionaryCreate, although it can be done with less type casting by defining local arrays.
Created attachment 405471 [details] Patch
Committed r265048: <https://trac.webkit.org/changeset/265048> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405471 [details].
<rdar://problem/66283757>