1137093 - B2G RIL: move telephony call related handling out of ril_worker

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

I'd like to move telephony call related handling out of ril_worker, ril_worker should only be responsible of passing ril request results or unsolicited messages to |RadioInterface|.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 01: Pass number instead of callIndex in notifySupplementaryService

In later patch, we'll remove currentCalls in ril_worker.  Therefore, the function could not provide a callIndex.

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 02: Move from ril_worker to TelephonyService: hangup and reject

Mark hangUpLocal in TelephonyService.js.  The flag will be referred in the later patch.

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 03: Move from ril_worker to TelephonyService: answer

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 04: Refactor _switchActiveCall

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 05: Refactor _dialInCallMMI

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 06: Refactor _isActive

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 07: Use notifyCurrentCalls (idl)

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08: Use notifyCurrentCalls (ril)

Sorry, the patch is kind of huge.  I've already do my best to separate it (so you got part 1 to 7) but it contains too much dependency....

Comment 9

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09: Modify test case

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10: Turn on radio for emergency call in TelephonyService

Comment 11

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

Comment on attachment 8573070 [details] [diff] [review]
Part 03: Move from ril_worker to TelephonyService: answer

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

::: dom/telephony/gonk/TelephonyService.js
@@ +953,5 @@
> +    }
> +
> +    let callNum = Object.keys(this._currentCalls[aClientId]).length;
> +    if (callNum !== 1) {
> +      this._switchActiveCall(aClientId, aCallback);

_switchActiveCall function is actually defined in part 4.

Comment 12

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

Comment on attachment 8573073 [details] [diff] [review]
Part 06: Refactor _isActive

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

This makes more sense!

Comment 13

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

Comment on attachment 8573079 [details] [diff] [review]
Part 08: Use notifyCurrentCalls (ril)

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

::: dom/system/gonk/ril_worker.js
@@ +4797,3 @@
>      return;
>    }
>  

If options.rilRequestError is true and the retry count exceeds the maximum, we should send callback back to TelephonyService via |this.sendChromeMessage| here. Otherwise, there will be a hanging callback.

