[webkit-reviews] review denied: [Bug 204137] [GTK][WPE] Add same-site cookie support : [Attachment 383420] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 204137: [GTK][WPE] Add same-site cookie support
https://bugs.webkit.org/show_bug.cgi?id=204137

Attachment 383420: Patch

https://bugs.webkit.org/attachment.cgi?id=383420&action=review




--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 383420
  --> https://bugs.webkit.org/attachment.cgi?id=383420
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=383420&action=review

r- because tests will fail, we need to update the libsoup in jhbuild too.

> Source/WebCore/ChangeLog:11
> +	   Added an httpMethod argument to cookie lookup functions for the
> +	   soup API to use. Modified other backends to simply ignore this.

Do you know why other ports don't need the http method?

> Source/WebCore/loader/CookieJar.cpp:84
> +	   result = session->cookiesForDOM(document.firstPartyForCookies(),
sameSiteInfo(document), url, frameID, pageID, includeSecureCookies,
httpMethod);

We only need the http method to determine whether it's safe, right? and it's
only used by same site cookies, right? If that's the case maybe it would be
easier to add bool isSafeHTTPMethod to SameSitoInfo and we don't need to add a
new parameter. We still have time to change the libsoup API or we can use any
safe HTTP method when it's safe, since it's only used for that.

> Source/WebCore/platform/network/soup/CookieSoup.cpp:35
> +#if SOUP_CHECK_VERSION(2, 69, 0)

Better use the first version including the new api, even if it's an unstable
release. That way we don't need to wait until the next stable release to test
this.

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:486
> +	   httpMethodStr = httpMethod->ascii().data();

This is a temporary, either cache the CString returned by ascii() or do
httpMethod ? httpMethod->ascii().data() : nullptr directly in the call to
soup_cookie_jar_get_cookie_list_with_same_site_info().

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:187
> +	   URL cookieURL = soupURIToURL(siteForCookies);
> +	   setIsSameSite(areRegistrableDomainsEqual(cookieURL, m_url));

This could be one line.

> LayoutTests/ChangeLog:9
> +	   Updated GTK/WPE test expectations to pass most same-site cookie
tests
> +	   matching the Apple ports.

To do this, we need to wait for the next libsoup unstable release and use it in
out jhbuild, otherwise the tests will fail in the bots that use a previous
version.