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

Conference Calling (CC) provides a subscriber with the ability to have a multi-
connection call, i.e., a simultaneous communication between three or more parties
(conferees). 

See 3GPP2 S.R0006-510-A

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #0)
> Conference Calling (CC) provides a subscriber with the ability to have a
> multi-
> connection call, i.e., a simultaneous communication between three or more
> parties
> (conferees). 
> 
> See 3GPP2 S.R0006-510-A

We can use RIL_REQUEST_CDMA_FLASH to join a conference call.

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

Per spec, to invoke a conference call on cdma networks, we shall use a specific command to setup a second call first. That is, we shall make telephony API support |conferenceGroup.add(DOMString number)| in addition to |conferenceGroup.add(nsIDOMTelephonyCall all)| on gsm.

Reference:
1) 3GPP2 S.R0006-510-A, http://www.3gpp2.org/Public_html/specs/S.R0006-510-A_v1.0_070624.pdf
2) CDMA AT command: http://www.sendsms.com.cn/download/CDMA_AT_178.pdf, Section 8.5

	Michael Schwartz [:m4]
	Comment 3 • 11 years ago

Do we need a separate bug for the Gaia component of the change?  The current code assumes that initiating a call will return a call object and that call state transitions will occur after the hold and dial - that will not happen for CDMA.

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

http://www.3gpp2.org/Public_html/specs/s.r0006-522-a_v1.0_070624.pdf Our partner's reference.

	Hiroyuki Ikezoe (:hiro)
	Comment 5 • 11 years ago

I guess this is the new one:
http://www.3gpp2.org/public_html/specs/s.r0125-522-0_v1.0_080409.pdf‎

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> I guess this is the new one:
> r0125-522-0_v1.0_080409.pdf">http://www.3gpp2.org/public_html/specs/s.r0125-
> 522-0_v1.0_080409.pdf‎

Thank you! I'll implement the feature based on this document. :)

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

Some notes for the behaviour:

1) A has a call with B
2) A dials out to C: no call state changes. no changes on +CLCC
3) C answers: no call state changes. no changes on +CLCC
4) A taps on "merge" button: still, no signal from network
5) If anyone of B or C leaves the conference, A has no idea about that. Very similar to CDMA-call-waiting situation.

I am still thinking about how to address this issue with the existing telephony conference API. The attempt is to have a unified API for both networks. However, due to the very difference, and considering the CDMA-call-waiting API design, it's very likely that we need to have a different API for that...

	Ken Chang[:kenkai]
	Comment 8 • 11 years ago

Blocking 1.4 JCDMA feature.

	Ken Chang[:kenkai]
	Comment 9 • 11 years ago

This is a 1.4+ bug.

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

Due to the very difference among CDMA and GSM conference call, we might need separate API for them. I'll try to elaborate my thoughts and make them clear as much as possible.

In CDMA 3-way calling, we need to dial out the second call before making a 3-way call. But like CDMA call waiting scenario, RIL doesn't get any notification from network once the second call is successfully made. To have a consistent behaviour on CDMA networks, I'd like to follow the current code -- gecko doesn't fake an additional call object for the 2nd call because it's impossible to correctly maintain the 2nd call object's life cycle. That said, we don't receive any notification if the 2nd call is released by the remote party. So, I'd propose following the design concept we have for CDMA call waiting, i.e. updating a Telephony Call object's secondNumber rather than creating another new call object if we are on CDMA networks. To achieve this idea, this bug should depend on bug 889737 because bug 889737 is going to make dial() return a Promise. 

And, one more thing to note: now telephonyCall.secondNumber can be assigned in two cases: one is cdma call waiting and one is cdma  3-way calling. But only when it's assigned in the latter case can two calls be merged. So a new attribute will be exposed to tell gaia if the call object is merge-able or not.

Below is the proposal.

============
API Proposal
============
interface TelephonyCallGroup : EventTarget {
  readonly attribute CallsList calls;

  [Throws]
  void add(); //NEW for CDMA

  [Throws]
  void add(TelephonyCall call);

  [Throws]
  void add(TelephonyCall call, TelephonyCall secondCall);

  [Throws]
  void remove(); //NEW for CDMA

  [Throws]
  void remove(TelephonyCall call);

  [Throws]
  void hold();

  [Throws]
  void resume();

  readonly attribute DOMString state;

  attribute EventHandler onstatechange;
  attribute EventHandler onconnected;
  attribute EventHandler onholding;
  attribute EventHandler onheld;
  attribute EventHandler onresuming;
  attribute EventHandler oncallschanged;
  attribute EventHandler onerror;
};


partial interface TelephonyCall : EventTarget {
  readonly attribute boolean isMergeable; // NEW for CDMA
};

=========================
API behaviour and use case
==========================
0) Have a connected call already. We now have a TelephonyCall object, naming telCall.
1) Dial a second call by |var promise = mozTelephony.dial(secondNumber)|  <== this step is the same as what we need for gsm
2) Update |telCall.secondNumber| and |telCall.isMergeable| on Telephony DOM. Then |promise.resolve(telCall)|. mozTelephony.oncallschanged event will be fired as what we do for CDMA call waiting.
3) To merge them, gaia calls |mozTelephony.telephonyCallGroup.add()|. telCall will be moved from mozTelephony.calls to mozTelephony.telephonyCallGroup.calls. oncallschanged event will be dispatched to both mozTelephony and mozTelephony.telephonyCallGroup.
4) To release the second call, gaia calls |mozTelephony.telephonyCallGroup.remove()|, and telCall.secondNumber will be removed. oncallschanged event will be dispatched to mozTelephony.telephonyCallGroup. Note that on cdma, it's only possible to remove the last/second call. And the last call will be released.


Comments and feedbacks are welcome! Thank you :)

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

Hi Gabriele,

As you know the cdma call waiting behaviour quite well and as cdma 3-way calling shares the same behaviour, may I ask for your feedback on my proposal comment 10? Thank you. :)

	Gabriele Svelto [:gsvelto]
	Comment 13 • 11 years ago

So if I read this correctly three-way calling works exactly like call-waiting except that it's the first user calling the third one instead of the third one coming in while the first is busy. The API you propose looks sound from a functional perspective but I think that a casual user would find quite confusing the add() and remove() methods that take no parameter.

Similarly I'm OK with having an extra field to indicate that a CDMA call can be merged but just calling it |isMergeable| seems too generic; as somebody who wouldn't know the API well I would assume that all calls that can be add()ed to a TelephonyGroup would have that field, not just the CDMA calls.

One alternative I can think of that would make the scope clearer would be to have an object for the second call holding the number and mergeable flag, e.g.:

interface CDMASecondaryCall : EventTarget {
  readonly attribute DOMString number;
  readonly attribute boolean isMergeable;
};

partial interface TelephonyCall : EventTarget {
  readonly attribute CDMASecondaryCall? secondCall;
};

Then one would have:


partial interface TelephonyCallGroup : EventTarget {

  [Throws]
  void add(CDMASecondaryCall call);

  [Throws]
  void remove(CDMASecondaryCall call);

};

What do you think?

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

