Currently B2G supports a single SIM architecture. We use navigator.mozTelephony to talk to the sim. In multi-SIMs configuration, there shall be multiple telephony objects in each content process. Each of those objects communicates with an individual sim card. A superior manager object is required to manage and access the telephony objects.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1) > Created attachment 691746 [details] > Working branch Working branch updated - more test cases are coming

added nsIDOMTelephonyManager

Revised 3 basic tests to verify the design concept first. More modifications are coming.

Comment on attachment 701725 [details] [diff] [review] WIP: part1 - idl change Hi Jonas and Mounir, May I ask for your feedback on this multisim-telephony API please? The main concept here is introducing TelephonyManager as the superior manager of multiple telephony services. You may find more details here: https://wiki.mozilla.org/WebAPI/WebTelephony/Multi-SIM Thank you :)

Comment on attachment 701725 [details] [diff] [review] WIP: part1 - idl change Review of attachment 701725 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure if the current approach is the best approach but I don't know much about multiple SIM support. What is the phone able to do in that case? Is switching from a SIM to another switch the entire phone to that SIM or can the phone use 3G on a SIM and call on another or even, is the phone always using all sims? How are applications expected to work? Should each app allow you to set the SIM you currently want to use? I think we need more discussions about the technical limitations and the use cases. Also, I'm not sure this is taking into account all APIs related to a SIM. Like data connections, SMS and voicemails (for example) are also related to a SIM. Should we have a similar mechanism for each of these APIs or should we simply have an API dedicated to SIM management?

(In reply to Mounir Lamouri (:mounir) from comment #14) > Comment on attachment 701725 [details] [diff] [review] > WIP: part1 - idl change > > Review of attachment 701725 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure if the current approach is the best approach but I don't know > much about multiple SIM support. What is the phone able to do in that case? > Is switching from a SIM to another switch the entire phone to that SIM or > can the phone use 3G on a SIM and call on another or even, is the phone > always using all sims? How are applications expected to work? Should each > app allow you to set the SIM you currently want to use? > Thanks for the feedback, Mounir. In the multisim scenario, the phone, the whole device, is using all sims that are all on standby. And all these sims might use different technology, like gsm, cdma, etc. Each app should be able to allow user to choose the sim as the user currently wants. Also, among different APIs such as mobile connections and telephony, technically user can set a default sim for 3G and another default sim for telephony (though the default setting isn't addressed in this API, it should be another issue IMO). Taking telephony as example, user should be able to choose via which sim he wants to make a call. User can choose to use a specific sim or use the default one simply. For example, he can call friend A by sim no. 1 and to call friend B by sim no. 2. He is able to receive calls from any sim. At the same time, he could even to set up a data connection by sim no. X. > I think we need more discussions about the technical limitations and the use > cases. Sure, this is always the first step! >Also, I'm not sure this is taking into account all APIs related to a > SIM. Like data connections, SMS and voicemails (for example) are also > related to a SIM. Should we have a similar mechanism for each of these APIs > or should we simply have an API dedicated to SIM management? We are also considering multisim API for mobile connections (bug 814629), voicemails (bug 814634), sms (bug 814635) and stk (bug 814637). You can also find related platform implementation for multisim in the meta bug 799023.

Blocking-, no longer targeting Leo release. Product will retarget for future release.

It's been a while since my last ping... Any further comments or thoughts about this API? To meet partners' requirements and requests, we really need to have progress here. Some partners have developed multisim-dialer based on the proposal here, so far so good. Other multisim WebAPIs, such as mobile connection and voicemail, are currently following the similar design. The design works though we also know there is always room for improvement. It would be very helpful to have more opinions and feedbacks. Thanks :)

Working branch updated - existing test cases revision

added nsIRILTelephonyManagerCallback

implemented nsIRILTelephonyManagerCallback, instead of using nsIObserver

implemented TelephonyManagerCallback

revised test cases: using |window.navigator.mozTelephonyManager.defaultPhone|

This is the right patch!

Usually, discussions regarding APIs do not happen in bugs unless they are brief. Here, I think it would be interesting to reach a broader audience. Also, the discussion should not happen with only one API in mind. The fact that multiple API will be involved is very important for the design. I think it would be interesting if you could post to dev-webapi about the current plans for multi-sim and have the discussion happen there.

