Bug 160889 – Clean up WebSockets

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Clean up WebSockets

[Alex Christensen]

Created attachment 286140 [details]
Patch

[Alex Christensen]

Created attachment 286141 [details]
Patch

[Alex Christensen]

Created attachment 286142 [details]
Patch

[Alex Christensen]

Created attachment 286143 [details]
Patch

[Alex Christensen]

Created attachment 286145 [details]
Patch

[Alex Christensen]

Created attachment 286146 [details]
Patch

[Darin Adler]

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.

[Alex Christensen]

Created attachment 286158 [details]
Patch

[Alex Christensen]

Comment on attachment 286158 [details]
Patch

This patch changes something.  Will look into later.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

Created attachment 286180 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/changeset/204512