Thanks for the feedback, Gabriele.

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> So if I read this correctly three-way calling works exactly like
> call-waiting except that it's the first user calling the third one instead
> of the third one coming in while the first is busy. The API you propose
> looks sound from a functional perspective but I think that a casual user
> would find quite confusing the add() and remove() methods that take no
> parameter.
> 
> Similarly I'm OK with having an extra field to indicate that a CDMA call can
> be merged but just calling it |isMergeable| seems too generic; as somebody
> who wouldn't know the API well I would assume that all calls that can be
> add()ed to a TelephonyGroup would have that field, not just the CDMA calls.
> 
> One alternative I can think of that would make the scope clearer would be to
> have an object for the second call holding the number and mergeable flag,
> e.g.:
> 
> interface CDMASecondaryCall : EventTarget {
>   readonly attribute DOMString number;
>   readonly attribute boolean isMergeable;
> };
> 

This looks really good. I like this.

> partial interface TelephonyCall : EventTarget {
>   readonly attribute CDMASecondaryCall? secondCall;
> };
> 

This, too.

> Then one would have:
> 
> 
> partial interface TelephonyCallGroup : EventTarget {
> 
>   [Throws]
>   void add(CDMASecondaryCall call);
> 
>   [Throws]
>   void remove(CDMASecondaryCall call);
> 
> };
> 

However for the callgroup Api, after my second thought, I think we could take advantage of what we have now. I.e.
add(TelephonyCall) and remove(TelephonyCall)

Let me explain myself.

Since TelephonyCall is the one maintained in the telephony.calls, it makes sense and looks intuitive to add it into a conference. And the implementation checks if it's the right condition to add the object.

The condition would be:
a) there already are calls in conference. This is what we have for gsm now.
b) there are no calls in conference yet. But if we detect it's a cdma call with a second call, we know it is addable.

Regarding remove, similar. As TelephonyCall is the object kept in calls list, it seems better to take it as an argument. Implementation would be: remove the TelephonyCall from conference, and if there's CdmaSecondCall, the second call would be released.

I think this design abstracts difference between gsm and cdma, and hopefully makes the api much clearer.

What do you think? Thanks!
> What do you think?

	Gabriele Svelto [:gsvelto]
	Comment 15 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #14)
> However for the callgroup Api, after my second thought, I think we could
> take advantage of what we have now. I.e.
> add(TelephonyCall) and remove(TelephonyCall)

Excellent idea!

> I think this design abstracts difference between gsm and cdma, and hopefully
> makes the api much clearer.
> 
> What do you think? Thanks!

+1! Let's go for this.

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

Please see comment 14 for details.

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

Comment on attachment 8373114 [details] [diff] [review]
WIP - part1 - webidl

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

Basically this patch introduces a new attribute 'telephonyCall.secondCall' which ideally will replace 'telephony.secondNumber' in the end because .secondCall provides a structural way to tell the difference between a cdma waiting call and a cdma 2nd-outgoing call. 'telephony.secondNumber' was created for cdma-waiting calls only. Thanks Gabriele for the idea!

Besides, we don't really touch TelephonyCallGroup API though the implementation needs to care about gsm and cdma differences. The API behaviour is typically the same on both networks.

Please see comment 14 for details.

Hi Gene,

May I have your feedback on this? Thanks!

	Michael Schwartz [:m4]
	Comment 18 • 11 years ago

Comment on attachment 8373114 [details] [diff] [review]
WIP - part1 - webidl

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

::: dom/webidl/TelephonyCall.webidl
@@ +17,5 @@
>    readonly attribute DOMString? secondNumber;
>  
> +  // In CDMA networks, the 2nd call shares the connection with the 1st call.
> +  // We need an additional attribute for the 2nd call.
> +  readonly attribute TelephonyCdmaSecondaryCall? secondCall;

So initiate an MO call which gets immediately connected without us knowing (as there is no signaling for that state transition).  We then get a waiting call which overwrites this?

::: dom/webidl/TelephonyCdmaSecondaryCall.webidl
@@ +9,5 @@
> +  readonly attribute DOMString number;
> +
> +  // Flag indicating whether the second call can be merged into a 3-way
> +  // (conference) call.
> +  readonly attribute boolean isMergeable;

What is this dependent on?

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

