Last Comment Bug 757587 - WebTelephony: investigate .active and .calls behaviour
: WebTelephony: investigate .active and .calls behaviour
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 13:12 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-06-14 02:45 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch: test script for 'callschanged' event (5.80 KB, patch)
2012-06-05 20:30 PDT, Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi]
no flags Details | Diff | Review
Patch: test scripts for incoming and outgoing calls on 'callschanged' event (9.74 KB, patch)
2012-06-06 03:40 PDT, Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi]
no flags Details | Diff | Review
Patch: update mActiveCall and test scripts (16.87 KB, patch)
2012-06-08 02:25 PDT, Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi]
philipp: review+
Details | Diff | Review
patch (v2) (15.24 KB, patch)
2012-06-13 03:44 PDT, Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi]
no flags Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-05-22 13:12:45 PDT
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] 2012-05-29 14:26:33 PDT
At this point we should also add test coverage for the "callschanged" event.
Comment 2 Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi] 2012-05-29 18:59:17 PDT
Good point, I will consider the "callschanged" event along with this issue.
Comment 3 Dietrich Ayala (:dietrich) 2012-05-31 20:46:22 PDT
Not holding the release for this. Nominate for blocking if it becomes a problem.
Comment 4 Hsin-Yi Tsai (OOO June 20 ~ June 24) [:hsinyi] 2012-06-05 20:30:12 PDT
Created attachment 630429 [details] [diff] [review]
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-05 20:46:30 PDT
(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!) 2012-06-05 21:44:33 PDT
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!) 2012-06-05 21:47:06 PDT
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-05 21:57:32 PDT
(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) PTO Until July 5th 2012-06-05 22:07:38 PDT
(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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-06 02:28:58 PDT
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-06 03:40:59 PDT
Created attachment 630494 [details] [diff] [review]
Patch: test scripts for incoming and outgoing calls on 'callschanged' event

WIP
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-06 17:45:58 PDT
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-06 21:35:04 PDT
(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) PTO Until July 5th 2012-06-07 00:13:50 PDT
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-08 02:25:47 PDT
Created attachment 631314 [details] [diff] [review]
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] 2012-06-11 15:23:32 PDT
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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-11 18:56:09 PDT
(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 (OOO June 20 ~ June 24) [:hsinyi] 2012-06-13 03:44:57 PDT
Created attachment 632627 [details] [diff] [review]
patch (v2)

addressed comment #16. Thanks!
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-06-13 18:17:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d89a0b44df52
Comment 20 Ed Morley [:emorley] 2012-06-14 02:45:23 PDT
https://hg.mozilla.org/mozilla-central/rev/d89a0b44df52

Note You need to log in before you can comment on or make changes to this bug.