Closed
Bug 1172662
Opened 9 years ago
Closed 9 years ago
ICE failures should be reported to the user
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox44 fixed)
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: dmosedale, Assigned: mancas)
References
Details
(Whiteboard: [display error][user quality])
User Story
As a user, when ICE fails under the hood, I should see a message explaining that the call has failed (ideally with suggestions about what I might do next) and offer a "Retry" button. - If an exception of SDK type PUBLISHER_ICE_WORKFLOW_FAILED or SUBSCRIBER_ICE_WORKFLOW_FAILED occurs in OtSdkDriver#_onOTException, then we should: -- Dispatch a ConnectionFailure action with a new FAILURE_DETAILS value. -- Keep the existing exception _notifyMetricsEvent - In the failure view(s), add a case for the new FAILURE_DETAILS value, which selects a new string (string: "Connection failed. Your firewall may be blocking calls."). -- Make sure this is done for both desktop & standalone - Ensure that on standalone & desktop, the failure views have a retry button for this case Dispatching the ConnectionFailure action will hit the existing code which will leave the conversation or end the call attempt.
Attachments
(1 file, 4 obsolete files)
20.48 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
Before 1141296 landed, the sdk displayed a message and there was a failure, and then did nothing, leaving the user stranded. We no longer display a message.
Flags: firefox-backlog+
Comment 2•9 years ago
|
||
Adam: as we fix this - since we just added logging for ice failures - but it's an absolute number. we should start logging ice successes as well so we have a percentage for visibility out of data. Dan: create a new bug for the success part (separate user stories).... that should be P2 - 29.
Rank: 35
Flags: needinfo?(dmose)
Priority: -- → P3
Whiteboard: [display error]
Reporter | ||
Comment 3•9 years ago
|
||
Bug 1179513 spun off.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(dmose)
Updated•9 years ago
|
Points: --- → 5
Rank: 35 → 28
User Story: (updated)
Priority: P3 → P2
Whiteboard: [display error] → [display error][user quality]
Updated•9 years ago
|
User Story: (updated)
Summary: log ICE failures and handle them gracefully → ICE failures should be reported to the user
Comment 4•9 years ago
|
||
Sevaan: Do we have any strings for this already, or can you come up with some. The general failure mode is that due to some kind of issue we've not been able to connect the user - this is most likely network/firewalls being too aggressive and blocking too much. It could be either the user's or the peer's firewall or somewhere in-between. So something like "Couldn't establish a connection. Is your firewall blocking calls?"
Flags: needinfo?(sfranks)
Comment 5•9 years ago
|
||
Is there a sumo article we could link to here (cursory searching doesn't seem to turn up anything)? I worry slightly that "Is your firewall blocking calls" won't mean much to our less tech-savvy users. If we're going to give a helpful message, it would be nice to give some instructions/more information. As for string, what you suggested sounds reasonable. Matej, any way to make it pithier? "Couldn't establish a connection. Is your firewall blocking calls?" "Couldn't establish a connection. Your firewall may be blocking calls."
Flags: needinfo?(sfranks) → needinfo?(matej)
Comment 6•9 years ago
|
||
(In reply to Sevaan Franks [:sevaan] from comment #5) > Is there a sumo article we could link to here (cursory searching doesn't > seem to turn up anything)? I worry slightly that "Is your firewall blocking > calls" won't mean much to our less tech-savvy users. If we're going to give > a helpful message, it would be nice to give some instructions/more > information. > > As for string, what you suggested sounds reasonable. Matej, any way to make > it pithier? > > "Couldn't establish a connection. Is your firewall blocking calls?" > "Couldn't establish a connection. Your firewall may be blocking calls." We could shorten the first part to "Couldn't connect" or "Connection failed." For the second part, I prefer the statement, since the question feels like it relies on the user knowing the answer. "Couldn't connect. Your firewall may be blocking calls." "Connection failed. Your firewall may be blocking calls."
Flags: needinfo?(matej)
Comment 7•9 years ago
|
||
Thanks, Matej! I like the last one: "Connection failed. Your firewall may be blocking calls."
Updated•9 years ago
|
User Story: (updated)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → b.mcb
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8666660 -
Attachment is obsolete: true
Attachment #8667129 -
Flags: review?(standard8)
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8667129 [details] [diff] [review] ICE failures are reported to the user Let me add more unit tests to cover all the cases
Attachment #8667129 -
Flags: review?(standard8)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8667129 -
Attachment is obsolete: true
Attachment #8667138 -
Flags: review?(standard8)
Comment 12•9 years ago
|
||
Comment on attachment 8667138 [details] [diff] [review] ICE failures are reported to the user Review of attachment 8667138 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, unfortunately I couldn't test it as there seems to be a lot of bitrot. Can you try updating your tree, fixing the comments and then reattaching? ::: browser/components/loop/content/shared/js/otSdkDriver.js @@ +919,5 @@ > _onOTException: function(event) { > + switch(event.code) { > + case OT.ExceptionCodes.PUBLISHER_ICE_WORKFLOW_FAILED: > + case OT.ExceptionCodes.SUBSCRIBER_ICE_WORKFLOW_FAILED: > + this.dispatcher.dispatch(new sharedActions.ConnectionFailure({ We should also log the metrics event here. Although we're handling it from a user perspective, we still need to know its happening. (also please add a test for the metrics event for these cases). ::: browser/components/loop/content/shared/js/utils.js @@ +80,5 @@ > COULD_NOT_CONNECT: "reason-could-not-connect", > NETWORK_DISCONNECTED: "reason-network-disconnected", > EXPIRED_OR_INVALID: "reason-expired-or-invalid", > + UNKNOWN: "reason-unknown", > + ICE_FAILED: "ice-failed" Can you make this reason-ice-failed please to be consistent with the rest. We wanted to make this distinct from some of the other failure strings.
Attachment #8667138 -
Flags: review?(standard8)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8667138 -
Attachment is obsolete: true
Attachment #8669567 -
Flags: review?(standard8)
Assignee | ||
Comment 14•9 years ago
|
||
Hey Mark! I took into account your comments, so the patch could be reviewed when you get a chance Thanks!
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8669567 -
Attachment is obsolete: true
Attachment #8669567 -
Flags: review?(standard8)
Attachment #8669604 -
Flags: review?(standard8)
Comment 16•9 years ago
|
||
Comment on attachment 8669604 [details] [diff] [review] ICE failures messages are reported to the user Looks good, r=Standard8
Attachment #8669604 -
Flags: review?(standard8) → review+
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9cb85a2c008e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•9 years ago
|
Iteration: --- → 44.2 - Oct 19
You need to log in
before you can comment on or make changes to this bug.
Description
•