(In reply to Michael Schwartz [:m4] from comment #18)
> Comment on attachment 8373114 [details] [diff] [review]
> WIP - part1 - webidl
> 
> Review of attachment 8373114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/TelephonyCall.webidl
> @@ +17,5 @@
> >    readonly attribute DOMString? secondNumber;
> >  
> > +  // In CDMA networks, the 2nd call shares the connection with the 1st call.
> > +  // We need an additional attribute for the 2nd call.
> > +  readonly attribute TelephonyCdmaSecondaryCall? secondCall;
> 
> So initiate an MO call which gets immediately connected without us knowing
> (as there is no signaling for that state transition).  We then get a waiting
> call which overwrites this?
> 

Once the command of making out the 2nd call succeeds, we will create TelephonyCdmaSecondaryCall(). 

> ::: dom/webidl/TelephonyCdmaSecondaryCall.webidl
> @@ +9,5 @@
> > +  readonly attribute DOMString number;
> > +
> > +  // Flag indicating whether the second call can be merged into a 3-way
> > +  // (conference) call.
> > +  readonly attribute boolean isMergeable;
> 
> What is this dependent on?

If this second call is a "waiting" call rather than a second call we are dialing out, than it's not mergeable.

	Gene Lian (I already quit Mozilla)
	Comment 20 • 11 years ago

Will catch up this review today.

	Michael Schwartz [:m4]
	Comment 21 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #19)

Side question, does the review tool not thread your comments inline with my own?  It looks like it could do that but when I use the review link it only shows my comments.  It would make it easier to follow the conversation.  Anyway...

> Once the command of making out the 2nd call succeeds, we will create TelephonyCdmaSecondaryCall().

I'm sorry but where is that?  You're talking about populating TelephonyCall::secondCall, right?  Seems like we're using secondCall for both incoming and waiting so what happens if both are (apparently but not in reality) happening at the same time?  Sorry if I'm missing something.

> If this second call is a "waiting" call rather than a second call we are dialing out, than it's not mergeable.

Perhaps we shouldn't be using secondCall for both situations.  Perhaps we use a waitingCall and outgoingCall field?  It's more explicit and would solve the issue I'm seeing (perhaps wrongly) above.

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

(In reply to Michael Schwartz [:m4] from comment #21)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #19)
> 
> Side question, does the review tool not thread your comments inline with my
> own? 

Sadly, no...

> It looks like it could do that but when I use the review link it only
> shows my comments.  It would make it easier to follow the conversation. 
> Anyway...
> 
> > Once the command of making out the 2nd call succeeds, we will create TelephonyCdmaSecondaryCall().
> 
> I'm sorry but where is that?  You're talking about populating
> TelephonyCall::secondCall, right?  Seems like we're using secondCall for
> both incoming and waiting so what happens if both are (apparently but not in
> reality) happening at the same time?  

It's impossible according to the spec, isn't it? Please correct me if I am wrong.

> Sorry if I'm missing something.
> 
> > If this second call is a "waiting" call rather than a second call we are dialing out, than it's not mergeable.
> 
> Perhaps we shouldn't be using secondCall for both situations.  Perhaps we
> use a waitingCall and outgoingCall field?  It's more explicit and would
> solve the issue I'm seeing (perhaps wrongly) above.

	Michael Schwartz [:m4]
	Comment 23 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #22)

> It's impossible according to the spec, isn't it? Please correct me if I am wrong.

That's why I said "apparently but not in reality".  When we initiate a dial request and it succeeds, we get no further signaling ie. when the call disconnects, we don't know that it has disconnected and assume it's still going on.  Then a call waiting comes in.

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

(In reply to Michael Schwartz [:m4] from comment #23)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #22)
> 
> > It's impossible according to the spec, isn't it? Please correct me if I am wrong.
> 
> That's why I said "apparently but not in reality".  When we initiate a dial
> request and it succeeds, we get no further signaling ie. when the call
> disconnects, we don't know that it has disconnected and assume it's still
> going on.  Then a call waiting comes in.

If that's the case, the second call will be replaced.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 25 • 11 years ago

remove 1.4+ flag for it's a feature. We can use meta bug "b2g-LTE-1.4" to track.

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

Comment on attachment 8373114 [details] [diff] [review]
WIP - part1 - webidl

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

We got new ideas after discussing this with the team. I will provide another WIP.

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

After a long long discussion and more investigation, I realized no matter how the API looks like, even with the v1 proposal, we cannot avoid the fact that we need to maintain (fake) call objects and (fake) state transition on gecko. Given the sad fact, I think it would be better to keep a unified API between gsm and cdma.

So the usage will be the same:
1) dialer calls telephony.dial(2ndNumber) to get the 2nd call
2) dialer calls telephoy.telephonyCallGroup.add(1stCall, 2ndCall) to create a 3-way (conference call)

However to meet different UX on gsm or cdma networks, dialer needs to know if a telephonyCall is a gsm or cdma call. Due to that reason, I created a 'TelephonyCdmaCall' which inherits 'TelephonyCall.' I like this more than exposing an additional 'networkType' because it's more structural and extensible.

One more thing to mention, on cdma networks, in some situation, a call can't be added to a conference; in others, a call can't be put on hold. So two new attributes 'mergeable' and 'switchable' are introduced.

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #27)
> Created attachment 8377060 [details] [diff] [review]
> WIP - part 1 - webidl (v2)
> 
> After a long long discussion and more investigation, I realized no matter
> how the API looks like, even with the v1 proposal, we cannot avoid the fact
> that we need to maintain (fake) call objects and (fake) state transition on
> gecko. Given the sad fact, I think it would be better to keep a unified API
> between gsm and cdma.
> 
> So the usage will be the same:
> 1) dialer calls telephony.dial(2ndNumber) to get the 2nd call
> 2) dialer calls telephoy.telephonyCallGroup.add(1stCall, 2ndCall) to create
> a 3-way (conference call)
> 
> However to meet different UX on gsm or cdma networks, dialer needs to know
> if a telephonyCall is a gsm or cdma call. Due to that reason, I created a
> 'TelephonyCdmaCall' which inherits 'TelephonyCall.' I like this more than
> exposing an additional 'networkType' because it's more structural and
> extensible.
> 

Since we are going to provide a unified API for cdma and gsm, the API behaviour and usage would be the same on both cdma and gsm networks. I am wondering if it's necessary or really helpful to distinguish a CDMA call object or not. Yes, we still need to meet different UX specs for gsm or cdma networks. Dialer would need to implement different UX specs for each network. But isn't network information from mobileconnection API is helpful or enough for the case?

Etienne and Gabriele, may I have your opinions from gaia perspective? Thank you.

> One more thing to mention, on cdma networks, in some situation, a call can't
> be added to a conference; in others, a call can't be put on hold. So two new
> attributes 'mergeable' and 'switchable' are introduced.

	Etienne Segonzac (:etienne)
	Comment 29 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #28)
> Since we are going to provide a unified API for cdma and gsm, the API
> behaviour and usage would be the same on both cdma and gsm networks. I am
> wondering if it's necessary or really helpful to distinguish a CDMA call
> object or not. Yes, we still need to meet different UX specs for gsm or cdma
> networks. Dialer would need to implement different UX specs for each
> network. But isn't network information from mobileconnection API is helpful
> or enough for the case?
> 
> Etienne and Gabriele, may I have your opinions from gaia perspective? Thank
> you.
> 

Well it depends on how good gecko is at faking the call objects and state transitions :)

If gaia is "protected" from the fact that on a CDMA network we don't know if any action succeeds because we get all the state transitions anyway, the UX probably won't differ much (which is good!). And we will probably be able to implement the small variations just by checking mozMobileConnection.

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

(In reply to Etienne Segonzac (:etienne) from comment #29)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #28)
> > Since we are going to provide a unified API for cdma and gsm, the API
> > behaviour and usage would be the same on both cdma and gsm networks. I am
> > wondering if it's necessary or really helpful to distinguish a CDMA call
> > object or not. Yes, we still need to meet different UX specs for gsm or cdma
> > networks. Dialer would need to implement different UX specs for each
> > network. But isn't network information from mobileconnection API is helpful
> > or enough for the case?
> > 
> > Etienne and Gabriele, may I have your opinions from gaia perspective? Thank
> > you.
> > 
> 

Thanks for the feedback, Etienne!

> Well it depends on how good gecko is at faking the call objects and state
> transitions :)
> 
> If gaia is "protected" from the fact that on a CDMA network we don't know if
> any action succeeds because we get all the state transitions anyway, the UX
> probably won't differ much (which is good!). 

This is my ambition. ;) 

Even cdma network doesn't send us notification of a certain action, gecko will still try it's best to deliver sensible and plausible state transition to gaia so that gaia could proceed to the next action depending on the state. 

> And we will probably be able to implement the small variations just by checking mozMobileConnection.

I have a WIP ongoing. And with the patch applied, the conference screen displays as expected even dialer does almost no modification. Actually, adding only one mozMobileConnection check. It looks TelephonyCdmaCall.webidl isn't necessary or not that helpful at the moment.

So, considering the schedule pressure and the fact that introducing TelephonyCdmaCall.webidl requires huge refactor of Telephony DOM code and other webidl changes, I am going to let this bug focus on telephony backend support without TelephonyCdmaCall.webidl. I'll be happy to file a followup bug for that webidl if we are convinced by the need. :)

	Gabriele Svelto [:gsvelto]
	Comment 31 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #27)
> After a long long discussion and more investigation, I realized no matter
> how the API looks like, even with the v1 proposal, we cannot avoid the fact
> that we need to maintain (fake) call objects and (fake) state transition on
> gecko. Given the sad fact, I think it would be better to keep a unified API
> between gsm and cdma.

Could you provide some additional details on why you came to this conclusion? When we looked into faking call objects for CDMA operation we came to the conclusion that any possible implementation would be flaky one way or another. Considering that 3-way calling is not really much different than call-waiting I don't see why we need such a big and potentially risky change. What was wrong with the proposal in attachment 8373114 [details] [diff] [review]?

