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.
Created attachment 311880 [details] Patch
Created attachment 311888 [details] [Image] After Patch is applied
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.
Created attachment 312014 [details] Patch
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.
Created attachment 312251 [details] Patch
Comment on attachment 312251 [details] Patch Clearing flags on attachment: 312251 Committed r217912: <http://trac.webkit.org/changeset/217912>
All reviewed patches have been landed. Closing bug.