Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Description • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 2 • 12 years ago

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

Working branch updated - more test cases are coming

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 3 • 12 years ago

added nsIDOMTelephonyManager

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 12 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 13 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 14 • 12 years ago

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?

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 15 • 12 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 16 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 17 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 18 • 12 years ago

Working branch updated - existing test cases revision

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 19 • 12 years ago

added nsIRILTelephonyManagerCallback

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 21 • 12 years ago

implemented nsIRILTelephonyManagerCallback, instead of using nsIObserver

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 22 • 12 years ago

implemented TelephonyManagerCallback

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 23 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 24 • 12 years ago

This is the right patch!

	Mounir Lamouri (:mounir)
	Comment 25 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 26 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 12 years ago

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?

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 28 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 29 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 30 • 11 years ago

Comment on attachment 715399 [details] [diff] [review]
WIP: part1 - idl change

Our current design is as comment 29.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 31 • 11 years ago

Comment on attachment 715400 [details] [diff] [review]
WIP: part2 - DOMClassInfo

Our current design is as comment 29.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 32 • 11 years ago

Comment on attachment 715401 [details] [diff] [review]
WIP: part3 - DOM impl - WebTelephony

Our current design is as comment 29.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 33 • 11 years ago

Comment on attachment 715403 [details] [diff] [review]
WIP: part5 - test cases

Our current design is as comment 29.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 34 • 11 years ago

Comment on attachment 715405 [details] [diff] [review]
WIP: part4 - ril impl

Our current design is as comment 29.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 38 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 39 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 46 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 50 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 51 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 52 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 53 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 54 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 55 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 56 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 57 • 11 years ago

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?

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 58 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 59 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 60 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 61 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 62 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 63 • 11 years ago

(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'?

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 64 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 71 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 73 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 74 • 11 years ago

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)

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 75 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 76 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 77 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 78 • 11 years ago

Comment on attachment 817634 [details] [diff] [review]
Part 7#2: ril

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

Looks good. Thank you.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 79 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 80 • 11 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 81 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 82 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 87 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 88 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 89 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 90 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 91 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 92 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 93 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 94 • 11 years ago

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

	Gina Yeh [:gyeh] [:ginayeh]
	Comment 96 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 97 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 98 • 11 years ago

Comment on attachment 818863 [details] [diff] [review]
(WIP) Sample dsds outgoing marionette test

Move test case develop to Bug 931697.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 101 • 11 years ago

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?

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 102 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 103 • 11 years ago

Comment on attachment 823155 [details] [diff] [review]
Part 9#3: rewrite conference test case

Conference test development is moved to Bug 932148.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 104 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 105 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 106 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 107 • 11 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 108 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 109 • 11 years ago

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.

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 110 • 11 years ago

Comment on attachment 813050 [details] [diff] [review]
Part 1: webidl: code reorder

Cancel the review according to Comment 107

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 114 • 11 years ago

Part 4a before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 115 • 11 years ago

Part 4b before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 116 • 11 years ago

Part 5 before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 117 • 11 years ago

Part 6 before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 118 • 11 years ago

Part 7 before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 119 • 11 years ago

Part 8 before

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 120 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=40fd1605866f

	Kai-Chih Hu [:khu]
	Comment 121 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 122 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 123 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 128 • 11 years ago

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

	Szu-Yu Chen [:aknow] (inactive) Assignee
	Comment 133 • 11 years ago

Sorry for the backout. I should verify on other platforms.
Now, everything looks good.

https://tbpl.mozilla.org/?tree=Try&rev=a424b712de2c

	Ryan VanderMeulen [:RyanVM]
	Comment 134 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 135 • 11 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi] Reporter
	Comment 136 • 11 years ago

\o/