Currently we can hold/resume a call locally, but we are not notified when the call is held/resumed by the remote party, i.e. the one we are talking to via phone. It's nice to have this notification, so that we can know if we can resume the call.

Nice to have. Re-open if partners require it.

See TS 24.083 and TS 27.007 Sec. 7.17

How about notify by event? Two options: associate the event on (1) Telephony or (2) TelephonyCall For (2) TelephonyCall, it seems more reasonable. That indicates the remote party held the 'call'. Maybe we got several calls, we could know actually which one has been put on hold. However, from my test experience, there might be some technical difficulty. The notification from ril does not guarantee to contain the phone number information. Yes, I just test it, and get nothing. Thus we don't know how to deliver the event to the correct call object. So, maybe we could just fire the event on Telephony. It depends on the requirement. Do we really need to know which call is held? For event, we need a name. Any suggestion? on'RemoteHeld', on'RemoteResumed' ? More general, there are other notifications which are similar to this one. For exmaple: the call is a forwarded call, we enter a a multiparty call, the outgoing/incoming call are barred, call is waiting, call has been forwarded. (See TS 27.007 7.l7 for more items) If we would like to support them in the future, we need to think about whether we should cluster them into one event because they are all related to some kinds of additional call notification.

Szu-Yu, I think that Hsin-Yi can help you with that design.

(In reply to Szu-Yu Chen [:aknow] from comment #3) > How about notify by event? > Two options: associate the event on (1) Telephony or (2) TelephonyCall > > For (2) TelephonyCall, it seems more reasonable. That indicates the remote > party held the 'call'. Maybe we got several calls, we could know actually > which one has been put on hold. However, from my test experience, there > might be some technical difficulty. The notification from ril does not > guarantee to contain the phone number information. Yes, I just test it, and > get nothing. Thus we don't know how to deliver the event to the correct call > object. > > So, maybe we could just fire the event on Telephony. It depends on the > requirement. Do we really need to know which call is held? > I think it's good to know which call has been put on hold. However, due to technical restriction, we may not really want to have a design of exposing the event to nsIDOMTelephonyCall that will lead to miss call held notifications if the modem doesn't provide 'number.' Thus another way I would suggest is exposing the event to nsIDOMTelephony and using nsIDOMCallEvent. App can then get evt.call if the modem does provide 'number' in a notification. How does that sound? > For event, we need a name. Any suggestion? > on'RemoteHeld', on'RemoteResumed' ? > Look good to me! > More general, there are other notifications which are similar to this one. > For exmaple: the call is a forwarded call, we enter a a multiparty call, the > outgoing/incoming call are barred, call is waiting, call has been forwarded. > (See TS 27.007 7.l7 for more items) > If we would like to support them in the future, we need to think about > whether we should cluster them into one event because they are all related > to some kinds of additional call notification. It looks not so convincing to group that bunch of notifications into only one event type, since the notifications are so various. We should group some while that makes sense and helps our API, but it is not necessary to do so unconditionally.

Comment on attachment 765219 [details] [diff] [review] Part 1: Add remoteheld/remoteresumed event (idl). Hsin-Yi's review should be enough.

Comment on attachment 765220 [details] [diff] [review] Part 2: Add remoteheld/remoteresumed event (dom). Review of attachment 765220 [details] [diff] [review]: ----------------------------------------------------------------- Looks great to me for Bluetooth part. Thanks.

Comment on attachment 765219 [details] [diff] [review] Part 1: Add remoteheld/remoteresumed event (idl). Review of attachment 765219 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comment addressed. ::: dom/telephony/nsITelephonyProvider.idl @@ +62,4 @@ > in boolean isEmergency); > > /** > + * Notify when RIL receive supplementary service notification. nit: s/RIL receive/the RIL receives @@ +66,5 @@ > + * > + * @param callIndex > + * Call identifier assigned by the RIL. -1 if not specified > + * @param notification > + * Notification type Please comment on the possible values of notification type. @@ +104,3 @@ > const unsigned short CALL_STATE_DISCONNECTED = 10; > const unsigned short CALL_STATE_INCOMING = 11; > /** * Supplementary service notifications. */

