1020448 - Call rejection does not turn off camera

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Adam Roach [:abr]]

If the other side rejects the call, you end up in a surprising state with the camera and microphone live but with an indication that your call has ended. This is different from hangup, which works.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020448-Added-Loop-pending-call-timeout.patch

This patch adds support for pending outgoing calls timeout, so link clickers are notified when their call didn't go through after a given amount of time (20 seconds by default, but this value is configurable.)

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8438429 [details] [diff] [review]
0001-Bug-1020448-Added-Loop-pending-call-timeout.patch

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

I think this is heading in the right direction. There's a couple of things I'm concerned about:

- If a call times out at the link-clicker end, then the incoming call notification isn't removed from the desktop end. I'm not sure what the plan is here, so I'd like feedback from Darrin/Adam as to the expected UX - even if we do that work in a follow-up bug.

- We should probably start getting ui-reviews now, in this case, the general flow here, the 20s timeout and the string added in data.ini.

::: browser/components/loop/content/shared/js/models.js
@@ +61,5 @@
>          throw new Error("missing required sdk");
>        }
>        this.sdk = options.sdk;
> +      if (!options.pendingCallTimeout) {
> +        throw new Error("missing required pendingCallTimeout");

This causes the desktop side to fail, as there's no default passed in. I think it would be reasonable just to have a default value here.

::: browser/components/loop/standalone/Makefile
@@ +22,5 @@
>  	@echo "Not implemented yet."
>  
>  config:
> +	@echo "var loop = loop || {};" > content/config.js
> +	@echo "loop.config.serverUrl          = '`echo $(LOOP_SERVER_URL)`';" >> content/config.js

For me, this failed because loop.config was undefined.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020448-Added-Loop-pending-call-timeout-2.patch

Addressed review comments.

Comment 4

[Mark Banner (:standard8)]

Attachment: Screenshot of timeout after 20s

This is the current screenshot after the timeout 20 seconds. Obviously at some stage we'll need to reformat it for the new error formats.

Is the text & timeout reasonable for now?

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8439867 [details] [diff] [review]
0001-Bug-1020448-Added-Loop-pending-call-timeout-2.patch

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

Ok, generally looks good, but a couple of small issues to fix before we can land this.

::: browser/components/loop/content/shared/js/models.js
@@ +98,5 @@
> +      // Outgoing call has never reach destination, closing - see bug 1020448
> +      function handleOutgoingCallTimeout() {
> +        /*jshint validthis:true */
> +        if (!this.get("ongoing")) {
> +          this.trigger("timeout").endSession();

The timer needs clearing when the call is connected - otherwise if its accepted quickly, and is a short call, then the timer will still kick in and generate the warning notice.

::: browser/components/loop/standalone/content/l10n/data.ini
@@ +1,3 @@
>  [en]
>  call_has_ended=Your call has ended.
> +call_did_not_go_through=Your call did not go through.

We should make this more meaningful, e.g. call_timeout_notification_text

(the other ones could then be call_ended_notification_text etc, not that we need to change them here).

Comment 6

[Darrin Henein [:darrin]]

Comment on attachment 8446445 [details]
Screenshot of timeout after 20s

This works for the time being, the MVP has error reporting built into the spec. https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#error

This will work slightly different for the web app version, which is yet to be finished.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020448-Added-Loop-pending-call-timeout-2.patch

Addressed comments.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020448-Added-Loop-pending-call-timeout-rev3.patch

New patch rebased on top of latest master.

Comment 9

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #5)
> Comment on attachment 8439867 [details] [diff] [review]
> 0001-Bug-1020448-Added-Loop-pending-call-timeout-2.patch
> 
> Review of attachment 8439867 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, generally looks good, but a couple of small issues to fix before we can
> land this.
> 
> ::: browser/components/loop/content/shared/js/models.js
> @@ +98,5 @@
> > +      // Outgoing call has never reach destination, closing - see bug 1020448
> > +      function handleOutgoingCallTimeout() {
> > +        /*jshint validthis:true */
> > +        if (!this.get("ongoing")) {
> > +          this.trigger("timeout").endSession();
> 
> The timer needs clearing when the call is connected - otherwise if its
> accepted quickly, and is a short call, then the timer will still kick in and
> generate the warning notice.

I don't think this comment was addressed, did it get missed in the rebase?

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Mark Banner (:standard8) from comment #9)
> > The timer needs clearing when the call is connected - otherwise if its
> > accepted quickly, and is a short call, then the timer will still kick in and
> > generate the warning notice.
> 
> I don't think this comment was addressed, did it get missed in the rebase?

Hmm, maybe you reviewed the wrong version of the patch? I can see the clearTimeout call in attachment 8451074 [details] [diff] [review]… Or am I missing something else? (clearing the timeout in handleOutgoingCallTimeout wouldn't make much sense to me…)

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8451074 [details] [diff] [review]
0001-Bug-1020448-Added-Loop-pending-call-timeout-rev3.patch

The problem with the current clearTimeout is that it'll only clear the timeout the next time a call initiate is requested.

Hence, as I understand it at the moment, the timeout is currently measuring the time between one get from the server of the details for a particular call, to the next time we get the details for the same call (which we'd ever only do on a restart call).

I think clearTimeout wants to be called from functions like _onConnectCompletion, _connectionDestroyed, _sessionDisconnected, _networkDisconnected (depending on which ones might get called for an error during the connection process).

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1020448-Added-Loop-pending-call-timeout-rev4.patch

Addressed comments.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8452948 [details] [diff] [review]
0001-Bug-1020448-Added-Loop-pending-call-timeout-rev4.patch

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

Looks good. r=Standard8. We should co-ordinate landing of this with dmose as he's concerned about rebasing changes for the react work.

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Just rebased on top of latest master (which includes the reactified conversation) and rebuilt, it works perfectly with no further tweaking needed.

Comment 15

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a73ed0f644d6

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a73ed0f644d6

Comment 17

[u279076]

Does this need manual testing or is it sufficiently covered by automation?

Comment 18

[u279076]

We have a smoketest for this and it's passed several times. Marking this bug verified fixed.