772765 - B2G telephony: support conference calls

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Beatriz Rodríguez [:brg]]

This is a supplementary service that allows the user to have a call with 2
different parties at the same time. See 3GPP TS 24.084.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 1 - idl

Web APIs (nsIDOMTelephony, nsIDOMTelephonyCall and nsIDOMTelephonyCallGroup) have been discussed in bug 768925.

Regarding internal APIs, 
1) add some conference call functions in nsITelephonyProvider
2) add some callbacks for conference call in nsITelephonyListener
3) add nsITelephonyMptyCallInfo used in nsITelephonyListener::ConferenceCallsChanged

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - DOM impl

Covering Telephony.cpp/.h, TelephonyCall.cpp/.h, TelephonyCallGroup.cpp/.h ...

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 1 - idl

Web APIs (nsIDOMTelephony, nsIDOMTelephonyCall and nsIDOMTelephonyCallGroup) have been discussed in bug 768925.

Regarding internal APIs, 
1) add some conference call functions in nsITelephonyProvider
2) add some callbacks for conference call in nsITelephonyListener
3) add nsITelephonyMptyCallInfo used in nsITelephonyListener::ConferenceCallsChanged

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - DOM impl

Covering Telephony.cpp/.h, TelephonyCall.cpp/.h, TelephonyCallGroup.cpp/.h ...

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - RIL impl

Including implementation of internal APIs, RIL call state transition ...

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 4 - BT impl

BluetoothTelephonyListener impl.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 759047 [details] [diff] [review]
WIP - part 1 - idl

Hi Ben,

Here's WIP for conference call. Would you please give me some feedback? Thank you.

Web APIs (nsIDOMTelephony, nsIDOMTelephonyCall and nsIDOMTelephonyCallGroup) have been discussed in bug 768925. The patch is based on that.

Regarding internal APIs, changes below are made:
1) add some conference call functions in nsITelephonyProvider
2) add some callbacks for conference call in nsITelephonyListener
3) add nsITelephonyMptyCallInfo used in nsITelephonyListener::ConferenceCallsChanged. nsITelephonyMptyCallInfo.isJoining indicates whether the call is joining or leaving conference call group, so corresponding update can be correctly applied to TelephonyCallGroup::mCalls

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 759048 [details] [diff] [review]
WIP - part 2 - DOM impl

Hi Ben,

Here's a WIP for conference call implementation; would you please give me some feedback? Please kindly note that this is not the final patch and debug messages will be removed in the end. Most API implementation is done, but I am still thinking of the need of creating a new CallGroupEvent (or something like that).

Summary of the patch:
1) TelephonyCallGroup.cpp implements nsIDOMTelephonyCallGroup
2) Telephony.cpp implements nsITelephonyListener. While nsITelephonyListener::ConferenceCallsChanged is called, we search for calls in Telephony::mCalls and update TelephonyCallGroup::mCalls accordingly. In the meanwhile, call.group might change as well.
3) While nsITelephonyListener::ConferenceCallStateChanged, TelephonyCallGroup::mCallState is updated as well as callstate of the calls in TelephonyCallGroup::mCalls.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 5 - generate CallGroupEvent

Dispatch CallGroupEvent when nsIDOMTelephonyCallGroup::onstatechange, onconnected, onholding, onheld, onresuming.

Still use CallEvent for nsIDOMTelephonyCallGroup::oncallschanged and nsIDOMTelephonyCall::ongroupchange.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 759047 [details] [diff] [review]
WIP - part 1 - idl

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

Ping again. Thanks! :)

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 759048 [details] [diff] [review]
WIP - part 2 - DOM impl

Hi Ben, 
Ping again. Thanks :)

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sorry, I've been swamped. I'll be able to look at this first thing Monday.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 759047 [details] [diff] [review]
WIP - part 1 - idl

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

Hi Hsin-Yi, I've started looking over this and I realized that it's not using WebIDL yet. Since a lot of the features we want on the Telephony API require WebIDL (e.g. having a proper 'calls' array, having 'active' be either a call or call group, etc.) and since we're really really trying not to add any additional features that use XPIDL instead I think I have to require WebIDL here.

On the bright side your implementation won't have to change too much so it isn't as if your work is wasted.  The function signatures will change a bit but by and large most of the code should keep working about the same.

I'm really sorry it took me so long to get around to this. Please feel free to ping me if you need pointers on how to convert this to WebIDL. I'd be happy to help also if you'd like.

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #16)
> Comment on attachment 759047 [details] [diff] [review]
> WIP - part 1 - idl
> 
> Review of attachment 759047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Hsin-Yi, I've started looking over this and I realized that it's not
> using WebIDL yet. Since a lot of the features we want on the Telephony API
> require WebIDL (e.g. having a proper 'calls' array, having 'active' be
> either a call or call group, etc.) and since we're really really trying not
> to add any additional features that use XPIDL instead I think I have to
> require WebIDL here.
> 

Oh, I see. Sorry for not being aware of that before. I will look into WebIDL first then. 

> On the bright side your implementation won't have to change too much so it
> isn't as if your work is wasted.  The function signatures will change a bit
> but by and large most of the code should keep working about the same.
> 
> I'm really sorry it took me so long to get around to this. Please feel free
> to ping me if you need pointers on how to convert this to WebIDL. I'd be
> happy to help also if you'd like.

Don't worry. I will ask if I do encounter some questions. ;-) Thanks in advance!

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

listed as must have in v1.2 for COMM team. koi+

Comment 19

[leo.bugzilla.gecko]

Will the conference call be not included in V1.1? 
The target is V1.2?

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to leo.bugzilla.gecko from comment #19)
> Will the conference call be not included in V1.1? 
> The target is V1.2?

It's a V1.2 feature.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 1 - v2 - idl/webidl

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 1 - v2 - idl/webidl

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - v2 - telephony DOM

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 773155 [details] [diff] [review]
WIP - part 1 - v2 - idl/webidl

Hi Ben,

I am using WebIDL per your request. Would you please take a look at it and give me some feedback?

I have a simple question about the API behaviour, and would like to confirm with you:
A call is in either nsIDOMTelephony::calls or nsIDOMTelephony::conferenceGroup. Is that expected behaviour?

Thank you :)

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - v2 - RIL impl

TODO: update phone audio state in RadioInterfaceLayer

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 4 - v2 - BT impl

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - v2 - telephony DOM

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 773761 [details] [diff] [review]
WIP - part 2 - v2 - telephony DOM

Hi Ben,

Would you please give me some feedback to see if the patch is on the right direction, though this patch isn't fully completely? I've implemented most things, except Telephony::EnumerateCallState, which would be basically very similar to Telephony::CallStateChanged. Thank you!

Patch summary:
1) TelephonyCallGroup.cpp implements TelephonyCallGroup.webidl
2) Telephony.cpp implements nsITelephonyListener. While nsITelephonyListener::CallStateChanged is called, we check if a call is multiparty, and then update Telephony::mCalls or TelephonyCallGroup::mCalls accordingly. In the meanwhile, call.group might change as well.
3) While nsITelephonyListener::ConferenceCallStateChanged, TelephonyCallGroup::mCallState is updated as well as callstate of the calls in TelephonyCallGroup::mCalls.

Comment 29

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! Would you be so kind to include a link to the specification (or maybe add it as an attachment) on which you may be basing this implementation? :-) Thanks!

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - v3 - telephony DOM

Telephony::EnumerateCallState done.

Comment 31

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 774438 [details] [diff] [review]
WIP - part 2 - v3 - telephony DOM

Hi Ben,

Telephony::EnumerateCallState is done. Would you please give me some feedback?

In the patch, when a call's group changes, even its state change accompanies, call.onstatechange isn't fired. But it's updated internally. However, when conferenceGroup.state changes, the state of all contained calls changes and event fires at the same time. Is that expected? Thank you.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 773155 [details] [diff] [review]
WIP - part 1 - v2 - idl/webidl

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

This looks great, it's totally going in the right direction! I think we should take the opportunity to switch Telephony/TelephonyCall too, though, rather than adding more JSAPI usage here.

::: dom/telephony/nsIDOMTelephony.idl
@@ +20,5 @@
>  
>    // The call that is "active", i.e. receives microphone input and tones
> +  // generated via startTone. It could be a nsIDOMTelephonyCall or a
> +  // TelephonyCallGroup.
> +  readonly attribute jsval active;

We really shouldn't be adding any more jsvals to XPIDL, WebIDL solves this problem for us in a much safer way.

::: dom/telephony/nsITelephonyProvider.idl
@@ +30,5 @@
>                          in AString number,
>                          in boolean isActive,
>                          in boolean isOutgoing,
> +                        in boolean isEmergency,
> +                        in boolean isMpty);

"isConference" maybe?

