Closed Bug 922569 Opened 9 years ago Closed 9 years ago

[wasabi][DTMF] When tone is set to long, long pressing dial keypad during the call won't have long sent tone heard by remote side. Only a short audio tone after release the key.

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 verified)

VERIFIED FIXED
1.2 C4(Nov8)
blocking-b2g koi+
Tracking Status
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- verified

People

(Reporter: echu, Assigned: gsvelto)

References

Details

(Whiteboard: [FT:RIL, ETA:10/29])

Attachments

(3 files, 2 obsolete files)

When tone is set to long, long pressing dial keypad during the call won't have long audio tone heard by remote side. Only a short audio tone after release the key.

* Build Number                
Gaia:     1e9470b9b6df630eddf1c4c8b25b3170ee786b0e
Gecko:    31cc146c80e45d87211c647c5601b3cd7b6234f8
BuildID   20131001031552
Version   26.0a2
http://release1-qa.corp.tpe1.mozilla.com:8080/view/B2G.v1.2.0/job/B2G.v1.2.0.wasabi/

* Reproduce Steps
1. Set audible touch tones to long in sound setting.
2. Established a voice call
3. Long press dial pad from DUT.
4. Listen to the audio tone on remote side.

* Expected Result
Remote side should hear the tone as long as key pressed on DUT.

* Actual Result
Only heard a short tone after user releases key on DUT.

* Occurrence rate
100%
blocking-b2g: --- → koi?
Whiteboard: [FT:RIL]
Component: Gaia::Settings → Gaia::Dialer
Triaged and assigned to Aknow. ETA is provided by Ken.
Assignee: nobody → szchen
blocking-b2g: koi? → koi+
Whiteboard: [FT:RIL] → [FT:RIL, ETA:10/29]
Target Milestone: --- → 1.3 Sprint 4 - 11/8
(In reply to khu from comment #2)
> Triaged and assigned to Aknow. ETA is provided by Ken.

This is a gaia issue, see bug 917196 comment 8. 

Any updates, Gabriele?
Flags: needinfo?(gsvelto)
I wanted to work on this bug this week or early next week at the latest; it shouldn't take long because I already know the code. If it's urgent however feel free to assign it to somebody else.
Flags: needinfo?(gsvelto)
Blocks: 890816
Summary: [wasabi][DTMF] When tone is set to long, long pressing dial keypad during the call won't have long audio tone heard by remote side. Only a short audio tone after release the key. → [wasabi][DTMF] When tone is set to long, long pressing dial keypad during the call won't have long sent tone heard by remote side. Only a short audio tone after release the key.
Assignee: szchen → nobody
Assignee: nobody → gsvelto
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.2 C4(Nov8)
Duplicate of this bug: 924454
I've investigated this a little more and I've found that the issue was introduced by this commit:

https://github.com/mozilla-b2g/gaia/commit/0fda04b390af6d311bc55dcda18f6599f11748a9

It split the timer used for stopping the DTMF tone in two and introduced the problem.
This might be a slightly controversial change because besides refactoring the short-DTMF tone behavior it effectively reverts the changes in bug 829406. There's two reasons for that:

- The first one is that it didn't actually work. The patch in that bug effectively chopped long tones to an event loop turn at most (40-60ms) instead of playing a tone as long as the user pressed the button.

- The second one is that the behavior that bug tried to implement is different than any other phone I tried (an Android 4.1.2 phone and a Nokia S60 one) and somewhat jarring. Let me explain why: the idea in that bug was to send the dialtone only after |touchend| and not send it at all if the user's finger moved outside of the button. This had a few unintended consequences: the first one is that if I left the button then went over it again before lifting my finger the tone would not be sent. The second one is that if I kept a button pressed for say 10 seconds it wouldn't have any effect on the other side as no tone would be sent until I lifted the finger. I know at least two *cough* very close *cough* users who are actually used to keep a button pressed until they hear a response to their action on the other side and both would be very surprised of this behavior.

BTW I was thinking of how this could be covered with unit tests without depending on timing. One possibility would be to mock touch events and then spy setTimeout and the other methods to see if they were called with the right values and in the right order. This should be timing-independent but still check the overall consistency. Since it's a considerable amount of work and there are no unit-tests covering that functionality yet I'd like to address it in a follow-up.
Attachment #817923 - Flags: review?(etienne)
(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> if I kept a button pressed for say 10 seconds it wouldn't have any
> effect on the other side as no tone would be sent until I lifted the finger.
> I know at least two *cough* very close *cough* users who are actually used
> to keep a button pressed until they hear a response to their action on the
> other side and both would be very surprised of this behavior.

Don't have strong opinion on this one, but I've heard the API might end up enforcing this behavior
(ie. having a play(tone, duration) instead of playtone/stoptone).
(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> BTW I was thinking of how this could be covered with unit tests without
> depending on timing. One possibility would be to mock touch events and then
> spy setTimeout and the other methods to see if they were called with the
> right values and in the right order. This should be timing-independent but
> still check the overall consistency. Since it's a considerable amount of
> work and there are no unit-tests covering that functionality yet I'd like to
> address it in a follow-up.

Touch events? Timing-related code?
You should find a lot of inspiration in my working branch for edge gestures [1].
Spoiler: sinonJS' fake clock is awesome :)

[1] https://github.com/etiennesegonzac/gaia/blob/bug-919492-app-to-app-edge-gesture/apps/system/test/unit/edge_swipe_detector_test.js
[2] http://sinonjs.org/docs/#clock
Comment on attachment 817923 [details] [diff] [review]
[PATCH] Fix DTMF tone length when in long mode

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

Tests could certainly make a controversial patch less controversial :)

That said:
- the audio (non-dtmf) code will move to the WebAudio API soon so we don't want to invest time testing the old mozaudio code
- if we can easily get some coverage that's a big win, but I wouldn't block on it

::: apps/communications/dialer/js/keypad.js
@@ +447,3 @@
>        TonePlayer.stop();
> +      this._stopDtmfTone();
> +    }).bind(this), 0);

