Closed Bug 1020448 Opened 6 years ago Closed 5 years ago

Call rejection does not turn off camera

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla33

People

(Reporter: abr, Unassigned)

Details

(Whiteboard: p=2)

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → nperriault
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.)
Attachment #8438429 - Flags: review?(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.
Attachment #8438429 - Flags: ui-review?(dhenein)
Attachment #8438429 - Flags: review?(standard8)
Attachment #8438429 - Flags: review-
Attachment #8438429 - Flags: feedback?(adam)
Addressed review comments.
Attachment #8438429 - Attachment is obsolete: true
Attachment #8438429 - Flags: ui-review?(dhenein)
Attachment #8438429 - Flags: feedback?(adam)
Attachment #8439867 - Flags: review?(standard8)
Priority: -- → P1
Whiteboard: p=2
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?
Attachment #8446445 - Flags: ui-review?(dhenein)
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).
Attachment #8439867 - Flags: review?(standard8) → review-
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.
Attachment #8446445 - Flags: ui-review?(dhenein) → ui-review+
Target Milestone: --- → mozilla33
Addressed comments.
Attachment #8439867 - Attachment is obsolete: true
Attachment #8450056 - Flags: review?(standard8)
New patch rebased on top of latest master.
Attachment #8450056 - Attachment is obsolete: true
Attachment #8450056 - Flags: review?(standard8)
Attachment #8451074 - Flags: review?(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?
(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 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).
Attachment #8451074 - Flags: review?(standard8)
Addressed comments.
Attachment #8451074 - Attachment is obsolete: true
Attachment #8452948 - Flags: review?(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.
Attachment #8452948 - Flags: review?(standard8) → review+
Just rebased on top of latest master (which includes the reactified conversation) and rebuilt, it works perfectly with no further tweaking needed.
Does this need manual testing or is it sufficiently covered by automation?
Whiteboard: p=2 → p=2 [qa?]
We have a smoketest for this and it's passed several times. Marking this bug verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: anthony.s.hughes
Whiteboard: p=2 [qa?] → p=2
You need to log in before you can comment on or make changes to this bug.