Closed Bug 814625 Opened 7 years ago Closed 6 years ago

WebTelephony API: support multiple SIM cards

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: hsinyi, Assigned: aknow)

References

()

Details

(Keywords: dev-doc-needed, Whiteboard: [FT:RIL])

Attachments

(9 files, 57 obsolete files)

1.28 KB, patch
Details | Diff | Splinter Review
2.64 KB, patch
Details | Diff | Splinter Review
6.76 KB, patch
Details | Diff | Splinter Review
16.82 KB, patch
Details | Diff | Splinter Review
28.12 KB, patch
Details | Diff | Splinter Review
23.97 KB, patch
Details | Diff | Splinter Review
31.58 KB, patch
Details | Diff | Splinter Review
2.28 KB, patch
Details | Diff | Splinter Review
4.02 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → htsai
Depends on: 819234
Attached file Working branch (obsolete) —
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> Created attachment 691746 [details]
> Working branch

Working branch updated - more test cases are coming
Attached patch WIP: part1 - idl change (obsolete) — Splinter Review
added nsIDOMTelephonyManager
Attached patch WIP: part2 - DOMClassInfo (obsolete) — Splinter Review
Attached patch WIP: part3 - TelephonyManager (obsolete) — Splinter Review
Attached patch WIP: part5 - test cases (obsolete) — Splinter Review
Attached patch WIP: part1 - idl change (obsolete) — Splinter Review
Attachment #701719 - Attachment is obsolete: true
Attached patch WIP: part2 - DOMClassInfo (obsolete) — Splinter Review
Attachment #701720 - Attachment is obsolete: true
Attachment #701721 - Attachment is obsolete: true
Attachment #701722 - Attachment is obsolete: true
Attached patch WIP: part4 - ril impl (obsolete) — Splinter Review
Attached patch WIP: part5 - test cases (obsolete) — Splinter Review
Revised 3 basic tests to verify the design concept first. More modifications are coming.
Attachment #701723 - Attachment is obsolete: true
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 :)
Attachment #701725 - Flags: feedback?(mounir)
Attachment #701725 - Flags: feedback?(jonas)
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?
Attachment #701725 - Flags: feedback?(mounir)
(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-b2g: --- → leo?
Blocking-, no longer targeting Leo release. Product will retarget for future release.
blocking-b2g: leo? → -
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
Attached patch WIP: part1 - idl change (obsolete) — Splinter Review
added nsIRILTelephonyManagerCallback
Attachment #701725 - Attachment is obsolete: true
Attachment #701725 - Flags: feedback?(jonas)
Attached patch WIP: part2 - DOMClassInfo (obsolete) — Splinter Review
Attachment #701726 - Attachment is obsolete: true
implemented nsIRILTelephonyManagerCallback, instead of using nsIObserver
Attachment #701727 - Attachment is obsolete: true
Attached patch WIP: part4 - ril impl (obsolete) — Splinter Review
implemented TelephonyManagerCallback
Attachment #701728 - Attachment is obsolete: true
Attached patch WIP: part5 - test cases (obsolete) — Splinter Review
revised test cases: using |window.navigator.mozTelephonyManager.defaultPhone|
Attachment #701729 - Attachment is obsolete: true
Attached patch WIP: part4 - ril impl (obsolete) — Splinter Review
This is the right patch!
Attachment #715402 - Attachment is obsolete: true
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.
Attachment #715399 - Attachment is obsolete: true
Comment on attachment 715400 [details] [diff] [review]
WIP: part2 - DOMClassInfo

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

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

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

Our current design is as comment 29.
Attachment #715405 - Attachment is obsolete: true
Assignee: htsai → szchen
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Blocks: 918556
Blocks: 921393
No longer blocks: 921393
Blocks: 921394
Blocks: 921991
Attached patch Part 1: webidl: code reorder (obsolete) — Splinter Review
Attachment #813050 - Flags: review?(htsai)
Attachment #813051 - Flags: review?(htsai)
Attached patch Part 3: dom: code reorder (obsolete) — Splinter Review
Attachment #813052 - Flags: review?(htsai)
Attached patch (WIP) Part 4: dom (obsolete) — Splinter Review
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.
Attachment #813053 - Flags: review?(htsai)
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
Attachment #813056 - Flags: review?(htsai)
Attached patch Part 6: ipc (obsolete) — Splinter Review
Attachment #813057 - Flags: review?(htsai)
Attached patch Part 7: ril (obsolete) — Splinter Review
Attachment #813058 - Flags: review?(htsai)
Attached patch Part 8: bluetooth (obsolete) — Splinter Review
Attachment #813059 - Flags: review?(htsai)
Blocks: 921400
Attached patch Part 3#2: dom: code reorder (obsolete) — Splinter Review
Attachment #813052 - Attachment is obsolete: true
Attachment #813052 - Flags: review?(htsai)
Attachment #816419 - Flags: review?(htsai)
Attached patch (WIP) Part 4a: dom: refactor (obsolete) — Splinter Review
Attachment #813053 - Attachment is obsolete: true
Attachment #813053 - Flags: review?(htsai)
Attachment #816420 - Flags: review?(htsai)
Attached patch (WIP) Part 4b: dom: multisim (obsolete) — Splinter Review
Attachment #816421 - Flags: review?(htsai)
Attachment #816421 - Attachment description: Part 4b: dom: multisim → (WIP) Part 4b: dom: multisim
Depends on: 926302
Not dependent on bug 814584 anymore as TelephonyProvider is moved out of RILContentHelper.
No longer depends on: 814584
blocking-b2g: - → ---
Duplicate of this bug: 818351
Attachment #813056 - Attachment is obsolete: true
Attachment #813056 - Flags: review?(htsai)
Attachment #816509 - Flags: review?(htsai)
Attachment #816509 - Attachment is obsolete: true
Attachment #816509 - Flags: review?(htsai)
Attachment #816928 - Flags: review?(htsai)
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.
Attachment #813050 - Flags: review?(htsai) → 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.
Attachment #813051 - Flags: review?(htsai) → review+
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.
Attachment #816419 - Flags: review?(htsai) → feedback+
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.
Attachment #816420 - Flags: review?(htsai) → feedback-
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.
Attachment #816421 - Flags: review?(htsai)
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.
Attachment #816928 - Flags: review?(htsai)
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.
Attachment #813058 - Flags: review?(htsai)
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
Attachment #813057 - Flags: review?(htsai)
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.
Attachment #813059 - Flags: review?(htsai)
(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.
Attached patch Part 4a#2: dom: refactor (obsolete) — Splinter Review
Attachment #816420 - Attachment is obsolete: true
Attachment #817629 - Flags: feedback?(htsai)
Attached patch Part 4b#2: dom: multisim (obsolete) — Splinter Review
Attachment #816421 - Attachment is obsolete: true
Attachment #817630 - Flags: feedback?(htsai)
Attachment #816928 - Attachment is obsolete: true
Attachment #817632 - Flags: review?(htsai)
Attached patch Part 6#2: ipc (obsolete) — Splinter Review
Attachment #813057 - Attachment is obsolete: true
Attachment #817633 - Flags: review?(htsai)
Attached patch Part 7#2: ril (obsolete) — Splinter Review
Attachment #813058 - Attachment is obsolete: true
Attachment #817634 - Flags: review?(htsai)
Attached patch Part 8#2: bluetooth (obsolete) — Splinter Review
Attachment #813059 - Attachment is obsolete: true
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.
Blocks: 927764
blocking-b2g: --- → 1.3+
Attachment #817632 - Attachment is obsolete: true
Attachment #817632 - Flags: review?(htsai)
Attachment #818384 - Flags: review?(htsai)
Target Milestone: --- → 1.3 Sprint 3 - 10/25
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.
Attachment #818863 - Flags: feedback?(htsai)
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)
Attachment #817629 - Flags: feedback?(htsai) → feedback+
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
Attachment #817630 - Flags: feedback?(htsai) → feedback+
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
Attachment #818384 - Flags: review?(htsai)
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.
Attachment #817633 - Flags: review?(khuey)
Attachment #817633 - Flags: review?(htsai)
Attachment #817633 - Flags: feedback+
Comment on attachment 817634 [details] [diff] [review]
Part 7#2: ril

Review of attachment 817634 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thank you.
Attachment #817634 - Flags: review?(htsai) → review+
Attachment #691746 - Attachment is obsolete: true
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.
Attachment #818863 - Flags: feedback?(htsai)
(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
Attachment #817633 - Flags: review?(khuey) → review+
Whiteboard: [FT:RIL]
(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.
Attachment #818384 - Attachment is obsolete: true
Attachment #820903 - Flags: review?(htsai)
Attachment #820903 - Attachment is obsolete: true
Attachment #820903 - Flags: review?(htsai)
Attachment #820906 - Flags: review?(htsai)
Attached patch Part 4a#3: dom: refactor (obsolete) — Splinter Review
Attachment #817629 - Attachment is obsolete: true
Attachment #820908 - Flags: feedback+
Attached patch Part 4b#3: dom: multisim (obsolete) — Splinter Review
Attachment #817630 - Attachment is obsolete: true
Attachment #820910 - Flags: feedback+
Modify conference test based on Comment 74.
However, I rewrite the whole file. It contains huge difference...
Attachment #820912 - Flags: review?(htsai)
Attached patch Part 8#3: bluetooth (obsolete) — Splinter Review
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.
Attachment #817636 - Attachment is obsolete: true
Attachment #820917 - Flags: review?(gyeh)
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.
Attachment #816419 - Flags: review?(khuey)
Attachment #820908 - Flags: review?(khuey)
Attachment #820910 - Flags: review?(khuey)
Attachment #818863 - Attachment description: (Ref) Sample dsds outgoing marionette test → (WIP) Sample dsds outgoing marionette test
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.
Attachment #820906 - Flags: review?(htsai) → review+
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.
Attachment #820908 - Flags: review?(khuey) → review+
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.
Attachment #820910 - Flags: review?(khuey) → review+
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].
Attachment #820912 - Attachment is obsolete: true
Attachment #820912 - Flags: review?(htsai)
Attachment #822239 - Flags: review?(htsai)
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.
Attachment #820917 - Flags: review?(gyeh) → review+
Also with some renaming:
- outgoingCall => outCall
- incomingCall => inCall
Attachment #822239 - Attachment is obsolete: true
Attachment #822239 - Flags: review?(htsai)
Attachment #823155 - Flags: review?(htsai)
Blocks: 931697
Comment on attachment 818863 [details] [diff] [review]
(WIP) Sample dsds outgoing marionette test

Move test case develop to Bug 931697.
Attachment #818863 - Attachment is obsolete: true
Attached patch (x) wrong upload (obsolete) — Splinter Review
Attachment #820910 - Attachment is obsolete: true
Attachment #823224 - Attachment description: Part 4b#4: dom: multisim → (x) wrong upload
Attachment #823224 - Attachment is obsolete: true
Attached patch Part 4b#4: dom: multisim (obsolete) — Splinter Review
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?
Attachment #823225 - Flags: review?(khuey)
Attachment #823225 - Flags: feedback?(htsai)
(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.
Attachment #823155 - Attachment is obsolete: true
Attachment #823155 - Flags: review?(htsai)
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.
Attachment #823225 - Flags: review?(khuey) → review+
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.
Attachment #823225 - Flags: feedback?(htsai) → feedback+
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.
Attachment #813050 - Flags: review?(khuey)
Attachment #813051 - Flags: review?(khuey)
Comment on attachment 813050 [details] [diff] [review]
Part 1: webidl: code reorder

Cancel the review according to Comment 107
Attachment #813050 - Flags: review?(khuey)
Attachment #813051 - Flags: review?(khuey)
Attachment #813050 - Attachment is obsolete: true
Attachment #816419 - Attachment is obsolete: true
Part 4a before
Attachment #820908 - Attachment is obsolete: true
Part 4b before
Attachment #823225 - Attachment is obsolete: true
Part 5 before
Attachment #820906 - Attachment is obsolete: true
Part 6 before
Attachment #817633 - Attachment is obsolete: true
Part 7 before
Attachment #817634 - Attachment is obsolete: true
Part 8 before
Attachment #820917 - Attachment is obsolete: true
Keywords: checkin-needed
Because Sprint 3 was already passed, should we assign new target milestone? Thanks.
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Attachment #824460 - Attachment is obsolete: true
I made a mistake in previous version of this patch. Now fixed.
Attachment #824461 - Attachment is obsolete: true
Sorry for the backout. I should verify on other platforms.
Now, everything looks good.

https://tbpl.mozilla.org/?tree=Try&rev=a424b712de2c
Keywords: checkin-needed
\o/
You need to log in before you can comment on or make changes to this bug.