[webkit-reviews] review granted: [Bug 169945] Web Inspector: add context menu item to log content of WebSocket frame : [Attachment 305263] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 24 00:20:34 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 169945: Web Inspector: add context menu item to log content of WebSocket
frame
https://bugs.webkit.org/show_bug.cgi?id=169945

Attachment 305263: Patch

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




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

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

r=me. I'm still iffy on the strings. I think we may want to discuss them. But
the patch looks great otherwise.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127
> -	       node = new WebInspector.WebSocketDataGridNode({data, time,
isOutgoing});
> +	       node = new
WebInspector.WebSocketDataGridNode(Object.shallowMerge({data, time},
attributes));
>	   else
> -	       node = new WebInspector.WebSocketDataGridNode({data,
isOutgoing});
> +	       node = new
WebInspector.WebSocketDataGridNode(Object.shallowMerge({data}, attributes));

I think we can use Object.assign instead of Object.shallowMerge. I think using
builtins might be more natural here. What do you think?

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:66
> +		       const wasThrown = false;
> +		       logResult(remoteObject, wasThrown, savedResultIndex);

Style: I don't actually think the const variable here, since we are calling a
local function so close by that its easy to check.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:72
> +		   let json = JSON.parse(this._data.data);
> +		   if (json) {

This doesn't allow for valid falsey values (undefined, 0, null, ""). Maybe we
don't care about those, but it would be odd to allow `true` but not `false`.

> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75
> +		       if (Array.isArray(json))
> +			   title = WebInspector.UIString("Log Frame as Array");

Could handle type "boolean". The JSON site has a super simple at a glance
diagram: <http://www.json.org>

How do all of these customizations feel? Is just `Log Value` enough or do these
customizations make it feel nicer?


More information about the webkit-reviews mailing list