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 :)

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

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 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.

(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!

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.)

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.