Also keep in mind that we need to implement this functionality in the dialer too and until we do we won't really be able to test the faked calls properly which adds to the risk.

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

(In reply to Gabriele Svelto [:gsvelto] from comment #31)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #27)
> > After a long long discussion and more investigation, I realized no matter
> > how the API looks like, even with the v1 proposal, we cannot avoid the fact
> > that we need to maintain (fake) call objects and (fake) state transition on
> > gecko. Given the sad fact, I think it would be better to keep a unified API
> > between gsm and cdma.
> 
> Could you provide some additional details on why you came to this
> conclusion? When we looked into faking call objects for CDMA operation we
> came to the conclusion that any possible implementation would be flaky one
> way or another. Considering that 3-way calling is not really much different
> than call-waiting I don't see why we need such a big and potentially risky
> change. What was wrong with the proposal in attachment 8373114 [details] [diff] [review]
> [diff] [review]?

Attachment 8373114 [details] [diff] has nothing wrong. But in that proposal, we still need to maintain the state transition for the secondaryCall object and the 1st telephonyCall object on gecko by ourselves to make the API work. For example, after calling telephonyCallGroup.add(), on gecko, we need to manually, according to the request result, change the first telephonyCall object state and move it into conferenceGroup. After calling telephonyCallGroup.remove(), we also need to manually remove .secondaryCall and move the 1st telephonyCall object state out of the conferenceGroup.

That is why I said "we cannot avoid the fact that we need to maintain (fake) call objects and (fake) state transition on gecko."

> 
> Also keep in mind that we need to implement this functionality in the dialer
> too and until we do we won't really be able to test the faked calls properly
> which adds to the risk.

Due to network and partner requirements, the UX flow is somehow different. For example, on cdma, we don't show the participants. On cdma, we don't allow user to remove a participant. So, dialer would need to display another screen. Except that, I just added one more check in telephony_helper.js then everything works. I could test the whole 3-way scenario, such as adding a second call, merging calls, etc, on a real cdma network with the existing dialer app. However, the concern always remains, not just for this issue. I mean, without gaia implementation we are unable to test a whole new feature thoroughly. But I (gecko developers) could and should test the gecko part with test scripts or some ugly hacked gaia code. I would work with QA to see how we verify this work even without gaia part being ready.

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

Add TelephonyCall::Switchable() and ::Mergeable()

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

Maintain call state on TelephonyProvider. 

Details:
1) maintain _currentCalls for each client on TelephonyProvider, so Provider does't need to send 'enumerateCalls' to ril_worker once Provider receives the request from Telephony DOM.
2) create additional call object for CDMA scenario. Calls are updated according to the request type and request result.

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

internal API: add isSwitchable and isMergeable

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

Telephony DOM and IPC changes

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

BT changes due to internal API change

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

Gaia test patch

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

The above patches are all based on Bug 969218 and attachment 8373839 [details] [diff] [review] [diff] [review].

	Gabriele Svelto [:gsvelto]
	Comment 40 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #32)
> Attachment 8373114 [details] [diff] has nothing wrong. But in that proposal,
> we still need to maintain the state transition for the secondaryCall object
> and the 1st telephonyCall object on gecko by ourselves to make the API work.
> For example, after calling telephonyCallGroup.add(), on gecko, we need to
> manually, according to the request result, change the first telephonyCall
> object state and move it into conferenceGroup.

You mean after calling add(CDMASecondaryCall call) we should move the first call into the conference group implicitly? Then what about having a void add(TelephonyCall call, CDMASecondaryCall secondCall) for CDMA instead of the one-parameter version.

Also I'm not sure how the dialing part is going to work. When we make the second call in your new proposal do we get a new fake call object for the second connected call? If that's the case then that's precisely the part that I find scary since IIRC we don't really know if the call is connected or not. Plus if we get a fake call object a user might be tempted to treat it as a separate call but we know that's not the case and calling other methods on one or the other would actually yield surprising results (it's the same call after all).

> After calling
> telephonyCallGroup.remove(), we also need to manually remove .secondaryCall
> and move the 1st telephonyCall object state out of the conferenceGroup.

Same as above, what if we'd had a remove(TelephonyCall call, CDMASecondaryCall secondCall) call to explicitly remove both or just imply that if you call remove(TelephonyCall call) on a call that has a secondary call object then both are removed (I'd find this more natural as on the underlying network they're actually just one call).

My point is, we know that in CDMA 3-way mode we still have only one call on the network, just like with call waiting, so why don't we strive to make that explicit in the API? I'm really wary of faking transactions and concepts that are not really there...

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

(In reply to Gabriele Svelto [:gsvelto] from comment #40)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #32)
> > Attachment 8373114 [details] [diff] has nothing wrong. But in that proposal,
> > we still need to maintain the state transition for the secondaryCall object
> > and the 1st telephonyCall object on gecko by ourselves to make the API work.
> > For example, after calling telephonyCallGroup.add(), on gecko, we need to
> > manually, according to the request result, change the first telephonyCall
> > object state and move it into conferenceGroup.
> 
> You mean after calling add(CDMASecondaryCall call) we should move the first
> call into the conference group implicitly? Then what about having a void
> add(TelephonyCall call, CDMASecondaryCall secondCall) for CDMA instead of
> the one-parameter version.
> 
> Also I'm not sure how the dialing part is going to work. When we make the
> second call in your new proposal do we get a new fake call object for the
> second connected call? If that's the case then that's precisely the part
> that I find scary since IIRC we don't really know if the call is connected
> or not. 

Hey Gabriele, really appreciate your comments. Thank you :) And let me explain myself more.

The logic for creating either a separate TelephonyCall or a CdmaSecondaryCall should be the same.
We send a request to modem/network saying we are going to add one more call. Once we get the success response from modem, then we create a object for the 2nd call. You are right. We don't explicitly have a signal from network saying that "hey, here comes a second call." We rely on modem's response. IMO no matter we provide one more TelephonyCall or a CdmaSecondaryCall, we are telling the API user we have two calls now. So I don't really get the point why it looks okay to provide CdmaSecondaryCall but not okay to provide another TelephonyCall.

> Plus if we get a fake call object a user might be tempted to treat
> it as a separate call but we know that's not the case and calling other
> methods on one or the other would actually yield surprising results (it's
> the same call after all).
> 

I was almost overwhelmed by this argument. However, when I reviewed the whole telephony network nature again, I realized that in the telephony field, many operations on one call trigger changes on the other, even on gsm networks where we do see distinct calls from the network. For example, we resume a held call leads to putting another call on hold. They are interactive. And we have no way to avoid that. 

Even on cdma, it's tricky that we have only one connection with network; however the network still holds two connections with the two remote parties. That means, even we think we do a certain operation on the only one connection we hold b/w network, the truth is both remote parties are handled separately. Considering that nature I feel like what we should do instead is make sure if a call is affected by remote party or by another call, the changes can be reflected correctly.

