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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
Attachments
(1 file, 3 obsolete files)
9.46 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
See Bug 1145551
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8584710 -
Attachment is obsolete: true
Attachment #8585484 -
Flags: review?(htsai)
Assignee | ||
Updated•10 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•10 years ago
|
||
Attachment #8585484 -
Attachment is obsolete: true
Attachment #8585484 -
Flags: review?(htsai)
Attachment #8585485 -
Flags: review?(htsai)
Comment 4•10 years ago
|
||
Comment on attachment 8585485 [details] [diff] [review]
v1
Aknow could help this :)
Attachment #8585485 -
Flags: review?(htsai) → review?(szchen)
Comment 5•10 years ago
|
||
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•10 years ago
|
||
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 7•10 years ago
|
||
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•10 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)
Comment 9•10 years ago
|
||
(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 10•10 years ago
|
||
Thanks! Try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=aff2ce4fcbfd
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
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.
Description
•