Comment on attachment 765220 [details] [diff] [review] Part 2: Add remoteheld/remoteresumed event (dom). Review of attachment 765220 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but we will need a DOM peer here. bent should be the best person to ask for. :)

Comment on attachment 765221 [details] [diff] [review] Part 3: Handle supplementary service notification in RIL. Review of attachment 765221 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Just some convention style issue. ::: dom/system/gonk/ril_consts.js @@ +2591,5 @@ > "750" // Falkland Islands (Malvinas) > ]; > > +// Supplementary service notifications, code2, as defined in 3GPP 27.007 7.17 > +this.SUPP_SVC_NOTIFICATION_REMOTE_HELD = "RemoteHeld"; Let's follow the convention, using the style of this.GECKO_SUPP_SVC_REMOTE_HELD ... So this part would become: // Supplementary service notifications, code2, as defined in 3GPP 27.007 7.17 this.SUPP_SVC_NOTIFICATION_CODE2_PUT_ON_HOLD = 2; this.SUPP_SVC_NOTIFICATION_CODE2_RETRIEVED = 3; this.GECKO_SUPP_SVC_REMOTE_HELD = "RemoteHeld"; this.GECKO_SUPP_SVC_REMOTE_RESUMED = "RemoteResumed"; this.GECKO_SUPP_SVC_NOTIFICATION_FROM_CODE2 = {}; ... ... ::: dom/system/gonk/ril_worker.js @@ +3451,5 @@ > + let notification = null; > + let callIndex = -1; > + > + if (info.notificationType === 0) { > + // MO intermediate result code (ref to code1). refer to code1 defined in 3GPP 27.007 7.17 @@ +3452,5 @@ > + let callIndex = -1; > + > + if (info.notificationType === 0) { > + // MO intermediate result code (ref to code1). > + nit: remove the extra line @@ +3454,5 @@ > + if (info.notificationType === 0) { > + // MO intermediate result code (ref to code1). > + > + } else if (info.notificationType === 1) { > + // MT unsolicited result code (ref to code2). ditto. @@ +3464,5 @@ > + default: > + // Do nothing. > + } > + > + // Type and Number are MT only. Where is Type? @@ +3466,5 @@ > + } > + > + // Type and Number are MT only. > + if (info.number) { > + // Match in current calls. nit: 'Match in' seems not the right idiom for this case.

