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

Thu Jun 1 18:53:48 PDT 2017

Joseph Pecoraro <joepeck at webkit.org> has granted 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 311782: Patch

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

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

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

> Source/WebCore/ChangeLog:9
> +	   New tests:
> +	    - http/tests/websocket/tests/hybi/inspector/before-load.html

The isn't the right syntax for a ChangeLog. It should be:

    Test: http/tests/websocket/tests/hybi/inspector/before-load.html

> Source/WebCore/Modules/websockets/WebSocket.cpp:155
> +

Style: remove this unnecessary empty line.

> Source/WebCore/Modules/websockets/WebSocket.h:88
> +    const RefPtr<ThreadableWebSocketChannel> channel() const;

I don't think we normally use `const RefPtr`, that doesn't really add any value
over `RefPtr`.

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

Based on your last comments it sounds like you may be able to ASSERT that this
is not the case, or just leave this in.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:41
> +	   test(resolve, reject) {

Can we also assert that the number of WebSocketResource instances is 1:

    let webSocketResources =
WebInspector.frameResourceManager.mainFrame.resourceCollection.resourceCollecti
onForType(WebInspector.Resource.Type.WebSocket);
    InspectorTest.expectEqual(webSocketResources.items.size, 1, "Main frame
should have 1 WebSocketResource.");

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:53
> +	       logReadyState(webSocketResource.readyState);

Style: Just log String(webSocketResource.readyState).
Style: Lets shorten the Symbol descriptions from "web-socket-ready-state-foo"
to just "foo".
Style: We normally phrase expectation messages as "Foo should be bar."
sentences. See other tests for examples.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/before-load.html:63
> +<body onload="testRunner.waitUntilDone();">

Style: I'd prefer this be broken out into a function above. Its super easy to
miss here.