[webkit-reviews] review granted: [Bug 168948] Web Inspector: Indicate whether a WebSocket connection is open or close : [Attachment 305261] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 168948: Web Inspector: Indicate whether a WebSocket connection is open or
close
https://bugs.webkit.org/show_bug.cgi?id=168948

Attachment 305261: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:353
>	   var column = this.dataGrid.columns.get(columnIdentifier);
> +	   if (column) {

Okay, I see what you are doing here. SpanningDataGrid calls
this.createCell("text") and it has data["text"] but the DataGrid might not have
a "text" column.

This feels hacky but I can see why you did it. I could try to create some
generalized code, but I think what you have is fine.

You should eliminate the two `if (column)` checks by moving the `var div`
creation up above this if block.

> Source/WebInspectorUI/UserInterface/Views/SpanningDataGridNode.js:32
> +	   let cellElement = this.createCell("text");

"text" is a generic enough name that this SpanningDataGridNode might
accidentally pick up DataGrid.column.text properties if a column named "text"
exists. I think you should use a less generic name. You can do that with a
constructor taking a string and:

    constructor(text)
    {
	super({"spanning-text": text});
    }

And using "spanning-text" here. Far less likely of a collision since we never
use hyphens in columns names.