1148036 - Bug 1145551 follow-up. Platform fix. DTMF should be sent using the given SIM, the active or the default one (in that order)

Status: RESOLVED FIXED

Description

[Fernando Jiménez Moreno [:ferjm]]

See Bug 1145551

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Created attachment 8584710 [details] [diff] [review]
v1

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Created attachment 8585484 [details] [diff] [review]
v1

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Created attachment 8585485 [details] [diff] [review]
v1

Comment 4

[Hsin-Yi Tsai [:hsinyi]]

Comment on attachment 8585485 [details] [diff] [review]
v1

Aknow could help this :)

Comment 5

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

Comment on attachment 8585485 [details] [diff] [review]
v1

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

Please add a test like testDtmfDsds, but at that time serviceId is not provided in sendTone.
We should verfify gecko is correctly send the request to the sim that has the active call.

::: dom/telephony/Telephony.cpp
@@ +182,5 @@
> +    for (uint32_t i = 0; i < mCalls.Length(); i++) {
> +      if (IsActiveState(mCalls[i]->CallState())) {
> +        return mCalls[i]->mServiceId;
> +      }
> +    }

In telephony, conference call is not listed in mCalls.  You still need to check |mGroup.CallsArray()|.
Or, you may use Telephony::GetActive(...) to retrieve an active call or active conference group. 
One of the facts is that all calls in conference group have the same state and it is equal to mGroup.CallState()

::: dom/telephony/test/marionette/manifest.ini
@@ +51,5 @@
>  [test_ready.js]
>  [test_redundant_operations.js]
>  [test_swap_held_and_active.js]
>  [test_temporary_clir.js]
> +[test_dtmf.js]

Let's sort the cases by name.

::: dom/telephony/test/marionette/test_dtmf.js
@@ +4,5 @@
> +MARIONETTE_TIMEOUT = 60000;
> +MARIONETTE_HEAD_JS = 'head.js';
> +
> +function muxModem(id) {
> +  let deferred = Promise.defer();

How about use the same style for creating promise?
return new Promise((resolve, reject) => {
...
});

@@ +16,5 @@
> +
> +function testDtmfNoActiveCall() {
> +  log("= testDtmfNoActiveCall =");
> +  return new Promise((resolve, reject) => {
> +    gSendTone('1', 5, 0).then(reject, resolve);

It takes me some time to figure out why do you reverse the reject and resolve cases.
So I think we could add some logs for both cases.  Then, the code could be easier to undestand.

@@ +40,5 @@
> +    .then(() => gSendTone('1', 5, serviceId))
> +    .then(() => emulator.runCmd("modem dtmf"))
> +    .then((tone) => {
> +      is(tone, '1,OK', 'Sent tone is 1');
> +      gSendTone('1', 5, otherServiceId).catch((e) => {

Why not put this part out of callback? So both two gSendTone() calls could be placed in the same hierarchy.

@@ +43,5 @@
> +      is(tone, '1,OK', 'Sent tone is 1');
> +      gSendTone('1', 5, otherServiceId).catch((e) => {
> +        log('Expected Error ' + e);
> +        return gRemoteHangUp(outCall);
> +      });

No return in this function.
The callback will be treated as resolved and immediately go to next stage.

@@ +56,5 @@
> +  testDtmfNoActiveCall()
> +    .then(testDtmfDsds)
> +    .then(emulator.runCmd("modem dtmf reset"))
> +    .then(finish)
> +    .catch(error => ok(false, "Promise reject: " + error));

We usually do |.catch| before |finish|.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Created attachment 8589097 [details] [diff] [review]
v2

Thanks for your feedback. I think this new version address all your comments.

Comment 7

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

Comment on attachment 8589097 [details] [diff] [review]
v2

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

Looks very good.  Thank you.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

I think I can't land this one until bug 1148047 changes are applied on the emulator that runs the tbpl tests. Is there any way to bump the version of the tbpl emulator?

Comment 9

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> I think I can't land this one until bug 1148047 changes are applied on the
> emulator that runs the tbpl tests. Is there any way to bump the version of
> the tbpl emulator?

It will bump the emulator version automatically but the process may take a little time.  So, I usually wait for an extra day.  Then check <gecko>/b2g/config/emulator/source.xml to see the exact version of emulator on tbpl.

From this line.

<project name="platform_external_qemu" path="external/qemu" remote="b2g" revision="a89cebcccc1e067ebdb71a93194f4ee79d71bd69"/>

It shows that your change on emulator is already applied and you can safely land this bug.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Thanks! Try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff2ce4fcbfd

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84908721cf9e

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/mozilla-central/rev/84908721cf9e