Closed Bug 1148036 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → ferjmoreno
Depends on: 1148047
Attached patch v1 (obsolete) — Splinter Review
See Also: → 1145551
Attached patch v1 (obsolete) — Splinter Review
Attachment #8584710 - Attachment is obsolete: true
Attachment #8585484 - Flags: review?(htsai)
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)
Attached patch v1 (obsolete) — Splinter Review
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-
Attached patch v2Splinter Review
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+
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: