[webkit-reviews] review denied: [Bug 172878] Web Inspector: Add ContextMenu item to log WebSocket object to console : [Attachment 311880] 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 172878: Web Inspector: Add ContextMenu item to log WebSocket object to
console
https://bugs.webkit.org/show_bug.cgi?id=172878

Attachment 311880: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 311880
  --> https://bugs.webkit.org/attachment.cgi?id=311880
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.