Indicate that WS connection is closed
Categories
(DevTools :: Netmonitor, enhancement, P3)
Tracking
(firefox73 fixed)
Tracking | Status | |
---|---|---|
firefox73 | --- | fixed |
People
(Reporter: Honza, Assigned: saihemanth9019)
References
(Blocks 2 open bugs)
Details
(Keywords: good-first-bug)
User Story
When a WS connection closes, I want to see a clear indicator in the Messages panel, so that I don't wait for additional messages.
Attachments
(3 files)
There should be a message displayed at the end of the message list (if WS frame list) indicating that the (WS) connection has been closed.
https://www.figma.com/file/OWflRU1bKHXMR6nh921AQiMD/Network-WebSocket-Inspector?node-id=1%3A5
Honza
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Closing a WS can provide a code
and/or reason
that could be displayed here as well: https://html.spec.whatwg.org/multipage/web-sockets.html#the-websocket-interface:dom-websocket-close
Reporter | ||
Comment 3•5 years ago
|
||
Yep, we have this callback:
https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/devtools/client/netmonitor/src/connector/firefox-data-provider.js#457
/**
* The "webSocketClosed" message type handler.
*
* @param {boolean} wasClean
* @param {number} code
* @param {string} reason
*/
async onWebSocketClosed(wasClean, code, reason) {}
code & reason are described in the spec, but there is also wasClean
.
Baku, what the wasClean
argument indicates?
Something interesting for our users?
Honza
Reporter | ||
Updated•5 years ago
|
Comment 4•5 years ago
|
||
'wasClean' is set to false when the webSocket is terminated because of an error. For instance, the page is reloaded, or a network failure can trigger a 'wasClean'=false close event.
Reporter | ||
Comment 5•5 years ago
|
||
Attched screenshot showing the notification about truncated messages.
@Harald, do we want something (visually) similar for the "connection closed" notification?
Honza
Reporter | ||
Comment 6•5 years ago
|
||
Ah, just found the Figma link.
Mockup showing how to message should look like attached.
Honza
Reporter | ||
Comment 7•5 years ago
|
||
Some instructions for anyone interested in fixing this bug
-
The message should be rendered at the end of the WS frame list.
The list is rendered here:
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/client/netmonitor/src/components/websockets/FrameListContent.js#320 -
Here is the backend place where we have a hook for observing closed notifications
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/server/actors/network-monitor/websocket-actor.js#92
The method emits an event that is sent to the client side (front end, web socket panel UI):
this.emit("serverWebSocketClosed", wasClean, code, reason);
It needs to also send "httpChannelId" so, the client side can identify, which websocket connection is closed.
It can be done as follows:
const httpChannelId = this.connections.get(webSocketSerialID);
(see frameReceived
method in the same file)
Also, RDP packet spec needs to be updated if new field is added
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/shared/specs/websocket.js#23
- Here is a callback executed on the client side (when receiving the emitted packet)
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/client/netmonitor/src/connector/firefox-data-provider.js#460
It should have new first argument httpChannelId
async onWebSocketClosed(httpChannelId, wasClean, code, reason) {}
The method should fire a new Redux action, call something like:
if (this.actionsEnabled && this.actions.closeConnection {
await this.actions.closeConnection(httpChannelId, wasClean, code, reason);
}
The action should be implemented (and exported) in this file
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/actions/web-sockets.js
- The action should be handled in WebSocket reducer
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/client/netmonitor/src/reducers/web-sockets.js#189
We could introduce a new map: closedConnections
httpChannelId => boolean, where true == closed
in the reducer
Consequently we could implement a selector isCurrentChannelClosed
in this file:
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/client/netmonitor/src/selectors/web-sockets.js
- Finally, we could access the selector in the
connect
method here
https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/devtools/client/netmonitor/src/components/websockets/FrameListContent.js#342
and call is as follows:
state => ({
selectedFrame: getSelectedFrame(state),
frames: getDisplayedFrames(state),
columns: state.webSockets.columns,
isClosed: isCurrentChannelClosed(state),
}),
The isClosed
property would be utilized in the render method (see step #1) and if true, the final "Connection closed" message would be rendered (perhaps together with a reason). The message (not the reason) should be properly localized.
Honza
Reporter | ||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
Hey Honza,
I'd like to work on this as my first bug. I've worked with this stack before and the fix is pretty clear. Can I get the repo link for the same? I've looked at mozilla-central
but it seems like that's not being used.
saihemanth9019
Reporter | ||
Comment 9•5 years ago
|
||
(In reply to saihemanth9019 from comment #8)
I'd like to work on this as my first bug. I've worked with this stack before and the fix is pretty clear.
Excellent, thanks!
Can I get the repo link for the same? I've looked at
mozilla-central
but it seems like that's not being used.
Not sure if I understand the question, you need to clone mozilla-central: hg clone https://hg.mozilla.org/mozilla-central
You can read these instructions: https://docs.firefox-dev.tools/getting-started/build.html
Honza
Assignee | ||
Comment 11•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
I've made the changes and submitted a revision. Added 'honza' as the reviewer.
Comment 13•5 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c84f90739d8e Indicate that WS connection is closed. r=Honza,Harald
Comment 14•5 years ago
|
||
bugherder |
Description
•