Last Comment Bug 714968 - B2G telephony: support a second incoming call
: B2G telephony: support a second incoming call
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
: Andrew Overholt [:overholt]
Mentors:
: 735167 (view as bug list)
Depends on: 735170
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2012-01-03 14:47 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-04-12 10:04 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: answer() and hangUp() a second incoming call (4.96 KB, patch)
2012-04-05 04:23 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Splinter Review
Patch: answer() and hangUp() a second incoming call (5.19 KB, patch)
2012-04-09 04:30 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Splinter Review
Patch (v3): answer() and hangUp() a second incoming call (5.37 KB, patch)
2012-04-10 03:58 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-01-03 14:47:30 PST
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 2012-02-08 17:31:35 PST
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] 2012-02-08 17:35:27 PST
Yup. Multiple calls are currently not supported at all. This is known, and it's why this bug exists :)
Comment 3 Hsin-Yi Tsai [:hsinyi] 2012-04-05 03:16:51 PDT
*** Bug 735167 has been marked as a duplicate of this bug. ***
Comment 4 Hsin-Yi Tsai [:hsinyi] 2012-04-05 04:23:01 PDT
Created attachment 612501 [details] [diff] [review]
Patch: answer() and hangUp() a second incoming call

This patch supports answering or hanging up the second call. Thanks.
Comment 5 Hsin-Yi Tsai [:hsinyi] 2012-04-05 04:50:13 PDT
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] 2012-04-05 12:12:15 PDT
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 [:hsinyi] 2012-04-05 19:25:56 PDT
(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 [:hsinyi] 2012-04-06 01:31:25 PDT
(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 [:hsinyi] 2012-04-06 07:07:22 PDT
(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] 2012-04-06 08:08:56 PDT
(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 [:hsinyi] 2012-04-06 08:41:58 PDT
(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 [:hsinyi] 2012-04-09 04:30:55 PDT
Created attachment 613268 [details] [diff] [review]
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] 2012-04-10 01:49:17 PDT
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 [:hsinyi] 2012-04-10 03:58:12 PDT
Created attachment 613543 [details] [diff] [review]
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] 2012-04-10 22:57:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3c7a491be1

Note You need to log in before you can comment on or make changes to this bug.