Currently we can only answer() or hangUp() the call. We should be able to hold a call and answer it later, or hold a call when we are already connected to someone, to answer another incoming call.

Indeed this would be nice to have. Let's start by proposing and discussing the API on the dev-webapi mailinglist (https://lists.mozilla.org/listinfo/dev-webapi) before we get bogged down in the implementation.

Here is the proposal for WebTelephony API to hold a call. I posted it on dev-webapi as well. http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/415c0d8696a1fc1d# How do you think about this? Any ideas? Thanks!

Comment on attachment 606125 [details] [diff] [review] Propopal: WebTelephony API for holding a call This looks good to me. Jonas and Ben, what do you guys think?

Dear Phillipp, I drew a call state diagram, which includes the current B2G design and the proposal for holding a call, to better express the thoughts about this bug. Please refer to the attachments. In Part 1, the white blocks show the current design of B2G telephony call states. The yellow blocks are proposed to enhance the telephony functions -- hold a call. Part 2 shows our idea that how dialer may deal with the event in the proposal. The current call flows are shown on the left-hand side, while we show the proposal on the right-hand side. How do you think about the thoughts? Do they look feasible and compatible with the current design? Thank you, Hsinyi

Comment on attachment 607094 [details] Part 1: Proposal: enhance telephony call states for holding a call Thanks for this! I'm PTO for the rest of the week, jonas and/or bent should really look at it.

Comment on attachment 607095 [details] Part 2: Proposal: enhance telephony call states for holding a call I think you've been chatting with jonas about this already, will let him weigh in.

This patch updates |nsIDOMTelephonyCall.idl| to support call holding. Thanks!

Comment on attachment 607094 [details] Part 1: Proposal: enhance telephony call states for holding a call I think we came to agreement on the list.

This patch updates nsIDOMTelephony.idl and nsIDOMTelephonyCall.idl to support call holding. |oncallschanged| event has been replaced with specific events in this patch. Thanks.

This patch implements holdCall() and resumeCall(). Thanks.

The patch is a Marionette Python test. Thanks.

