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

RESOLVED FIXED in 2.1 S9 (21Nov)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

unspecified
2.1 S9 (21Nov)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
+++ 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.
Duplicate of this bug: 1098426
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.
blocking-b2g: --- → 2.2?
triage: huge user impact according bug 1098426
blocking-b2g: 2.2? → 2.2+
Duplicate of this bug: 1097312
Duplicate of this bug: 1099358
Assignee: nobody → gsvelto
Target Milestone: --- → 2.1 S9 (21Nov)
(Assignee)

Comment 6

5 years ago
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.
Attachment #8524641 - Flags: review?(drs.bugzilla)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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);
```
Attachment #8524641 - Flags: review?(drs.bugzilla) → review-
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
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.
Attachment #8524641 - Attachment is obsolete: true
Attachment #8525609 - Flags: review?(drs.bugzilla)
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.
Attachment #8525609 - Flags: review?(francisco)
Attachment #8525609 - Flags: review?(drs.bugzilla)
Attachment #8525609 - Flags: review+
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
Flags: needinfo?(gsvelto)
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.
Attachment #8525609 - Flags: review?(francisco) → review+
QA Contact: jlorenzo
(Assignee)

Comment 15

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(gsvelto)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.