922569 - [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.

Status: VERIFIED FIXED

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

Description

[Enpei]

Attachment: time stamp: 18:32, 10/1.

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%

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Root cause is as bug 917196 comment 8

Comment 2

[Kai-Chih Hu [:khu]]

Triaged and assigned to Aknow. ETA is provided by Ken.

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 4

[Gabriele Svelto [: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.

Comment 6

[Gabriele Svelto [:gsvelto]]

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.

Comment 7

[Gabriele Svelto [:gsvelto]]

Woops, wrong commit, I meant this one:

https://github.com/mozilla-b2g/gaia/commit/8337493c1f50e0c9175bc2f8c0054b2f74732cbf

Comment 8

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Fix DTMF tone length when in long mode

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.

Comment 9

[Etienne Segonzac (: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).

Comment 10

[Etienne Segonzac (:etienne)]

(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 11

[Etienne Segonzac (:etienne)]

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

Comment 12

[Gabriele Svelto [:gsvelto]]

(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.

Comment 13

[bhavana bajaj [:bajaj]]

needinfo Gabriele here on next steps ?

Comment 14

[Gabriele Svelto [: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.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Fix DTMF tone length when in long mode

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

Comment 16

[Etienne Segonzac (: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.

Comment 17

[Etienne Segonzac (:etienne)]

Quick heads up, bug 927852 is probably going to land soon, hope the merge won't be too much of a pain.

Comment 18

[Gabriele Svelto [:gsvelto]]

(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.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Fix DTMF tone length when in long mode

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Fix DTMF tone length when in long mode

This addresses all the notes in comment 16 and adds additional tests covering DTMF tone-length both in long and short mode.

Comment 21

[Etienne Segonzac (: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.

Comment 22

[Gabriele Svelto [:gsvelto]]

Thanks for the review! I'm waiting for Travis to turn green before landing:

https://travis-ci.org/mozilla-b2g/gaia/builds/13625830

Comment 23

[Gabriele Svelto [:gsvelto]]

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.

Comment 24

[Gabriele Svelto [:gsvelto]]

Testing went fine, landed on gaia/v1.2 18226f516a196119b3960990dcf943b1d1428297.

Comment 25

[Enpei]

Verified on Wasabi v1.2

Gaia:     b401bfed469e1d6db24e910f78732bad32843e8a
Gecko:    134c78dbcfc2f7c1bb665f6da46581f4785ce140
BuildID   20131108031537
Version   26.0

Hi Gabriele, is this patched also landed on master?

Comment 26

[Gabriele Svelto [:gsvelto]]

(In reply to Enpei from comment #25)
> Hi Gabriele, is this patched also landed on master?

Yes, the commit hash is 2658b8d7e10ca333498439c3299a44259dfea0cf