Bug 73093 – InspectorServer: Add a Generic WebSocket Server.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Jocelyn Turcotte]

Created attachment 116539 [details]
Original New Part 3 patch

Tracks the "New Part 3" and "New Part 4" patches of bug #51364.

[Jocelyn Turcotte]

Created attachment 116540 [details]
Original New Part 4 patch

[Jocelyn Turcotte]

Created attachment 116541 [details]
Patch

This replaces parts 3 and 4.
The shared logic was exposed as static methods of WebSocketHandshake instead of straight in the namespace as Patch 3 did.

[Jocelyn Turcotte]

Created attachment 117917 [details]
Patch

Changes from last patch:
- Removed the server socket abstraction (bug #73090) and merged it into WebSocketServer.
- Moved WebSocketServer from WebCore to WebKit2 until it needs to belong there. It still uses static methods from the client WebSocket code inWebCore.

[Yael]

You should merge the patch from https://gitorious.org/~jturcotte/webkit/jturcottes-webkit/commit/4512d6e8a37bad10fa9fd20286fc24b6339a2d9b into this one.
Without that patch I was getting a protocol error.

[Vsevolod Vlasov]

Comment on attachment 117917 [details]
Patch

r- per comment #4

[Jocelyn Turcotte]

Created attachment 134621 [details]
Patch

Rebased and merged in the fixup patch specified in comment #4

[Early Warning System Bot]

Comment on attachment 134621 [details]
Patch

Attachment 134621 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12194417

[Kenneth Rohde Christiansen]

Comment on attachment 134621 [details]
Patch

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

This looks pretty good to me.

> Source/WebKit2/ChangeLog:10
> +        that handles passes on what it doesn't know on to its Client for extended

client*

> Source/WebKit2/ChangeLog:13
> +        There are no tests yet for a built-in WebSocket server.

Better explain why

> Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:193
> +    m_p = new SocketStreamHandlePrivate(this, socket);

I think m_impl is more common in webkit code

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:31
> +#include "config.h"
> +
> +#if ENABLE(INSPECTOR_SERVER)
> +
> +#include "WebSocketServer.h"

Wouldn't it makes sense to include the config.h after the ENABLE and then immediately follow it by the header file

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:85
> +    for (Deque<OwnPtr<WebSocketServerConnection> >::iterator iter = m_connections.begin(); iter != end; ++iter) {

nit, we normally just use 'it' and not 'iter'

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:89
> +            m_connections.remove(iter);
> +            break;
> +        }

should we assert if it is not found?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerClient.h:46
> +    // Received an HTTP request but don't know what to do with it.

didn't know ?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:41
> +#include <wtf/UnusedParam.h>

Where are you using that?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:87
> +    WebSocketFrame frame(WebSocketFrame::OpCodeText, true, false, false, payload.data(), payload.length());

true, false, false :-) Very hard to understand what that means, comments?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:104
> +    for (HTTPHeaderMap::const_iterator iter = headerFields.begin(); iter != end; ++iter) {

it?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:146
> +    // HTTP Mode.
> +    if (m_mode == HTTP) {
> +        readHTTPMessage();
> +        return;
> +    }
> +
> +    // Web Socket Mode.
> +    if (m_mode == WebSocket) {
> +        readWebSocketFrames();
> +        return;
> +    }

why not switch?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:156
> +void WebSocketServerConnection::didFailSocketStream(SocketStreamHandle*, const SocketStreamError&)
> +{
> +    // Possible read or write error.
> +}
> +

ASSERT_NOT_REACHED?

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:175
> +    if (upgradeHeaderValue == "WebSocket")
> +        LOG_ERROR("WebSocket protocol version < Hybi-10 not supported. Upgrade your client.");

No return here? comment?

[Simon Hausmann]

Comment on attachment 134621 [details]
Patch

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

Only nitpicks from my side, after Kenneth's review. Let's do another round :)

>> Source/WebKit2/ChangeLog:10
>> +        that handles passes on what it doesn't know on to its Client for extended
> 
> client*

and remove "handles", so the sentence just reads "... server that passes on what it doesn't know ..."

> Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:90
> +#ifndef QT_NO_OPENSSL

It's not exactly related to your patch, but I don't think we should disable the websocket feature altogether if Qt comes without SSL support. So I don't think we need the #ifdef here :)

> Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:111
> +    CString header = builder.toString().latin1();

I hope this is as efficient as we want it to be ;)

[Jocelyn Turcotte]

(In reply to comment #8)
> > Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:193
> > +    m_p = new SocketStreamHandlePrivate(this, socket);
> 
> I think m_impl is more common in webkit code
> 
Humm it's not quite an impl here, it's more just a class that inherits QObject to allow us using signals/slots. Since I didn't chose the name in this patch I think I'll leave it.

> > Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:31
> > +#include "config.h"
> > +
> > +#if ENABLE(INSPECTOR_SERVER)
> > +
> > +#include "WebSocketServer.h"
> 
> Wouldn't it makes sense to include the config.h after the ENABLE and then immediately follow it by the header file
> 
I think ENABLE is ultimately defined through config.h.

> > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:156
> > +void WebSocketServerConnection::didFailSocketStream(SocketStreamHandle*, const SocketStreamError&)
> > +{
> > +    // Possible read or write error.
> > +}
> > +
> 
> ASSERT_NOT_REACHED?
> 
An assert could be bothersome in unstable networks, without much benefit since the code should work in those situations anyway.

(In reply to comment #9)
> > Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:90
> > +#ifndef QT_NO_OPENSSL
> 
> It's not exactly related to your patch, but I don't think we should disable the websocket feature altogether if Qt comes without SSL support. So I don't think we need the #ifdef here :)
> 
Humm I'm not sure I see what you mean, this is only supposed to guard references to QSslSocket.

> > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:111
> > +    CString header = builder.toString().latin1();
> 
> I hope this is as efficient as we want it to be ;)
Well I didn't notice any slowdown yet, neither on devices :)

I'm uploading a patch fixing the rest.

[Jocelyn Turcotte]

Created attachment 135097 [details]
Patch

Fixing the rest.

[Early Warning System Bot]

Comment on attachment 135097 [details]
Patch

Attachment 135097 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12306786

[Jocelyn Turcotte]

Committed r113026: <http://trac.webkit.org/changeset/113026>