757587 - WebTelephony: investigate .active and .calls behaviour

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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

Comment 1

[Philipp von Weitershausen [:philikon]]

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

Comment 2

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

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

Comment 3

[Dietrich Ayala (:dietrich)]

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

Comment 4

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

Attachment: Patch: test script for 'callschanged' event

WIP 

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

Comment 5

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

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

Comment 6

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

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?

Comment 7

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

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)

Comment 8

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

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

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 10

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

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?

Comment 11

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

Attachment: Patch: test scripts for incoming and outgoing calls on 'callschanged' event

WIP

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 13

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

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 15

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

Attachment: Patch: update mActiveCall and test scripts

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 16

[Philipp von Weitershausen [:philikon]]

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

Comment 17

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

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

Comment 18

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

Attachment: patch (v2)

addressed comment #16. Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d89a0b44df52

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d89a0b44df52