::: dom/webidl/TelephonyCallGroup.webidl
@@ +6,5 @@
> +
> +interface TelephonyCall;
> +
> +interface TelephonyCallGroup : EventTarget {
> +  readonly attribute any calls;

This should be a sequence of TelephonyCall objects. This way you won't have to use JSAPI to make an array. See 'DOMMediaStream::GetAudioTracks' for an example. You can do this on Telephony.calls too.

Comment 33

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 774438 [details] [diff] [review]
WIP - part 2 - v3 - telephony DOM

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

This looks pretty good too! I'll want to review the final patches still but overall I think this is looking good.

::: dom/telephony/Telephony.cpp
@@ +134,5 @@
>    telephony->BindToOwner(aOwner);
>  
>    telephony->mProvider = aProvider;
>    telephony->mListener = new Listener(telephony);
> +  telephony->mGroup = TelephonyCallGroup::Create(telephony);

Is there any reason to create this eagerly? Having a null group is what I would expect if there was no conference call yet.

::: dom/telephony/TelephonyCallGroup.cpp
@@ +120,5 @@
> +  nsresult rv = DispatchCallEvent(NS_LITERAL_STRING("statechange"), nullptr);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed to dispatch specific event!");
> +  }
> +  if (!stateString.IsEmpty()) {

I don't think this is quite right. You don't want to dispatch an event here if the previous DispatchCallEvent() somehow changed the state again (similar to how TelephonyCall::ChangeState works).

@@ +194,5 @@
> +
> +nsresult
> +TelephonyCallGroup::EnsureState(uint16_t aCallState)
> +{
> +  if (mCalls.Length() == 1) {

This seems kind of bizarre... What problem are you trying to solve here?

@@ +208,5 @@
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(TelephonyCallGroup,
> +                                                  nsDOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

Not a script holder so you can remove this, and the NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED below

@@ +211,5 @@
> +                                                  nsDOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +  for (uint32_t index = 0; index < tmp->mCalls.Length(); index++) {
> +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mCalls[i]");
> +    cb.NoteXPCOMChild(tmp->mCalls[index]->ToISupports());

Nowadays we have much better helpers for this stuff, I think you can skip the array enumeration and just call:

  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCalls)

::: dom/telephony/TelephonyCallGroup.h
@@ +30,5 @@
> +
> +  uint16_t mCallState;
> +public:
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(TelephonyCallGroup,

This class doesn't hold any script objects, you just need NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED

@@ +66,5 @@
> +  static already_AddRefed<TelephonyCallGroup>
> +  Create(Telephony* aTelephony);
> +
> +  nsISupports*
> +  ToISupports()

Hopefully this is unused in the final patch and can be removed.

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #32)
> Comment on attachment 773155 [details] [diff] [review]
> WIP - part 1 - v2 - idl/webidl
> 
> Review of attachment 773155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great, it's totally going in the right direction! I think we
> should take the opportunity to switch Telephony/TelephonyCall too, though,
> rather than adding more JSAPI usage here.
> 

Thanks for the feedback, Ben. I will add the bug dependency to Bug 888592, and move telephony and telephony call to webidl first.

> ::: dom/telephony/nsIDOMTelephony.idl
> @@ +20,5 @@
> >  
> >    // The call that is "active", i.e. receives microphone input and tones
> > +  // generated via startTone. It could be a nsIDOMTelephonyCall or a
> > +  // TelephonyCallGroup.
> > +  readonly attribute jsval active;
> 
> We really shouldn't be adding any more jsvals to XPIDL, WebIDL solves this
> problem for us in a much safer way.
> 
> ::: dom/telephony/nsITelephonyProvider.idl
> @@ +30,5 @@
> >                          in AString number,
> >                          in boolean isActive,
> >                          in boolean isOutgoing,
> > +                        in boolean isEmergency,
> > +                        in boolean isMpty);
> 
> "isConference" maybe?

Sounds good.

> 
> ::: dom/webidl/TelephonyCallGroup.webidl
> @@ +6,5 @@
> > +
> > +interface TelephonyCall;
> > +
> > +interface TelephonyCallGroup : EventTarget {
> > +  readonly attribute any calls;
> 
> This should be a sequence of TelephonyCall objects. This way you won't have
> to use JSAPI to make an array. See 'DOMMediaStream::GetAudioTracks' for an
> example. You can do this on Telephony.calls too.

Good to know that!

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

In reply to ben turner [:bent] from comment #33)
> Comment on attachment 774438 [details] [diff] [review]
> WIP - part 2 - v3 - telephony DOM
> 
> Review of attachment 774438 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty good too! I'll want to review the final patches still but
> overall I think this is looking good.
> 

Thanks for the feedback.

> ::: dom/telephony/Telephony.cpp
> @@ +134,5 @@
> >    telephony->BindToOwner(aOwner);
> >  
> >    telephony->mProvider = aProvider;
> >    telephony->mListener = new Listener(telephony);
> > +  telephony->mGroup = TelephonyCallGroup::Create(telephony);
> 
> Is there any reason to create this eagerly? Having a null group is what I
> would expect if there was no conference call yet.
> 

According to the API design, we use mozTelephony.conferenceGroup.add() to initialize a conference call, so we need to create it in the beginning.


> ::: dom/telephony/TelephonyCallGroup.cpp
> @@ +120,5 @@
> > +  nsresult rv = DispatchCallEvent(NS_LITERAL_STRING("statechange"), nullptr);
> > +  if (NS_FAILED(rv)) {
> > +    NS_WARNING("Failed to dispatch specific event!");
> > +  }
> > +  if (!stateString.IsEmpty()) {
> 
> I don't think this is quite right. You don't want to dispatch an event here
> if the previous DispatchCallEvent() somehow changed the state again (similar
> to how TelephonyCall::ChangeState works).

Got it!

> 
> @@ +194,5 @@
> > +
> > +nsresult
> > +TelephonyCallGroup::EnsureState(uint16_t aCallState)
> > +{
> > +  if (mCalls.Length() == 1) {
> 
> This seems kind of bizarre... What problem are you trying to solve here?

Indeed... I wanted to play it safer to make sure the length of this conference group array is never 1. However, since ril code ensures that, I think we can get rid of it.

> 
> @@ +208,5 @@
> > +}
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(TelephonyCallGroup,
> > +                                                  nsDOMEventTargetHelper)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> 
> Not a script holder so you can remove this, and the
> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED below
> 

Thanks for pointing this out. Cycle collection and the macro are so profound...

> @@ +211,5 @@
> > +                                                  nsDOMEventTargetHelper)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> > +  for (uint32_t index = 0; index < tmp->mCalls.Length(); index++) {
> > +    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mCalls[i]");
> > +    cb.NoteXPCOMChild(tmp->mCalls[index]->ToISupports());
> 
> Nowadays we have much better helpers for this stuff, I think you can skip
> the array enumeration and just call:
> 
>   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCalls)
> 
> ::: dom/telephony/TelephonyCallGroup.h
> @@ +30,5 @@
> > +
> > +  uint16_t mCallState;
> > +public:
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(TelephonyCallGroup,
> 
> This class doesn't hold any script objects, you just need
> NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED
> 
> @@ +66,5 @@
> > +  static already_AddRefed<TelephonyCallGroup>
> > +  Create(Telephony* aTelephony);
> > +
> > +  nsISupports*
> > +  ToISupports()
> 
> Hopefully this is unused in the final patch and can be removed.

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Status update: Bug 888592 is in review process. I will rebase the conference call patches upon that these days.

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part1 - v3 - webidl

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - v4 - telephony DOM

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - v3 - RIL impl

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 4 - v3 - BT impl

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

It bases on But 888592.

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 779628 [details] [diff] [review]
WIP - part 2 - v4 - telephony DOM

It bases on Bug 888592.

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - v4 - RIL impl

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - v4 - RIL impl

This is the right one ...

Comment 45

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 779630 [details] [diff] [review]
WIP - part 4 - v3 - BT impl

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

Thanks, hsinyi.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 779695 [details] [diff] [review]
WIP - part 3 - v4 - RIL impl

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

::: dom/system/gonk/ril_worker.js
@@ +3626,5 @@
> +        if (newCall.isMpty) {
> +          conferenceChanged = true;
> +          newCall.isConference = true;
> +          this.currentConference.participants[newCall.callIndex] = newCall;
> +        }

Please also set |isConference| to false when |newCall.isMpty| is false.  Then we can ensure all call attributes are correctly set outside ril_worker, and we don't need "data.isConference || false" in RILContentHelper.

@@ +3651,5 @@
> +
> +    if (remaining.length == 1) {
> +      // Remove that if only does one remain in a conference call.
> +      let call = this.currentConference.participants[remaining[0]];
> +      this.currentCalls[call.callIndex].isConference = false;

Isn't |call| and |this.currentCalls[call.callIndex]| the same object?  You can simply do |call.isConference = false| here.  Or a shorter version:

  let call = this.currentCalls[remaining[0]];
  call.isConference = false;

@@ +3679,5 @@
> +    if (oldState != state) {
> +      this.currentConference.state = state;
> +      let message = {rilMessageType: "conferenceCallStateChanged",
> +                     state: state};
> +      this.sendDOMMessage(message);

Had been renamed to 'sendChromeMessage'.  See bug 761732.

@@ +5403,5 @@
> +RIL[REQUEST_SEPARATE_CONNECTION] = function REQUEST_SEPARATE_CONNECTION(length, options) {
> +  if (options.rilRequestError) {
> +    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
> +    return;
> +  }

Did you miss something like a `sendChromeMessage()`?  These lines have nearly no effect.

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Cancelling r? as I need to solve 'CallsList' first.

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part1 - webidl (v4)

TelephonyCallGroup.webidl

Comment 49

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part2 - telephony dom (v5)

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part3 - RIL impl (v5)

Comment #46 addressed.

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part2 - telephony dom (v5)

Comment 52

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 781515 [details] [diff] [review]
patch - part1 - webidl (v4)

The patch is based on bug 888592 (moving telephony and telephonyCall to webidl).

Comment 53

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 781517 [details] [diff] [review]
patch - part3 - RIL impl (v5)

Comments have been addressed. Asking for review again. Thanks!

Comment 54

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 781517 [details] [diff] [review]
patch - part3 - RIL impl (v5)

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

::: dom/system/gonk/ril_worker.js
@@ +3710,5 @@
> +
> +    if (remaining.length == 1) {
> +      // Remove that if only does one remain in a conference call.
> +      let call = this.currentCalls[remaining[0]];
> +      call.isConference = false;

Call state isn't updated correctly here. Another revision is coming ...

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part3 - RIL impl (v6)

Comment 56

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part3 - RIL impl (v7)

This is.

Comment 57

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 781515 [details] [diff] [review]
patch - part1 - webidl (v4)

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

Looks good! Two things:

::: dom/webidl/Telephony.webidl
@@ +15,5 @@
>    [Throws]
>    attribute boolean speakerEnabled;
>  
> +  // Bug 865998 - Implement WebIDL union return values
> +  //readonly attribute (TelephonyCall or TelephonyCallGroup)? active;

You're going to hate me, but this bug has already been fixed. Can we change this before checkin?

::: dom/webidl/TelephonyCall.webidl
@@ +49,5 @@
>    attribute EventHandler onresuming;
>    [SetterThrows]
>    attribute EventHandler onerror;
> +
> +  // Fired whenever the .group attribute changes.

Nit: s/.group/group/

Comment 58

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #57)
> Comment on attachment 781515 [details] [diff] [review]
> patch - part1 - webidl (v4)
> 
> Review of attachment 781515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Two things:
> 
> ::: dom/webidl/Telephony.webidl
> @@ +15,5 @@
> >    [Throws]
> >    attribute boolean speakerEnabled;
> >  
> > +  // Bug 865998 - Implement WebIDL union return values
> > +  //readonly attribute (TelephonyCall or TelephonyCallGroup)? active;
> 
> You're going to hate me, but this bug has already been fixed. Can we change
> this before checkin?
> 

Oh... oh... oh... I knew that bug might be fixed before this, and I knew you would say so once it happens. ;) 
 
> ::: dom/webidl/TelephonyCall.webidl
> @@ +49,5 @@
> >    attribute EventHandler onresuming;
> >    [SetterThrows]
> >    attribute EventHandler onerror;
> > +
> > +  // Fired whenever the .group attribute changes.
> 
> Nit: s/.group/group/

Comment 59

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 781537 [details] [diff] [review]
patch - part2 - telephony dom (v5)

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

Looks pretty good!

::: dom/telephony/CallsList.cpp
@@ +19,5 @@
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
> +CallsList::CallsList(Telephony* aTelephony, TelephonyCallGroup* aGroup)
> +: mTelephony(aTelephony), mGroup(aGroup)

Please assert aTelephony.

@@ +53,1 @@
>    return call ? call.forget() : nullptr;

I think I mentioned this in the other bug but forget() works just fine on a null pointer. In IndexedGetter() too.

::: dom/telephony/Telephony.cpp
@@ +154,5 @@
>  
>    telephony->mProvider = ril;
>    telephony->mListener = new Listener(telephony);
>    telephony->mCallsList = new CallsList(telephony);
> +  telephony->mGroup = TelephonyCallGroup::Create(telephony);

Ah, right, since we always create this the webidl needs to not have a '?' on the attribute (indicating that it is never null).

@@ +270,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Telephony,
>                                                  nsDOMEventTargetHelper)
>    tmp->mCalls.Clear();
>    tmp->mActiveCall = nullptr;
> +  tmp->mGroup = nullptr;

You also need to traverse mGroup above.

@@ +343,5 @@
>    }
>  }
>  
> +JS::Value
> +Telephony::GetActive(JSContext* aCx, ErrorResult& aRv) const

I'd like to see what this looks like with the union.

@@ -395,4 @@
>  {
>    NS_ASSERTION(aCallIndex != kOutgoingPlaceholderCallIndex,
>                 "This should never happen!");
> -

Nit: I'd add this newline back.

@@ +450,5 @@
> +    }
> +    if (modifiedCall) {
> +      modifiedCall->ChangeStateInternal(aCallState, false);
> +      RemoveCall(modifiedCall);
> +      mGroup->AddCall(modifiedCall);

Whether or not we're going to add or remove the call here should depend on the call state, right? I mean, if callstate is DISCONNECTING or something then we might not want to move it around. What possible values of callstate do we need to consider here?

@@ +454,5 @@
> +      mGroup->AddCall(modifiedCall);
> +      return NS_OK;
> +    }
> +
> +    // Didn't know this call in mCalls or mGroup. Create a new call.

Nit: s/know/find/

@@ +461,5 @@
> +                            aIsEmergency, mGroup);
> +    NS_ASSERTION(call, "This should never fail!");
> +
> +    return NS_OK;
> +  } else {

Nit: No need for else after returns.

@@ +582,5 @@
>      }
> +
> +    for (uint32_t index = 0; index < mCalls.Length(); index++) {
> +      nsRefPtr<TelephonyCall>& tempCall = mCalls[index];
> +      if (tempCall->CallIndex() == aCallIndex) {

Hm, you've added quite a few copies of this loop. I think it's probably a good idea to make a GetCall(callIndex) helper function on Telephony that does this (just like you did on TelephonyCallGroup).

Another helper function you might want is one like:

  bool MoveCall(aCallIndex, aIsConference);

That would take care of moving a call in or out of the group. It could return false if the callindex wasn't found.

Wouldn't that simplify this (and the above) code a bit?

::: dom/telephony/Telephony.h
@@ +153,5 @@
>    void
>    EnqueueEnumerationAck();
> +
> +  void
> +  UpdateActiveCall(TelephonyCall*aCall, bool aIsAdding);

Nit: Space after *

::: dom/telephony/TelephonyCall.cpp
@@ +20,5 @@
>  // static
>  already_AddRefed<TelephonyCall>
>  TelephonyCall::Create(Telephony* aTelephony, const nsAString& aNumber,
> +                      uint16_t aCallState, uint32_t aCallIndex,
> +                      bool aEmergency, TelephonyCallGroup* aGroup)

I don't think you need to pass aGroup like this. You can simply pull the group off of aTelephony, right? Just pass a boolean isConference maybe?

::: dom/telephony/TelephonyCallGroup.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "Telephony.h"
> +#include "TelephonyCall.h"
> +#include "TelephonyCallGroup.h"

Please make sure that the header for this cpp is always the first include, and clean these up consistent with whatever you decided on the other bug

@@ +60,5 @@
> +uint32_t
> +TelephonyCallGroup::AddCall(TelephonyCall* aCall)
> +{
> +  NS_ASSERTION(!mCalls.Contains(aCall), "Already know about this one!");
> +  aCall->ChangeGroup(this);

This will send a notification before the call has been put into mCalls... We should make sure that all the lists are updated before we fire any events.

@@ +70,5 @@
> +uint32_t
> +TelephonyCallGroup::RemoveCall(TelephonyCall* aCall)
> +{
> +  NS_ASSERTION(mCalls.Contains(aCall), "Didn't know about this one!");
> +  aCall->ChangeGroup(nullptr);

Here too.

@@ +131,5 @@
> +  }
> +}
> +
> +nsresult
> +TelephonyCallGroup::NotifyCallsChanged(TelephonyCall* aCall) {

Nit: { on its own line

@@ +146,5 @@
> +  return DispatchTrustedEvent(event);
> +}
> +
> +bool
> +TelephonyCallGroup::CanConference(TelephonyCall* aCall, TelephonyCall* aSecondCall)

Why not make aCall be a ref since it's passed in that way and is never null?

@@ +151,5 @@
> +{
> +  MOZ_ASSERT(aCall);
> +
> +  if (!aSecondCall) {
> +    if (!mCalls.Length()) {

Nit: mCalls.IsEmpty()

@@ +188,5 @@
> +}
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(TelephonyCallGroup,
> +                                                  nsDOMEventTargetHelper)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCalls)

Need to also traverse mTelephony and mCallsList. And use the unlink macros in unlink.

@@ +203,5 @@
> +
> +NS_IMPL_ADDREF_INHERITED(TelephonyCallGroup, nsDOMEventTargetHelper)
> +NS_IMPL_RELEASE_INHERITED(TelephonyCallGroup, nsDOMEventTargetHelper)
> +
> +

Nit: only one newline here.

@@ +221,5 @@
> +    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
> +    return;
> +  }
> +
> +  nsresult rv =mTelephony->Provider()->ConferenceCall();

Nit: Here and below, need a space after =

@@ +306,5 @@
> +  ChangeState(nsITelephonyProvider::CALL_STATE_RESUMING);
> +}
> +
> +void
> +TelephonyCallGroup::GetState(nsString& aState) const

This can be inlined

::: dom/telephony/TelephonyCallGroup.h
@@ +8,5 @@
> +#define mozilla_dom_telephony_telephonycallgroup_h__
> +
> +#include "TelephonyCommon.h"
> +
> +#include "nsITelephonyProvider.h"

This isn't needed, right?

@@ +19,5 @@
> +#include "CallsList.h"
> +
> +BEGIN_TELEPHONY_NAMESPACE
> +
> +class TelephonyCallGroup : public nsDOMEventTargetHelper

Similar comments here as in the other bug, basically clean up the headers and forward-declare everything you can, make everything consistent.

@@ +74,5 @@
> +  already_AddRefed<TelephonyCall>
> +  GetCall(uint32_t aCallIndex);
> +
> +  const nsTArray<nsRefPtr<TelephonyCall> >&
> +  GetCalls()

Similar to comments on the other bug, this should be "CallsInternal" or something so that we know it will always return something.

Comment 60

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part2 - telephony dom (v6)

Addressed review comments except using UnionType

Comment 61

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part5 - using uniontype (WIP)

Comment 62

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #61)
> Created attachment 786929 [details] [diff] [review]
> patch - part5 - using uniontype (WIP)

Hi Ben and bz,

I am trying to use webidl union type. However, once I applied this WIP patch, crash occurs. I am still debugging it. In the meantime, would you please give me some feedback to help me move forwards? Thank you.

gdb bt:
=================
Program received signal SIGSEGV, Segmentation fault.
0xfffffffc in ?? ()
(gdb) bt
#0  0xfffffffc in ?? ()
#1  0x4117b100 in WrapNewBindingObject<mozilla::dom::telephony::TelephonyCallGroup> (this=<value optimized out>, cx=0x43a3b7c0, scopeObj=..., rval=...)
    at ../../dist/include/mozilla/dom/BindingUtils.h:654
#2  mozilla::dom::WrapNewBindingObjectHelper<mozilla::dom::OwningNonNull<mozilla::dom::telephony::TelephonyCallGroup> const, true>::Wrap (
    this=<value optimized out>, cx=0x43a3b7c0, scopeObj=..., rval=...) at ../../dist/include/mozilla/dom/BindingUtils.h:1296
#3  WrapNewBindingObject<mozilla::dom::OwningNonNull<mozilla::dom::telephony::TelephonyCallGroup> const> (this=<value optimized out>, cx=0x43a3b7c0, 
    scopeObj=..., rval=...) at ../../dist/include/mozilla/dom/BindingUtils.h:1315
#4  mozilla::dom::TelephonyCallOrTelephonyCallGroupReturnValue::ToJSVal (this=<value optimized out>, cx=0x43a3b7c0, scopeObj=..., rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/objdir-gecko/dom/bindings/UnionTypes.cpp:926
#5  0x4113a958 in get_active (cx=0x43a3b7c0, obj=..., self=<value optimized out>, args=...)
    at /home/hsinyi/src/mozilla/mozilla-central/objdir-gecko/dom/bindings/TelephonyBinding.cpp:222
#6  0x4113ab42 in genericGetter (cx=0x43a3b7c0, argc=<value optimized out>, vp=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/objdir-gecko/dom/bindings/TelephonyBinding.cpp:623
#7  0x415651a6 in CallJSNative (cx=0x43a3b7c0, obj=<value optimized out>, fval=<value optimized out>, argc=0, argv=0x0, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/jscntxtinlines.h:225
#8  Invoke (cx=0x43a3b7c0, obj=<value optimized out>, fval=<value optimized out>, argc=0, argv=0x0, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:486
#9  Invoke (cx=0x43a3b7c0, obj=<value optimized out>, fval=<value optimized out>, argc=0, argv=0x0, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:536
#10 js::InvokeGetterOrSetter (cx=0x43a3b7c0, obj=<value optimized out>, fval=<value optimized out>, argc=0, argv=0x0, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:607
#11 0x41639cd0 in js::Shape::get (cx=0x43a3b7c0, obj=..., id=<value optimized out>, getHow=<value optimized out>, vp=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Shape-inl.h:263
#12 NativeGetInline<(js::AllowGC)1> (cx=0x43a3b7c0, obj=..., id=<value optimized out>, getHow=<value optimized out>, vp=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/jsobj.cpp:4025
#13 GetPropertyHelperInline<(js::AllowGC)1> (cx=0x43a3b7c0, obj=..., id=<value optimized out>, getHow=<value optimized out>, vp=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/jsobj.cpp:4200
#14 js::GetPropertyHelper (cx=0x43a3b7c0, obj=..., id=<value optimized out>, getHow=<value optimized out>, vp=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/jsobj.cpp:4209
#15 0x41562a82 in GetPropertyOperation (cx=0x43a3b7c0, state=<value optimized out>) at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:289
#16 Interpret (cx=0x43a3b7c0, state=<value optimized out>) at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:2353
#17 0x41564da8 in RunScript (cx=0x43a3b7c0, thisv=..., fval=<value optimized out>, argc=<value optimized out>, argv=0xbe955970, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:443
#18 Invoke (cx=0x43a3b7c0, thisv=..., fval=<value optimized out>, argc=<value optimized out>, argv=0xbe955970, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:505
#19 Invoke (cx=0x43a3b7c0, thisv=..., fval=<value optimized out>, argc=<value optimized out>, argv=0xbe955970, rval=...)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:536
#20 0x415cbcc4 in JS_CallFunctionValue (cx=0x43a3b7c0, objArg=<value optimized out>, fval=..., argc=1, argv=0xbe955970, rval=0xbe9559c8)
    at /home/hsinyi/src/mozilla/mozilla-central/js/src/jsapi.cpp:5381
---Type <return> to continue, or q <return> to quit---
#21 0x4101c088 in mozilla::dom::EventHandlerNonNull::Call (this=0x404d36b0, cx=0x43a3b7c0, aThisObj=..., event=..., aRv=...)
    at /home/hsinyi/src/mozilla/mozilla-central/objdir-gecko/dom/bindings/EventHandlerBinding.cpp:38
#22 0x40c58518 in Call<nsISupports*> (this=0x452cc340, aEvent=0x43a63fc0) at ../../../dist/include/mozilla/dom/EventHandlerBinding.h:58
#23 nsJSEventListener::HandleEvent (this=0x452cc340, aEvent=0x43a63fc0) at /home/hsinyi/src/mozilla/mozilla-central/dom/src/events/nsJSEventListener.cpp:249
#24 0x40b11530 in nsEventListenerManager::HandleEventSubType (this=<value optimized out>, aListenerStruct=<value optimized out>, aListener=<value optimized out>, 
    aDOMEvent=0x43a63fc0, aCurrentTarget=0x44532640, aPusher=0xbe955de0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventListenerManager.cpp:957
#25 0x40b116a8 in nsEventListenerManager::HandleEventInternal (this=0x453cf510, aPresContext=<value optimized out>, aEvent=0x43a6e300, aDOMEvent=0xbe955e40, 
    aCurrentTarget=0x44532640, aEventStatus=0xbe955e44, aPusher=0xbe955de0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventListenerManager.cpp:1028
#26 0x40b1009a in nsEventListenerManager::HandleEvent (this=<value optimized out>, aVisitor=<value optimized out>, aCd=<value optimized out>, aPusher=0xbe955de0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventListenerManager.h:329
#27 nsEventTargetChainItem::HandleEvent (this=<value optimized out>, aVisitor=<value optimized out>, aCd=<value optimized out>, aPusher=0xbe955de0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventDispatcher.cpp:223
#28 0x40b10154 in nsEventTargetChainItem::HandleEventTargetChain (this=0x446cfc00, aVisitor=..., aCallback=0x0, aCd=..., aPusher=0xbe955de0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventDispatcher.cpp:342
#29 0x40b106fe in nsEventDispatcher::Dispatch (aTarget=<value optimized out>, aPresContext=0x0, aEvent=0x43a6e300, aDOMEvent=<value optimized out>, 
    aEventStatus=0xbe955ed4, aCallback=0x0, aTargets=0x0) at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventDispatcher.cpp:647
#30 0x40b10866 in nsEventDispatcher::DispatchDOMEvent (aTarget=0x44532640, aEvent=0x43a6e300, aDOMEvent=0x43a63fc0, aPresContext=0x0, aEventStatus=0xbe955ed4)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsEventDispatcher.cpp:708
#31 0x40b07a98 in nsDOMEventTargetHelper::DispatchEvent (this=<value optimized out>, aEvent=<value optimized out>, aRetVal=0xbe955eef)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsDOMEventTargetHelper.cpp:257
#32 0x40c382f2 in nsDOMDeviceStorage::DispatchEvent (this=0xbe955ed4, aEvt=0x43a63fc0, aRetval=0xbe955eef)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/devicestorage/nsDeviceStorage.cpp:3381
#33 0x40b0793e in nsDOMEventTargetHelper::DispatchTrustedEvent (this=0x44532640, event=0x43a63fc0)
    at /home/hsinyi/src/mozilla/mozilla-central/content/events/src/nsDOMEventTargetHelper.cpp:280
#34 0x40d3fb3e in mozilla::dom::telephony::Telephony::DispatchCallEvent (this=0x44532640, aType=<value optimized out>, aCall=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:714
#35 0x40d3fc6c in mozilla::dom::telephony::Telephony::NotifyCallsChanged (this=0x0, aCall=0x0)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:191
#36 0x40d3fc8a in mozilla::dom::telephony::Telephony::EnumerationAck::Run (this=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:74
#37 0x411a55b4 in nsThread::ProcessNextEvent (this=0x40416940, mayWait=<value optimized out>, result=0xbe955f87)
    at /home/hsinyi/src/mozilla/mozilla-central/xpcom/threads/nsThread.cpp:622
#38 0x41184cf8 in NS_ProcessNextEvent (thread=0x0, mayWait=false) at /home/hsinyi/src/mozilla/mozilla-central/objdir-gecko/xpcom/build/nsThreadUtils.cpp:238
#39 0x40f35b98 in mozilla::ipc::MessagePump::Run (this=0x40401b80, aDelegate=0xbe956894) at /home/hsinyi/src/mozilla/mozilla-central/ipc/glue/MessagePump.cpp:81
#40 0x40f35c4a in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40401b80, aDelegate=0xbe956894)
    at /home/hsinyi/src/mozilla/mozilla-central/ipc/glue/MessagePump.cpp:234
#41 0x411c1ecc in MessageLoop::RunInternal (this=0x1000000) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:220
---Type <return> to continue, or q <return> to quit---
#42 0x411c1f42 in MessageLoop::RunHandler (this=0xbe956894) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:213
#43 MessageLoop::Run (this=0xbe956894) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:187
#44 0x40eead04 in nsBaseAppShell::Run (this=0x40452d00) at /home/hsinyi/src/mozilla/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:163
#45 0x407c6726 in XRE_RunAppShell () at /home/hsinyi/src/mozilla/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:676
#46 0x40f35c18 in mozilla::ipc::MessagePumpForChildProcess::Run (this=0x40401b80, aDelegate=0xbe956894)
    at /home/hsinyi/src/mozilla/mozilla-central/ipc/glue/MessagePump.cpp:201
#47 0x411c1ecc in MessageLoop::RunInternal (this=0x40452d00) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:220
#48 0x411c1f42 in MessageLoop::RunHandler (this=0xbe956894) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:213
#49 MessageLoop::Run (this=0xbe956894) at /home/hsinyi/src/mozilla/mozilla-central/ipc/chromium/src/base/message_loop.cc:187
#50 0x407c6c32 in XRE_InitChildProcess (aArgc=-1097504220, aArgv=0xbe95699c, aProcess=1078182912)
    at /home/hsinyi/src/mozilla/mozilla-central/toolkit/xre/nsEmbedFunctions.cpp:513
#51 0x00008650 in main (argc=7, argv=0xbe956a24) at /home/hsinyi/src/mozilla/mozilla-central/ipc/app/MozillaRuntimeMain.cpp:85
(gdb) fr 1
#1  0x4117b100 in WrapNewBindingObject<mozilla::dom::telephony::TelephonyCallGroup> (this=<value optimized out>, cx=0x43a3b7c0, scopeObj=..., rval=...)
    at ../../dist/include/mozilla/dom/BindingUtils.h:654
654	    obj = value->WrapObject(cx, scope);
(gdb) p obj
$1 = (struct JSObject *) 0x0

Comment 63

[Boris Zbarsky [:bzbarsky]]

>(gdb) p obj
>$1 = (struct JSObject *) 0x0

Well, right, since the WrapObject call has not happened yet.

Of much more interest is what "value" is.

I'd also like to see your generated code for the TelephonyCallOrTelephonyCallGroupReturnValue (in UnionType.h, presumably).  The way you're using it is weird, but I want to check whether it should lead to actual problems...

Comment 64

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Generated UnionTypes.h

Hi bz,

Here's the generated UnionTypes.h for your reference. Thank you.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Actually, nevermind.  I just tried this with some other WebIDL types and the problem is pretty clear: there is no assignment operator on *ReturnValue union structs, and presumably the compiler generates one that uses memcpy or some such insanity when you do this:

>+    aValue.SetValue() = value;

But copying via memcpy does the wrong thing when the objects being copied have interesting destructor behavior (in this case decrementing refcounts), so I expect you end up with objects being prematurely destroyed.

I filed bug 902485 on making code like this not compile.

You don't need the intermediate "value" object at all.  Your code should look something like this:

   if (mActiveCall) {
     aValue.SetValue().SetAsTelephonyCall() = mActiveCall;
   } else if (mGroup->CallState() == nsITelephonyProvider::CALL_STATE_CONNECTED) {
     aValue.SetValue().SetAsTelephonyCallGroup() = mGroup;
   } else {
     aValue.SetNull();
   }

Comment 66

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Boris Zbarsky (:bz) (vacation Aug 10-19) from comment #65)
> Actually, nevermind.  I just tried this with some other WebIDL types and the
> problem is pretty clear: there is no assignment operator on *ReturnValue
> union structs, and presumably the compiler generates one that uses memcpy or
> some such insanity when you do this:
> 
> >+    aValue.SetValue() = value;
> 
> But copying via memcpy does the wrong thing when the objects being copied
> have interesting destructor behavior (in this case decrementing refcounts),
> so I expect you end up with objects being prematurely destroyed.
> 
> I filed bug 902485 on making code like this not compile.
> 
Oh, yes, everything looks very obvious right now :\ Thank you for the feedback and the bug 902485. :)

Comment 67

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part1 - webidl (v5) - sr=sicking, r=bent

Using Uniontype for telephony.active

Comment 68

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part2 - telephony dom (v7)

Comments addressed & using uniontype

Comment 69

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part3 - RIL impl (v7). r=vicamo

rebased.

Comment 70

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part4 - BT impl (v3). r=gyeh

rebased.

Comment 71

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #59)
> Comment on attachment 781537 [details] [diff] [review]
> patch - part2 - telephony dom (v5)
> 
> Review of attachment 781537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks pretty good!
> 
> ::: dom/telephony/CallsList.cpp
> @@ +19,5 @@
> >    NS_INTERFACE_MAP_ENTRY(nsISupports)
> >  NS_INTERFACE_MAP_END
> >  
> > +CallsList::CallsList(Telephony* aTelephony, TelephonyCallGroup* aGroup)
> > +: mTelephony(aTelephony), mGroup(aGroup)
> 
> Please assert aTelephony.
> 
Already done in another bug.
> @@ +53,1 @@
> >    return call ? call.forget() : nullptr;
> 
> I think I mentioned this in the other bug but forget() works just fine on a
> null pointer. In IndexedGetter() too.
> 
Yes. Revised already.

> ::: dom/telephony/Telephony.cpp
> @@ +154,5 @@
> >  
> >    telephony->mProvider = ril;
> >    telephony->mListener = new Listener(telephony);
> >    telephony->mCallsList = new CallsList(telephony);
> > +  telephony->mGroup = TelephonyCallGroup::Create(telephony);
> 
> Ah, right, since we always create this the webidl needs to not have a '?' on
> the attribute (indicating that it is never null).

You got it!
> 
> @@ +270,5 @@
> >  NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(Telephony,
> >                                                  nsDOMEventTargetHelper)
> >    tmp->mCalls.Clear();
> >    tmp->mActiveCall = nullptr;
> > +  tmp->mGroup = nullptr;
> 
> You also need to traverse mGroup above.
> 

Done.
> @@ +343,5 @@
> >    }
> >  }
> >  
> > +JS::Value
> > +Telephony::GetActive(JSContext* aCx, ErrorResult& aRv) const
> 
> I'd like to see what this looks like with the union.
> 
Revised as you wish.
> @@ -395,4 @@
> >  {
> >    NS_ASSERTION(aCallIndex != kOutgoingPlaceholderCallIndex,
> >                 "This should never happen!");
> > -
> 
> Nit: I'd add this newline back.
> 
Sure.
> @@ +450,5 @@
> > +    }
> > +    if (modifiedCall) {
> > +      modifiedCall->ChangeStateInternal(aCallState, false);
> > +      RemoveCall(modifiedCall);
> > +      mGroup->AddCall(modifiedCall);
> 
> Whether or not we're going to add or remove the call here should depend on
> the call state, right? I mean, if callstate is DISCONNECTING or something
> then we might not want to move it around. What possible values of callstate
> do we need to consider here?
> 

Yes, it depends on the call state as well.

Telephony::CallStateChanged() is a callback called by backend RIL. The only possible call states are: incoming, dialing, alerting, connected, and disconnected, no disconnecting. Only the disconnected state requires being considered in a different way. We simply update its call state when receiving disconnected state change, and no need to move it to anywhere as it's going to be removed anyway.

> @@ +454,5 @@
> > +      mGroup->AddCall(modifiedCall);
> > +      return NS_OK;
> > +    }
> > +
> > +    // Didn't know this call in mCalls or mGroup. Create a new call.
> 
> Nit: s/know/find/
> 
OK.
> @@ +461,5 @@
> > +                            aIsEmergency, mGroup);
> > +    NS_ASSERTION(call, "This should never fail!");
> > +
> > +    return NS_OK;
> > +  } else {
> 
> Nit: No need for else after returns.
> 

Done.

> @@ +582,5 @@
> >      }
> > +
> > +    for (uint32_t index = 0; index < mCalls.Length(); index++) {
> > +      nsRefPtr<TelephonyCall>& tempCall = mCalls[index];
> > +      if (tempCall->CallIndex() == aCallIndex) {
> 
> Hm, you've added quite a few copies of this loop. I think it's probably a
> good idea to make a GetCall(callIndex) helper function on Telephony that
> does this (just like you did on TelephonyCallGroup).
> 
> Another helper function you might want is one like:
> 
>   bool MoveCall(aCallIndex, aIsConference);
> 
> That would take care of moving a call in or out of the group. It could
> return false if the callindex wasn't found.
> 
> Wouldn't that simplify this (and the above) code a bit?
> 

Good point. Thank you.

> ::: dom/telephony/Telephony.h
> @@ +153,5 @@
> >    void
> >    EnqueueEnumerationAck();
> > +
> > +  void
> > +  UpdateActiveCall(TelephonyCall*aCall, bool aIsAdding);
> 
> Nit: Space after *
> 

Done.
> ::: dom/telephony/TelephonyCall.cpp
> @@ +20,5 @@
> >  // static
> >  already_AddRefed<TelephonyCall>
> >  TelephonyCall::Create(Telephony* aTelephony, const nsAString& aNumber,
> > +                      uint16_t aCallState, uint32_t aCallIndex,
> > +                      bool aEmergency, TelephonyCallGroup* aGroup)
> 
> I don't think you need to pass aGroup like this. You can simply pull the
> group off of aTelephony, right? Just pass a boolean isConference maybe?
> 

True!
> ::: dom/telephony/TelephonyCallGroup.cpp
> @@ +5,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +#include "Telephony.h"
> > +#include "TelephonyCall.h"
> > +#include "TelephonyCallGroup.h"
> 
> Please make sure that the header for this cpp is always the first include,
> and clean these up consistent with whatever you decided on the other bug
> 

Made the pattern consistent already. 

> @@ +60,5 @@
> > +uint32_t
> > +TelephonyCallGroup::AddCall(TelephonyCall* aCall)
> > +{
> > +  NS_ASSERTION(!mCalls.Contains(aCall), "Already know about this one!");
> > +  aCall->ChangeGroup(this);
> 
> This will send a notification before the call has been put into mCalls... We
> should make sure that all the lists are updated before we fire any events.
> 

Revised.
> @@ +70,5 @@
> > +uint32_t
> > +TelephonyCallGroup::RemoveCall(TelephonyCall* aCall)
> > +{
> > +  NS_ASSERTION(mCalls.Contains(aCall), "Didn't know about this one!");
> > +  aCall->ChangeGroup(nullptr);
> 
> Here too.
> 

Done.
> @@ +131,5 @@
> > +  }
> > +}
> > +
> > +nsresult
> > +TelephonyCallGroup::NotifyCallsChanged(TelephonyCall* aCall) {
> 
> Nit: { on its own line
> 
OK.
> @@ +146,5 @@
> > +  return DispatchTrustedEvent(event);
> > +}
> > +
> > +bool
> > +TelephonyCallGroup::CanConference(TelephonyCall* aCall, TelephonyCall* aSecondCall)
> 
> Why not make aCall be a ref since it's passed in that way and is never null?

Sure.
> 
> @@ +151,5 @@
> > +{
> > +  MOZ_ASSERT(aCall);
> > +
> > +  if (!aSecondCall) {
> > +    if (!mCalls.Length()) {
> 
> Nit: mCalls.IsEmpty()
> 
Done.
> @@ +188,5 @@
> > +}
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(TelephonyCallGroup,
> > +                                                  nsDOMEventTargetHelper)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCalls)
> 
> Need to also traverse mTelephony and mCallsList. And use the unlink macros
> in unlink.
> 
Done.
> @@ +203,5 @@
> > +
> > +NS_IMPL_ADDREF_INHERITED(TelephonyCallGroup, nsDOMEventTargetHelper)
> > +NS_IMPL_RELEASE_INHERITED(TelephonyCallGroup, nsDOMEventTargetHelper)
> > +
> > +
> 
> Nit: only one newline here.
> 
Ok.

> @@ +221,5 @@
> > +    aRv.Throw(NS_ERROR_NOT_AVAILABLE);
> > +    return;
> > +  }
> > +
> > +  nsresult rv =mTelephony->Provider()->ConferenceCall();
> 
> Nit: Here and below, need a space after =
> 
OK.

> @@ +306,5 @@
> > +  ChangeState(nsITelephonyProvider::CALL_STATE_RESUMING);
> > +}
> > +
> > +void
> > +TelephonyCallGroup::GetState(nsString& aState) const
> 
> This can be inlined
> 
Done.
> ::: dom/telephony/TelephonyCallGroup.h
> @@ +8,5 @@
> > +#define mozilla_dom_telephony_telephonycallgroup_h__
> > +
> > +#include "TelephonyCommon.h"
> > +
> > +#include "nsITelephonyProvider.h"
> 
> This isn't needed, right?
> 
Removed.

> @@ +19,5 @@
> > +#include "CallsList.h"
> > +
> > +BEGIN_TELEPHONY_NAMESPACE
> > +
> > +class TelephonyCallGroup : public nsDOMEventTargetHelper
> 
> Similar comments here as in the other bug, basically clean up the headers
> and forward-declare everything you can, make everything consistent.
> 

Revised.

> @@ +74,5 @@
> > +  already_AddRefed<TelephonyCall>
> > +  GetCall(uint32_t aCallIndex);
> > +
> > +  const nsTArray<nsRefPtr<TelephonyCall> >&
> > +  GetCalls()
> 
> Similar to comments on the other bug, this should be "CallsInternal" or
> something so that we know it will always return something.

Rename 'CallsArray()' as we used in the other bug. 

Thank you for the review!

Comment 72

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 787536 [details] [diff] [review]
patch - part2 - telephony dom (v7)

Ben,

I have addressed your comments. Please help review it again. Thank you :)

Comment 73

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 787536 [details] [diff] [review]
patch - part2 - telephony dom (v7)

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

Telephony::CallStateChanged needs some work I think, please see below:

::: dom/telephony/Telephony.cpp
@@ +250,5 @@
> +      NS_ASSERTION(!mActiveCall, "Already have an active call!");
> +      mActiveCall = aCall;
> +    }
> +  } else {
> +    if (mActiveCall && mActiveCall->CallIndex() == aCall->CallIndex()) {

Nit: else if

@@ +433,5 @@
>  
>  // nsITelephonyListener
>  
>  NS_IMETHODIMP
>  Telephony::CallStateChanged(uint32_t aCallIndex, uint16_t aCallState,

It seems to me that you should be leveraging the MoveCall function here, right?

@@ +471,5 @@
> +                            aIsEmergency, aIsConference);
> +    NS_ASSERTION(call, "This should never fail!");
> +
> +    return NS_OK;
> +  } 

Nit: trailing whitespace here.

::: dom/telephony/Telephony.h
@@ +8,5 @@
>  #define mozilla_dom_telephony_telephony_h__
>  
>  #include "TelephonyCommon.h"
> +
> +#include "nsITelephonyProvider.h"

This doesn't look needed here.

::: dom/telephony/TelephonyCallGroup.cpp
@@ +73,5 @@
> +
> +  nsString stateString;
> +  switch (aCallState) {
> +    case nsITelephonyProvider::CALL_STATE_UNKNOWN:
> +      stateString = EmptyString();

Nit: No need for this, stateString is already empty.

@@ +128,5 @@
> +TelephonyCallGroup::DispatchCallEvent(const nsAString& aType,
> +                                      TelephonyCall* aCall)
> +{
> +  nsRefPtr<CallEvent> event = CallEvent::Create(this, aType, aCall, false, false);
> +

Nit: No need for this newline.

@@ +133,5 @@
> +  return DispatchTrustedEvent(event);
> +}
> +
> +bool
> +TelephonyCallGroup::CanConference(const TelephonyCall& aCall, TelephonyCall* aSecondCall)

Nit: Wrap to 80 chars

@@ +135,5 @@
> +
> +bool
> +TelephonyCallGroup::CanConference(const TelephonyCall& aCall, TelephonyCall* aSecondCall)
> +{
> +  MOZ_ASSERT(aCall);

Asserting a reference is... not needed at least.

@@ +138,5 @@
> +{
> +  MOZ_ASSERT(aCall);
> +
> +  if (!aSecondCall) {
> +    if (mCalls.IsEmpty()) {

Can you just assert this instead of having  this early-bail?

@@ +261,5 @@
> +    NS_WARNING("Hold non-connected call ignored!");
> +    return;
> +  }
> +
> +  nsresult rv =mTelephony->Provider()->HoldConference();

Nit: space after =

::: dom/telephony/TelephonyCallGroup.h
@@ +21,5 @@
> +
> +  nsString mState;
> +
> +  uint16_t mCallState;
> +public:

Nit: newline before public.

@@ +94,5 @@
> +  CallState() const
> +  {
> +    return mCallState;
> +  }
> +private:

Nit: newline before private

Comment 74

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #73)
> Comment on attachment 787536 [details] [diff] [review]
> patch - part2 - telephony dom (v7)
> 
> Review of attachment 787536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Telephony::CallStateChanged needs some work I think, please see below:
> 
> ::: dom/telephony/Telephony.cpp
> @@ +250,5 @@
> > +      NS_ASSERTION(!mActiveCall, "Already have an active call!");
> > +      mActiveCall = aCall;
> > +    }
> > +  } else {
> > +    if (mActiveCall && mActiveCall->CallIndex() == aCall->CallIndex()) {
> 
> Nit: else if
> 
OK
> @@ +433,5 @@
> >  
> >  // nsITelephonyListener
> >  
> >  NS_IMETHODIMP
> >  Telephony::CallStateChanged(uint32_t aCallIndex, uint16_t aCallState,
> 
> It seems to me that you should be leveraging the MoveCall function here,
> right?
> 

I didn't use MoveCall() in CallStateChanged() because here we need to update call state as well, more than what MoveCall() does. If you would like to make CallStateChanged() simpler and clearer, then I think we could do the following.

bool
Telephony::MoveCall(uint32_t aCallIndex, uint16_t aCallState,
                    bool aIsConference)
{
  nsRefPtr<TelephonyCall> call;

  // Move a call to mGroup.
  if (aIsConference) {
    call = GetCall(aCallIndex);
    if (call) {
      call->ChangeStateInternal(aCallState, false);
      RemoveCall(call);
      mGroup->AddCall(call);
      return true;
    }

    return false;
  }

  // Remove a call from mGroup.
  call = mGroup->GetCall(aCallIndex);
  if (call) {
    if (aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
      call->ChangeStateInternal(aCallState, false);
      mGroup->RemoveCall(call);
      AddCall(call);
    } else {
      // The call has been ended already. No need to move it.
      call->ChangeState(aCallIndex);
    }
    return true;
  }

  return false;
}

But please note that some redundant code will be executed when we call this new MoveCall() in EnumerateCallState(). For example, aCallState will never be Disconnected in EnumerateCallState(). Or, we don't really need call->ChangeStateInternal() in EnumerateCallState().

I am not sure this idea is that perfect, but I don't see another better way to have a unifying MoveCall() for both CallStateChanged() and EnumerateCallState(). Any other ideas? Thank you.  


> @@ +471,5 @@
> > +                            aIsEmergency, aIsConference);
> > +    NS_ASSERTION(call, "This should never fail!");
> > +
> > +    return NS_OK;
> > +  } 
> 
> Nit: trailing whitespace here.
> 
Thanks.
> ::: dom/telephony/Telephony.h
> @@ +8,5 @@
> >  #define mozilla_dom_telephony_telephony_h__
> >  
> >  #include "TelephonyCommon.h"
> > +
> > +#include "nsITelephonyProvider.h"
> 
> This doesn't look needed here.
> 

We need this. This has been here before for an inline function. I just moved its position to make the sequence of header files consistent.

> ::: dom/telephony/TelephonyCallGroup.cpp
> @@ +73,5 @@
> > +
> > +  nsString stateString;
> > +  switch (aCallState) {
> > +    case nsITelephonyProvider::CALL_STATE_UNKNOWN:
> > +      stateString = EmptyString();
> 
> Nit: No need for this, stateString is already empty.
OK
> 
> @@ +128,5 @@
> > +TelephonyCallGroup::DispatchCallEvent(const nsAString& aType,
> > +                                      TelephonyCall* aCall)
> > +{
> > +  nsRefPtr<CallEvent> event = CallEvent::Create(this, aType, aCall, false, false);
> > +
> 
> Nit: No need for this newline.
> 
OK
> @@ +133,5 @@
> > +  return DispatchTrustedEvent(event);
> > +}
> > +
> > +bool
> > +TelephonyCallGroup::CanConference(const TelephonyCall& aCall, TelephonyCall* aSecondCall)
> 
> Nit: Wrap to 80 chars
> 
OK.
> @@ +135,5 @@
> > +
> > +bool
> > +TelephonyCallGroup::CanConference(const TelephonyCall& aCall, TelephonyCall* aSecondCall)
> > +{
> > +  MOZ_ASSERT(aCall);
> 
> Asserting a reference is... not needed at least.
> 
Thanks for the catch.
> @@ +138,5 @@
> > +{
> > +  MOZ_ASSERT(aCall);
> > +
> > +  if (!aSecondCall) {
> > +    if (mCalls.IsEmpty()) {
> 
> Can you just assert this instead of having  this early-bail?
> 
Sure.
> @@ +261,5 @@
> > +    NS_WARNING("Hold non-connected call ignored!");
> > +    return;
> > +  }
> > +
> > +  nsresult rv =mTelephony->Provider()->HoldConference();
> 
> Nit: space after =
> 
OK.
> ::: dom/telephony/TelephonyCallGroup.h
> @@ +21,5 @@
> > +
> > +  nsString mState;
> > +
> > +  uint16_t mCallState;
> > +public:
> 
> Nit: newline before public.
> 
OK.
> @@ +94,5 @@
> > +  CallState() const
> > +  {
> > +    return mCallState;
> > +  }
> > +private:
> 
> Nit: newline before private
OK.

Comment 75

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #74)
> (In reply to ben turner [:bent] from comment #73)
> > Comment on attachment 787536 [details] [diff] [review]
> > patch - part2 - telephony dom (v7)
> > 
> > Review of attachment 787536 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Telephony::CallStateChanged needs some work I think, please see below:
> > 
> > ::: dom/telephony/Telephony.cpp
> > @@ +250,5 @@
> > > +      NS_ASSERTION(!mActiveCall, "Already have an active call!");
> > > +      mActiveCall = aCall;
> > > +    }
> > > +  } else {
> > > +    if (mActiveCall && mActiveCall->CallIndex() == aCall->CallIndex()) {
> > 
> > Nit: else if
> > 
> OK
> > @@ +433,5 @@
> > >  
> > >  // nsITelephonyListener
> > >  
> > >  NS_IMETHODIMP
> > >  Telephony::CallStateChanged(uint32_t aCallIndex, uint16_t aCallState,
> > 
> > It seems to me that you should be leveraging the MoveCall function here,
> > right?
> > 
> 
> I didn't use MoveCall() in CallStateChanged() because here we need to update
> call state as well, more than what MoveCall() does. If you would like to
> make CallStateChanged() simpler and clearer, then I think we could do the
> following.
> 
> bool
> Telephony::MoveCall(uint32_t aCallIndex, uint16_t aCallState,
>                     bool aIsConference)
> {
>   nsRefPtr<TelephonyCall> call;
> 
>   // Move a call to mGroup.
>   if (aIsConference) {
>     call = GetCall(aCallIndex);
>     if (call) {
>       call->ChangeStateInternal(aCallState, false);
>       RemoveCall(call);
>       mGroup->AddCall(call);
>       return true;
>     }
> 
>     return false;
>   }
> 
>   // Remove a call from mGroup.
>   call = mGroup->GetCall(aCallIndex);
>   if (call) {
>     if (aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
>       call->ChangeStateInternal(aCallState, false);
>       mGroup->RemoveCall(call);
>       AddCall(call);
>     } else {
>       // The call has been ended already. No need to move it.
>       call->ChangeState(aCallIndex);
>     }
>     return true;
>   }
> 
>   return false;
> }
> 
> But please note that some redundant code will be executed when we call this
> new MoveCall() in EnumerateCallState(). For example, aCallState will never
> be Disconnected in EnumerateCallState(). Or, we don't really need
> call->ChangeStateInternal() in EnumerateCallState().
> 
> I am not sure this idea is that perfect, but I don't see another better way
> to have a unifying MoveCall() for both CallStateChanged() and
> EnumerateCallState(). Any other ideas? Thank you.  
> 
> 
Or we keep the original MoveCall(aCallIndex, aIsConference) function, and introduce one more func(aCallIndex, aCallState, aIsConference) (not having a nice name right now)?

Comment 76

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Third option, but we might need a better name here? Any suggestions?

bool
Telephony::MoveCall(uint32_t aCallIndex, uint16_t aCallState,
                    bool aIsConference, bool aIsCallStateChanged)
{
  nsRefPtr<TelephonyCall> call;

  // Move a call to mGroup.
  if (aIsConference) {
    call = GetCall(aCallIndex);
    if (call) {
      if (aIsCallStateChanged) {
        call->ChangeStateInternal(aCallState, false);
      }
      RemoveCall(call);
      mGroup->AddCall(call);
      return true;
    }

    return false;
  }

  // Remove a call from mGroup.
  call = mGroup->GetCall(aCallIndex);
  if (call) {
    if (aIsCallStateChanged) {
      if (aCallState != nsITelephonyProvider::CALL_STATE_DISCONNECTED) {
        call->ChangeStateInternal(aCallState, false);
        mGroup->RemoveCall(call);
        AddCall(call);
      } else {
        // The call has been ended already. No need to move it.
        call->ChangeState(aCallIndex);
      }
    } else {
      mGroup->RemoveCall(call);
      AddCall(call);
    }
    return true;
  }

  return false;
}

Comment 77

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 787536 [details] [diff] [review]
patch - part2 - telephony dom (v7)

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

::: dom/telephony/Telephony.cpp
@@ +273,5 @@
> +  return call.forget();
> +}
> +
> +bool
> +Telephony::MoveCall(uint32_t aCallIndex, bool aIsConference)

Ok, I take back my earlier suggestion. The MoveCall function doesn't seem to really help us, let's just remove it. Sorry :(

@@ +433,5 @@
>  
>  // nsITelephonyListener
>  
>  NS_IMETHODIMP
>  Telephony::CallStateChanged(uint32_t aCallIndex, uint16_t aCallState,

Ok, I've looked through this a little more and I see what you mean about MoveCall not being a great fit here (hence my earlier suggestion to remove it now). However, I think these changes are wrong.

The goal of this method is to figure out which call has a new state, make sure it's in the right list, and then to dispatch a state change event. Your changes here bail out early for conference calls and never dispatch any events beyond groupchange.

I have a suggestion for what this should look like, I'll attach in a second.

@@ +590,3 @@
>    }
>  
> +  if (MoveCall(aCallIndex, aIsConference)) {

I don't think it makes sense for this to work here. EnumerateCallState shouldn't be called twice for the same callIndex with different aIsConference values, so this should be dead code.

I think you should replace this with a (reversed from above) assertion:

  #ifdef DEBUG
    {
      nsRefPtr<TelephonyCall> badCall = aIsConference ?
                                        GetCall(aCallIndex) :
                                        mGroup->GetCall(aCallIndex);
      NS_ASSERTION(!badCall,
                   "EnumerateCallState called twice for same call index!");
    }
  #endif

Comment 78

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Telephony::CallStateChanged proposal

How does this look for the Telephony::CallStateChanged function? I haven't had a chance to really think it all the way through but I *hope* it works. This way we generate all the appropriate events I think.

Comment 79

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #77)
> Comment on attachment 787536 [details] [diff] [review]
> patch - part2 - telephony dom (v7)
> 
> Review of attachment 787536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/Telephony.cpp
> @@ +273,5 @@
> > +  return call.forget();
> > +}
> > +
> > +bool
> > +Telephony::MoveCall(uint32_t aCallIndex, bool aIsConference)
> 
> Ok, I take back my earlier suggestion. The MoveCall function doesn't seem to
> really help us, let's just remove it. Sorry :(
> 
> @@ +433,5 @@
> >  
> >  // nsITelephonyListener
> >  
> >  NS_IMETHODIMP
> >  Telephony::CallStateChanged(uint32_t aCallIndex, uint16_t aCallState,
> 
> Ok, I've looked through this a little more and I see what you mean about
> MoveCall not being a great fit here (hence my earlier suggestion to remove
> it now). However, I think these changes are wrong.
> 
> The goal of this method is to figure out which call has a new state, make
> sure it's in the right list, and then to dispatch a state change event. Your
> changes here bail out early for conference calls and never dispatch any
> events beyond groupchange.

It's not true. I proposed the method and asked for feedback in comment 31, saying call state would be updated internally here. And when conferenceGroup.state changes, the state of all contained calls changes and event fires at the same time.

> 
> I have a suggestion for what this should look like, I'll attach in a second.
> 

Thanks and will look at it right away!

> @@ +590,3 @@
> >    }
> >  
> > +  if (MoveCall(aCallIndex, aIsConference)) {
> 
> I don't think it makes sense for this to work here. EnumerateCallState
> shouldn't be called twice for the same callIndex with different
> aIsConference values, so this should be dead code.

I am not sure I got your point.

Yes, as you mentioned, EnumerateCallState() shouldn't be called twice for the same callIndex with different aIsConference values. But the above MoveCall() isn't dead code. It's there not because of the case mentioned above, but because that there might be some timing issue between EnumerateCallState() and CallStateChanged(). Please see bug 821559 comment5 for more details.

> 
> I think you should replace this with a (reversed from above) assertion:
> 
>   #ifdef DEBUG
>     {
>       nsRefPtr<TelephonyCall> badCall = aIsConference ?
>                                         GetCall(aCallIndex) :
>                                         mGroup->GetCall(aCallIndex);
>       NS_ASSERTION(!badCall,
>                    "EnumerateCallState called twice for same call index!");
>     }
>   #endif

Comment 80

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #31)
> However, when conferenceGroup.state changes, the state of all contained
> calls changes and event fires at the same time. Is that expected? Thank you.

Ok, sorry for the delay. I tracked down sicking and we chatted about this. Sorry I missed it the first time.

We both agree that, long term, the statechange events should continue to fire on individual TelephonyCall objects even if they are part of a conference. The statechange event of the TelephonyConferenceGroup should fire first though.

Short term, please make sure that the dialer gaia team knows about this issue. If they are ok with not receiving individual call statechange notifications then I think we can fix this later (maybe post-1.2) in a followup bug. If they think it's going to be difficult then I think we should just go ahead and fix it now. Please chat with them and then let's do whatever is easiest for you that also meets their needs.

The only other thing I was surprised by was the MoveCall in EnumerateCallState. Thank you for explaining the race condition to me, I had completely forgotten about it! Please add a comment there that briefly explains the race you're preventing.

Thanks!

Comment 81

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to ben turner [:bent] from comment #80)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #31)
> > However, when conferenceGroup.state changes, the state of all contained
> > calls changes and event fires at the same time. Is that expected? Thank you.
> 
> Ok, sorry for the delay. I tracked down sicking and we chatted about this.
> Sorry I missed it the first time.
> 
> We both agree that, long term, the statechange events should continue to
> fire on individual TelephonyCall objects even if they are part of a
> conference. The statechange event of the TelephonyConferenceGroup should
> fire first though.
> 
> Short term, please make sure that the dialer gaia team knows about this
> issue. If they are ok with not receiving individual call statechange
> notifications then I think we can fix this later (maybe post-1.2) in a
> followup bug. If they think it's going to be difficult then I think we
> should just go ahead and fix it now. Please chat with them and then let's do
> whatever is easiest for you that also meets their needs.
> 

Sorry I may not express myself clearly enough. 

The statechange events of the patch right now not only fire on TelephonyConferenceGroup but also on individual TelephonyCall objects as well. However, all of the events fire at TelephonyCallGroup::ChangeState(), not immediately in Telephony::CallStateChanged(). At that moment, all the conference attendees share the same state for sure. We won't miss any events in the patch, just with different timing. I could change the event firing sequence: first TelephonyCallGroup, then individual TelephonyCall objects in the next revision. But of course, I will discuss with gaia team to make sure it meets their needs. :)

The tricky part is DISCONNECTING state. In TelephonyCall::HangUp(), the internal state will be changed to DISCONNECTING with an event fired and will changed to DISCONNECTED in Telephony::CallStateChanged() when the call is successfully released. 

My question is: should we change the call state in TelephonyCall::HangUp() if it is in conference?

> The only other thing I was surprised by was the MoveCall in
> EnumerateCallState. Thank you for explaining the race condition to me, I had
> completely forgotten about it! Please add a comment there that briefly
> explains the race you're preventing.
> 

No problem!

> Thanks!

Thank you very much for the help :)

Comment 82

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Per discussion with Ben, see comment #80 and comment #81, below I summarize the what the conference API behaviour would look like (a little difference in event firing *sequence* from my previous working patches). 

A) When a call becomes part of a conference call, events will be fired in the following sequence:
a.1) call.ongroupchange
a.2) callgroup.oncallschanged
a.3) telephony.oncallschanged

Once all the calls in the group has been updated,
a.4) callgroup.onstatechange
a.5) call.onstatechange (All the attendees of the conference share the same state. Event fires on every individual TelephonyCall object)

B) We will have the same sequence when a call leaves a conference.

C) When a conference call state changes:
c.1) callgroup.onstatechange
c.2) call.onstatechage (event fires on every individual TelephonyCall object)