> > After calling
> > telephonyCallGroup.remove(), we also need to manually remove .secondaryCall
> > and move the 1st telephonyCall object state out of the conferenceGroup.
> 
> Same as above, what if we'd had a remove(TelephonyCall call,
> CDMASecondaryCall secondCall) call to explicitly remove both or just imply
> that if you call remove(TelephonyCall call) on a call that has a secondary
> call object then both are removed (I'd find this more natural as on the
> underlying network they're actually just one call).
> 
> My point is, we know that in CDMA 3-way mode we still have only one call on
> the network, just like with call waiting, so why don't we strive to make
> that explicit in the API? I'm really wary of faking transactions and
> concepts that are not really there...

Same as above, it's tricky that we have only one connection with network; however the network still holds two connections with the two remote parties. I have been thinking about your another proposal all day today -- introduce new call states for cdma, such as holding2way state, 3way state, etc. And I re-studied the spec word by word again. The spec indicates the condition for every state on the connection very clearly. According to the spec every state is composed of individual state of two remote parties. Taking holding2way state as an example, this state is "1st call is on hold and the other is alerting/active." I can't stop thinking about isn't holding2way state implying the same thing as "one on hold and one call active." Exposing holding2way state seems provide as much and as true information as having two call objects.

I understand and agree your concern on faking transactions but I don't really get why exposing one more TelephonyCall and exposing a CdmaSecondaryCall leads to differences on worries. For me, a CdmaSecondaryCall is also a one we abstracted. No matter what kind of abstract we take we must be very careful of maintenance in both cases.

Despite I think it's nice to have a unified API on both networks, I am not a follower of the faith that we "should" have a unified one in any circumstance, especially when we see a better proposal. However the concern I have is I don't see how having CdmaSecondaryCall could release doubts or concerns better. That's why I think it's nice to keep a unified API for this case.

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

Add TelephonyCall.Switchable() and .Mergeable()

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

Maintain call state on TelephonyProvider. 

Details:
1) maintain _currentCalls for each client on TelephonyProvider, so Provider does't need to send 'enumerateCalls' to ril_worker once Provider receives the request from Telephony DOM.
2) create additional call object for CDMA scenario. Calls are updated according to the request type and request result

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

Maintain call state on TelephonyProvider. 

Details:
1) maintain _currentCalls for each client on TelephonyProvider, so Provider does't need to send 'enumerateCalls' to ril_worker once Provider receives the request from Telephony DOM.
2) create additional call object for CDMA scenario. Calls are updated according to the request type and request result

Note: this patch doesn't touch cdma call waiting flow which should be another issue.

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

Comment on attachment 8380316 [details] [diff] [review]
0001-3way-part1-webidl.patch - v2

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

Some calls are switchable or mergeable and some aren't. Exposing these attributes so that API users can show corresponding icons or screens easily.

	Vicamo Yang [:vicamo][:vyang]
	Comment 47 • 11 years ago

Comment on attachment 8380318 [details] [diff] [review]
0004-3way-part4-DOM-IPC.patch - v2

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

