Bug 73093 - InspectorServer: Add a Generic WebSocket Server.
Summary: InspectorServer: Add a Generic WebSocket Server.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Jocelyn Turcotte
URL:
Keywords:
Depends on:
Blocks: 73094
  Show dependency treegraph
 
Reported: 2011-11-24 13:23 PST by Jocelyn Turcotte
Modified: 2012-04-03 06:34 PDT (History)
7 users (show)

See Also:


Attachments
Original New Part 3 patch (20.79 KB, patch)
2011-11-24 13:23 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Original New Part 4 patch (44.36 KB, patch)
2011-11-24 13:23 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (38.62 KB, patch)
2011-11-24 13:28 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (40.32 KB, patch)
2011-12-05 12:34 PST, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (41.30 KB, patch)
2012-03-29 11:17 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Patch (41.40 KB, patch)
2012-04-02 07:33 PDT, Jocelyn Turcotte
hausmann: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2011-11-24 13:23:33 PST
Created attachment 116539 [details]
Original New Part 3 patch

Tracks the "New Part 3" and "New Part 4" patches of bug #51364.
Comment 1 Jocelyn Turcotte 2011-11-24 13:23:52 PST
Created attachment 116540 [details]
Original New Part 4 patch
Comment 2 Jocelyn Turcotte 2011-11-24 13:28:53 PST
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.
Comment 3 Jocelyn Turcotte 2011-12-05 12:34:01 PST
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.
Comment 4 Yael 2012-01-14 12:50:55 PST
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.
Comment 5 Vsevolod Vlasov 2012-02-14 15:15:02 PST
Comment on attachment 117917 [details]
Patch

r- per comment #4
Comment 6 Jocelyn Turcotte 2012-03-29 11:17:54 PDT
Created attachment 134621 [details]
Patch

Rebased and merged in the fixup patch specified in comment #4
Comment 7 Early Warning System Bot 2012-03-29 11:54:16 PDT
Comment on attachment 134621 [details]
Patch

Attachment 134621 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12194417
Comment 8 Kenneth Rohde Christiansen 2012-04-02 04:48:50 PDT
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?
Comment 9 Simon Hausmann 2012-04-02 05:24:36 PDT
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 ;)
Comment 10 Jocelyn Turcotte 2012-04-02 07:32:17 PDT
(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.
Comment 11 Jocelyn Turcotte 2012-04-02 07:33:55 PDT
Created attachment 135097 [details]
Patch

Fixing the rest.
Comment 12 Early Warning System Bot 2012-04-02 11:01:45 PDT
Comment on attachment 135097 [details]
Patch

Attachment 135097 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12306786
Comment 13 Jocelyn Turcotte 2012-04-03 06:34:59 PDT
Committed r113026: <http://trac.webkit.org/changeset/113026>