D) When user hangs up a call in conference:
d.1) call.onstatechange
d.2) call.ondisconnecting

Once the call is released successfully, the call isn't in conference anymore:
d.3) call.ongroupchange
d.4) callgroup.oncallschange
d.5) call.onstatechange
d.6) call.ondisconnected

Regarding D), I am not sure how much we would like to have all the calls in conference share the same state. States of all call members in conference would NOT be the same in d.1) and d.2). Is that distinctiveness expected? Or should I discard the internal change in d.1) and d.2), simply make that state change only when receiving RIL notifications?

May I ask for your feedback to see if the behaviour meets your need from gaia's view, Anthony and Etienne? Thanks.

Comment 83

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #82)
> Per discussion with Ben, see comment #80 and comment #81, below I summarize
> the what the conference API behaviour would look like (a little difference
> in event firing *sequence* from my previous working patches). 
> 
> A) When a call becomes part of a conference call, events will be fired in
> the following sequence:
> a.1) call.ongroupchange
> a.2) callgroup.oncallschanged
> a.3) telephony.oncallschanged
> 
> Once all the calls in the group has been updated,
> a.4) callgroup.onstatechange
> a.5) call.onstatechange (All the attendees of the conference share the same
> state. Event fires on every individual TelephonyCall object)
> 
> B) We will have the same sequence when a call leaves a conference.

