1092362 - Make the dialer use the `notification' channel for playing the touch tones

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #834530 +++

The dialer is currently using the `content' channel to play touch-tones as a workaround for bug 1092346. As soon as that bug gets fixed we should make it use the `notification' channel again.

Comment 2

[Johan Lorenzo [:jlorenzo] - On PTO, back on May 13th]

Carrying over the nomination from the duplicate (Bug 1098426 - Music stops brutally when you press a key on the keypad)

[Blocking Requested - why for this release]: Audible regression with a high user impact.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: huge user impact according bug 1098426

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Use the default audio channel when playing audible tones outside of calls

This is a quick and safe workaround that makes the tones use the default audio channel. The default and content channels share the same volume control so the keypad behavior is unchanged; the upside is that the default channel doesn't cause the play icon to appear in the status bar nor does it interrupt applications explicitly using the content channel.

I've tested that the case were music was interrupted by typing in the keypad works correctly with this patch.

This isn't a proper fix because a proper fix requires far more investigation than I have time for at this stage.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] Use the default audio channel when playing audible tones outside of calls

Comment 8

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8524641 [details] [diff] [review]
[PATCH] Use the default audio channel when playing audible tones outside of calls

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

Why is the Emergency Call app still using the 'notification' channel? r- for this but please re-request review if you have an explanation.

::: apps/communications/dialer/test/unit/keypad_test.js
@@ +108,5 @@
>      function() {
>        this.sinon.spy(MockTonePlayer, 'init');
>        KeypadManager.init(/* oncall */ false);
>  
> +      sinon.assert.calledWith(MockTonePlayer.init);

s/calledWith/calledOnce/

::: shared/js/dialer/tone_player.js
@@ +16,5 @@
>    _channel: null,
>    _gainNode: null,
>    _playingNodes: [],
>    _maybeTrashAudio: null,
> +  _initialized: false,

We should test this functionality.

@@ +82,4 @@
>        return;
>      }
>  
> +    if (this._channel) {

I think this would work as well:

```js
var channel = this._channel ? this._channel : undefined;
this._audioContext = new AudioContext(channel);
```

Comment 9

[Gabriele Svelto [:gsvelto]]

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #8)
> Why is the Emergency Call app still using the 'notification' channel? r- for
> this but please re-request review if you have an explanation.

Using the `notification' channel in the emergency call app works. It's only in the dialer that the channel switching is borked.

> s/calledWith/calledOnce/

I used calledWith() in the hope that it will check that the call was made w/o any arguments. I'm not sure if that's the case though.

> We should test this functionality.

It should be covered by the unit-test that ensure nothing happens before initialization though with your comment below you gave me the idea that I don't need that extra field. Let me refresh the patch.

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> I used calledWith() in the hope that it will check that the call was made
> w/o any arguments. I'm not sure if that's the case though.

Replying to myself, what I was looking for is calledWithExactly(). It can be coupled with calledOnce() to ensure that we call the method only once and with those parameters alone.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Use the default audio channel when playing audible tones outside of calls

Updated patch with review comments taken into account. Unfortunately your suggestion of setting the channel explicitly to |undefined| doesn't work so I left that part unchanged. I've also updated the PR by pushing the new changes on top of the existing ones.

Comment 12

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8525609 [details] [diff] [review]
[PATCH v2] Use the default audio channel when playing audible tones outside of calls

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

Looks good.

We'll need a Contacts peer to review this, so I'm asking Francisco.

Comment 13

[Doug Sherk (:drs) (inactive)]

Please make a demo and put it on the sprint demos page when you get a chance:
https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S9#Demos

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8525609 [details] [diff] [review]
[PATCH v2] Use the default audio channel when playing audible tones outside of calls

r+ for the contacts part.

Comment 15

[Gabriele Svelto [:gsvelto]]

Try was finally green after having re-triggered an intermittent we should fix soon:

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=f2bd9b1bb3f4

Pushed to gaia/master e327daf01fac1e5fbe2ba732c26e3a21db32443a

https://github.com/mozilla-b2g/gaia/commit/e327daf01fac1e5fbe2ba732c26e3a21db32443a