[Webkit-unassigned] [Bug 166967] [SOUP] Simplify cookie storage handling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=166967

--- Comment #6 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 298670
  --> https://bugs.webkit.org/attachment.cgi?id=298670
Try to fix EFL build

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

I have a couple questions.

Also, please look at bug #166029.

> 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.

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?

> Source/WebCore/ChangeLog:41
> +        (WebCore::NetworkStorageSession::cookieStorage): Return the cookie sotrage from the SoupNetworkSession if we

sotrage -> storage

> 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);

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.

> 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();
>  }

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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170112/213272be/attachment.html>