Comment on attachment 765222 [details] [diff] [review] Part 4: Test for supplementary service notification in RIL. Review of attachment 765222 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_ssn.js @@ +50,5 @@ > + }); > +} > + > +add_test_notification_type(SUPP_SVC_NOTIFICATION_CODE2_PUT_ON_HOLD, SUPP_SVC_NOTIFICATION_REMOTE_HELD); > +add_test_notification_type(SUPP_SVC_NOTIFICATION_CODE2_RETRIEVED, SUPP_SVC_NOTIFICATION_REMOTE_RESUMED); How about we change it a little bit? We create a single test "test_notification_type" which test all different input types. I.e., 56 add_test(function test_notification_type() { 57 let workerHelper = _getWorker(); 58 let worker = workerHelper.worker; 59 60 function testNotificationType(code, result) { 61 62 let info = { 63 notificationType: 1, // MT 64 code: code, 65 index: 0, 66 type: 0, 67 number: null 68 }; 69 70 worker.RIL._processSuppSvcNotification(info); 71 72 let postedMessage = workerHelper.postedMessage; 73 do_check_eq(postedMessage.rilMessageType, 'suppSvcNotification'); 74 do_check_eq(postedMessage.notification, result); 75 do_check_eq(postedMessage.callIndex, -1); 76 } 77 78 testNotificationType(SUPP_SVC_NOTIFICATION_CODE2_PUT_ON_HOLD, SUPP_SVC_NOTIFICATION_REMOTE_HELD); 79 testNotificationType(SUPP_SVC_NOTIFICATION_CODE2_RETRIEVED, SUPP_SVC_NOTIFICATION_REMOTE_RESUMED); 80 81 run_next_test(); 82 }); @@ +105,5 @@ > + > + let postedMessage = workerHelper.postedMessage; > + do_check_eq(postedMessage.rilMessageType, 'suppSvcNotification'); > + do_check_eq(postedMessage.notification, SUPP_SVC_NOTIFICATION_REMOTE_HELD); > + do_check_eq(postedMessage.callIndex, 1); Let's test one more case for null number. And I feel like we could merge test_notification_type and test_match_number into one.

Comment on attachment 768243 [details] [diff] [review] Part 4#2: Test for supplementary service notification in RIL. Review of attachment 768243 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Comment on attachment 768242 [details] [diff] [review] Part 3#2: Handle supplementary service notification in RIL. Review of attachment 768242 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3452,5 @@ > + let callIndex = -1; > + > + if (info.notificationType === 0) { > + // MO intermediate result code. > + // (refer to code1 defined in 3GPP 27.007 7.17). nit: Wrap it at 80th character. @@ +3455,5 @@ > + // MO intermediate result code. > + // (refer to code1 defined in 3GPP 27.007 7.17). > + } else if (info.notificationType === 1) { > + // MT unsolicited result code. > + // (refer to code2 defined in 3GPP 27.007 7.17). ditto. @@ +3473,5 @@ > + // national, network specific, .... (refer to 3GPP TS 24.008 10.5.4.7) > + // > + // We find the call in |currentCalls| by the given number. This is the > + // call object associated with the notification. Note that |type| is not > + // used here because there is no type information in |currentCalls|. nit: It's a bit weird to me that we explain so much about a thing not in our implementation. Could we just remove it?

Comment on attachment 765220 [details] [diff] [review] Part 2: Add remoteheld/remoteresumed event (dom). Review of attachment 765220 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks generally fine but I'd like to hear the reason why we would be receiving a notification without a valid call index. Can you please explain how this can happen? It certainly seems weird. ::: dom/bluetooth/BluetoothTelephonyListener.cpp @@ +66,5 @@ > NS_IMETHODIMP > +TelephonyListener::NotifySuppSvcNotification(int32_t aCallIndex, > + uint16_t aNotification) > +{ > + return NS_OK; This needs a followup bug and probably a NS_WARNING until it is implemented, right? ::: dom/telephony/Telephony.cpp @@ +508,4 @@ > } > > NS_IMETHODIMP > +Telephony::NotifySuppSvcNotification(int32_t aCallIndex, Nit: it's not clear what "SuppSvc" means. Abbreviations in general are not so awesome. Can we name this something better? @@ +510,5 @@ > NS_IMETHODIMP > +Telephony::NotifySuppSvcNotification(int32_t aCallIndex, > + uint16_t aNotification) > +{ > + nsRefPtr<TelephonyCall> associateCall; Nit: 'associatedCall' @@ +514,5 @@ > + nsRefPtr<TelephonyCall> associateCall; > + if (!mCalls.IsEmpty() && aCallIndex != -1) { > + for (uint32_t index = 0; index < mCalls.Length(); index++) { > + nsRefPtr<TelephonyCall>& call = mCalls[index]; > + if (call->CallIndex() == (uint32_t)aCallIndex) { Nit: Please use C++-style conversion: |uint32_t(aCallIndex)|

Comment on attachment 765220 [details] [diff] [review] Part 2: Add remoteheld/remoteresumed event (dom). Review of attachment 765220 [details] [diff] [review]: ----------------------------------------------------------------- Hi ben, Thanks for the review. >> why we would be receiving a notification without a valid call index The notification from ril does not guarantee to contain the phone number information. Thus we could not identify the correct target. You could refer to Comment 3 and Comment 5 for more discussion. ::: dom/bluetooth/BluetoothTelephonyListener.cpp @@ +66,5 @@ > NS_IMETHODIMP > +TelephonyListener::NotifySuppSvcNotification(int32_t aCallIndex, > + uint16_t aNotification) > +{ > + return NS_OK; Maybe not. just like TelephonyListener::EnumerateCallStateComplete() http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothTelephonyListener.cpp If bluetooth does not care about the notification, it could just let it blank. ::: dom/telephony/Telephony.cpp @@ +508,4 @@ > } > > NS_IMETHODIMP > +Telephony::NotifySuppSvcNotification(int32_t aCallIndex, Any idea for this part? SUPP_SVC_NOTIFICATION is used in reference RIL. I try to keep the same naming everywhere to make it consistent. The full name of SuppSvc is "supplementary service" and the common abbreviation used in 3gpp domain is "SS". Is it better to use the full name? NotifySupplementaryServiceNotification ?

(In reply to Szu-Yu Chen [:aknow] from comment #22) > Thanks for the review. You bet! I'd like to review the final patch though. > > >> why we would be receiving a notification without a valid call index > The notification from ril does not guarantee to contain the phone number > information. That's really too bad :( Is it only for phones that have a limit of one call? > Is it better to use the full name? > NotifySupplementaryServiceNotification ? Hm, How about just "SupplementaryServiceNotification"?

As Comment 23, use full name for 'SuppSvc'.

As Comment 23, use full name for 'SuppSvc'.

As discussion, if there is no number information from ril but only one call existed at that time, we will 'guess' it is the target call.

Comment on attachment 775542 [details] [diff] [review] Part 3#3: Handle supplementary service notification in RIL. Review of attachment 775542 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3578,5 @@ > + } else if (info.notificationType === 1) { > + // MT unsolicited result code. Refer to code2 defined in 3GPP 27.007 7.17. > + switch (info.code) { > + case SUPP_SVC_NOTIFICATION_CODE2_PUT_ON_HOLD: > + case SUPP_SVC_NOTIFICATION_CODE2_RETRIEVED: nit: indention @@ +3580,5 @@ > + switch (info.code) { > + case SUPP_SVC_NOTIFICATION_CODE2_PUT_ON_HOLD: > + case SUPP_SVC_NOTIFICATION_CODE2_RETRIEVED: > + notification = GECKO_SUPP_SVC_NOTIFICATION_FROM_CODE2[info.code]; > + break; nit: indention @@ +3583,5 @@ > + notification = GECKO_SUPP_SVC_NOTIFICATION_FROM_CODE2[info.code]; > + break; > + default: > + // Do nothing. > + break; Sorry for not mentioning this in the previous review. Let's bail out earlier when no notification. default: // Notification type not supported. return; @@ +3586,5 @@ > + // Do nothing. > + break; > + } > + > + // Get target call object for this notification. nit: s/target/ the target @@ +3588,5 @@ > + } > + > + // Get target call object for this notification. > + let currentCallIndexes = Object.keys(this.currentCalls); > + if (currentCallIndexes.length === 1) { Good idea! @@ +3589,5 @@ > + > + // Get target call object for this notification. > + let currentCallIndexes = Object.keys(this.currentCalls); > + if (currentCallIndexes.length === 1) { > + // Only one call existed. This should be the target. nit: s/existed/exists @@ +3604,5 @@ > + } > + } > + } > + > + if (notification) { With the change in line #3857, it should be fine to get rid of this condition.

Comment on attachment 775543 [details] [diff] [review] Part 4#3: Test for supplementary service notification in RIL. Review of attachment 775543 [details] [diff] [review]: ----------------------------------------------------------------- Thank you.

Comment on attachment 775539 [details] [diff] [review] Part 2#2: Add remoteheld/remoteresumed event (dom). Review of attachment 775539 [details] [diff] [review]: ----------------------------------------------------------------- Looks great for Bluetooth part.

Comment on attachment 775539 [details] [diff] [review] Part 2#2: Add remoteheld/remoteresumed event (dom). Review of attachment 775539 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

https://hg.mozilla.org/integration/b2g-inbound/rev/549ed41014b9 https://hg.mozilla.org/integration/b2g-inbound/rev/0885b43c985e https://hg.mozilla.org/integration/b2g-inbound/rev/afdac3a82131 https://hg.mozilla.org/integration/b2g-inbound/rev/1b5c2e0fca56

https://hg.mozilla.org/mozilla-central/rev/549ed41014b9 https://hg.mozilla.org/mozilla-central/rev/0885b43c985e https://hg.mozilla.org/mozilla-central/rev/afdac3a82131 https://hg.mozilla.org/mozilla-central/rev/1b5c2e0fca56

Sorry to arrive so late on this but I'm not sure we need a special case for this when we already need an in-call SS handler (see 3GPP 22.030). In fact, why is this even needed when the RIL_UNSOL_RESPONSE_CALL_STATE_CHANGED mechanism is already available to provide the information for this state transition?

(In reply to Michael Schwartz [:m4] from comment #41) > Sorry to arrive so late on this but I'm not sure we need a special case for > this when we already need an in-call SS handler (see 3GPP 22.030). In fact, > why is this even needed when the RIL_UNSOL_RESPONSE_CALL_STATE_CHANGED > mechanism is already available to provide the information for this state > transition? Hi Michael, Maybe I don't get your idea clearly. Would you describe more about the in-call SS handler? How could the handler solve this problem? Let me explain what we do in this bug. The bug aims at providing a notification when remote called party hold/resume the call. In this case, we will not get call_state_changed at out side because it is still in the connected state and no state transition occurs.

Hi there: RIL_UNSOL_SUPP_SVC_NOTIFICATION and RIL_UNSOL_RESPONSE_CALL_STATE_CHANGED are two different stories. RIL_UNSOL_RESPONSE_CALL_STATE_CHANGED is an event trigged from user's action, for example, user can answer/hold/resume/swap/hangup call, and, get according events. However, RIL_UNSOL_SUPP_SVC_NOTIFICATION is about remote call behaviour. device doesn't even know what action has been requested remotely. Therefore, device will not get any RIL_UNSOL_RESPONSE_CALL_STATE_CHANGE (if remote device request to hold/resume/swap etc...), but RIL_UNSOL_SUPP_SVC_NOTIFICATION. Note: 3GPP 22.007 clause 7.17 Supplementary service notifications +CSSN has definition about information MSC provide. Thanks!! sku

sku, is it correct to actually put a call on hold for example rather than just display the information to the user which is what Android does. I am not sure if user would be able to actually resume the call if the other party has put the call on hold.

(In reply to Anshul from comment #44) > sku, is it correct to actually put a call on hold for example rather than > just display the information to the user which is what Android does. I am > not sure if user would be able to actually resume the call if the other > party has put the call on hold. No, user cannot resume the call which is put on call by the remote party.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #45) > (In reply to Anshul from comment #44) > > sku, is it correct to actually put a call on hold for example rather than > > just display the information to the user which is what Android does. I am > > not sure if user would be able to actually resume the call if the other > > party has put the call on hold. > > No, user cannot resume the call which is put on call by the remote party. ^^^^^^^^^^^^^ put on hold

Hi Hsin-Yi, Anshul, Shawn, and Szu-Yu, Yes, thank you for clarifying - without seeing the Gaia usage I was unclear on what was intended by this change. Why did we implement CALL HOLD? I haven't seen OEMs ask for support of this; however I have seen support of the redirection info which is not included here. What I see in Android and BREW is simply passing all the supp service info up to the UI in the same manner since in all cases we simply provide some user-indication as an fyi. The current implementation would require many callbacks which seems like overkill.

Hi Michael: Yes, this feature is a kind of FYI. For such cases, device (user) can not do actions but just be aware of having something happened remotely. Besides, Android did not have too much implementation on UI side as my memory. Finally, this is a nice-to-have feature if UI disaplyed is locked down, or, misleading/confusion will be troubles we have to face with. Thanks!! sku