714968 - B2G telephony: support a second incoming call

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

There are some holes and possibly some inaccuracies in the current telephony implementation revolving around putting calls on hold and accurately representing their state in such cases.

Comment 1

[John Hammink]

Currently (2/8) it is not possible to answer a second call or put the first one on hold from the b2g device;  when the b2g device call is held remotely; it show no changes in state.

Comment 2

[Philipp von Weitershausen [:philikon]]

Yup. Multiple calls are currently not supported at all. This is known, and it's why this bug exists :)

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch: answer() and hangUp() a second incoming call

This patch supports answering or hanging up the second call. Thanks.

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This bug depends on Bug 735170 because answer() the second call makes the first call into holding/held state. And the holding/held/resuming states and related events are addressed in 735170.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 612501 [details] [diff] [review]
Patch: answer() and hangUp() a second incoming call

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

Nice work! I'm going to clear review until we have sorted the question below. Also please address the nits.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +87,2 @@
>      case RIL.CALL_STATE_WAITING:
> +      return nsIRadioInterfaceLayer.CALL_STATE_INCOMING;

This is already part of your patch for bug 735170! Please make sure your patches are rebased properly!

::: dom/system/gonk/ril_worker.js
@@ +935,5 @@
> +    // when another call is held/background.
> +    // After the request, the call originally held/background will be resumed.
> +    // And a call is hang up normally, 
> +    // when no other call is held/background.
> +    Buf.simpleRequest(REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND);

Are you sure we can simply substitute REQUEST_HANGUP with REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND in all cases? Have you tested this on the devices we support right now (SGS2, Nexus S, Akami)?

In any case, we should check that `options.callIndex` is indeed the active call. And what should happen when you try to hang up a non-active call? We should probably make it the active call and then hang it up, no?

Also: Please consistently wrap at 79 characters. There are several grammatical problems in the comment as well, and it uses a lot of words to document things one can find out from reading the code. But I suspect it will become obsolete or will have to change anyway when we address my points from above.

@@ +970,5 @@
> +          Buf.simpleRequest(REQUEST_ANSWER);
> +          break;
> +        case CALL_STATE_WAITING:
> +          // Answer the waiting (second) call, and
> +          // make the already connected call on-hold

Nit: please wrap at 79 characters. There's also no comma before the "and" and there should be a period at the end of the sentence.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 612501 [details] [diff] [review]
> Patch: answer() and hangUp() a second incoming call
> 
> Review of attachment 612501 [details] [diff] [review]:
> 
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +935,5 @@
> > +    // when another call is held/background.
> > +    // After the request, the call originally held/background will be resumed.
> > +    // And a call is hang up normally, 
> > +    // when no other call is held/background.
> > +    Buf.simpleRequest(REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND);
> 
> Are you sure we can simply substitute REQUEST_HANGUP with
> REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND in all cases? Have you tested
> this on the devices we support right now (SGS2, Nexus S, Akami)?
> 
> In any case, we should check that `options.callIndex` is indeed the active
> call. And what should happen when you try to hang up a non-active call? We
> should probably make it the active call and then hang it up, no?
> 
Thanks for the comment.

According to my test on SGS2, REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND works for hanging up a foreground (active) call no matter there's background call or not. The background call, if any, will be resumed after the request.

Per my understanding, to hang up a held/background call, we can simply use REQUEST_HANGUP_WAITING_OR_BACKGROUND. We don't need to make it active in advance.

According to your points, I think the implementation can be revised as follows.

When the call state is CALL_STATE_HOLDING, use REQUEST_HANGUP_WAITING_OR_BACKGROUND. Another active call isn't affected after this request. 

When the call is active, i.e. the call state is not CALL_STATE_HOLDING, we have two choices. 1) use REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hang up the call. The held/background call is resumed subsequently. 2) we use REQUEST_HANGUP; after the request, the held/background remains.

How do you think about this? Thanks.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> 
> When the call is active, i.e. the call state is not CALL_STATE_HOLDING, we
> have two choices. 1) use REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hang
> up the call. The held/background call is resumed subsequently. 2) we use
> REQUEST_HANGUP; after the request, the held/background remains.
> 
After my second thought, I think REQUEST_HANGUP seems better. Because hanging up a call doesn't necessarily imply that the other call should be resumed immediately. REQUEST_HANGUP makes hangUp() more clear.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> (In reply to Philipp von Weitershausen [:philikon] from comment #6)
> > Comment on attachment 612501 [details] [diff] [review]
> > Patch: answer() and hangUp() a second incoming call
> > 
> > Review of attachment 612501 [details] [diff] [review]:
> > 
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +935,5 @@
> > > +    // when another call is held/background.
> > > +    // After the request, the call originally held/background will be resumed.
> > > +    // And a call is hang up normally, 
> > > +    // when no other call is held/background.
> > > +    Buf.simpleRequest(REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND);
> > 
> > Are you sure we can simply substitute REQUEST_HANGUP with
> > REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND in all cases? Have you tested
> > this on the devices we support right now (SGS2, Nexus S, Akami)?
> > 
> > In any case, we should check that `options.callIndex` is indeed the active
> > call. And what should happen when you try to hang up a non-active call? We
> > should probably make it the active call and then hang it up, no?
> > 
> Thanks for the comment.
> 
> According to my test on SGS2, REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND
> works for hanging up a foreground (active) call no matter there's background
> call or not. The background call, if any, will be resumed after the request.
> 

