[webkit-reviews] review denied: [Bug 172312] Web Inspector: Should see active Web Sockets when opening Web Inspector : [Attachment 311753] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 172312: Web Inspector: Should see active Web Sockets when opening Web
Inspector
https://bugs.webkit.org/show_bug.cgi?id=172312

Attachment 311753: Patch

https://bugs.webkit.org/attachment.cgi?id=311753&action=review




--- Comment #10 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311753
  --> https://bugs.webkit.org/attachment.cgi?id=311753
Patch

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

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:681
> +	   if (channel->handshakeMode() != WebSocketHandshake::Connected)
> +	       didReceiveWebSocketHandshakeResponse(identifier,
channel->serverHandshakeResponse());

This condition doesn't make sense. If the mode is Incomplete won't this be
empty?

It seems like this should be:

    if (webSocket->readyState() != CONNECTING)

In which case you wouldn't need to expose the handshakeMode. But I didn't look
deeply, maybe the handshake mode is more accurate.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:684
> +	   if (webSocket->readyState() == WebSocket::CLOSED)
> +	       didCloseWebSocket(identifier);

We should extend your test below to cover this.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:11
> +const url =
"ws://127.0.0.1:8880/websocket/tests/hybi/inspector/before-load";
> +let webSocket = new WebSocket(url);
> +webSocket.addEventListener("open", (event) => {
> +    // Only run the tests once the websocket has connected.
> +    runTest();
> +});

Lets add a test for two web sockets.

    (1) one that is open and connected (window.webSocketConnected)
    (2) one that is closed but still alive (window.webSocketClosed)

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:21
> +	       InspectorTest.evaluateInPage("webSocket.close()");

Why are we doing this?

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:25
> +		   InspectorTest.pass(`WebSocket resource exists for url
"${url}"`);

You should confirm that this is a WebInspector.WebSocket and that its
request/response data has headers etc.