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

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

unspecified
x86
Mac OS X
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Updated

4 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Updated

4 years ago
Depends on: 1148047
(Assignee)

Updated

4 years ago
See Also: → bug 1145551
(Assignee)

Comment 2

4 years ago
Created attachment 8585484 [details] [diff] [review]
v1
Attachment #8584710 - Attachment is obsolete: true
Attachment #8585484 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Summary: Bug 1145551 follow-up. Platform fix. DTMF should be sent using the active SIM, the given or the default one (in that order) → Bug 1145551 follow-up. Platform fix. DTMF should be sent using the given SIM, the active or the default one (in that order)
(Assignee)

Comment 3

4 years ago
Created attachment 8585485 [details] [diff] [review]
v1
Attachment #8585484 - Attachment is obsolete: true
Attachment #8585484 - Flags: review?(htsai)
Attachment #8585485 - Flags: review?(htsai)
Comment on attachment 8585485 [details] [diff] [review]
v1

Aknow could help this :)
Attachment #8585485 - Flags: review?(htsai) → review?(szchen)
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|.
Attachment #8585485 - Flags: review?(szchen) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 8589097 [details] [diff] [review]
v2

Thanks for your feedback. I think this new version address all your comments.
Attachment #8585485 - Attachment is obsolete: true
Attachment #8589097 - Flags: review?(szchen)
Comment on attachment 8589097 [details] [diff] [review]
v2

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

Looks very good.  Thank you.
Attachment #8589097 - Flags: review?(szchen) → review+
(Assignee)

Comment 8

4 years ago
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?
Flags: needinfo?(szchen)
(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.
Flags: needinfo?(szchen)
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/mozilla-central/rev/84908721cf9e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.