(In reply to htsai from comment #10) > This patch updates nsIDOMTelephony.idl and nsIDOMTelephonyCall.idl to > support call holding. |oncallschanged| event has been replaced with specific > events in this patch. Is there any benefit to removing this event? It seems like a useful event, but perhaps on second thought it isn't?

Comment on attachment 610076 [details] [diff] [review] Patch Part2: implementation of holdCall() and resumeCall() Review of attachment 610076 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! There are many coding style nits, please be sure to read https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide as well as observe the coding style present in the file you're modifying. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +86,4 @@ > case RIL.CALL_STATE_INCOMING: > return nsIRadioInterfaceLayer.CALL_STATE_INCOMING; > case RIL.CALL_STATE_WAITING: > + return nsIRadioInterfaceLayer.CALL_STATE_INCOMING; You can collapse the "case" with the previous one... @@ +380,5 @@ > + this._deliverCallback("callStateChanged", > + [call.callIndex, > + nsIRadioInterfaceLayer.CALL_STATE_CONNECTED, > + call.number]); > + break; Why not do this in ril_worker.js. Then RadioInterfaceLayer.js doesn't have to know about SUPP_SVC_CODE_* constants at all. Seems cleaner to me. @@ +387,1 @@ > [call.callIndex, call.state, call.number]); Nit: indentation @@ +532,5 @@ > rejectCall: function rejectCall(callIndex) { > this.worker.postMessage({type: "rejectCall", callIndex: callIndex}); > }, > + > + holdCall: function holdCall (callIndex) { Nit: no space after function name. @@ +536,5 @@ > + holdCall: function holdCall (callIndex) { > + this.worker.postMessage({type: "holdCall", callIndex: callIndex}); > + }, > + > + resumeCall: function resumeCall (callIndex) { Ditto. ::: dom/system/gonk/ril_consts.js @@ +250,4 @@ > const CALL_STATE_INCOMING = 4; > const CALL_STATE_WAITING = 5; > > +//Unsolicited supplementary service notification: resulted code, see TS 27.007 +CSSU commands Nit: space after // and overlong line (wrap at 79 characters or less!) ::: dom/system/gonk/ril_worker.js @@ +585,4 @@ > IMSI: null, > SMSC: null, > MSISDN: null, > + Unrelated whitespace change. Please remove. @@ +973,5 @@ > + holdCall: function holdCall(options) { > + let call = this.currentCalls[options.callIndex]; > + if (call && call.state == CALL_STATE_ACTIVE) { > + Buf.simpleRequest(REQUEST_SWITCH_HOLDING_AND_ACTIVE); > + } Coding style nit: indentation @@ +980,5 @@ > + resumeCall: function resumeCall(options) { > + let call = this.currentCalls[options.callIndex]; > + if (call && call.state == CALL_STATE_HOLDING) { > + Buf.simpleRequest(REQUEST_SWITCH_HOLDING_AND_ACTIVE); > + } Ditto. @@ +1380,5 @@ > + if (newCall.state != currentCall.state) { > + currentCall.state = newCall.state; > + this._handleChangedCallState(currentCall); > + } else { > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. What is RIL_CALL_STATE? @@ +1382,5 @@ > + this._handleChangedCallState(currentCall); > + } else { > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. > + // So, use suppSvcNotificationCode to update call states > + // when the call is held or resumed remotely This makes it sound like we have a choice. I would phrase it like so: // Instead we receive a supplementary service notification when a call is // held or resumed remotely. @@ +1701,5 @@ > this.IMSI = Buf.readString(); > }; > +RIL[REQUEST_HANGUP] = function REQUEST_HANGUP(length) { > + this.getCurrentCalls(); > +}; Why this? Normally we should get a UNSOLICITED_RESPONSE_CALL_STATE_CHANGED parcel notifying us of call state changes... @@ +1707,5 @@ > RIL[REQUEST_HANGUP_FOREGROUND_RESUME_BACKGROUND] = null; > RIL[REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE] = null; > +RIL[REQUEST_SWITCH_HOLDING_AND_ACTIVE] = function REQUEST_SWITCH_HOLDING_AND_ACTIVE (length) { > + this.getCurrentCalls(); > +}; Same question here. Also, nit: no space after function name. @@ +2133,5 @@ > + this.suppSvcNotification.notificationType = Buf.readUint32(); > + this.suppSvcNotification.code = Buf.readUint32(); > + this.suppSvcNotification.index = Buf.readUint32(); > + this.suppSvcNotification.type = Buf.readUint32(); > + this.suppSvcNotification.number = Buf.readString(); I don't like using a global singleton object (this.suppSvcNotification) for storing transient notification data. Please read these values into an object and add them to a mapping based on the number. Then in REQUEST_GET_CURRENT_CALLS you can look up this object from the mapping by number, and use it. Then delete it from the mapping. @@ +2134,5 @@ > + this.suppSvcNotification.code = Buf.readUint32(); > + this.suppSvcNotification.index = Buf.readUint32(); > + this.suppSvcNotification.type = Buf.readUint32(); > + this.suppSvcNotification.number = Buf.readString(); > + this.getCurrentCalls(); Why is this needed? ::: dom/telephony/TelephonyCall.cpp @@ +272,5 @@ > + > + ChangeStateInternal(nsIRadioInterfaceLayer::CALL_STATE_RESUMING, true); > + return NS_OK; > + > +} Coding style nit: indent by 2 spaces, remove extraneous whitespace (double spaces, empty lines at the end of the function body,e tc.), spaces after commas.

(In reply to Philipp von Weitershausen [:philikon] from comment #13) > (In reply to htsai from comment #10) > > This patch updates nsIDOMTelephony.idl and nsIDOMTelephonyCall.idl to > > support call holding. |oncallschanged| event has been replaced with specific > > events in this patch. > > Is there any benefit to removing this event? It seems like a useful event, > but perhaps on second thought it isn't? Yes, |oncallschanged| is useful but also seems confusing. Therefore, this patch proposes some more specific events to substitute for it. Please see http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/d630a5329d64cb6f# for the discussion.

(In reply to htsai from comment #15) > Yes, |oncallschanged| is useful but also seems confusing. Therefore, this > patch proposes some more specific events to substitute for it. > Please see > http://groups.google.com/group/mozilla.dev.webapi/browse_thread/thread/ > d630a5329d64cb6f# for the discussion. Ah, I was looking for that discussion, but I couldn't find it. Thanks! I'll let Jonas and Ben make this call.

(In reply to Philipp von Weitershausen [:philikon] from comment #14) > Comment on attachment 610076 [details] [diff] [review] > Patch Part2: implementation of holdCall() and resumeCall() > > Review of attachment 610076 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the patch! There are many coding style nits, please be sure to > read https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide as well as > observe the coding style present in the file you're modifying. > Thanks for the comments. Will modify the code to make it meet the Mozilla coding style. > ::: dom/system/gonk/RadioInterfaceLayer.js > > @@ +380,5 @@ > > + this._deliverCallback("callStateChanged", > > + [call.callIndex, > > + nsIRadioInterfaceLayer.CALL_STATE_CONNECTED, > > + call.number]); > > + break; > > Why not do this in ril_worker.js. Then RadioInterfaceLayer.js doesn't have > to know about SUPP_SVC_CODE_* constants at all. Seems cleaner to me. > Mmm, actually, I was struggling between ril_worker.js and RadioInterfaceLayer.js. Now looks that ril_worker.js makes much sense. Thanks. > > @@ +1380,5 @@ > > + if (newCall.state != currentCall.state) { > > + currentCall.state = newCall.state; > > + this._handleChangedCallState(currentCall); > > + } else { > > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. > > What is RIL_CALL_STATE? > I should have given a more explicit comment. This should be RIL_CallState, the state from the response of rild. > @@ +1382,5 @@ > > + this._handleChangedCallState(currentCall); > > + } else { > > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. > > + // So, use suppSvcNotificationCode to update call states > > + // when the call is held or resumed remotely > > This makes it sound like we have a choice. I would phrase it like so: > > // Instead we receive a supplementary service notification when a call is > // held or resumed remotely. > Good point! > @@ +1701,5 @@ > > this.IMSI = Buf.readString(); > > }; > > +RIL[REQUEST_HANGUP] = function REQUEST_HANGUP(length) { > > + this.getCurrentCalls(); > > +}; > > Why this? Normally we should get a UNSOLICITED_RESPONSE_CALL_STATE_CHANGED > parcel notifying us of call state changes... > This is for Bug737793. > @@ +2133,5 @@ > > + this.suppSvcNotification.notificationType = Buf.readUint32(); > > + this.suppSvcNotification.code = Buf.readUint32(); > > + this.suppSvcNotification.index = Buf.readUint32(); > > + this.suppSvcNotification.type = Buf.readUint32(); > > + this.suppSvcNotification.number = Buf.readString(); > > I don't like using a global singleton object (this.suppSvcNotification) for > storing transient notification data. Please read these values into an object > and add them to a mapping based on the number. Then in > REQUEST_GET_CURRENT_CALLS you can look up this object from the mapping by > number, and use it. Then delete it from the mapping. > Will re-implement this part according to your suggestion. > @@ +2134,5 @@ > > + this.suppSvcNotification.code = Buf.readUint32(); > > + this.suppSvcNotification.index = Buf.readUint32(); > > + this.suppSvcNotification.type = Buf.readUint32(); > > + this.suppSvcNotification.number = Buf.readString(); > > + this.getCurrentCalls(); > > Why is this needed? > As explained right before, RIL_CallState, from the response of rild, doesn't change when the remote party holds/resumes a call. We need the response from RIL_UNSOL_SUPP_SVC_NOTIFICATION to get supplementary service related notification from the network. That's why we need the above code to update the call state in this case.

(In reply to htsai from comment #17) > > @@ +1380,5 @@ > > > + if (newCall.state != currentCall.state) { > > > + currentCall.state = newCall.state; > > > + this._handleChangedCallState(currentCall); > > > + } else { > > > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. > > > > What is RIL_CALL_STATE? > > > I should have given a more explicit comment. > This should be RIL_CallState, the state from the response of rild. This will not be obvious without reading ril.h, so I would express it in terms of the constants that are known to ril_worker.js, e.g.: // We are not notified of call state changes when the remote party holds/resumes // a call. Instead we receive a supplementary service notification when a call is // held or resumed remotely. (This is paraphrasing your initial comment.) Although I'm a bit confused by the "remotely" word... *We* are the ones who are holding/resuming the call, not the remote side, right? > > @@ +1701,5 @@ > > > this.IMSI = Buf.readString(); > > > }; > > > +RIL[REQUEST_HANGUP] = function REQUEST_HANGUP(length) { > > > + this.getCurrentCalls(); > > > +}; > > > > Why this? Normally we should get a UNSOLICITED_RESPONSE_CALL_STATE_CHANGED > > parcel notifying us of call state changes... > > > This is for Bug737793. I see! Why is it in this patch then? Please rebase your patches properly. > > @@ +2134,5 @@ > > > + this.suppSvcNotification.code = Buf.readUint32(); > > > + this.suppSvcNotification.index = Buf.readUint32(); > > > + this.suppSvcNotification.type = Buf.readUint32(); > > > + this.suppSvcNotification.number = Buf.readString(); > > > + this.getCurrentCalls(); > > > > Why is this needed? > > > As explained right before, RIL_CallState, from the response of rild, doesn't > change when the remote party holds/resumes a call. > We need the response from RIL_UNSOL_SUPP_SVC_NOTIFICATION to get > supplementary service related notification from the network. > That's why we need the above code to update the call state in this case. Yes, that makes sense. Can you add a comment above `this.getCurrentCalls()` that explains this, please? Thanks!

(In reply to Philipp von Weitershausen [:philikon] from comment #18) > (In reply to htsai from comment #17) > > > @@ +1380,5 @@ > > > > + if (newCall.state != currentCall.state) { > > > > + currentCall.state = newCall.state; > > > > + this._handleChangedCallState(currentCall); > > > > + } else { > > > > + // RIL_CALL_STATE isn't changed when the remote party holds/resumes a call. > > > > > > What is RIL_CALL_STATE? > > > > > I should have given a more explicit comment. > > This should be RIL_CallState, the state from the response of rild. > > This will not be obvious without reading ril.h, so I would express it in > terms of the constants that are known to ril_worker.js, e.g.: > > // We are not notified of call state changes when the remote party > holds/resumes > // a call. Instead we receive a supplementary service notification when a > call is > // held or resumed remotely. > > (This is paraphrasing your initial comment.) Although I'm a bit confused by > the "remotely" word... *We* are the ones who are holding/resuming the call, > not the remote side, right? > Here "remotely" means the call held/resumed "by the remote party, i.e. the one we are talking to via phone." The call is "not" held/resumed "by us." How about the following phrase? More clear? // We are not notified of call state changes when the remote party // holds/resumes a call. //Instead we receive a supplementary service notification in this case.

This patch implements holdCall() and resumeCall(). Modification has been made according to the comments above. Thanks!

Comment on attachment 609583 [details] [diff] [review] Patch Part1: add WebTelephony API to hold a call _v2 Review of attachment 609583 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Comment on attachment 610847 [details] [diff] [review] Patch Part2: implementation of holdCall() and resumeCall()_v2 Some modification needs to be made to meet |Patch Part1: add WebTelephony API to hold a call _v2|.

This patch implements hold() and resume(). Modification has been made according to the comments discussed above. Modification also made to meet the newly proposed API in |Patch Part1: add WebTelephony API to hold a call _v2| Thanks!

The patch is an updated Marionette Python test for hold() and resume() a call. Thanks.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #19) > Here "remotely" means the call held/resumed "by the remote party, i.e. the > one we are talking to via phone." > The call is "not" held/resumed "by us." I see. Thanks for the clarification! Sorry, I didn't understand this earlier. So it seems that we're now overloading the "holding" state: the RIL will set this flag when we are holding the call locally, and you're setting this flag based on `suppSvcNotificationCode` when the call is held remotely. I think that's very confusing. In my understanding, "holding" means the call is held locally. If the remote party is holding us, then for us the call is still active. Sure, we could expose an *additional* flag, e.g. a Boolean `heldRemotely`, but that's an additional feature that IMHO is outside the scope of this bug.

Comment on attachment 611751 [details] [diff] [review] Patch Part2: implementation of hold() and resume()_v2 > this.updateCallAudioState(); > this._deliverCallback("callStateChanged", > [call.callIndex, call.state, call.number]); >+ // TODO When the call is held by the remote party, not by us, >+ // we cannot resume the call. >+ // Deliver a "resumable flag" to notify UI >+ // whether the call can be resumed by *us* or not. > }, ... >+ switch (newCall.suppSvcNotificationCode) { >+ // Update call state according to the supplementary >+ // servecie notification in specific cases, e.g. >+ // call is held/resumed by the remote party >+ case SUPP_SVC_CODE_REMOTE_ONHOLD: >+ currentCall.state = CALL_STATE_HOLDING; >+ currentCall.suppSvcNotificationCode = newCall.suppSvcNotificationCode; >+ this._handleChangedCallState(currentCall); >+ break; >+ case SUPP_SVC_CODE_REMOTE_RESUMED: >+ currentCall.state = CALL_STATE_ACTIVE; >+ currentCall.suppSvcNotificationCode = newCall.suppSvcNotificationCode; >+ this._handleChangedCallState(currentCall); >+ break; Like I said my previous comment, we should conflate the concept of holding a call remotely and locally. Let's leave this out for a (low-priority) follow-up bug.

Comment on attachment 611751 [details] [diff] [review] Patch Part2: implementation of hold() and resume()_v2 Review of attachment 611751 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +222,5 @@ > + rv = event->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("disconnected")); > + NS_ENSURE_SUCCESS(rv, rv); > + break; > + default: > + NS_NOTREACHED("Unknow situation!"); It's not an unknown situation, it's just one that isn't interesting to calls array. (Also, you misspelled "Unknown".) I would prefer to simply remove the whole `default` clause. ::: dom/telephony/TelephonyCall.cpp @@ +136,5 @@ > + NS_LITERAL_STRING("connected")))) { > + NS_WARNING("Failed to dispatch specific event!"); > + } > + } > + Can you explain why you're adding this block? Shouldn't this case already be covered by the `case nsIRadioInterfaceLayer::CALL_STATE_CONNECTED` statement above and the event dispatch below? @@ +254,5 @@ > > +NS_IMETHODIMP > +TelephonyCall::Hold() > +{ > + if (mCallState != nsIRadioInterfaceLayer::CALL_STATE_CONNECTED) { Nit: double space after != @@ +269,5 @@ > + > +NS_IMETHODIMP > +TelephonyCall::Resume() > +{ > + if (mCallState != nsIRadioInterfaceLayer::CALL_STATE_HELD) { Ditto.

(In reply to Philipp von Weitershausen [:philikon] from comment #27) > Comment on attachment 611751 [details] [diff] [review] > Patch Part2: implementation of hold() and resume()_v2 > > Review of attachment 611751 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/Telephony.cpp > @@ +222,5 @@ > > + rv = event->Dispatch(ToIDOMEventTarget(), NS_LITERAL_STRING("disconnected")); > > + NS_ENSURE_SUCCESS(rv, rv); > > + break; > > + default: > > + NS_NOTREACHED("Unknow situation!"); > > It's not an unknown situation, it's just one that isn't interesting to calls > array. (Also, you misspelled "Unknown".) Oops! Thank you for pointing this out. > I would prefer to simply remove the > whole `default` clause. > Good point! > ::: dom/telephony/TelephonyCall.cpp > @@ +136,5 @@ > > + NS_LITERAL_STRING("connected")))) { > > + NS_WARNING("Failed to dispatch specific event!"); > > + } > > + } > > + > > Can you explain why you're adding this block? Shouldn't this case already be > covered by the `case nsIRadioInterfaceLayer::CALL_STATE_CONNECTED` statement > above and the event dispatch below? > I added this block for the *Telephony* connected event according to the newly proposed API. It's different from the event dispatch below because below is about *TelephonyCall* event.

(In reply to Philipp von Weitershausen [:philikon] from comment #26) > Like I said my previous comment, we should conflate the concept of holding a > call remotely and locally. Let's leave this out for a (low-priority) > follow-up bug. Sorry, the first sentence here is missing an important word: we should NOT conflate these concepts!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #28) > I added this block for the *Telephony* connected event according to the > newly proposed API. It's different from the event dispatch below because > below is about *TelephonyCall* event. Aaaah, that makes sense. Can you add a comment above the block so that others don't get confused like I did? Thanks! Other than that and the nits I already pointed out, the patch looks good!

Sorry that I'm a little late to this party but I'm not sold on the changes to the Telephony object (oncallschanged, etc.). Can we restrict this bug (and this patch) to the hold/resume stuff only please? I'll post my thoughts on the oncallschanged in webapi unless you'd prefer to do it here.

(In reply to ben turner [:bent] from comment #31) > Sorry that I'm a little late to this party but I'm not sold on the changes > to the Telephony object (oncallschanged, etc.). Can we restrict this bug > (and this patch) to the hold/resume stuff only please? I'll post my thoughts > on the oncallschanged in webapi unless you'd prefer to do it here. Ok. I don't mind splitting this up in several bugs. Small iterations ftw! Let's focus on holding calls locally in this bug and do other stuff in follow-ups.

(In reply to Philipp von Weitershausen [:philikon] from comment #32) > (In reply to ben turner [:bent] from comment #31) > > Sorry that I'm a little late to this party but I'm not sold on the changes > > to the Telephony object (oncallschanged, etc.). Can we restrict this bug > > (and this patch) to the hold/resume stuff only please? I'll post my thoughts > > on the oncallschanged in webapi unless you'd prefer to do it here. > > Ok. I don't mind splitting this up in several bugs. Small iterations ftw! > Let's focus on holding calls locally in this bug and do other stuff in > follow-ups. Great, let's do that. Updated patch will be submitted later.

This patch focuses on implementing hold() and resume() a call LOCALLY. Attachment 611751 [details] [diff] is obsoleted because it tries to handle "call held/resumed REMOTELY." Also, as discussion above, this bug is better not handling the issues about oncallschanged event. So I obsolete attachment 609583 [details] [diff] [review] here. Thanks.

Updated: this attachment is an Marionette python testcase for hold() and resume() a call.

Comment on attachment 612474 [details] [diff] [review] Patch: hold() and resume() a call LOCALLY Review of attachment 612474 [details] [diff] [review]: ----------------------------------------------------------------- r=me with small nits applied ::: dom/system/gonk/RadioInterfaceLayer.js @@ +370,5 @@ > [call.callIndex, call.state, call.number]); > + // TODO When the call is held by the remote party, not by us, > + // we cannot resume the call. > + // Deliver a "heldRemotely" flag to notify UI > + // the call has been held by the remote party. This comment, without a TODO flag or a bug reference, is a bit confusing. Let's leave it out. ::: dom/system/gonk/ril_worker.js @@ +996,5 @@ > + resumeCall: function resumeCall(options) { > + let call = this.currentCalls[options.callIndex]; > + if (call && call.state == CALL_STATE_HOLDING) { > + Buf.simpleRequest(REQUEST_SWITCH_HOLDING_AND_ACTIVE); > + } Nit: two spaces for indention

I addressed those small nits and pushed the patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/60f7fc980511

(In reply to Philipp von Weitershausen [:philikon] from comment #37) > I addressed those small nits and pushed the patch: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/60f7fc980511 Thanks :) Also, I filed a bug 743150 for Comment 36.