Closed Bug 714968 Opened 9 years ago Closed 9 years ago

B2G telephony: support a second incoming call

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: philikon, Assigned: hsinyi)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Yup. Multiple calls are currently not supported at all. This is known, and it's why this bug exists :)
Assignee: nobody → htsai
Depends on: 735170
Duplicate of this bug: 735167
No longer depends on: 735170
Summary: B2G telephony: accurately represent waiting/held calls → B2G telephony: support a second incoming call
This patch supports answering or hanging up the second call. Thanks.
Attachment #612501 - Flags: review?(philipp)
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.
Depends on: 735170
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.
Attachment #612501 - Flags: review?(philipp)
(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.
(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.
(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.
(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!
(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!
Here's the updated patch which addresses the discussion and comments above. Thanks!
Attachment #612501 - Attachment is obsolete: true
Attachment #613268 - Flags: review?(philipp)
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.)
Attachment #613268 - Flags: review?(philipp)
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.
Attachment #613268 - Attachment is obsolete: true
Attachment #613543 - Flags: review?(philipp)
Attachment #613543 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/0b3c7a491be1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.