At very least, we should show it in the message log.
<rdar://problem/30746031>
Created attachment 305070 [details] Patch
Created attachment 305071 [details] [Image] After Patch is applied
(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.
(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!
(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).
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.
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.
Created attachment 305261 [details] Patch
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
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
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.
Created attachment 305289 [details] Patch
Comment on attachment 305289 [details] Patch Clearing flags on attachment: 305289 Committed r214354: <http://trac.webkit.org/changeset/214354>
All reviewed patches have been landed. Closing bug.