John, Brent: I know cookies are outside your purview, but perhaps you could triage this to the right folks?

I've started putting together a test suite at http://rfc6265.biz/tests/, which might be helpful if y'all start taking a closer look at this.

(In reply to Mike West from comment #3) > I've started putting together a test suite at http://rfc6265.biz/tests/, > which might be helpful if y'all start taking a closer look at this. The tests seem to be broken. The first four tests either all fail or timeout when run in Chrome Canary 68.0.3398.0. I did not continue to test the other tests linked off of this site given the success of the first four.

(In reply to John Wilander from comment #2) > rdar://problem/27196358 This was duped to...

<rdar://problem/27196358>

Created attachment 338163 [details] Patch and layout tests

Comment on attachment 338163 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338163&action=review > Source/WebCore/loader/CookieJar.cpp:65 > + const auto& request = document.loader()->request(); > + return SameSiteInfo::create(request); Will inline line 64 into line 65 such that this reads: return SameSiteInfo::create(document.loader()->request());

Created attachment 338166 [details] Patch and layout tests

Created attachment 338174 [details] Patch and layout tests

Created attachment 338175 [details] Patch and layout tests

Created attachment 338191 [details] Patch and layout tests

Created attachment 338197 [details] Patch and layout tests

Attachment 338197 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/mac/CookieJarMac.mm:73: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 111 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 338197 [details] Patch and layout tests Attachment 338197 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7355865 New failing tests: http/tests/preload/onload_event.html

Created attachment 338209 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment on attachment 338197 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review This looks reasonable. Is it safe to land for internal machines? > Source/WebCore/loader/CookieJar.cpp:64 > + return SameSiteInfo::create(document.loader()->request()); If we are inlining, might this be better? if (auto loader = document.loader()) return SameSiteInfo::create(loader->request()); return { }] > Source/WebCore/platform/network/mac/CookieJarMac.mm:-151 > - }]; Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue?

Comment on attachment 338197 [details] Patch and layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=338197&action=review >> Source/WebCore/loader/CookieJar.cpp:64 >> + return SameSiteInfo::create(document.loader()->request()); > > If we are inlining, might this be better? > > if (auto loader = document.loader()) > return SameSiteInfo::create(loader->request()); > > return { }] Will change code before landing to match your suggestion excluding the empty line above "return { }" as I do not feel this improves the readability of the code, adding a * after auto for clarity, and substituting a ';' for ']': if (auto* loader = document.loader()) return SameSiteInfo::create(loader->request()); return { }; >> Source/WebCore/platform/network/mac/CookieJarMac.mm:-151 >> - }]; > > Was this synchronous behavior important to the logic? Is changing to 'cookiesForURL' an issue? There is no behavior change. Notice that cookiesForURL is overloaded and I moved this code to cookiesForURL(NSHTTPCookieStorage *, NSURL *, NSURL *, const std::optional<SameSiteInfo>&, NSString *partition). Maybe your remark indicates confusion with having two cookiesForURL()s? I am open to naming suggestions (or other ideas) to improve the clarity of the code and remove the need to overload cookiesForURL().

(In reply to Brent Fulgham from comment #22) > Comment on attachment 338197 [details] > Patch and layout tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338197&action=review > > [...] Is it safe to land for internal machines? > Builds and runs on my Apple Internal machine :)

Committed r230921: <https://trac.webkit.org/changeset/230921>

This change broke 32-bit macOS builds: ./platform/network/mac/CookieJarMac.mm:101:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertySiteForCookies' ./platform/network/mac/CookieJarMac.mm:102:21: error: use of undeclared identifier '_kCFHTTPCookiePolicyPropertyIsTopLevelNavigation' https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20%2832-bit%20Build%29/builds/5399

(In reply to Ryan Haddad from comment #26) > This change broke 32-bit macOS builds: Correction: It broke all the builds.

(In reply to Ryan Haddad from comment #27) > (In reply to Ryan Haddad from comment #26) > > This change broke 32-bit macOS builds: > > Correction: It broke all the builds. Committed build fixes in <https://trac.webkit.org/changeset/230923> and <https://trac.webkit.org/changeset/230924>

Re-opened since this is blocked by bug 184903

Here were the remaining failures after the last build fix attempt: https://build.webkit.org/builders/Apple%20Sierra%20Release%20%2832-bit%20Build%29/builds/10535 https://build.webkit.org/builders/Apple%20Win%20Release%20(Build)/builds/8989 https://build.webkit.org/builders/Apple%20Win%20Debug%20(Build)/builds/8584

Committed r230944: <https://trac.webkit.org/changeset/230944>

Committed ChangeLog fix in <https://trac.webkit.org/changeset/230945>. Committed build fix in <https://trac.webkit.org/changeset/230946>

Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230947>.

Another attempt to fix the build committed in <https://trac.webkit.org/changeset/230948>.

Committed r230981: <https://trac.webkit.org/changeset/230981>