[webkit-reviews] review granted: [Bug 126384] [SOUP] WebSockets must use system proxy settings : [Attachment 330682] Patch with unit tests

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 126384: [SOUP] WebSockets must use system proxy settings
https://bugs.webkit.org/show_bug.cgi?id=126384

Attachment 330682: Patch with unit tests

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




--- Comment #31 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 330682
  --> https://bugs.webkit.org/attachment.cgi?id=330682
Patch with unit tests

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

r=me because I can't find any problems with the code, and I like the new test.
But I'm still concerned about not blocking WebSocket connections when a proxy
is in use in the fallback codepath.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:700
> +    void webSockedConnected(WebSocketServerType serverType)

webSocketConnected

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:56
> +#if SOUP_CHECK_VERSION(2, 50, 0)

Why do we switch to (2, 50, 0) here? What requires libsoup 2.50.0?

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.cpp:117
> +    soup_uri_free(uri);

Why are you not able to use GUniquePtr<SoupURI>?

> Tools/TestWebKitAPI/glib/WebKitGLib/WebKitTestServer.h:55
> +    SoupURI* m_baseURI { nullptr };
> +#if SOUP_CHECK_VERSION(2, 50, 0)
> +    SoupURI* m_baseWebSocketURI { nullptr };
> +#endif

I'd convert these to use GUniquePtr.