<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Simplify cookie storage handling"
href="https://bugs.webkit.org/show_bug.cgi?id=166967#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [SOUP] Simplify cookie storage handling"
href="https://bugs.webkit.org/show_bug.cgi?id=166967">bug 166967</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=298670&action=diff" name="attach_298670" title="Try to fix EFL build">attachment 298670</a> <a href="attachment.cgi?id=298670&action=edit" title="Try to fix EFL build">[details]</a></span>
Try to fix EFL build
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=298670&action=review">https://bugs.webkit.org/attachment.cgi?id=298670&action=review</a>
I have a couple questions.
Also, please look at <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] SoupCookieJar is never released (resulting in sqlite temp files lying around)"
href="show_bug.cgi?id=166029">bug #166029</a>.
<span class="quote">> Source/WebCore/ChangeLog:11
> + NetworkStorageSession and removing all create() methods from SoupNetworkSession. This patch also removes the
> + default SoupNetworkSession in favor of using the default NetworkStorageSession.</span >
That's a big change to hide in a patch named "Simplify cookie storage handling". Then again, I'm probably not one to complain, having just tried to sneak WebKitSecurityOrigin into a notifications patch... but consider landing in two parts if you have time.
But I'm curious: what was the motivation for this change?
<span class="quote">> Source/WebCore/ChangeLog:41
> + (WebCore::NetworkStorageSession::cookieStorage): Return the cookie sotrage from the SoupNetworkSession if we</span >
sotrage -> storage
<span class="quote">> Source/WebCore/platform/network/soup/DNSSoup.cpp:82
> - g_object_get(SoupNetworkSession::defaultSession().soupSession(), "proxy-resolver", &resolver.outPtr(), nullptr);
> + g_object_get(NetworkStorageSession::defaultStorageSession().soupNetworkSession().soupSession(), "proxy-resolver", &resolver.outPtr(), nullptr);</span >
Shame it's a lot more typing to get the SoupNetworkSession for the default NetworkStorageSession than it was to just get the default SoupNetworkSession. Hence why I'm curious to know your reasoning for changing this.
<span class="quote">> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:110
> +SoupCookieJar* NetworkStorageSession::cookieStorage() const
> +{
> + if (m_session) {
> + ASSERT(m_session->cookieJar());
> + return m_session->cookieJar();
> + }
> +
> + if (!m_cookieStorage) {
> + m_cookieStorage = adoptGRef(soup_cookie_jar_new());
> + soup_cookie_jar_set_accept_policy(m_cookieStorage.get(), SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY);
> + }
> + return m_cookieStorage.get();
> }</span >
What? What is the advantage of having the NetworkStorageSession and SoupNetworkSession store separate pointers to the SoupCookieJar? It looks like you intentionally allow a SoupNetworkSession to "override" the SoupCookieJar of the NetworkStorageSession. Why is that?
This has a serious disadvantage that you have to create a SoupCookieJar in two different places, and now have the default accept policy hardcoded in two different places. If you really want to do this, then you should introduce some global defaultCookieAcceptPolicy() function somewhere that returns SOUP_COOKIE_JAR_ACCEPT_NO_THIRD_PARTY. But even avoiding that problem, I don't understand why you've done this. That the implementation of what should be a simple const getter function is so convoluted should be a warning sign.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>