Closed Bug 1131255 Opened 7 years ago Closed 6 years ago

Use ICE connection state to better determine when a video call is successful

Categories

(Instantbird :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1018060

People

(Reporter: aleth, Assigned: aleth)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

The existing patch assumes a video call is established when we send the accept call stanza. In fact, we should only infer the call has been successful if the ICE connection process is successful.

Since ICE can take a while, there should also be a notification bar state for this waiting period.
Attached patch fixice.diffSplinter Review
Also tidies up some of the notification box stuff a bit.
Attachment #8561638 - Flags: review?(clokep)
Comment on attachment 8561638 [details] [diff] [review]
fixice.diff

Review of attachment 8561638 [details] [diff] [review]:
-----------------------------------------------------------------

clokep said he'd prefer flo would review this.
Attachment #8561638 - Flags: review?(clokep) → review?(florian)
Comment on attachment 8561638 [details] [diff] [review]
fixice.diff

Review of attachment 8561638 [details] [diff] [review]:
-----------------------------------------------------------------

r+ as this looks good overall, but I'm not entirely confident in my review because all the unrelated refactoring/cleanup changes were distracting me from the main point of the bug. I'm not going to waste your time by requiring you split them into separate patches though.

::: im/content/conversation.xml
@@ +356,5 @@
>            // Display error notification for call failure to the user.
> +          let notificationBox = this.getElt("convNotificationBox");
> +          notificationBox.appendNotification(
> +            this.convBundle.GetStringFromName("callError.label"),
> +            "video-call-error", null, notificationBox.PRIORITY_INFO_HIGH);

It seems this notification is never removed (both before and after this patch), we should probably remove it when the user attempts to call again.
Attachment #8561638 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> r+ as this looks good overall, but I'm not entirely confident in my review
> because all the unrelated refactoring/cleanup changes were distracting me
> from the main point of the bug. 

Folded into the main bug.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1018060
You need to log in before you can comment on or make changes to this bug.