Also, please let me explain why I used REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND. :)

I adopted this request according to the call state diagram in https://wiki.mozilla.org/WebAPI/WebTelephony. The proposed diagram has been discussed in dev-webapi, and this is the latest version. In this diagram, we don't hang up a held call. Therefore, REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND can replace REQUEST_HANGUP. 

But according to your comment, maybe we should re-examine the proposed diagram before we get into implementation? Any further thoughts about the diagram? Thanks!

> Per my understanding, to hang up a held/background call, we can simply use
> REQUEST_HANGUP_WAITING_OR_BACKGROUND. We don't need to make it active in
> advance.
> 
> According to your points, I think the implementation can be revised as
> follows.
> 
> When the call state is CALL_STATE_HOLDING, use
> REQUEST_HANGUP_WAITING_OR_BACKGROUND. Another active call isn't affected
> after this request. 
> 
> When the call is active, i.e. the call state is not CALL_STATE_HOLDING, we
> have two choices. 1) use REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hang
> up the call. The held/background call is resumed subsequently. 2) we use
> REQUEST_HANGUP; after the request, the held/background remains.
> 
> How do you think about this? Thanks.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > 
> > When the call is active, i.e. the call state is not CALL_STATE_HOLDING, we
> > have two choices. 1) use REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hang
> > up the call. The held/background call is resumed subsequently. 2) we use
> > REQUEST_HANGUP; after the request, the held/background remains.
> > 
> After my second thought, I think REQUEST_HANGUP seems better. Because
> hanging up a call doesn't necessarily imply that the other call should be
> resumed immediately. REQUEST_HANGUP makes hangUp() more clear.

Is it even possible to hang up a call without switching to a background one?

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> > According to my test on SGS2, REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND
> > works for hanging up a foreground (active) call no matter there's background
> > call or not. The background call, if any, will be resumed after the request.
...
> But according to your comment, maybe we should re-examine the proposed
> diagram before we get into implementation? Any further thoughts about the
> diagram? Thanks!

I don't think there's anything wrong with the diagram. I was just curious about hanging up a single call when there's no call in the background. My fear is that while some RILs will make REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND work regardless, some may not. But ok, let's go with REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND for now. We can always fix later. :)

Can you please address my other points from comment 6 and upload a new patch? Thanks!

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > > 
> > > When the call is active, i.e. the call state is not CALL_STATE_HOLDING, we
> > > have two choices. 1) use REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND to hang
> > > up the call. The held/background call is resumed subsequently. 2) we use
> > > REQUEST_HANGUP; after the request, the held/background remains.
> > > 
> > After my second thought, I think REQUEST_HANGUP seems better. Because
> > hanging up a call doesn't necessarily imply that the other call should be
> > resumed immediately. REQUEST_HANGUP makes hangUp() more clear.
> 
> Is it even possible to hang up a call without switching to a background one?
> 
Yes, we can use REQUEST_HANGUP to hang up a call directly. 

> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> > > According to my test on SGS2, REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND
> > > works for hanging up a foreground (active) call no matter there's background
> > > call or not. The background call, if any, will be resumed after the request.
> ...
> > But according to your comment, maybe we should re-examine the proposed
> > diagram before we get into implementation? Any further thoughts about the
> > diagram? Thanks!
> 
> I don't think there's anything wrong with the diagram. I was just curious
> about hanging up a single call when there's no call in the background. My
> fear is that while some RILs will make
> REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND work regardless, some may not.
> But ok, let's go with REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND for now.
> We can always fix later. :)
> 
> Can you please address my other points from comment 6 and upload a new
> patch? Thanks!
Sure, I'll provide a new patch addressing all your comments later:) Thanks!

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch: answer() and hangUp() a second incoming call

Here's the updated patch which addresses the discussion and comments above. Thanks!

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 613268 [details] [diff] [review]
Patch: answer() and hangUp() a second incoming call

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

Just a few minor comments.

::: dom/system/gonk/ril_worker.js
@@ +961,2 @@
>      let call = this.currentCalls[options.callIndex];
> +    if (call) {

You can do

  if (!call) {
    return;
  }

here so you don't have to nest the `switch` block inside the `if`. That makes the code flatter and nicer to read.

@@ +986,2 @@
>      let call = this.currentCalls[options.callIndex];
> +    if (call) {

Ditto.

@@ +1870,5 @@
> +  this.getCurrentCalls();
> +};
> +RIL[REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE] = function REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE(length) {
> +  this.getCurrentCalls();
> +};

In bug 727319 we changed the way we deal with request errors. Please add the following stanza to all these parcel handlers:

  if (options.rilRequestError) {
    return;
  }

(The `options` object is the 2nd argument.)

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v3): answer() and hangUp() a second incoming call

Thanks for the above comments. This patch addresses them all. 

Also, the patch fixes the broken rejectCall() in ril_worker.js as 'options' object should have been assigned. Otherwise, we cannot successfully reject an incoming call.

Comment 15

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3c7a491be1

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0b3c7a491be1