More explanation here:
There are two ways triggering a call leaving a conference, 1) remove the call but it's still connected, and 2) the call is released.

For 1), the state of that call won't change, always "connected". No statechange event fired on that call. But as the conference call state changes to "held", calls in conference change accordingly.

For 2), the state of that call becomes DISCONNECTED. After the call leaves/disconnects, if there are more than 1 call in conference, then no callgroup.onstatechange beyond callgroup.oncallschange. If only one left in conference, then the remaining one will be enforced moved out of the group. In that case, both state and calls of the call group change.

Thank you for your patience with reading the loooong explanation. ;-)

Comment 84

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 787536 [details] [diff] [review]
patch - part2 - telephony dom (v7)

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

Thanks for the explanations! I definitely got confused here. Assuming that the gaia folks are fine with this behavior this looks fine to me with the comment added.

Thanks!

Comment 85

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #81)
> My question is: should we change the call state in TelephonyCall::HangUp()
> if it is in conference?

I think we should, yes.

Comment 86

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Previous try looked good but the recent one is orange :( Seems a crash.
https://tbpl.mozilla.org/?tree=Try&rev=29dd40fdbd04

After making tens of calls, I got a crash happening on TelephonyCallGroup::GetCall. Still debugging... ... 

#0  mozilla::dom::telephony::TelephonyCallGroup::GetCall (this=<value optimized out>, aCallIndex=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/TelephonyCallGroup.cpp:160
#1  0x40d257ca in mozilla::dom::telephony::Telephony::CallStateChanged (this=0x45463ec0, aCallIndex=1, aCallState=1, aNumber=..., aIsActive=true, 
    aIsOutgoing=true, aIsEmergency=false, aIsConference=false) at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:481