::: dom/telephony/TelephonyCall.cpp
@@ +259,5 @@
>    }
>  
> +  if (!mSwitchable) {
> +    NS_WARNING("Hold a non-switchable call ignored!");
> +    return;

Drop a failed request silently without any notification to Gaia isn't really a good idea.  Per offline discuss, please help file a follow-up to correct all the calls in TelephonyCall and TelephonyProvider.

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #47)
> Comment on attachment 8380318 [details] [diff] [review]
> 0004-3way-part4-DOM-IPC.patch - v2
> 
> Review of attachment 8380318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/TelephonyCall.cpp
> @@ +259,5 @@
> >    }
> >  
> > +  if (!mSwitchable) {
> > +    NS_WARNING("Hold a non-switchable call ignored!");
> > +    return;
> 
> Drop a failed request silently without any notification to Gaia isn't really
> a good idea.  Per offline discuss, please help file a follow-up to correct
> all the calls in TelephonyCall and TelephonyProvider.

No problem. Bug 975949 filed!

	Gene Lian (I already quit Mozilla)
	Comment 49 • 11 years ago

Comment on attachment 8380316 [details] [diff] [review]
0001-3way-part1-webidl.patch - v2

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

Looks nice! r=gene

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

Comment on attachment 8380451 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v3

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

::: dom/system/gonk/ril_worker.js
@@ -1461,4 @@
>    },
>  
>    dialEmergencyNumber: function(options, onerror) {
> -    options.request = RILQUIRKS_REQUEST_USE_DIAL_EMERGENCY_CALL ?

Sorry I shouldn't move it to the end. I'll fix it in 2b.patch. And squish 2b with part2 in the next version.

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

This is to fix a bug in 0002-3way-part2b-ril-impl.patch - v3.
I guess 0002-3way-part2b-ril-impl.patch (v3) is under review so I provide a patch 2b.

This one (part 2b) and part2 will be squished into only one in the next version.

	Vicamo Yang [:vicamo][:vyang]
	Comment 52 • 11 years ago

Comment on attachment 8379624 [details] [diff] [review]
0003-3way-part3-internal-api.patch - v1

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

nsIGonkTelephonyProvider::notifyCallStateChanged is changed in attachment 8380451 [details] [diff] [review] but isn't updated here.

	Vicamo Yang [:vicamo][:vyang]
	Comment 53 • 11 years ago

Comment on attachment 8380451 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v3

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

::: dom/system/gonk/ril_worker.js
@@ +1627,5 @@
>  
>    holdCall: function(options) {
>      let call = this.currentCalls[options.callIndex];
> +    if (!call) {
> +      return;

TelephonyProvider::holdCall is expecting a callback invoked.  We can't just return nothing here.

@@ +1635,5 @@
> +    if (this._isCdma) {
> +      options.featureStr = "";
> +      this.sendCdmaFlashCommand(options);
> +    } else if (call.state == CALL_STATE_ACTIVE) {
> +      Buf.simpleRequest(REQUEST_SWITCH_HOLDING_AND_ACTIVE);

TelephonyProvider::holdCall is expecting a callback invoked, but REQUEST_SWITCH_HOLDING_AND_ACTIVE handler never does.  This results in a orphaned callback kept in WorkerMessenger in RadioInterfaceLayer.js.  So applies to dial(no handler for REQUEST_DIAL_EMERGENCY_CALL), resumeCall, conferenceCall(no callback on success), separateCall(no callback on success). Besides, we should skip callback installing in RadioInterface::sendWorkerMessage when callback is omitted.

::: dom/telephony/gonk/TelephonyProvider.js
@@ +32,5 @@
>  
>  const CALL_WAKELOCK_TIMEOUT = 5000;
>  
> +// Index of a call which isn't held in RIL but only in TelephoyProvider.
> +const FAKE_CALL_INDEX = 9;

This should be 2.  Rename to something like 'CDMA_SECOND_CALL_INDEX' would be even better.

@@ +447,5 @@
> +      }
> +
> +      if (indexes.length == 1) {
> +        // RIL doesn't hold the 2nd call. We create one by ourselves.
> +        let call = {

nit: please use |childCall| to give a clear hint that it's the second call in CDMA.

@@ +467,5 @@
> +
> +        call.state = RIL.CALL_STATE_ACTIVE;
> +        this.notifyCallStateChanged(aClientId, call);
> +
> +        call = this._currentCalls[aClientId][call.parentId];

nit: parentCall

@@ +491,3 @@
>        } else {
>          aTelephonyCallback.notifyDialError(response.errorMsg);
>        }

I think our bail-out early rule is still valid.  Please handle error conditions first, then early returns, and process normal cases.

@@ +495,5 @@
>      }).bind(this));
>    },
>  
>    hangUp: function(aClientId, aCallIndex) {
> +    if (aCallIndex == FAKE_CALL_INDEX) {

FAKE_CALL_INDEX was originally assigned with some large value that can't be mixed up with those available in GSM systems.  With that value defined to a smaller 2, this equality may be also true in other cases.  Check validity of |call.parentId| instead here and in all following similar lines.

@@ +502,5 @@
> +      let parentId = this._currentCalls[aClientId][aCallIndex].parentId;
> +      if (parentId) {
> +        this.hangUp(aClientId, parentId);
> +      }
> +      return;

nit: TODO no hangUp when parentId is not set.

@@ +528,5 @@
>    holdCall: function(aClientId, aCallIndex) {
> +    let call = this._currentCalls[aClientId][aCallIndex];
> +    if (!call || !call.isSwitchable) {
> +      return;
> +    }

nit: TODO here.

@@ +554,5 @@
> +    },(function(response) {
> +        if (response.success && response.isCdma) {
> +          onCdmaHoldCallSuccess.call(this);
> +        }
> +        return false;

nit: always bail-out early

@@ +562,5 @@
>    resumeCall: function(aClientId, aCallIndex) {
> +    let call = this._currentCalls[aClientId][aCallIndex];
> +    if (!call || !call.isSwitchable) {
> +      return;
> +    }

nit: TODO here.

@@ +587,5 @@
> +    },(function(response) {
> +      if (response.success && response.isCdma) {
> +        onCdmaResumeCallSuccess.call(this);
> +      }
> +      return false;

nit: always bail-out early

@@ +594,5 @@
>  
>    conferenceCall: function(aClientId) {
> +    let indexes = Object.keys(this._currentCalls[aClientId]);
> +    if (indexes.length < 2) {
> +      return;

nit: TODO here.

@@ +595,5 @@
>    conferenceCall: function(aClientId) {
> +    let indexes = Object.keys(this._currentCalls[aClientId]);
> +    if (indexes.length < 2) {
> +      return;
> +    }

We also need to check |isMergeable| here.

@@ +598,5 @@
> +      return;
> +    }
> +
> +    function onCdmaConferenceCallSuccess() {
> +      for (let i = 0; i < indexes.length; ++i) {

Don't capture |indexes| here, or it becomes not so correct if some CALL_STATE_CHANGE event comes up before this callback gets called.

@@ +618,3 @@
>    },
>  
>    separateCall: function(aClientId, aCallIndex) {

Check |isConference| here.

@@ +625,5 @@
> +    }
> +
> +    function onCdmaSeparateCallSuccess() {
> +      // See 3gpp2, S.R0006-522-A v1.0. Table 4, XID 6S.
> +      let call = this._currentCalls[aClientId][aCallIndex];

nit: parentCall

@@ +632,5 @@
> +      call.state = RIL.CALL_STATE_ACTIVE;
> +      call.isConference = false;
> +      call.isSwitchable = true;
> +      call.isMergeable = true;
> +      call.childId = null;

nit: |delete call.childId;|, so we have either undefined or a valid value in this attribute.

@@ +637,5 @@
> +      this.notifyCallStateChanged(aClientId, call);
> +
> +      if (childId) {
> +        let childCall = this._currentCalls[aClientId][childId];
> +        childCall.parentId = null;

nit: |delete childCall.parentId;|

@@ +731,5 @@
> +      let parentId = this._currentCalls[aClientId][aCall.callIndex].parentId;
> +      if (parentId) {
> +        let parentCall = this._currentCalls[aClientId][parentId];
> +        // Telling the parent that the child is going to be released.
> +        parentCall.childId = null;

nit: delete parentCall.childId;

@@ +737,5 @@
> +          // As the child is going to be gone, the parent should be moved out
> +          // of conference accordingly.
> +          manualConfStateChange = true;
> +          parentCall.isConference = false;
> +          aCall.isConference = false;

Need to reset isSwitchable and isMergeable back to true as well.

@@ +759,5 @@
> +      delete this._currentCalls[aClientId][aCall.callIndex];
> +
> +      if (manualConfStateChange) {
> +        this.notifyConferenceCallStateChanged(RIL.CALL_STATE_UNKNOWN);
> +      }

let it be:

  if (!aCall.failCause ||
      aCall.failCause === RIL.GECKO_CALL_ERROR_NORMAL_CALL_CLEARING) {
    this._notifyAllListeners("callStateChanged", ...);
  } else {
    this.notifyCallError(aClientId, aCall.callIndex, aCall.failCause);
  }

  delete this._currentCalls[aClientId][aCall.callIndex];
  if (manualConfStateChange) {
    this.notifyConferenceCallStateChanged(RIL.CALL_STATE_UNKNOWN);
  }

@@ +796,5 @@
>    /**
>     * Handle call state changes by updating our current state and the audio
>     * system.
>     */
> +  notifyCallStateChanged: function(aClientId, aCall, aSkipConvertState) {

nsIGonkTelephonyProvider::notifyCallStateChanged has only two parameters.  Please update the interface as well.

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

Comment 52 addressed.

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

Hey Vicamo, thanks for the review. I'll provide a new version based on all your comments except:

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #53)
> Comment on attachment 8380451 [details] [diff] [review]
> 0002-3way-part2-ril-impl.patch - v3
> 
> Review of attachment 8380451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +625,5 @@
> > +    }
> > +
> > +    function onCdmaSeparateCallSuccess() {
> > +      // See 3gpp2, S.R0006-522-A v1.0. Table 4, XID 6S.
> > +      let call = this._currentCalls[aClientId][aCallIndex];
> 
> nit: parentCall
> 

I'd like to call it 'call' whenever we retrieve from 'aCallIndex.' I will be making sure that we follow the consistent pattern in the patch.

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

Add an optional attribute "aSkipStateConversion' to nsIGonkTelephonyProvider.notifyCallStateChanged

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

Comment on attachment 8381906 [details] [diff] [review]
0003-3way-part3-internal-api.patch - v3

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

Review comment addressed. Please take a look again, thanks!

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

Comment on attachment 8379626 [details] [diff] [review]
0005-3way-part5-BT.patch - WIP v1

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

Hi Gina,
Telephony is going to provide two more attributes "isSwitchable" and "isMergeable" to indicate a call's feasibility. I am not sure how BT is going to use them, so I aim at making compile success in this patch. Please help review this, thank you!

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

Comment 53 addressed.

In summary:
1) Have a solid check on call.parentId/call.childId 
2) Have a solid check on operation availability
3) Make sure call flags, i.e. isConference, isSwitchable and isMergeable, are updated correctly. 
4) Make sure a telephonyProvider callback is called. ril_worker.js should send back response (no matter success or error) as required.

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

Comment on attachment 8381908 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v4

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

Comment 53 addressed.

In summary:
1) Have a solid check on call.parentId/call.childId 
2) Have a solid check on operation availability
3) Make sure call flags, i.e. isConference, isSwitchable and isMergeable, are updated correctly. 
4) Make sure a telephonyProvider callback is called. ril_worker.js should send back response (no matter success or error) as required.

Vicamo, could you please take a look again? Thanks!

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

Comment on attachment 8379626 [details] [diff] [review]
0005-3way-part5-BT.patch - WIP v1

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

No specific requests for now. Let's ship this patch first. :)

	Vicamo Yang [:vicamo][:vyang]
	Comment 62 • 11 years ago

Comment on attachment 8381908 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v4

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

::: dom/system/gonk/ril_worker.js
@@ +5373,4 @@
>      this._hasConferenceRequest = false;
>      options = {rilMessageType: "conferenceError",
>                 errorName: "addError",
>                 errorMsg: RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]};

We've registered callback for REQUEST_CONFERENCE, so this don't really have to be a unsolicited message anymore.  We can have following lines instead:

  options.success = (options.rilRequestError === 0);
  if (!options.success) {
    options.errorName = "addError";
    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
    this._hasConferenceRequest = false;
  }

  this.sendChromeMessage(options);

And we don't need a unsolicited message handler for "conferenceError" in RadinInterface, either.  Instead, in TelephonyProvider we do:

  conferenceCall: function(aClientId) {
    ...

    this._getClient(aClientId).sendWorkerMessage("conferenceCall", null,
                                                 (function(response) {
      if (!response.success) {
        this.notifyConferenceError(response.errorName, response.errorMsg);
        return false;
      }
      ...

Actually neither do we even need that nsIGonkTelephonyProvider::notifyConferenceError, so we can remove it now and move its body into that |if (!response.success)| block.

@@ +5970,4 @@
>      options = {rilMessageType: "conferenceError",
>                 errorName: "removeError",
>                 errorMsg: RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]};
>      this.sendChromeMessage(options);

Ditto.  Negative responses come unsolicitedly but positive ones come the other way. :X

@@ +6192,5 @@
>  };
> +RilObject.prototype[REQUEST_CDMA_FLASH] = function REQUEST_CDMA_FLASH(length, options) {
> +  options.success = (options.rilRequestError === 0);
> +  if (!options.success) {
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];

When used as "conferenceCall" and "separateCall", we're expecting |notifyConferenceError| gets called.  But that's not available here, and |options.errorName| is missed.

::: dom/telephony/gonk/TelephonyProvider.js
@@ +440,5 @@
>      }
>  
> +    function onCdmaDialSuccess() {
> +      let indexes = Object.keys(this._currentCalls[aClientId]);
> +      if (!indexes) {

|indexes| is at least an empty array, so this is always true.

@@ +445,5 @@
> +        aTelephonyCallback.notifyDialSuccess();
> +        return;
> +      }
> +
> +      if (indexes.length == 1) {

Bail-out early.

@@ +525,5 @@
>  
>    holdCall: function(aClientId, aCallIndex) {
> +    let call = this._currentCalls[aClientId][aCallIndex];
> +    if (!call || !call.isSwitchable) {
> +      return;

Please place comment “TODO: bug 975949 ...”

@@ +569,5 @@
>  
>    resumeCall: function(aClientId, aCallIndex) {
> +    let call = this._currentCalls[aClientId][aCallIndex];
> +    if (!call || !call.isSwitchable) {
> +      return;

TODO...

@@ +613,5 @@
>    },
>  
>    conferenceCall: function(aClientId) {
> +    let indexes = Object.keys(this._currentCalls[aClientId]);
> +    if (!indexes || indexes.length < 2) {

ditto, |indexes| is always evaluated as true.

@@ +620,5 @@
> +
> +    for (let i = 0; i < indexes.length; ++i) {
> +      let call = this._currentCalls[aClientId][indexes[i]];
> +      if (!call.isMergeable) {
> +        return;

TODO: ...

@@ +655,5 @@
>  
>    separateCall: function(aClientId, aCallIndex) {
> +    let call = this._currentCalls[aClientId][aCallIndex];
> +    if (!call || !call.isConference) {
> +      return;

TODO...

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

Comment on attachment 8381906 [details] [diff] [review]
0003-3way-part3-internal-api.patch - v3

Should revise based on comment 62.

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #62)
> Comment on attachment 8381908 [details] [diff] [review]
> 0002-3way-part2-ril-impl.patch - v4
> 
> Review of attachment 8381908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +5373,4 @@
> >      this._hasConferenceRequest = false;
> >      options = {rilMessageType: "conferenceError",
> >                 errorName: "addError",
> >                 errorMsg: RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]};
> 
> We've registered callback for REQUEST_CONFERENCE, so this don't really have
> to be a unsolicited message anymore.  We can have following lines instead:
> 
>   options.success = (options.rilRequestError === 0);
>   if (!options.success) {
>     options.errorName = "addError";
>     options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
>     this._hasConferenceRequest = false;
>   }
> 
>   this.sendChromeMessage(options);
> 
> And we don't need a unsolicited message handler for "conferenceError" in
> RadinInterface, either.  Instead, in TelephonyProvider we do:
> 
>   conferenceCall: function(aClientId) {
>     ...
> 
>     this._getClient(aClientId).sendWorkerMessage("conferenceCall", null,
>                                                  (function(response) {
>       if (!response.success) {
>         this.notifyConferenceError(response.errorName, response.errorMsg);
>         return false;
>       }
>       ...
> 
> Actually neither do we even need that
> nsIGonkTelephonyProvider::notifyConferenceError, so we can remove it now and
> move its body into that |if (!response.success)| block.
> 

I should have noticed this. Thanks for catching.

> @@ +5970,4 @@
> >      options = {rilMessageType: "conferenceError",
> >                 errorName: "removeError",
> >                 errorMsg: RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError]};
> >      this.sendChromeMessage(options);
> 
> Ditto.  Negative responses come unsolicitedly but positive ones come the
> other way. :X

My bad ... ...

> 
> Please place comment “TODO: bug 975949 ...”
> 

Now I got you :P
> @@ +569,5

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

We don't need nsIGonkTelephonyProvider.notifyConferenceError

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

Major changes: no nsIGonkTelephonyProvider.notifyConferenceError

	Vicamo Yang [:vicamo][:vyang]
	Comment 67 • 11 years ago

Comment on attachment 8382118 [details] [diff] [review]
0003-3way-part3-internal-api.patch - v4

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

::: dom/telephony/nsIGonkTelephonyProvider.idl
@@ +15,5 @@
>  {
>    void notifyCallDisconnected(in unsigned long clientId, in jsval call);
>  
>    void notifyCallError(in unsigned long clientId, in long callIndex,
>                         in AString error);

Had a check to all methods in this interface.  It happens that we don't need |notifyCallError|, either.  Filed as follow-up bug 977085.

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

Comment on attachment 8382138 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v5

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

Major changes: no nsIGonkTelephonyProvider.notifyConferenceError

Thank you.

	Vicamo Yang [:vicamo][:vyang]
	Comment 69 • 11 years ago

Comment on attachment 8382138 [details] [diff] [review]
0002-3way-part2-ril-impl.patch - v5

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

I'm placing a r+ here, but please have another revision for the last three items.  Thank you :)

::: dom/system/gonk/ril_worker.js
@@ +1627,5 @@
>  
>    holdCall: function(options) {
>      let call = this.currentCalls[options.callIndex];
> +    if (!call) {
> +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;

nit: |options.errorMsg| is not referenced in callback, but I think it's fine to leave it here.

@@ +1645,5 @@
>  
>    resumeCall: function(options) {
>      let call = this.currentCalls[options.callIndex];
> +    if (!call) {
> +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;

ditto.

@@ +1677,5 @@
>  
>    separateCall: function(options) {
> +    let call = this.currentCalls[options.callIndex];
> +    if (!call) {
> +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;

The callback in TelephonyProvider expects both |options.errorName| and |options.errorMsg| upon error returned.

@@ +5343,5 @@
>    this.getCurrentCalls();
>  };
>  RilObject.prototype[REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE] = function REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE(length, options) {
> +  options.success = (options.rilRequestError === 0);
> +  if (!options.success) {

This request is only used in |answerCall()| and it doesn't register a callback.  Sending errors back here always gets a "Ignore orphan token" message back in WorkerMessenger.  Had a check back, I didn't mention this in comment 58.

@@ +6210,5 @@
> +    case "holdCall":
> +    case "resumeCall":
> +    case "conferenceCall":
> +    case "separateCall":
> +      options.isCdma = this._isCdma;

Does this switch really mean anything?  I have a scan through all calls to |sendCdmaFlashCommand()| and it's only called when |this._isCdma === true| as its name suggests.  So this handler can be further simplified as:

  sendCdmaFlashCommand: function() {
    options.isCdma = true;
    ...
  },

  RilObject.prototype[REQUEST_CDMA_FLASH] = function(length, options) {
    options.success = (options.rilRequestError === 0);
    if (!options.success) {
      // Assign |options.errorName| and |options.errorMsg| only.
    }

    this.sendChromeMessage(options);
  };

	Vicamo Yang [:vicamo][:vyang]
	Comment 70 • 11 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #69)
> ::: dom/system/gonk/ril_worker.js
> @@ +5343,5 @@
> >    this.getCurrentCalls();
> >  };
> >  RilObject.prototype[REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE] = function REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE(length, options) {
> > +  options.success = (options.rilRequestError === 0);
> > +  if (!options.success) {
> 
> This request is only used in |answerCall()| and it doesn't register a
> callback.  Sending errors back here always gets a "Ignore orphan token"
> message back in WorkerMessenger.  Had a check back, I didn't mention this in
> comment 58.

HsinYi just told me that REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE and REQUEST_SWITCH_HOLDING_AND_ACTIVE share the same id in "ril.h"[1].  However, that doesn't make any sense because we cannot have two attributes of the same name in |RilObject.prototype|.  AOSP ICS/JB/KK only uses REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE.  So please just remove REQUEST_SWITCH_HOLDING_AND_ACTIVE and all related duplicates.

[1]: https://android.googlesource.com/platform/hardware/ril/+/android-4.0.4_r2.1/include/telephony/ril.h

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #69)
> Comment on attachment 8382138 [details] [diff] [review]
> 0002-3way-part2-ril-impl.patch - v5
> 
> Review of attachment 8382138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm placing a r+ here, but please have another revision for the last three
> items.  Thank you :)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1627,5 @@
> >  
> >    holdCall: function(options) {
> >      let call = this.currentCalls[options.callIndex];
> > +    if (!call) {
> > +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;
> 
> nit: |options.errorMsg| is not referenced in callback, but I think it's fine
> to leave it here.
> 

Yes, I left it on purpose, responding both options.success & options.errorMsg, to have a consistent 
pattern with other request types.

> @@ +1645,5 @@
> >  
> >    resumeCall: function(options) {
> >      let call = this.currentCalls[options.callIndex];
> > +    if (!call) {
> > +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;
> 
> ditto.
> 
> @@ +1677,5 @@
> >  
> >    separateCall: function(options) {
> > +    let call = this.currentCalls[options.callIndex];
> > +    if (!call) {
> > +      options.errorMsg = GECKO_ERROR_GENERIC_FAILURE;
> 
> The callback in TelephonyProvider expects both |options.errorName| and
> |options.errorMsg| upon error returned.
> 
> @@ +5343,5 @@
> >    this.getCurrentCalls();
> >  };
> >  RilObject.prototype[REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE] = function REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE(length, options) {
> > +  options.success = (options.rilRequestError === 0);
> > +  if (!options.success) {
> 
> This request is only used in |answerCall()| and it doesn't register a
> callback.  Sending errors back here always gets a "Ignore orphan token"
> message back in WorkerMessenger.  Had a check back, I didn't mention this in
> comment 58.

Remove all duplicates.

> 
> @@ +6210,5 @@
> > +    case "holdCall":
> > +    case "resumeCall":
> > +    case "conferenceCall":
> > +    case "separateCall":
> > +      options.isCdma = this._isCdma;
> 
> Does this switch really mean anything?  I have a scan through all calls to
> |sendCdmaFlashCommand()| and it's only called when |this._isCdma === true|
> as its name suggests.  So this handler can be further simplified as:
> 
>   sendCdmaFlashCommand: function() {
>     options.isCdma = true;
>     ...
>   },
> 
>   RilObject.prototype[REQUEST_CDMA_FLASH] = function(length, options) {
>     options.success = (options.rilRequestError === 0);
>     if (!options.success) {
>       // Assign |options.errorName| and |options.errorMsg| only.
>     }
> 
>     this.sendChromeMessage(options);
>   };

Will do!

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

Release a child when a waiting call coming.

	Enpei
	Comment 73 • 11 years ago

Test cases are in moztrap: https://moztrap.mozilla.org/manage/cases/?&pagenumber=1&pagesize=100&sortfield=created_on&sortdirection=desc&filter-tag=2573

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

comment 69 and comment 70 addressed, and covered Attachment #8382890 [details] [diff]

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

try looks good: https://tbpl.mozilla.org/?tree=Try&rev=b349e19752e9 except mochitest-debug-8.

There are known issues about mochitest-debug-8, and the patches of this bug are far away from the code path of mochitest-debug-8.

I've tested this on wasabi and buri phones, and in addition to try server, local automation tests were passed as well. I'd say it's safe to go.

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

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/761fb3484d0a
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/f98abdde7107
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/d6a671e2ba43
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/7bbe31846f98
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/88eb9472012e

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

https://hg.mozilla.org/mozilla-central/rev/761fb3484d0a
https://hg.mozilla.org/mozilla-central/rev/f98abdde7107
https://hg.mozilla.org/mozilla-central/rev/d6a671e2ba43
https://hg.mozilla.org/mozilla-central/rev/7bbe31846f98
https://hg.mozilla.org/mozilla-central/rev/88eb9472012e