Bug 45197 – [GTK] Need a WebSocket implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

This should be fairly easy to implement using GIO.

[Martin Robinson]

Created attachment 66558 [details]
Add WebSocket support

[Xan Lopez]

Comment on attachment 66558 [details]
Add WebSocket support

The only comment I have is why the socket source is created and stored at connection time if it's not going to be necessarily used (?). Or is writing less data than what we have in platformSend so common that it's worth to do things that way?

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 66558 [details])
> The only comment I have is why the socket source is created and stored at connection time if it's not going to be necessarily used (?).

Hmm. The GIO documentation seemed unclear about whether or not the reference to the GSocketClient needed to stick around until the connection was finished, so I figured it was safer to keep the reference until it was definitely unused.

> Or is writing less data than what we have in platformSend so common that it's worth to do things that way?

Yeah, we have to handle this. It also presents a good opportunity to start waiting for socket writability. It turns out that if you don't only do this when you need to, the GSource fires continuously and eats up all of your cycles.

[Dan Winship]

Comment on attachment 66558 [details]
Add WebSocket support

> +        PlatformRefPtr<GSocketClient> m_socketClient;

you don't actually need to keep it around; all gio async ops hold a ref on the "source" object for the duration of the async op

> +    PlatformRefPtr<GSocketConnectable> address = g_network_address_new(url.host().utf8().data(), port);
> +    g_socket_client_connect_async(m_socketClient.get(), address.get(), 0,
> +        reinterpret_cast<GAsyncReadyCallback>(connectedCallback), this);

Use g_socket_client_connect_to_host_async() here, then you don't need to create the temporary GNetworkAddress

> +void SocketStreamHandle::stopWaitingForSocketWritability()
> +{
> +    if (!m_writeReadySource || !m_waitingForSocketWritability)
> +        return;
> +    g_source_remove(g_source_get_id(m_writeReadySource.get()));

That's equivalent to "g_source_destroy(m_writeReadySource.get());". In both cases, you can't use the source again after doing that...

> +static gboolean writeReadyCallback(GSocket*, GIOCondition condition, SocketStreamHandle* handle)
> +{
> +    if (!isActiveHandle(handle))
> +        return FALSE;
> +    if (condition & G_IO_OUT)
> +        handle->writeReady();
> +    return TRUE;
> +}

You should check if "condition & (G_IO_ERR | G_IO_HUP)" is set, and fail the connection (and return FALSE) in that case.

[Dan Winship]

higher-level comment; the ickiness on the write end could be removed if the base API were changed to have a "didSend()" call instead of (or even in addition to) sendPending().

[Martin Robinson]

Created attachment 66751 [details]
Patch with Dan's suggestions

[Martin Robinson]

(In reply to comment #5)

Thanks Dan! I've attached a patch with your suggestions. One variation I made was that when the writability callback gets G_IO_HUP, it just calls ::close() on the SocketStreamHandle.

> higher-level comment; the ickiness on the write end could be removed if the base API were changed to have a "didSend()" call instead of (or even in addition to) sendPending().

I agree strongly with this.

[Xan Lopez]

Comment on attachment 66751 [details]
Patch with Dan's suggestions

Let's do this.

[Martin Robinson]

Comment on attachment 66751 [details]
Patch with Dan's suggestions

Clearing flags on attachment: 66751

Committed r66986: <http://trac.webkit.org/changeset/66986>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66986 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/66985
http://trac.webkit.org/changeset/66986