#2  0x40d24f6e in mozilla::dom::telephony::Telephony::Listener::CallStateChanged (this=<value optimized out>, callIndex=0, callState=1, number=..., 
    isActive=true, isOutgoing=true, isEmergency=false, isConference=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:46
#3  0x411a94fa in NS_InvokeByIndex (that=<value optimized out>, methodIndex=<value optimized out>, paramCount=<value optimized out>, params=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:164
#4  0x40ddf8e8 in CallMethodHelper::Invoke (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2808
#5  CallMethodHelper::Call (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2148
#6  XPCWrappedNative::CallMethod (ccx=<value optimized out>, mode=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2114
#7  0x40de3c3a in XPC_WN_CallMethod (cx=0x404261c0, argc=7, vp=<value optimized out>)
    at /home/hsinyi/src/mozilla/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1315
#8  0x4155d104 in CallJSNative (cx=0x404261c0, args=..., construct=<value optimized out>) at /home/hsinyi/src/mozilla/mozilla-central/js/src/jscntxtinlines.h:218
#9  Invoke (cx=0x404261c0, args=..., construct=<value optimized out>) at /home/hsinyi/src/mozilla/mozilla-central/js/src/vm/Interpreter.cpp:489
#10 0x415fdb78 in js_fun_apply (cx=0x404261c0, argc=<value optimized out>, vp=0x43a02190) at /home/hsinyi/src/mozilla/mozilla-central/js/src/jsfun.cpp:987
#11 0x4155d104 in CallJSNative (cx=0x404261c0, args=..., construct=<value optimized out>) at /home/hsinyi/src/mozilla/mozilla-central/js/src/jscntxtinlines.h:218

