While writing tests against the telephony API, I've found some puzzling behaviour that I thought I'd file: A lone incoming call is apparently not the active one and also not listed in the calls array: navigator.mozTelephony.onincoming = function (event) { incoming = event.call; ok(incoming); is(incoming.number, number); is(incoming.state, "incoming"); //is(incoming, telephony.active); //fails //is(calls.length, 1); //fails //is(calls[0], incoming); //fails }; Also, it seems that the 'disconnected' event is dispatched before 'active' is cleaned up: incoming.ondisconnected = function (event) { is(incoming, event.call); is(incoming.state, "disconnected"); ok(gotDisconnecting); //is(telephony.active, null); //fails is(telephony.calls.length, 0); }; Lastly, it seems the calls array is not updated in place: //ok(telephony.calls === calls); //fails

At this point we should also add test coverage for the "callschanged" event.

Good point, I will consider the "callschanged" event along with this issue.

Not holding the release for this. Nominate for blocking if it becomes a problem.

WIP oncallschanged = function oncallschanged(event) { ... is(incoming, telephony.active); //fails: needs to be fixed ... };

(In reply to Philipp von Weitershausen [:philikon] from comment #0) > > > Lastly, it seems the calls array is not updated in place: > > //ok(telephony.calls === calls); //fails Yes, the cached calls array object is cleared whenever there is new incoming call, new outgoing call or disconnected call. Then the cached calls array object is rebuilt once a page looks for the liveCalls attribute. 'telephony.calls' works fine to get liveCalls attribute. So, I am not sure whether we need to refactor this part to make calls array update in place.

I think sicking and I originally meant for 'active' to be the call that the microphone was going to. So 'incoming' wouldn't be 'active' until you got to 'connected'. Right, sicking?

The in-place calls array is tough to do due to limitations in our bindings generator. We could fix this by not having the DOM objects only generated #ifdef MOZ_B2G_RIL (See bug 717414)

(In reply to ben turner [:bent] from comment #7) > The in-place calls array is tough to do due to limitations in our bindings > generator. We could fix this by not having the DOM objects only generated > #ifdef MOZ_B2G_RIL (See bug 717414) I didn't know that before. Thanks for the information!

(In reply to ben turner [:bent] from comment #6) > I think sicking and I originally meant for 'active' to be the call that the > microphone was going to. So 'incoming' wouldn't be 'active' until you got to > 'connected'. Right, sicking? Correct.

Ben and Jonas, Thanks for the comment. In bug 746496, the logic for active calls in 'Telephony.cpp' was updated with 'ril_worker.js' and 'RadioInterfaceLayer.js'. So I think, according to that, 'incoming' is ought to be 'active' in the situation, right?

WIP

I'm not sure I understand. Surely there is no audio transmitted in either direction while a call is still in the "incomming" state? We don't want any audio from being transmitted until the user has "picked up the phone", right?

(In reply to Jonas Sicking (:sicking) from comment #12) > I'm not sure I understand. Surely there is no audio transmitted in either > direction while a call is still in the "incomming" state? We don't want any > audio from being transmitted until the user has "picked up the phone", right? Yes, you are right. No problem in the 'incoming' case that Philipp mentioned in comment #0. The situation is different in the case of 'outgoing' because the audio is transmitted once we place a call, right? We might want to do a little modification to address 'ok(outgoing, telephony.active); //fails' for a dialing call. Also, it seems more clear that telephony.active is cleaned up right before dispatching 'disconnected' event.

Indeed, for outgoing calls we should probably set .active to refer to the outgoing call which is in the "dialing" state.

Thanks for the comments above. The patch updates 'telephony.active' according to the discussion result. Related modifications are applied for test scripts in Bug 756607. Test for 'oncallschanged' event is covered as suggested.

Comment on attachment 631314 [details] [diff] [review] Patch: update mActiveCall and test scripts Review of attachment 631314 [details] [diff] [review]: ----------------------------------------------------------------- <3 r=me with simplification suggested below. ::: dom/telephony/test/marionette/test_incoming_answer_hangup_oncallschanged.js @@ +45,5 @@ > + "'callschanged' event only for incoming, dialing and disconnected call."); > + isnot(event.call.state, "held", > + "'callschanged' event only for incoming, dialing and disconnected call."); > + isnot(event.call.state, "resuming", > + "'callschanged' event only for incoming, dialing and disconnected call."); You could just do something like: let expected_states = ["incoming", "disconnected"] ok(expected_states.indexOf(event.call.state) != -1, "Unexpected call state: " + event.call.state);

(In reply to Philipp von Weitershausen [:philikon] from comment #16) > Comment on attachment 631314 [details] [diff] [review] > Patch: update mActiveCall and test scripts > > Review of attachment 631314 [details] [diff] [review]: > ----------------------------------------------------------------- > > <3 > > r=me with simplification suggested below. > > ::: > dom/telephony/test/marionette/test_incoming_answer_hangup_oncallschanged.js > @@ +45,5 @@ > > + "'callschanged' event only for incoming, dialing and disconnected call."); > > + isnot(event.call.state, "held", > > + "'callschanged' event only for incoming, dialing and disconnected call."); > > + isnot(event.call.state, "resuming", > > + "'callschanged' event only for incoming, dialing and disconnected call."); > > You could just do something like: > > let expected_states = ["incoming", "disconnected"] > ok(expected_states.indexOf(event.call.state) != -1, > "Unexpected call state: " + event.call.state); Good point! Thanks for the suggestion! I'll update a new patch according to this.

addressed comment #16. Thanks!