Created attachment 116539 [details] Original New Part 3 patch Tracks the "New Part 3" and "New Part 4" patches of bug #51364.
Created attachment 116540 [details] Original New Part 4 patch
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.
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.
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 on attachment 117917 [details] Patch r- per comment #4
Created attachment 134621 [details] Patch Rebased and merged in the fixup patch specified in comment #4
Comment on attachment 134621 [details] Patch Attachment 134621 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12194417
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 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 ;)
(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.
Created attachment 135097 [details] Patch Fixing the rest.
Comment on attachment 135097 [details] Patch Attachment 135097 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12306786
Committed r113026: <http://trac.webkit.org/changeset/113026>