Comment 87

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #82)
> Per discussion with Ben, see comment #80 and comment #81, below I summarize
> the what the conference API behaviour would look like (a little difference
> in event firing *sequence* from my previous working patches). 
> 
> A) When a call becomes part of a conference call, events will be fired in
> the following sequence:
> a.1) call.ongroupchange
> a.2) callgroup.oncallschanged
> a.3) telephony.oncallschanged
> 
> Once all the calls in the group has been updated,
> a.4) callgroup.onstatechange
> a.5) call.onstatechange (All the attendees of the conference share the same
> state. Event fires on every individual TelephonyCall object)
> 
> B) We will have the same sequence when a call leaves a conference.
> 
> C) When a conference call state changes:
> c.1) callgroup.onstatechange
> c.2) call.onstatechage (event fires on every individual TelephonyCall object)
> 
> D) When user hangs up a call in conference:
> d.1) call.onstatechange
> d.2) call.ondisconnecting
> 
> Once the call is released successfully, the call isn't in conference anymore:
> d.3) call.ongroupchange
> d.4) callgroup.oncallschange
> d.5) call.onstatechange
> d.6) call.ondisconnected
> 
> Regarding D), I am not sure how much we would like to have all the calls in
> conference share the same state. States of all call members in conference
> would NOT be the same in d.1) and d.2). Is that distinctiveness expected? Or
> should I discard the internal change in d.1) and d.2), simply make that
> state change only when receiving RIL notifications?
> 
> May I ask for your feedback to see if the behaviour meets your need from
> gaia's view, Anthony and Etienne? Thanks.

