Bug 168948 – Web Inspector: Indicate whether a WebSocket connection is open or close

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

[Nikita Vasilyev]

At very least, we should show it in the message log.

[Radar WebKit Bug Importer]

<rdar://problem/30746031>

[Devin Rousso]

Created attachment 305070 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> Created attachment 305071 [details]
> [Image] After Patch is applied

Why not have the open/closed state be part of the regular status icon? The default "blank document" doesn't add anything currently.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #4)
> (In reply to Devin Rousso from comment #3)
> > Created attachment 305071 [details]
> > [Image] After Patch is applied
> 
> Why not have the open/closed state be part of the regular status icon? The
> default "blank document" doesn't add anything currently.

Good thinking, I like it!

[Devin Rousso]

(In reply to Matt Baker from comment #4)
> (In reply to Devin Rousso from comment #3)
> > Created attachment 305071 [details]
> > [Image] After Patch is applied
> 
> Why not have the open/closed state be part of the regular status icon? The
> default "blank document" doesn't add anything currently.

While I do agree that we should do something better with the "blank document", I don't like the idea of conveying the "state" of the resource via that icon.  I think that having the info in the "status" area makes more sense since we already have similar UI for that space (namely the loading indicator).

[Joseph Pecoraro]

Comment on attachment 305070 [details]
Patch

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

This looks great to me except that one Scenario I have below. Does that behave as expected for you?

> Source/WebInspectorUI/ChangeLog:27
> +         - Closed: show no status indicator
> +         - Connecting: show a small circle with a yellow color
> +         - Open: show a small circle with a green color

I agree with these choices. I wonder if yellow is hard to see. I wonder what we could do in those cases (green circle that is not filled in?). I don't think this is something we need to change now, but something to consider. I'm not sure if there is a media query to detect cases where we might want to improve contrast.

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

Can this really return no column?! That would make me very worried about a lot of code.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.css:40
> +    -webkit-clip-path: circle(50% at 50% 50%);

Cool!!

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.css:46
> +    background-color: limegreen;

Now you're just showing off.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:129
> +    _updateState(event)
> +    {
> +        if (this._resource.readyState === WebInspector.WebSocketResource.ReadyState.Closed)
> +            this._dataGrid.appendChild(new WebInspector.SpanningDataGridNode({text: WebInspector.UIString("Connection Closed")}));
> +    }

This looks like we will add a new "Connection Closed" data grid node every time we show the WebSocketResource that is closed.

Scenario:

    1. Inspect a page with web sockets
    2. Close a web socket
    3. Show the Web Socket Content View
        => shown() should add the spanning data grid node
    4. Leave and go to another content view
        => hidden() should be called
    5. Show the Web Socket Content View
        => shown() should add another spanning data grid node

This could be remedied by having a simple boolean, like _framesRendered for whether or not we've added the "Closed" data grid node.

[Devin Rousso]

Comment on attachment 305070 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:27
>> +         - Open: show a small circle with a green color
> 
> I agree with these choices. I wonder if yellow is hard to see. I wonder what we could do in those cases (green circle that is not filled in?). I don't think this is something we need to change now, but something to consider. I'm not sure if there is a media query to detect cases where we might want to improve contrast.

I think that a hollow green circle is a bit hard to see and isn't as clear as a completely different color, but I have yet to actually see the WebSocket in a "connecting" state (our internet is very fast) so I think we can resolve this later if needed.

>> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:353
>> +        if (column) {
> 
> Can this really return no column?! That would make me very worried about a lot of code.

The reason for this is that a SpanningDataGridNode isn't really supposed to conform to a specific column.  Based on what I can see, it seems to be fine for this particular use case, but I am not sure about others.  Any suggestions?

>> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:129
>> +    }
> 
> This looks like we will add a new "Connection Closed" data grid node every time we show the WebSocketResource that is closed.
> 
> Scenario:
> 
>     1. Inspect a page with web sockets
>     2. Close a web socket
>     3. Show the Web Socket Content View
>         => shown() should add the spanning data grid node
>     4. Leave and go to another content view
>         => hidden() should be called
>     5. Show the Web Socket Content View
>         => shown() should add another spanning data grid node
> 
> This could be remedied by having a simple boolean, like _framesRendered for whether or not we've added the "Closed" data grid node.

Good catch.

[Devin Rousso]

Created attachment 305261 [details]
Patch

[Build Bot]

Comment on attachment 305261 [details]
Patch

Attachment 305261 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3401458

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html

[Build Bot]

Created attachment 305265 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 305261 [details]
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.

[Devin Rousso]

Created attachment 305289 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 305289 [details]
Patch

Clearing flags on attachment: 305289

Committed r214354: <http://trac.webkit.org/changeset/214354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.