(In reply to Mounir Lamouri (:mounir) from comment #25) > Usually, discussions regarding APIs do not happen in bugs unless they are > brief. Here, I think it would be interesting to reach a broader audience. > Also, the discussion should not happen with only one API in mind. The fact > that multiple API will be involved is very important for the design. > I think it would be interesting if you could post to dev-webapi about the > current plans for multi-sim and have the discussion happen there. Thanks for reminding, Mounir :) But yes, I have posted the proposal to dev-webapi, please see https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/ZmFvU47eSvA I will update the post there to describe more about the multisim scenario, and hopefully arousing more attention.

Hsin, sorry for being so slow to respond here. I've been trying to focus on the v1.0 release but I recognize that this is an important item. Sending another post to public-webapi sounds good. Could you also describe there what capabilities hardware that supports multiple sim cards usually has. For example, can you place calls with both simcards at the same time?

(In reply to Jonas Sicking (:sicking) from comment #27) > Hsin, sorry for being so slow to respond here. I've been trying to focus on > the v1.0 release but I recognize that this is an important item. > > Sending another post to public-webapi sounds good. Could you also describe > there what capabilities hardware that supports multiple sim cards usually > has. For example, can you place calls with both simcards at the same time? Jonas, thanks for your time and the reply, especially in the situation of the tight v1.0 schedule. :) Sure, let's have more discussion on dev-wepapi!

Updating MozWiki according to discussion on the mailing list dev-b2g https://wiki.mozilla.org/WebAPI/WebTelephony/Multi-SIM#Proposal:_WebTelephony_API_for_Multi-SIM

Comment on attachment 715399 [details] [diff] [review] WIP: part1 - idl change Our current design is as comment 29.

Comment on attachment 715400 [details] [diff] [review] WIP: part2 - DOMClassInfo Our current design is as comment 29.

Comment on attachment 715401 [details] [diff] [review] WIP: part3 - DOM impl - WebTelephony Our current design is as comment 29.

Comment on attachment 715403 [details] [diff] [review] WIP: part5 - test cases Our current design is as comment 29.

Comment on attachment 715405 [details] [diff] [review] WIP: part4 - ril impl Our current design is as comment 29.

This patch contains the multi-sim implementation and some code refactoring. If I have time, I may split it to several small commits. The patch is not yet complete. Need some info. 1. How to get the number of services 2. What is the default serviceId 179:// TODO(aknow): Find a better function name. 183: // TODO(aknow): Default id may not be 0. 198: // TODO(aknow): How to get the valid range.

Also, need to know how to get the total number of service for this todo. 304: // TODO(aknow): Should enumerate on all service, and merge them together

Not dependent on bug 814584 anymore as TelephonyProvider is moved out of RILContentHelper.

Comment on attachment 813050 [details] [diff] [review] Part 1: webidl: code reorder Review of attachment 813050 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. We still need DOM-peer's review.

Comment on attachment 813051 [details] [diff] [review] Part 2: webidl: add multisim support Review of attachment 813051 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. We still need DOM-peer's review. ::: dom/webidl/Telephony.webidl @@ +12,5 @@ > + * the implementation MUST use the default service. > + * > + * Possible values of |serviceId| are 0 ~ (number of services - 1), which is > + * simply the index of a service. Get number of services by acquiring > + * navigator.mozMobileConnections.length nit: place a period at the end of a line.

Comment on attachment 816419 [details] [diff] [review] Part 3#2: dom: code reorder Review of attachment 816419 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. We still need DOM-peer's review.

Comment on attachment 816420 [details] [diff] [review] (WIP) Part 4a: dom: refactor Review of attachment 816420 [details] [diff] [review]: ----------------------------------------------------------------- Seems this patch isn't really ready for review, but for feedback instead. f- because some problems in GetCallAndMoveItToProperGroup(). ::: dom/telephony/Telephony.cpp @@ +257,5 @@ > > nsRefPtr<TelephonyCall> call = CreateNewDialingCall(aNumber); > > // Notify other telephony objects that we just dialed. > + for (uint32_t i = 0; i < gTelephonyList->Length(); i++) { I like 'index' more than i. Could we just keep it? @@ +301,3 @@ > { > + if (aIsActive) { > + if (!aNeedCheckState || IsActiveState(aCall->CallState())) { The original code: aIsAdding means a call is added to Telephony.Calls or not, nothing related to its state. But aIsActive quite confuses here. |aIsActive| should equal |IsActiveSate()|. I don't get the conditions. And any possible situation that aIsActive == false while aNeedCheckState == true? I don't see the reason you introduced the extra parameter 'aNeedCheckState.' @@ +324,5 @@ > return call.forget(); > } > > +already_AddRefed<TelephonyCall> > +Telephony::GetLastCall() The naming doesn't match the implementation. You are getting the last 'dialing' call. @@ +552,2 @@ > > + modifiedCall = GetCallAndMoveItToProperGroup(aCallIndex, We can't simply do the group moving here. We need 'ChangeStateInternal' as well. Otherwise, we get problems with activeCall update.

Comment on attachment 816420 [details] [diff] [review] (WIP) Part 4a: dom: refactor Review of attachment 816420 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +177,5 @@ > > +// static > +// TODO(aknow): Find a better function name. > +uint32_t > +Telephony::GetServiceId(const Optional<uint32_t>& aServiceId) Please move this to part4b. @@ +192,5 @@ > +} > + > +// static > +bool > +Telephony::IsValidServiceId(uint32_t aServiceId) Please move this to part4b.

Comment on attachment 816421 [details] [diff] [review] (WIP) Part 4b: dom: multisim Review of attachment 816421 [details] [diff] [review]: ----------------------------------------------------------------- This highly depends on part4b. I would like to see my doubt in part4b resolve first before diving into this part. ::: dom/telephony/Telephony.cpp @@ +232,5 @@ > mActiveCall->ServiceId() == aCall->ServiceId()); > } > > already_AddRefed<TelephonyCall> > +Telephony::DialInternal(const nsAString& aNumber, uint32_t aServiceId, Per our last discussion, please move 'aServiceId' to the first place, here and below. ::: dom/telephony/Telephony.h @@ +169,5 @@ > bool > MatchActiveCall(TelephonyCall* aCall); > > already_AddRefed<TelephonyCall> > + DialInternal(const nsAString& aNumber, uint32_t aServiceId, Per our last discussion, please move 'aServiceId' to the first place, here and below.

Comment on attachment 816421 [details] [diff] [review] (WIP) Part 4b: dom: multisim Review of attachment 816421 [details] [diff] [review]: ----------------------------------------------------------------- This highly depends on part4a. I would like to see my doubt in part4a resolve first before diving into this part.

Comment on attachment 816420 [details] [diff] [review] (WIP) Part 4a: dom: refactor Review of attachment 816420 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +257,5 @@ > > nsRefPtr<TelephonyCall> call = CreateNewDialingCall(aNumber); > > // Notify other telephony objects that we just dialed. > + for (uint32_t i = 0; i < gTelephonyList->Length(); i++) { I don't like 'index'. It might be confused with 'callIndex'. And it doesn't have any special meaning in this for loop. @@ +301,3 @@ > { > + if (aIsActive) { > + if (!aNeedCheckState || IsActiveState(aCall->CallState())) { 'mActiveCall' should be controlled in only one place. That is the purpose of the modification in this function. I will separate this function into two functions and remove 2nd param: a. SetToActiveCall(call, needCheckState) b. RemoveFromActiveCall(call) Original function ensures 'IsActiveState(aCall->CallState())' then set it to 'mActiveCall'. However, in some usage, ex: line 533 (see original), we directly update 'mActiveCall' w/o check the state. So, I introduce 3rd param 'aNeedCheckState' to distinguish these two cases. @@ +552,2 @@ > > + modifiedCall = GetCallAndMoveItToProperGroup(aCallIndex, Could the problem you mentioned be caught by test case? The modification could pass all the marionette test cases. I am not really understand the problem. Could you show the detailed info?

Comment on attachment 816421 [details] [diff] [review] (WIP) Part 4b: dom: multisim Review of attachment 816421 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/Telephony.cpp @@ +232,5 @@ > mActiveCall->ServiceId() == aCall->ServiceId()); > } > > already_AddRefed<TelephonyCall> > +Telephony::DialInternal(const nsAString& aNumber, uint32_t aServiceId, I would like the param order match with 'Telephony::Dial'.

Comment on attachment 816928 [details] [diff] [review] Part 5#3: telephony provider and idl Review of attachment 816928 [details] [diff] [review]: ----------------------------------------------------------------- Please help rename nsIGonkTelephonyProvider/nsITelephonyProvider to nsIGonkTelephonyService/nsITelephonyProvider [1]. Thank you. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=864485#c6 ::: dom/telephony/gonk/TelephonyProvider.js @@ +78,5 @@ > return ns.PhoneNumberUtils; > }); > > function TelephonyProvider() { > + this._numServices = gRadioInterfaceLayer.numRadioInterfaces; _numClients @@ +109,5 @@ > this._callRingWakeLock = null; > } > }, > > + _getService: function _getService(aServiceId) { _getClient @@ +135,5 @@ > > + _matchActiveCall: function _matchActiveCall(aCall) { > + if (this._activeCall && > + this._activeCall.callIndex == aCall.callIndex && > + this._activeCall.serviceId == aCall.serviceId) { Where does .serviceId come from? @@ +301,5 @@ > > this._listeners.splice(index, 1); > }, > > + _enumerateCallsForService: function _enumerateCallsForService(aServiceId, _enumerateCallsForClient @@ +308,5 @@ > + > + let deferred = Promise.defer(); > + > + this._getService(aServiceId).sendWorkerMessage("enumerateCalls", null, > + (function(response) { indention? @@ +316,2 @@ > > + aListener.enumerateCallState(call.aServiceId, call.callIndex, Where does call.aServiceId come from? @@ +335,5 @@ > + this._enumerateCallsForService(aServiceId, aListener); > + }; > + } > + > + let promise = Promise.resolve(); Nice promise! @@ +337,5 @@ > + } > + > + let promise = Promise.resolve(); > + for (let i = 0; i < this._numServices; ++i) { > + promise = promise.then(makeEnumerateCallsFunction(i).bind(this)); Let's try this._enumerateCallsForService.apply() so that we might not need the wrapper makeEnumerateCallsFunction(). @@ +346,3 @@ > }, > > + dial: function(aServiceId, aNumber, aIsEmergency) { aClientId, please! @@ +456,5 @@ > gSystemMessenger.broadcastMessage("telephony-call-ended", data); > > this._updateCallAudioState(aCall, null); > > + this._notifyAllListeners("callStateChanged", [aCall.serviceId, Where does 'aCall.*serviceId*' come from? @@ +508,5 @@ > } > > this._updateCallAudioState(aCall, null); > > + this._notifyAllListeners("callStateChanged", [aCall.serviceId, Where does 'aCall.*serviceId*' come from? ::: dom/telephony/nsIGonkTelephonyProvider.idl @@ +10,4 @@ > "@mozilla.org/telephony/gonktelephonyprovider;1" > %} > > [scriptable, uuid(f072f334-e4ea-4754-9929-533da30444a8)] uuid change is necessary! @@ +12,5 @@ > > [scriptable, uuid(f072f334-e4ea-4754-9929-533da30444a8)] > interface nsIGonkTelephonyProvider : nsITelephonyProvider > { > void notifyCallDisconnected(in jsval call); Each function needs to take an additional 'clientId' parameter. @@ +14,5 @@ > interface nsIGonkTelephonyProvider : nsITelephonyProvider > { > void notifyCallDisconnected(in jsval call); > > + void notifyCallError(in unsigned long serviceId, Per our last discussion, use 'clientId' internally. ::: dom/telephony/nsITelephonyProvider.idl @@ +9,5 @@ > { > /** > * Notified when a telephony call changes state. > * > + * @param serviceId Per our last discussion, use 'clientId' internally.

Comment on attachment 813058 [details] [diff] [review] Part 7: ril Review of attachment 813058 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +931,5 @@ > case "conferenceCallStateChanged": > gTelephonyProvider.notifyConferenceCallStateChanged(message.state); > break; > case "cdmaCallWaiting": > gTelephonyProvider.notifyCdmaCallWaiting(message.number); More functions should carry 'this.clientId.' See review comments for part 5. ::: dom/system/gonk/ril_worker.js @@ +3591,5 @@ > } else { > newCall.isConference = false; > } > + > + newCall.serviceId = CLIENT_ID; If we carry a clientId in nsIGonkTelephonyService, we probably don't need to add a new property in calls in ril_worker.js.

Comment on attachment 813057 [details] [diff] [review] Part 6: ipc Review of attachment 813057 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks good except we need to rename 'serviceId' and modify some functions according to review comments for part 5. Thank you. ::: dom/telephony/ipc/PTelephony.ipdl @@ +16,5 @@ > manager PContent; > manages PTelephonyRequest; > > child: > + NotifyCallError(uint32_t aServiceId, int32_t aCallIndex, nsString aError); Let's use aClientId, here and below. @@ +21,4 @@ > > NotifyCallStateChanged(IPCCallStateData aData); > > NotifyCdmaCallWaiting(nsString aNumber); aClientId as well... ::: dom/telephony/ipc/TelephonyTypes.ipdlh @@ +9,5 @@ > namespace telephony { > > struct IPCCallStateData > { > + uint32_t serviceId; clientId

Comment on attachment 813059 [details] [diff] [review] Part 8: bluetooth Review of attachment 813059 [details] [diff] [review]: ----------------------------------------------------------------- We should address part 5 first. Once part 5 is ready, then I am fine in BT code. Kindly remember to ask for BT peers' review at that time.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #59) > Comment on attachment 816928 [details] [diff] [review] > Part 5#3: telephony provider and idl > > Review of attachment 816928 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please help rename nsIGonkTelephonyProvider/nsITelephonyProvider to > nsIGonkTelephonyService/nsITelephonyProvider [1]. Thank you. How about telephony/gonk/TelephonyProvider.js and TelephonyProvider.manifest? Should we also rename them to 'Service'?

(In reply to Szu-Yu Chen [:aknow] from comment #63) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #59) > > Comment on attachment 816928 [details] [diff] [review] > > Part 5#3: telephony provider and idl > > > > Review of attachment 816928 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Please help rename nsIGonkTelephonyProvider/nsITelephonyProvider to > > nsIGonkTelephonyService/nsITelephonyProvider [1]. Thank you. > > How about telephony/gonk/TelephonyProvider.js and TelephonyProvider.manifest? > Should we also rename them to 'Service'? Yes, please do. Thanks for raising this.

Comment on attachment 817632 [details] [diff] [review] Part 5#4: telephony provider and idl Review of attachment 817632 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/gonk/TelephonyProvider.js @@ +332,5 @@ > + if (DEBUG) debug("Requesting enumeration of calls for callback"); > + > + let promise = Promise.resolve(); > + for (let i = 0; i < this._numClients; ++i) { > + promise = promise.then(this._enumerateCallsForClient.call(this, i, aListener)); Should be => this._enumerateCallsForClient.bind(this, i, aListener) ? I will confirm it later.

As I know, multi-sim architecture is only available on jb emulator. So, I attach a reference test case showing how I test the modified multi-sim api.

Comment on attachment 817629 [details] [diff] [review] Part 4a#2: dom: refactor Review of attachment 817629 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for refactoring! It looks chic. Please also help modify test case for 'updateActiveCall' in the following situation: 1) A conference group contains two call. One call is removed from the group. So we have one connected call and one held call. 2) A conference group contains two call. One call is released by the remote party. So we have one connected call left. ::: dom/telephony/Telephony.cpp @@ +516,5 @@ > + // Add to conference. > + if (!group && aIsConference) { > + NS_ASSERTION(mCalls.Contains(modifiedCall), "Should in mCalls"); > + RemoveCall(modifiedCall); > + mGroup->AddCall(modifiedCall); Please switch the order of line#519 and #520. In [1], the API said the event firing sequence is fixed: callgroup.oncallschanged then telephony.oncallschanged. Let's keep it in case we break gaia logic as I don't see technical issues that we need to change that. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=772765#c82 @@ +521,4 @@ > } > > + // Remove from conference. > + if (group && !aIsConference) { else if (group && !aIsConference)

Comment on attachment 817630 [details] [diff] [review] Part 4b#2: dom: multisim Review of attachment 817630 [details] [diff] [review]: ----------------------------------------------------------------- Truly nice! ::: dom/telephony/Telephony.cpp @@ +179,5 @@ > +// TODO(aknow): Find a better function name. > +uint32_t > +Telephony::GetServiceId(const Optional<uint32_t>& aServiceId) > +{ > + // TODO(aknow): Default id may not be 0. Based on bug 926302 @@ +194,5 @@ > +// static > +bool > +Telephony::IsValidServiceId(uint32_t aServiceId) > +{ > + // TODO(aknow): How to get the valid range. Based on bug 926302

Comment on attachment 818384 [details] [diff] [review] Part 5#5: telephony provider and idl Review of attachment 818384 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/gonk/TelephonyProvider.js @@ +78,5 @@ > return ns.PhoneNumberUtils; > }); > > function TelephonyProvider() { > + this._numClients = gRadioInterfaceLayer.numRadioInterfaces; We encounter some problems in Bug 926302. So after that, we will need to read preference to get the number of radio interfaces... @@ +348,5 @@ > aNumber = gPhoneNumberUtils.normalize(aNumber); > } > if (this._validateNumber(aNumber)) { > + this._getClient(aClientId).sendWorkerMessage("dial", { number: aNumber, > + isDialEmergency: aIsEmergency }); nit: indention

Comment on attachment 817633 [details] [diff] [review] Part 6#2: ipc Review of attachment 817633 [details] [diff] [review]: ----------------------------------------------------------------- Change of interfaces looks good to me. Let's invite DOM peers for reviewing this. ::: dom/telephony/ipc/TelephonyIPCProvider.cpp @@ +70,5 @@ > } > > NS_IMETHODIMP > +TelephonyIPCProvider::Dial(uint32_t aClientId, > + const nsAString& aNumber, You could either wrap at 80th character or align every parameter. Just please follow the same coding style over the file. ::: dom/telephony/ipc/TelephonyParent.cpp @@ +104,5 @@ > return true; > } > > bool > +TelephonyParent::RecvHangUpCall(const uint32_t& aClientId, const uint32_t& aCallIndex) You could either wrap at 80th character or align every parameter. Just please follow the same coding style over the file. ::: dom/telephony/ipc/TelephonyParent.h @@ +47,5 @@ > RecvUnregisterListener() MOZ_OVERRIDE; > > virtual bool > + RecvDialCall(const uint32_t& aClientId, > + const nsString& aNumber, You could either wrap at 80th character or align every parameter. Just please follow the same coding style over the file.

Comment on attachment 817634 [details] [diff] [review] Part 7#2: ril Review of attachment 817634 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thank you.

Comment on attachment 818863 [details] [diff] [review] (WIP) Sample dsds outgoing marionette test Review of attachment 818863 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/test/marionette/test_dsds_outgoing.js @@ +106,5 @@ > +function muxModem(id) { > + let deferred = Promise.defer(); > + > + emulator.run("mux modem " + id, function() { > + deferred.resolve(); What if mux modem fails? We need to take care of 'reject()', right? @@ +151,5 @@ > + > +startTest(function() { > + muxModem(0) > + .then(makeCallForServiceId.bind(this, "0912345000", 0)) > + .then(muxModem.bind(this, 1)) It could fail when changing the modem. We shall be careful of the 'reject' callback.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #79) > Comment on attachment 818863 [details] [diff] [review] > (Ref) Sample dsds outgoing marionette test > > Review of attachment 818863 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/test/marionette/test_dsds_outgoing.js > @@ +106,5 @@ > > +function muxModem(id) { > > + let deferred = Promise.defer(); > > + > > + emulator.run("mux modem " + id, function() { > > + deferred.resolve(); > > What if mux modem fails? We need to take care of 'reject()', right? > > @@ +151,5 @@ > > + > > +startTest(function() { > > + muxModem(0) > > + .then(makeCallForServiceId.bind(this, "0912345000", 0)) > > + .then(muxModem.bind(this, 1)) > > It could fail when changing the modem. We shall be careful of the 'reject' > callback. Are we ready to land the multi-sim test case? Does it workable for emulator on TBPL? This test is just a demo for testing some basic part of DSDS api. If we want to have a thorough test, the one I provided is not complete enough. So if we need a test that could be landed, I will rewrite it. If not, the failing problem is not really crucial here.

Comment on attachment 817633 [details] [diff] [review] Part 6#2: ipc Review of attachment 817633 [details] [diff] [review]: ----------------------------------------------------------------- r=me with hsinyi's comment addressed

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #76) > Comment on attachment 818384 [details] [diff] [review] > Part 5#5: telephony provider and idl > > Review of attachment 818384 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/telephony/gonk/TelephonyProvider.js > @@ +78,5 @@ > > return ns.PhoneNumberUtils; > > }); > > > > function TelephonyProvider() { > > + this._numClients = gRadioInterfaceLayer.numRadioInterfaces > > We encounter some problems in Bug 926302. So after that, we will need to > read preference to get the number of radio interfaces... Confirmed with Vicamo. Currently, we could keep using "gRadioInterfaceLayer.numRadioInterfaces". The property "numRadioInterfaces" will not be removed after bug 926302. Also, we still need "gRadioInterfaceLayer" to get the specified RadioInterface. So original usage might be OK. No unnecessary interface is introduced or exposed.

Modify conference test based on Comment 74. However, I rewrite the whole file. It contains huge difference...

Hi Gina, For multi-sim architecture, an extra parameter "serviceId" is added to listener interface. It indicates the source of the call. The value could be 0 ~ N-1 in N-sim environment. So now, the unique key to identify a call becomes (serviceId, callIndex). That means, we might have two calls with same callIndex but different serviceId.

Comment on attachment 816419 [details] [diff] [review] Part 3#2: dom: code reorder Hi Kyle, Would you help review the part 3/4a/4b for the dom modification? - part 3/4a: some code reorder and refactoring - part 4b: add multi-sim support The previous version of those patched have got feedback+ from hsinyi and the comments are also addressed.

Comment on attachment 820906 [details] [diff] [review] Part 5#7: telephony provider and idl Review of attachment 820906 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Thank you. ::: dom/telephony/gonk/TelephonyProvider.js @@ +348,5 @@ > aNumber = gPhoneNumberUtils.normalize(aNumber); > } > if (this._validateNumber(aNumber)) { > + this._getClient(aClientId).sendWorkerMessage("dial", { number: aNumber, > + isDialEmergency: aIsEmergency }); Please line up the properties in js objects. this._getClient(aClientId).sendWorkerMessage("dial", { number: aNumber, isDialEmergency: aIsEmergency }); ::: dom/telephony/nsIGonkTelephonyProvider.idl @@ +20,1 @@ > in AString error); Please make the coding style consistent over the file. The convention here is lining parameters up. ::: dom/telephony/nsITelephonyProvider.idl @@ +10,5 @@ > /** > * Notified when a telephony call changes state. > * > + * @param clientId > + Indicate the ril client, 0 ~ (number of client - 1). nit: s/ril/RIL @@ +58,5 @@ > * telephony call state (nsITelephonyProvider::enumerateCalls). This is > * called once per call that is currently managed by the RIL. > * > + * @param clientId > + Indicate the ril client, 0 ~ (number of client - 1). ditto. @@ +85,5 @@ > /** > * Notify when RIL receives supplementary service notification. > * > + * @param clientId > + Indicate the ril client, 0 ~ (number of client - 1). ditto. @@ +99,5 @@ > /** > * Called when RIL error occurs. > * > + * @param clientId > + Indicate the ril client, 0 ~ (number of client - 1). ditto. @@ +113,5 @@ > /** > * Called when a waiting call comes in CDMA networks. > * > + * @param clientId > + Indicate the ril client, 0 ~ (number of client - 1). ditto. @@ +172,2 @@ > in boolean isEmergency); > + void hangUp(in unsigned long clientId, in unsigned long callIndex); Please make the coding style consistent over the file. The convention here is lining parameters up.

Comment on attachment 820908 [details] [diff] [review] Part 4a#3: dom: refactor Review of attachment 820908 [details] [diff] [review]: ----------------------------------------------------------------- I didn't review this very closely. I assume Hsin-Yi made sure you didn't change any of the underlying logic. ::: dom/telephony/Telephony.cpp @@ +185,5 @@ > +bool > +Telephony::HasDialingCall() > +{ > + for (uint32_t i = 0; i < mCalls.Length(); i++) { > + const nsRefPtr<TelephonyCall>& tempCall = mCalls[i]; Just name this variable 'call'? Not sure what the temp is supposed to refer to ... @@ +187,5 @@ > +{ > + for (uint32_t i = 0; i < mCalls.Length(); i++) { > + const nsRefPtr<TelephonyCall>& tempCall = mCalls[i]; > + if (tempCall->IsOutgoing() && > + tempCall->CallState() < nsITelephonyProvider::CALL_STATE_CONNECTED) { Do you really want to include CALL_STATE_UNKNOWN here? I know the existing code does but that seems odd. @@ +203,5 @@ > + aCallState == nsITelephonyProvider::CALL_STATE_CONNECTED) { > + return true; > + } > + > + return false; Just 'return aCallState == ... || aCallState == ...'? Doesn't really matter, but it's cleaner IMO. @@ +240,5 @@ > nsRefPtr<TelephonyCall> call = CreateNewDialingCall(aNumber); > > // Notify other telephony objects that we just dialed. > + for (uint32_t i = 0; i < gTelephonyList->Length(); i++) { > + Telephony*& telephony = gTelephonyList->ElementAt(i); Or gTelephonyList[i]. @@ +317,3 @@ > > + call = tempCall; > + // No break. We will search entire list to ensure only one outgoing call. It only makes sense to do that in debug builds (which we don't really test) right? NS_ASSERTION is #defined to nothing in opt builds. @@ +327,5 @@ > +Telephony::GetCallFromEverywhere(uint32_t aCallIndex) > +{ > + nsRefPtr<TelephonyCall> call; > + > + call = GetCall(aCallIndex); Combine this with the declaration of call. @@ +482,2 @@ > > + modifiedCall = GetCallFromEverywhere(aCallIndex); And combine this with the declaration of modifiedCall. @@ +528,5 @@ > return NS_OK; > } > > + // Do nothing since we didn't know anything about it before now and it's > + // been ended already. s/been// ::: dom/telephony/Telephony.h @@ +151,5 @@ > + bool > + HasDialingCall(); > + > + bool > + IsActiveState(uint16_t aCallState); This should be static too.

Comment on attachment 820910 [details] [diff] [review] Part 4b#3: dom: multisim Review of attachment 820910 [details] [diff] [review]: ----------------------------------------------------------------- Something to think about: Will we allow the default SIM to change without rebooting the phone and/or restarting calls in progress? If so code that does not pass a service ID will be unable to find any calls if the default SIM changes while that code is running, right? ::: dom/telephony/Telephony.cpp @@ +177,5 @@ > > // static > +// TODO: Maybe find a better function name. > +uint32_t > +Telephony::GetServiceId(const Optional<uint32_t>& aServiceId) ProvidedOrDefaultServiceId, perhaps? Might be a little long. @@ +233,5 @@ > } > > already_AddRefed<TelephonyCall> > +Telephony::DialInternal(uint32_t aServiceId, const nsAString& aNumber, > + bool isEmergency, ErrorResult& aRv) While you're here can you change isEmergency to aIsEmergency? @@ +416,5 @@ > aRv.Throw(NS_ERROR_INVALID_ARG); > return; > } > > + aRv = mProvider->StartTone(GetServiceId(aServiceId), aDTMFChar); Please call GetServiceId once at the beginning of the function and store the result in a local variable. This isn't the only function this gets called multiple times in, so please fix the others too.

Thank you, Kyle. > @@ +187,5 @@ > > +{ > > + for (uint32_t i = 0; i < mCalls.Length(); i++) { > > + const nsRefPtr<TelephonyCall>& tempCall = mCalls[i]; > > + if (tempCall->IsOutgoing() && > > + tempCall->CallState() < nsITelephonyProvider::CALL_STATE_CONNECTED) { > > Do you really want to include CALL_STATE_UNKNOWN here? I know the existing > code does but that seems odd. if (call->IsOutgoing() && call->CallState() > nsITelephonyProvider::CALL_STATE_UNKNOWN && call->CallState() < nsITelephonyProvider::CALL_STATE_CONNECTED) { return true; } Exclude CALL_STATE_UNKNOWN in my final version. > @@ +240,5 @@ > > nsRefPtr<TelephonyCall> call = CreateNewDialingCall(aNumber); > > > > // Notify other telephony objects that we just dialed. > > + for (uint32_t i = 0; i < gTelephonyList->Length(); i++) { > > + Telephony*& telephony = gTelephonyList->ElementAt(i); > > Or gTelephonyList[i]. gTelephonyList is a pointer. I should use (*gTelephonyList)[i]. However I prefer the original form. gTelephonyList->Length() gTelephonyList->ElementAt(i) <= It looks better with above line.

Ah, yes, I missed that it was a pointer. I agree that x->ElementAt(i) is nicer than (*x)[i].

Comment on attachment 820917 [details] [diff] [review] Part 8#3: bluetooth Review of attachment 820917 [details] [diff] [review]: ----------------------------------------------------------------- Just in case we forget, can you help to add a comment like we did in bug 818353 and bug 926343? ( // TODO: Bug 921991 - B2G BT: support multiple sim cards ) Since a new parameter is added for several functions, maybe we can just leave a comment at the top of the implementation of TelephonyListener.

Also with some renaming: - outgoingCall => outCall - incomingCall => inCall

Comment on attachment 818863 [details] [diff] [review] (WIP) Sample dsds outgoing marionette test Move test case develop to Bug 931697.

Comment on attachment 823225 [details] [diff] [review] Part 4b#4: dom: multisim Hi Hsinyi, Kyle, The patch got r+ before but I add some changes to clear the TODO items based on a recent fixed bug. Therefore, I request the review again. Looks like the diff with previous version is broken after rebase. I simply highlight the major changes below 1. Add Telephony::GetNumServices(): to get the total number of service from preference and use it for valid test. 2. Use 'ProvidedOrDefaultServiceId' for function name and get the actual default id from provider. 3. Add UpdateServiceId(aServiceId) => For hsinyi: As our offline discussion, the function might be needed?

(In reply to Szu-Yu Chen [:aknow] from comment #101) > Comment on attachment 823225 [details] [diff] [review] > Part 4b#4: dom: multisim > > Hi Hsinyi, Kyle, > > The patch got r+ before but I add some changes to clear the TODO items based > on a recent fixed bug. Therefore, I request the review again. > > Looks like the diff with previous version is broken after rebase. I simply > highlight the major changes below > 1. Add Telephony::GetNumServices(): to get the total number of service from > preference and use it for valid test. > 2. Use 'ProvidedOrDefaultServiceId' for function name and get the actual > default id from provider. > 3. Add UpdateServiceId(aServiceId) > => For hsinyi: As our offline discussion, the function might be needed? Aknow, thanks for reminding... I almost forgot it :( After confirming with Anshul, modem doesn't support this. We shouldn't worry about UpdateServiceId(). Please remove it. Thank you thank you.

Comment on attachment 823155 [details] [diff] [review] Part 9#3: rewrite conference test case Conference test development is moved to Bug 932148.

Comment on attachment 823225 [details] [diff] [review] Part 4b#4: dom: multisim Review of attachment 823225 [details] [diff] [review]: ----------------------------------------------------------------- r=me if hsinyi is happy with this.

Comment on attachment 823225 [details] [diff] [review] Part 4b#4: dom: multisim Review of attachment 823225 [details] [diff] [review]: ----------------------------------------------------------------- Please address the comment below. Except that, the patch looks good to me. Thank you. ::: dom/telephony/Telephony.cpp @@ +525,5 @@ > // If the call state isn't incoming but we do have an outgoing call then > // we must be seeing a status update for our outgoing call. > if (outgoingCall && > aCallState != nsITelephonyProvider::CALL_STATE_INCOMING) { > + outgoingCall->UpdateServiceId(aServiceId); We don't need this according to the confirmation from vendor. Please remove it. Thank you. ::: dom/telephony/TelephonyCall.h @@ +147,5 @@ > > void > + UpdateServiceId(bool aServiceId) > + { > + mServiceId = aServiceId; We don't need this according to the confirmation from vendor. Please remove it. Thank you.

Comment on attachment 813050 [details] [diff] [review] Part 1: webidl: code reorder Review of attachment 813050 [details] [diff] [review]: ----------------------------------------------------------------- I don't want to be a stickler, but mozilla has a strict review policy. We'd have one more reviewer for the WebAPI change. Please don't forget that! Thank you.

I'm happy to accept r=hsinyi on the webidl changes here. They are trivial and don't need to be reviewed by a DOM peer.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #107) > I'm happy to accept r=hsinyi on the webidl changes here. They are trivial > and don't need to be reviewed by a DOM peer. That would be great. Thank you, Kyle :)

Comment on attachment 813050 [details] [diff] [review] Part 1: webidl: code reorder Hi Kyle, One more help! Would you help review the interface part? Thank you.

Comment on attachment 813050 [details] [diff] [review] Part 1: webidl: code reorder Cancel the review according to Comment 107

Part 4a before

Part 4b before

Part 5 before

Part 6 before

Part 7 before

Part 8 before

Because Sprint 3 was already passed, should we assign new target milestone? Thanks.

https://hg.mozilla.org/integration/b2g-inbound/rev/de45eb7a8d16 https://hg.mozilla.org/integration/b2g-inbound/rev/8957cc31cce2 https://hg.mozilla.org/integration/b2g-inbound/rev/90bdcd099320 https://hg.mozilla.org/integration/b2g-inbound/rev/43a047c081c8 https://hg.mozilla.org/integration/b2g-inbound/rev/17b3050df362 https://hg.mozilla.org/integration/b2g-inbound/rev/9f5e2e44a914 https://hg.mozilla.org/integration/b2g-inbound/rev/b6fc8734864f https://hg.mozilla.org/integration/b2g-inbound/rev/416665a419f4 https://hg.mozilla.org/integration/b2g-inbound/rev/a107de5616a0

Backed out for desktop build bustage. https://hg.mozilla.org/integration/b2g-inbound/rev/55ffdfba3d1b https://tbpl.mozilla.org/php/getParsedLog.php?id=29890035&tree=B2g-Inbound

I made a mistake in previous version of this patch. Now fixed.

Sorry for the backout. I should verify on other platforms. Now, everything looks good. https://tbpl.mozilla.org/?tree=Try&rev=a424b712de2c

https://hg.mozilla.org/integration/b2g-inbound/rev/aa20b996260e https://hg.mozilla.org/integration/b2g-inbound/rev/48ce546499b8 https://hg.mozilla.org/integration/b2g-inbound/rev/597b7d448f3c https://hg.mozilla.org/integration/b2g-inbound/rev/4b7ae734feb7 https://hg.mozilla.org/integration/b2g-inbound/rev/17aae84442a6 https://hg.mozilla.org/integration/b2g-inbound/rev/dbfdb9d00d69 https://hg.mozilla.org/integration/b2g-inbound/rev/f0df80325bd2 https://hg.mozilla.org/integration/b2g-inbound/rev/07e6249d001a https://hg.mozilla.org/integration/b2g-inbound/rev/fe41f6070323

https://hg.mozilla.org/mozilla-central/rev/aa20b996260e https://hg.mozilla.org/mozilla-central/rev/48ce546499b8 https://hg.mozilla.org/mozilla-central/rev/597b7d448f3c https://hg.mozilla.org/mozilla-central/rev/4b7ae734feb7 https://hg.mozilla.org/mozilla-central/rev/17aae84442a6 https://hg.mozilla.org/mozilla-central/rev/dbfdb9d00d69 https://hg.mozilla.org/mozilla-central/rev/f0df80325bd2 https://hg.mozilla.org/mozilla-central/rev/07e6249d001a https://hg.mozilla.org/mozilla-central/rev/fe41f6070323

\o/