From what we have drafted right now we'll use the call.onstatechange (state=disconnected) to trigger the entry in the call log and the callgroup.oncallschanged to update the conference call UI.

So I don't think we'll use d.1/d.2, but they definitely won't hurt.

Comment 88

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #86)
> Previous try looked good but the recent one is orange :( Seems a crash.
> https://tbpl.mozilla.org/?tree=Try&rev=29dd40fdbd04
> 
> After making tens of calls, I got a crash happening on
> TelephonyCallGroup::GetCall. Still debugging... ... 
> 
> #0  mozilla::dom::telephony::TelephonyCallGroup::GetCall (this=<value
> optimized out>, aCallIndex=<value optimized out>)
>     at
> /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/TelephonyCallGroup.
> cpp:160
> #1  0x40d257ca in mozilla::dom::telephony::Telephony::CallStateChanged
> (this=0x45463ec0, aCallIndex=1, aCallState=1, aNumber=..., aIsActive=true, 
>     aIsOutgoing=true, aIsEmergency=false, aIsConference=false) at
> /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:481
> #2  0x40d24f6e in
> mozilla::dom::telephony::Telephony::Listener::CallStateChanged (this=<value
> optimized out>, callIndex=0, callState=1, number=..., 
>     isActive=true, isOutgoing=true, isEmergency=false, isConference=<value
> optimized out>)
>     at
> /home/hsinyi/src/mozilla/mozilla-central/dom/telephony/Telephony.cpp:46