nit: we usually don't pass the 0 at all in this case
Attachment #817923 - Flags: review?(etienne)
Blocks: 917196
(In reply to Etienne Segonzac (:etienne) from comment #11)
> Tests could certainly make a controversial patch less controversial :)

OK, I'll try and cook up one :-)

> - the audio (non-dtmf) code will move to the WebAudio API soon so we don't
> want to invest time testing the old mozaudio code

I'm glad to hear that because I have to fix that part too in bug 917196.
needinfo Gabriele here on next steps ?
Flags: needinfo?(gsvelto)
(In reply to bhavana bajaj [:bajaj] from comment #13)
> needinfo Gabriele here on next steps ?

I'm working on the unit-tests as mentioned in comment 12. Since there's no unit tests involving touch events it's taking me a while but I still hope to finish this and bug 917196 during this week.
Flags: needinfo?(gsvelto)
I've tried adding unit-tests mocking the touch events but the endeavor proved to be... complicated. Instead I decided to refactor the event-handler so that for every type of event I've got a separate method which can be easily tested in isolation.

I've written a bunch of tests in this patch to illustrate my approach. If you think it's good enough I'll finish it with extra tests to cover the following situations:

- timing tests using sinon's fake timers

- touchmove tests done by mocking document.elementFromPoint() with sinon's yield function
Attachment #817923 - Attachment is obsolete: true
Attachment #824134 - Flags: feedback?(etienne)
Comment on attachment 824134 [details] [diff] [review]
[PATCH v2] Fix DTMF tone length when in long mode

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

+1 on the refactoring, and yes, calling |_touch*| in the test is a compromise we can make :)

I can guarantee that using fake timers will be more fun than the touch handling, and it will cover an important part of the code!

::: apps/communications/dialer/test/unit/keypad_test.js
@@ +142,5 @@
> +      });
> +
> +      teardown(function() {
> +        MockMozTelephony.mTeardown();
> +        MockSettingsListener.mTeardown();

the mocksHelper should do that for you already (except for MockMozTelephony)

::: apps/communications/dialer/test/unit/mock_calls_handler.js
@@ +11,5 @@
>      this.mLastEntryAdded = entry;
>    },
>  
> +  get activeCall() {
> +    return navigator.mozTelephony.active;

I'd like to keep mocks independents.
we can probably return an |mActiveCall| here, reseted in the mTeardown.
Attachment #824134 - Flags: feedback?(etienne) → feedback+
Quick heads up, bug 927852 is probably going to land soon, hope the merge won't be too much of a pain.
(In reply to Etienne Segonzac (:etienne) from comment #17)
> Quick heads up, bug 927852 is probably going to land soon, hope the merge
> won't be too much of a pain.

Thanks for the heads-up. It shouldn't be a problem and in fact it's good that this lands before I start tackling  	bug 917196 which will involve the audible tones.
This addresses all the notes in comment 16 and adds additional tests covering DTMF tone-length both in long and short mode.
Attachment #824134 - Attachment is obsolete: true
Attachment #826018 - Flags: review?(etienne)
Comment on attachment 826018 [details] [diff] [review]
[PATCH v3] Fix DTMF tone length when in long mode

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

r=me!
Sorry for the delay!

It's probably good to land this patch before the WebAudio one to prevent uplift hell.
Attachment #826018 - Flags: review?(etienne) → review+
Thanks for the review! I'm waiting for Travis to turn green before landing:

https://travis-ci.org/mozilla-b2g/gaia/builds/13625830
Landed on gaia/master 2658b8d7e10ca333498439c3299a44259dfea0cf.

The patch applies cleanly to v1.2 but I want to run a few tests before landing it there too.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Testing went fine, landed on gaia/v1.2 18226f516a196119b3960990dcf943b1d1428297.
Verified on Wasabi v1.2

Gaia:     b401bfed469e1d6db24e910f78732bad32843e8a
Gecko:    134c78dbcfc2f7c1bb665f6da46581f4785ce140
BuildID   20131108031537
Version   26.0

Hi Gabriele, is this patched also landed on master?
Status: RESOLVED → VERIFIED
Flags: needinfo?(gsvelto)
(In reply to Enpei from comment #25)
> Hi Gabriele, is this patched also landed on master?

Yes, the commit hash is 2658b8d7e10ca333498439c3299a44259dfea0cf
Flags: needinfo?(gsvelto)
Duplicate of this bug: 972756
You need to log in before you can comment on or make changes to this bug.