[Webkit-unassigned] [Bug 200076] [SOUP] Move SoupNetworkSession ownership from NetworkStorageSession to NetworkSession

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #374771|1                           |0
        is obsolete|                            |

--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 374771
  --> https://bugs.webkit.org/attachment.cgi?id=374771
Patch

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

Nice, just don't break Mac please.

Also it needs an owner for the changes in NetworkConnectionToWebProcess.cpp, NetworkProcess.cpp, NetworkProcess.h, NetworkProcessCocoa.mm, and NetworkProcessIOS.mm.

> Source/WebCore/platform/network/NetworkStorageSession.h:99
> -    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID, std::unique_ptr<SoupNetworkSession>&&);
> +    WEBCORE_EXPORT NetworkStorageSession(PAL::SessionID);

explicit

> Source/WebCore/platform/network/soup/NetworkStorageSessionSoup.cpp:58
> +    , m_cookieStorage(adoptGRef(soup_cookie_jar_new()))

It's slightly unfortunate that we create this empty SoupCookieJar and then blow it away shortly thereafter in the usual case when setCookieStorage is called right after it's constructed, from the NetworkSessionSoup.cpp constructor. But that seems hard to avoid, so I suppose this is fine.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:-131
> -        SOUP_SESSION_ADD_FEATURE, jar.get(),

Important: where does this happen now?

If this is totally gone, then surely cookies won't work anymore?

I'm confused.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:692
> +    networkProcess().forEachNetworkSession([statistics = WTFMove(statistics) ](NetworkSession& networkSession) mutable {

No space before the ]

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:602
> +#if !USE(SOUP)
>      ASSERT(sessionID != PAL::SessionID::defaultSessionID());
> +#endif

Really needs a comment since people reading the code have no chance to know why soup is different here. "Refer to the comment in NetworkProcessMainSoup.cpp for why this needs to be destroyed" would suffice.

Actually, a comment would be bare minimum, but perhaps it would be better to remove this ASSERT altogether. There's no reason to prevent destroying the default NetworkSession on other ports if we allow it for soup, because the cross-platform code has to allow it now. I think the ASSERT is here because a bazillion functions will likely crash if the default NetworkSession is destroyed, so it's a little risky to remove, but I understand that the SoupSession must be destroyed no matter what, and if it's owned by the NetworkSession that means the NetworkSession must be too, and if it's done late enough it should be OK.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2166
> -    for (auto& session : m_networkSessions)
> -        session.value->storageManager().resume();
> +    forEachNetworkSession([](NetworkSession& session) {
> +        session.storageManager().resume();
> +    });

I'll let owners decide whether this sort of transformation is valuable. On the one hand, now that you've added forEachNetworkSession it makes sense to use it wherever possible, and seeing the name of the function at callsites makes it very clear that you are doing a thing for each NetworkSession. On the other hand, within NetworkProcess.cpp itself the for loop syntax is shorter. Either way is fine IMO.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190724/9b688403/attachment.html>