After investigation and debugging, the crash is due to null mGroup caused by telephony::cycleCollection::Unlink. In this case we have a valid this (mTelephony) but null mGroup.

Comment 89

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part1 - webidl (v6) - sr=sicking, r=bent

Change uuid

Comment 90

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part2 - telephony dom (v8). r=bent

Addressed review comments and discussion on comment 82, comment 85 and comment 87.

Thanks for the review and feedbacks!

Comment 91

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part3 - RIL impl (v8). r=vicamo

Rebased only.

Comment 92

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part5 - fix unlink crash

Comment 93

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 792856 [details] [diff] [review]
patch - part5 - fix unlink crash

Cycle collector releases mGroup before mListener::Disconnect(). So it causes a crash when it happened to have mListener->mTelephony->CallStateChanged() while mGroup is already null.

This patch unregisters mListener before destructor to fix the problem. Would you please help review this, Ben? Thank you.

Comment 94

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Try looks good with patch part-5 applied: https://tbpl.mozilla.org/?tree=Try&rev=741bb5f500fa

Comment 95

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 792856 [details] [diff] [review]
patch - part5 - fix unlink crash

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

Hm, CC ordering bites again. Sorry about that.

I think you should keep the gTelephonyList modifications in the destructor. Add rather than call Shutdown() from Navigator I think you can just call Shutdown in *both* the destructor and in Unlink.

Try that to make sure it works though :)

Comment 96

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: patch - part5 - fix unlink crash (v2)

Comment 95 works well by manual and automation marionette tests.

Comment 97

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 792925 [details] [diff] [review]
patch - part5 - fix unlink crash (v2)

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

r=me with this fixed:

::: dom/telephony/Telephony.h
@@ +142,5 @@
>    }
>  
>    virtual void EventListenerAdded(nsIAtom* aType) MOZ_OVERRIDE;
>  
> +  void Shutdown();

This should be private.

Comment 98

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment 97 addressed. Thanks!

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=edfafa887470

Comment 99

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/b2g-inbound/rev/dce012abf2fc
https://hg.mozilla.org/integration/b2g-inbound/rev/9cf19ed96e2a
https://hg.mozilla.org/integration/b2g-inbound/rev/aa452f603515
https://hg.mozilla.org/integration/b2g-inbound/rev/389db83dce26
https://hg.mozilla.org/integration/b2g-inbound/rev/37a1fbdaa344

Comment 100

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks for all your hard work Hsin-Yi! This looks great.

Comment 101

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/dce012abf2fc
https://hg.mozilla.org/mozilla-central/rev/9cf19ed96e2a
https://hg.mozilla.org/mozilla-central/rev/aa452f603515
https://hg.mozilla.org/mozilla-central/rev/389db83dce26
https://hg.mozilla.org/mozilla-central/rev/37a1fbdaa344

No tests?

Comment 102

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #101)
> https://hg.mozilla.org/mozilla-central/rev/dce012abf2fc
> https://hg.mozilla.org/mozilla-central/rev/9cf19ed96e2a
> https://hg.mozilla.org/mozilla-central/rev/aa452f603515
> https://hg.mozilla.org/mozilla-central/rev/389db83dce26
> https://hg.mozilla.org/mozilla-central/rev/37a1fbdaa344
> 
> No tests?

The emulator hasn't support conference call yet. I am working on that (bug 896920).