::: dom/telephony/gonk/TelephonyService.js
@@ +415,3 @@
>  
> +      for (let i in response.calls) {
> +        let call = this._currentCalls[aClientId][call.callIndex] =

s/call.callIndex/i/

@@ +748,5 @@
> +      childCall.parentId = CDMA_FIRST_CALL_INDEX;
> +      childCall.state = nsITelephonyService.CALL_STATE_DIALING;
> +      childCall.number = aNumber;
> +      childCall.isOutgoing = true;
> +      childCall.isEmergency = false;

Forgot to determine the flag .isEmergency based on gDialNumberUtils?

@@ +781,5 @@
> +        });
> +      } else {
> +        this._ongoingDial = {
> +          clientId: aClientId,
> +          options: aOptions,

options is never referenced. How about remove this?

@@ +1346,2 @@
>      aCall.state = nsITelephonyService.CALL_STATE_DISCONNECTED;
>      aCall.isEmergency = gDialNumberUtils.isEmergency(aCall.number);

It looks to me that the line of |aCall.isEmergency = ...| isn't necessary after the refactoring. Please double-check and address if needed, thanks.

@@ +1445,4 @@
>  
> +  _handleCurrentCalls: function(aClientId, aCalls,
> +                                aFailCause = RIL.GECKO_CALL_ERROR_NORMAL_CALL_CLEARING) {
> +    if (DEBUG) debug("handleCurrentCalls: " + JSON.stringify(aCalls));

I think it will be helpful to print out "aFailCause" as well. Please add it.

Comment 14

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

Comment on attachment 8573080 [details] [diff] [review]
Part 09: Modify test case

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

::: dom/telephony/test/marionette/test_outgoing_badNumber.js
@@ +15,5 @@
>    // from network side.
>    return telephony.dial(number).then(call => {
>      outCall = call;
>      ok(outCall);
> +    is(outCall.id.number, "");  // Emulator returns empty number for this call.

I don't expect to see marionette-webapi test modification in this bug. Is there recent change on emulator? Why do we need this change in this bug? Please explain more, thanks.

Comment 15

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

Comment on attachment 8573081 [details] [diff] [review]
Part 10: Turn on radio for emergency call in TelephonyService

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

::: dom/telephony/gonk/TelephonyService.js
@@ +188,5 @@
> +function MobileConnectionListener() {}
> +MobileConnectionListener.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIMobileConnectionListener]),
> +
> +  // nsIMobileCOnnectionListener

nit: s/COnnection/Connection/

@@ +311,5 @@
>    },
>  
> +  _isVoiceAvailable: function(aClientId) {
> +    let voice = gGonkMobileConnectionService.getItemByServiceId(aClientId).voice;
> +    return voice.connected || voice.emergencyCallsOnly;

Oops, sorry this is a flaw in the original code. :(

_isVoiceAvailable always returns true since [1].
[1] https://hg.mozilla.org/mozilla-central/annotate/eab4a81e4457/dom/system/gonk/ril_worker.js#l3508

@@ +716,5 @@
> +        // Turn on radio.
> +        connection.setRadioEnabled(true, {
> +          QueryInterface: XPCOMUtils.generateQI([Ci.nsIMobileConnectionCallback]),
> +          notifySuccess: () => {
> +            if (this._isVoiceAvailable(aClientId)) {

The original code was somehow mess that we handled cachedDialRequest in several places with inconsistent conditions. However, as far as I can tell, what we need here is to check radioTech and signalStrength.
Dialing an emergency number out can't be guarded by |voice.connected| if you think about the case with no sim.

@@ +722,5 @@
> +            } else {
> +              let listener = new MobileConnectionListener();
> +
> +              let tryToDial = () => {
> +                if (this._isVoiceAvailable(aClientId)) {

ditto.

@@ +728,5 @@
> +                  connection.unregisterListener(listener);
> +                }
> +              };
> +
> +              listener.notifyRadioStateChanged = tryToDial;

It's no harm but RadioOn isn't enough to dial out but voice signal and radio tech. So, line 733 is the one we should rely on.

We need to be very careful in this part. In addition to emulator marionette-webapi tests, please have manual tests run on devices.

Comment 16

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8573080 [details] [diff] [review]
> Part 09: Modify test case
> 
> Review of attachment 8573080 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/test/marionette/test_outgoing_badNumber.js
> @@ +15,5 @@
> >    // from network side.
> >    return telephony.dial(number).then(call => {
> >      outCall = call;
> >      ok(outCall);
> > +    is(outCall.id.number, "");  // Emulator returns empty number for this call.
> 
> I don't expect to see marionette-webapi test modification in this bug. Is
> there recent change on emulator? Why do we need this change in this bug?
> Please explain more, thanks.

The number of dial call is came from here
https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#752

Originally, in ril_worker, we set the number as the value we got from dial option (pendingMO option).
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#5257
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js#3684

In part8, I did some modification. We handle this._ongoingDial() when we got all the currentCalls from ril_worker. At that time, we already get the correct phone number from modem. So, I decide to use this value instead of the value from dial option.

And, in current emulator, the number of this call is an empty string.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08#2: Use notifyCurrentCalls (ril)

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#2: Turn on radio for emergency call in TelephonyService

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

> The original code was somehow mess that we handled cachedDialRequest in
> several places with inconsistent conditions. However, as far as I can tell,
> what we need here is to check radioTech and signalStrength.
> Dialing an emergency number out can't be guarded by |voice.connected| if you
> think about the case with no sim.

There is a problem. MobileConnection will not update the signalStrength if the state of voice is not in "registered" [1].  However, at that situation, dialing an emergency call should be okay.

So, I think checking radioState is enough.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/mobileconnection/gonk/MobileConnectionService.js?from=MobileConnectionService.js#581

> @@ +722,5 @@
> > +            } else {
> > +              let listener = new MobileConnectionListener();
> > +
> > +              let tryToDial = () => {
> > +                if (this._isVoiceAvailable(aClientId)) {
> 
> ditto.

Comment 20

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#3: Turn on radio for emergency call in TelephonyService

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Szu-Yu Chen [:aknow] from comment #20)
> Created attachment 8575875 [details] [diff] [review]
> Part 10#3: Turn on radio for emergency call in TelephonyService

Another way is updating signalStrength even when voice state is not registered

Comment 22

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Szu-Yu Chen [:aknow] from comment #21)
> (In reply to Szu-Yu Chen [:aknow] from comment #20)
> > Created attachment 8575875 [details] [diff] [review]
> > Part 10#3: Turn on radio for emergency call in TelephonyService
> 
> Another way is updating signalStrength even when voice state is not
> registered

I think we should fix the signalStrength problem. Then in TelephonyService, use signalStrength itself as the condition.

I noticed that on flame, "voice state change" come back earlier than "radio state change". So the above way is more precise and fast in some situation.

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#4: Turn on radio for emergency call in TelephonyService

Signla strength is not reliable when there is no sim card. Therefore, I decided to use radio state as the condition.

Comment 24

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

Comment on attachment 8573080 [details] [diff] [review]
Part 09: Modify test case

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

Thanks for the explanation.

Comment 25

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

Comment on attachment 8575844 [details] [diff] [review]
Part 08#2: Use notifyCurrentCalls (ril)

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

I love the revision!

Comment 26

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

Comment on attachment 8576519 [details] [diff] [review]
Part 10#4: Turn on radio for emergency call in TelephonyService

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

Thank you!

Comment 27

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11: Fix xpcshell test

Modify test case:
In part 1, the format of suppSvcNotification is changed.

Modify ril_worker.js
In some test case [1], it checks the postMessage() from ril_worker to radioInterface.  However, in the previous revision, when worker is initiated, we'll immediately send a message "currentCalls" that cause the check running on a wrong message.  The modification avoid the problem.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/tests/test_ril_worker_barring_password.js#45

Comment 28

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8577120 [details] [diff] [review]
Part 11: Fix xpcshell test

something wrong

Comment 29

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11#2: Fix xpcshell test

In previous revision, I sent a message (GET_CURRENT_CALL) to ril in initRILState().  However, at that time (very beginning), postRILMessage is not yet defined so it cause an error.

Comment 30

[Szu-Yu Chen [:aknow] (inactive)]

try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f59d1dbdf4

The failed tests are not related to this one.

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=7690d9e5ba8a

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e6bbb392c400
https://hg.mozilla.org/mozilla-central/rev/3998492533ff
https://hg.mozilla.org/mozilla-central/rev/efc84fd8f3d2
https://hg.mozilla.org/mozilla-central/rev/dc09457ca4d1
https://hg.mozilla.org/mozilla-central/rev/51e35cce870b
https://hg.mozilla.org/mozilla-central/rev/f85ea6b494af
https://hg.mozilla.org/mozilla-central/rev/614fb637c4c9
https://hg.mozilla.org/mozilla-central/rev/f55f46bfd8a2
https://hg.mozilla.org/mozilla-central/rev/00d5d6ee14a0
https://hg.mozilla.org/mozilla-central/rev/db752db6d34a
https://hg.mozilla.org/mozilla-central/rev/7690d9e5ba8a