Clean up WebSockets
Created attachment 286140 [details] Patch
Created attachment 286141 [details] Patch
Created attachment 286142 [details] Patch
Created attachment 286143 [details] Patch
Created attachment 286145 [details] Patch
Created attachment 286146 [details] Patch
Comment on attachment 286146 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286146&action=review Looks OK, but could be improved more. Seems that it introduces some small amount of new sloppiness. > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:313 > + LOG(Network, "WebSocketChannel %p didReceiveSocketStreamData() Received %zu bytes", this, len); This is going to look crazy when len is std::numeric_limits<size_t>::max(). > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:319 > + if (len == std::numeric_limits<size_t>::max()) { Why do we need this magic value here? Isn’t there a cleaner way to do this, like using a separate function for the "need to disconnect" case. Or using Optional to indicate there is no length? Or could we at least make this magic value a named constant? I don’t see the patch changing the call site, so I am concerned that this might not even be the correct value. > Source/WebCore/platform/network/SocketStreamHandle.cpp:69 > - if (bytesWritten < 0) > + if (bytesWritten == sendError) > return false; This should be nested inside the if statement above. > Source/WebCore/platform/network/SocketStreamHandle.h:58 > + const size_t sendError = std::numeric_limits<size_t>::max(); > + virtual size_t platformSend(const char* data, size_t length) = 0; Why use a magic value to indicate a send error? To indicate an error we could use Optional or some other technique without a magic number. > Source/WebCore/platform/network/soup/SocketStreamHandleImplSoup.cpp:192 > + return static_cast<size_t>(written); This relies on casting a -1 to size_t yielding std::numeric_limits<size_t>::max(), something I believe the patch avoids in the CFNetwork version. > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.h:74 > + void didOpenSocketStream(WebCore::SocketStreamHandle&) override { } > void didCloseSocketStream(WebCore::SocketStreamHandle&) override; > - void didReceiveSocketStreamData(WebCore::SocketStreamHandle&, const char* data, int length) override; > + void didReceiveSocketStreamData(WebCore::SocketStreamHandle&, const char* data, size_t length) override; > void didUpdateBufferedAmount(WebCore::SocketStreamHandle&, size_t bufferedAmount) override; > + void didFailSocketStream(WebCore::SocketStreamHandle&, const WebCore::SocketStreamError&) override { } Should use final instead of override.
Created attachment 286158 [details] Patch
Comment on attachment 286158 [details] Patch This patch changes something. Will look into later.
Comment on attachment 286158 [details] Patch Attachment 286158 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1879909 New failing tests: http/tests/websocket/tests/hybi/handshake-ok-with-legacy-sec-websocket-response-headers.html http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Created attachment 286165 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 286158 [details] Patch Attachment 286158 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1879917 New failing tests: http/tests/websocket/tests/hybi/handshake-ok-with-legacy-sec-websocket-response-headers.html http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Created attachment 286166 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 286158 [details] Patch Attachment 286158 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1879925 New failing tests: http/tests/websocket/tests/hybi/handshake-ok-with-legacy-sec-websocket-response-headers.html http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html
Created attachment 286168 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286180 [details] Patch
https://trac.webkit.org/changeset/204512