Bug 172878 – Web Inspector: Add ContextMenu item to log WebSocket object to console

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Since WebSocket resources are often kept alive as objects, we should be able to log them and interact with them.  A ContextMenu item in the Resources tab would be awesome.

[Devin Rousso]

Created attachment 311880 [details]
Patch

[Devin Rousso]

Created attachment 311888 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 311880 [details]
Patch

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

This patch looks great, I'm going to r- only so that I can see an update to all of the nits above.

> Source/JavaScriptCore/inspector/protocol/Network.json:200
> +                { "name": "requestId", "$ref": "RequestId", "description": "Id of the websocket to resolve." },

Lets try to make a good description.  "Identifier of the WebSocket resource to resource".

> Source/JavaScriptCore/inspector/protocol/Network.json:206
> +            "description": "Resolves JavaScript WebSocket object for given request id."

I'm a strong proponent of writing the type/command/event descriptions by the name. You'll see it in other JSON files. I think we should do that for all new things. And eventually clean it up for all JSON files.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:806
> +        if (!is<Document>(webSocket->scriptExecutionContext()))
> +            continue;

You should add a FIXME to handle WebSockets in Workers.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:828
> +        errorString = ASCIILiteral("No WebSocket with given requestId found");

Lets just say "WebSocket not found".

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:832
> +    Document* document = downcast<Document>(webSocket->scriptExecutionContext());

You should add a FIXME to handle WebSockets in Workers. I understand that currently this must be a document.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:835
> +        errorString = ASCIILiteral("WebSocket with given requestId does not belong to the document");

Lets clarify this error message.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:842
> +    if (injectedScript.hasNoValue())
> +        return;

I realize the other code paths do this however to not include an error message is a bug. Lets convert this to an ASSERT.

    ASSERT(!injectedScript.hasNoValue());

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:145
> +            if (!callback)
> +                return;

Does it ever make sense to call any of our resolve functions without a callback?

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:150
> +                callback(WebInspector.RemoteObject.fromPayload(object, WebInspector.mainTarget));

This won't work correctly for a WebSocket in a Worker. You should use: WebSocketResource.target, but that probably doesn't exist. So add a FIXME about WebSockets in Workers.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html:69
> +<p>Tests for the Network.resolveWebSocket command.</p>

Excellent test!

> LayoutTests/http/tests/websocket/tests/hybi/inspector/resolveWebSocket_wsh.py:1
> +from mod_pywebsocket import msgutil

This is now the 3rd or 4th WebSocket python file that does the exact same thing. Lets converge them all to a single "echo" file. Either with this patch or in a follow-up.

[Devin Rousso]

Created attachment 312014 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 312014 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:16
> +        Loops over the static allActiveWebSockets to find one with the given requestId.  If found,

Style: one space after a period, always.

> Source/JavaScriptCore/inspector/protocol/Network.json:201
> +                { "name": "requestId", "$ref": "RequestId", "description": "Identifier of the WebSocket resource to resolve" },

Nit: The description is a sentences, end it in a period.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:845
> +    String objectGroupName = objectGroup ? *objectGroup : emptyString();

I think you can just use String() instead of emptyString(). If so that should be slightly cheaper.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:183
> +        super.populateContextMenu(contextMenu, event);
> +
>          WebInspector.appendContextMenuItemsForSourceCode(contextMenu, this._resource);
>  
>          super.populateContextMenu(contextMenu, event);

super.foo() is now here twice. That seems incorrect. Eliminate the new one?

> LayoutTests/http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html:33
> +            const objectGroup = undefined;

We frequently use the "test" object group in tests. You could use that instead of undefined.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/resolveWebSocket.html:54
> +            const requestIdentifier = "-1";

Lets make a more unlikely to exist ID here. Like "DOES_NOT_EXIST" or something super obvious.

[Devin Rousso]

Created attachment 312251 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312251 [details]
Patch

Clearing flags on attachment: 312251

Committed r217912: <http://trac.webkit.org/changeset/217912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.