STR: 0. Have a passcode on your phone's lock screen. 1. Go to emergency dialer. (From lock screen, tap 'unlock' icon, and then hit "Emergency call" button at lower-left of the passcode keyboard) 2. Hit some of the dialer keys, to see how loud they are. 3. Try to use volume rocker to adjust dialer keys. ACTUAL RESULTS: DTMF volume is full-blast (for me at least), and the volume rocker has no effect on it. OPTIONAL FURTHER STEPS: 4. Unlock phone, go to normal dialer; repeat steps 2 & 3 there; re-lock phone and return to emergency dialer to see if its volume has been changed. RESULTS OF FURTHER STEPS: The *normal* dialer works fine (it responds to volume rocker correctly). When you return to emergency dialer, it's still at its original volume (again, super-loud for me).

NOTE: The "silence incoming calls" setting in the power-button-menu doesn't affect this, either -- I'd expect that to make most things quiet, but the emergency dialer's DTMF tones stay full-blast.

I'm requesting blocking because, for actual emergencies where people need the emergency dialer, it may be important that they can dial discretely (e.g. if they're hiding from an attacker). See also the arguments in bug 784184 comment 19, bug 784184 comment 29, and bug 784184 comment 32. (also, note that while that bug had a (buried) workaround, there's no workaround for this one that I can find.)

Build ID: 20130121070201

Not a blocker if this is not a certification blocker (which we do not believe it is).

(In reply to Daniel Holbert [:dholbert] from comment #2) > I'm requesting blocking because, for actual emergencies where people need > the emergency dialer, it may be important that they can dial discretely > (e.g. if they're hiding from an attacker). For this scenario I propose to implement bug 819842 instead of do what bug 784184 does. > > See also the arguments in bug 784184 comment 19, bug 784184 comment 29, and > bug 784184 comment 32. (also, note that while that bug had a (buried) > workaround, there's no workaround for this one that I can find.) There's also a workaround to this bug but bug 784184 would be backed out per https://bugzilla.mozilla.org/show_bug.cgi?id=832170#c17

I agree that the dial tone is way too loud. For today and yesterday's builds the dial tone on a separate channel from the main volume and needs to be adjusted while the dial noise is happening. ie hit a number button .25 seconds before the hardware volume down button. I think the bug is that the dial volume should be controlled in the settings separate from the ringer (or maybe controlled by the ringer volume)... in any cases, I don't think many people will think to do the above paragraph.

Naoki, in regard to the uiwanted, is this really for UI (so that the user can control it) or should it really be "the default dial tone should be quieter and informed by the main volume setting"?

Rob and I reviewed this, and the UX recommendation is that, ideally, secondary audio channels should follow the primary audio channel's volume setting. If this is not possible technically, please needinfo? firefoxos-ux and we'll come up with some alternative UI.

Hi, Bug 866036 already changed the default volume channel controlled by hw keys to normal/content not notification/ringer now. So in newest release, when you press the HW volume keys you can adjust volume on key tone of dialer or emergency screen.

Retested and still happening.

(sorry; re-reading my previous comment, I wrote it bit ambiguously. Hiding it & re-posting:) (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #6) > I agree that the dial tone is way too loud. For today and yesterday's > builds the dial tone on a separate channel from the main volume and needs to > be adjusted while the dial noise is happening. ie hit a number button .25 > seconds before the hardware volume down button. Note: That only adjusts the volume in the *normal* dialer app. It does not work at all to adjust the dial noise in the emergency dialer app. So -- this comment (and comment 7 and comment 8, in response) are tangential to this bug, I think. The fact remains that the *Emergency* dialer app's dial-tones are full-blast and are not volume-controllable (not even in real time while pressing a dialer-key). (Thanks francisco for noticing that this is still an issue).

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5) > (In reply to Daniel Holbert [:dholbert] from comment #2) > > I'm requesting blocking because, for actual emergencies where people need > > the emergency dialer, it may be important that they can dial discretely > > (e.g. if they're hiding from an attacker). > > For this scenario I propose to implement bug 819842 [silent mode] Repsonding to this -- I don't think "silent mode" is actually helping us here either, FWIW (not currently, at least). I tried entering silent mode in two ways (in separate experiments) -- I'm not 100% sure of the correct way to enter silent mode, but both of these ways were discussed in bug 819842, so I think they're valid: Method #1: I held down the power button and chose "silence incoming calls". Method #2: I used the volume rocker to reduce the volume slider to 0, while at the emergency dialer screen. Neither of those methods have any effect on the emergency dialer's DTMF tone volume.

(I'm currently testing using a 2.1.0 Aurora build on a Flame device, FWIW [though I originally filed it using an unagi])

Setting tracking flags based on comment 10.

Just reproduced this on my Flame, I'm looking into it.

So it seems that we have a copy-pasted version of the TonePlayer; I wonder if replacing it with the real thing will help here. I'll also have to check that the proper audio channel is being used.

This patch imports most of the fixes that I had done for the regular keypad into the emergency keypad thus replacing a lot of obsolete code and aligning the two so that they'll be easier to merge down the road. To my great surprise this didn't solve the issue and in fact I've verified that this issue is present in the regular keypad too when used in the dialer as opposed to the callscreen. What happens is that the volume regulation in both the emergency keypad and the dialer keypad is for the telephony channel but we play the audible tones on the media channel which is thus unaffected by the volume adjustments. I hope to be able to build up on this to make the volume changes work correctly in both scenarios.

(In reply to Gabriele Svelto [:gsvelto] from comment #18) > This patch imports most of the fixes that I had done for the regular keypad > into the emergency keypad thus replacing a lot of obsolete code and aligning > the two so that they'll be easier to merge down the road. We should be sharing these anyways, so we could make this the "part 1" patch to fix this issue.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #19) > We should be sharing these anyways, so we could make this the "part 1" patch > to fix this issue. Yes, definitely. The scope will be a little larger than this bug but it's going to save us a few headaches down the road. My plan is to land this harmonization of the emergency keypad plus the fix for the volume issue so we can open a follow up where we'll purge the shared Keypad object from the dialer-specific depdencies and make it fully shared. (BTW I just realized I attached the wrong patch)

Here's the PR, I'll post both patches in it so it will be easier to review it.

So here's a better analysis of what's happening with the volume control. When playing a sound we switch the current audio channel to 'normal', this however goes immediately back to 'none' after the sound has finished playing. When the current audio channel is set to 'none' then volume adjustment will default to the 'notification' channel. So basically instead of adjusting the key volumes while in the dialer or emergency call-screen you end up adjusting your ringtone's volume :-/ I have yet to figure out how to solve this problem in the system app (if it's solvable, that is) but worst case we can switch playing the touch-tones in the 'notification' channel instead so I'd like a more proper solution rather than a quick hack.

After some further bug archaeology I found out that the behavior we're seeing is actually expected though I think we should change it anyway because it's surprising for the user. Here's the deal: - bug 937937 set the default volume control channel for the dialer app to 'notification' - the callscreen and emergency call app don't set any default volume control channel but pick the system default instead - the system default for all those applications is again 'notification', this was specified in attachment 8420872 [details] under bug 991026 (see the last slide) Now I'd like to challenge that spec because it's confusing for the user to adjust the volume and not see it reflected in the sounds the app makes. My guess is that the spec didn't take into account that the dialer & friends were using the 'normal' channel for playing touch tones, they probably thought only about it using the 'telephony' channel which automatically overrides all other settings as far as volume goes (i.e. in a call we use the telephony channel and no matter what the default volume control channel is it will be ignored passing on the adjustments to the telephony channel). Patches coming.

Whoops, wrong comment.

Comment on attachment 8506213 [details] [diff] [review] [PATCH 1/3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer This is the first part of the patch-set, it modernizes the emergency call app keypad by copying over parts of the shared one. This introduces a number of fixes affecting both audible tones and dial tones plus it introduces vibration. We're not up to the point where we can use the shared keypad directly but we're a bit closer. BTW feel free to leave the review comment on GitHub, I'm asking for reviews separately on the attached patches here because they're really independent and so the individual reviews should be smaller.

Comment on attachment 8506214 [details] [diff] [review] [PATCH 2/3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer This patch makes the emergency call keypad use the notification channel when not in a call to play audible tones. We switch to the telephony channel during the call automatically. This way the volume regulation now correctly affects the audible tones both in and out of a call.

Comment on attachment 8506215 [details] [diff] [review] [PATCH 3/3] Make volume adjustments apply correctly to the callscreen and dialer touch tones Finally this patch makes the regular keypad use the notification channel instead of the normal one for playing audible tones. This makes the volume regulation work correctly both in the dialer and in the callscreen.

Comment on attachment 8506214 [details] [diff] [review] [PATCH 2/3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer One last comment about this patch: I did some testing and somehow switching from the telephony channel to the notification channel works, but the other way around doesn't seem to be working, I have to investigate further.

Comment on attachment 8506213 [details] [diff] [review] [PATCH 1/3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer Review of attachment 8506213 [details] [diff] [review]: ----------------------------------------------------------------- It really worries me that there were no changes needed for any tests here. I think that we should focus on unifying the keypads rather than wasting time rewriting tests, but could you at least move some of the tests over from the shared keypad? You could even just copy-and-paste the whole thing and keep whatever tests pass without changes. ::: apps/emergency-call/js/keypad.js @@ +1,1 @@ > +/* globals LazyLoader, SettingsListener, telephony, TonePlayer */ You didn't remove this file from build/jshint/xfail.list. @@ +20,5 @@ > + * @param {String} tone The tone to be played. > + * @param {Boolean} short True if this will be a short tone, false otherwise. > + * @param {Integer} serviceId The ID of the SIM card on which to play the tone. > + */ > +function DtmfTone(tone, short, serviceId) { Why not split this into another file, stick it in shared/js/dialer/, and lazy-load it? We might not be able to share the keypads yet, but we can at least share chunks of them. @@ +69,5 @@ > + _keypadSoundIsEnabled: false, > + _shortTone: false, > + _vibrationEnabled: false, > + > + // Keep in sync with Lockscreen and keyboard vibration And now the shared keypad as well. @@ +481,5 @@ > + self._callVoicemail(); > + > + self._phoneNumber = ''; > + self._updatePhoneNumberView(); > + }, 400, this); We don't need to bring the voicemail code into here.

Comment on attachment 8506214 [details] [diff] [review] [PATCH 2/3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer Review of attachment 8506214 [details] [diff] [review]: ----------------------------------------------------------------- I'm arbitrarily setting r- because of a potential problem that I see. If I'm wrong, feel free to set r? again on the same patch. ::: apps/emergency-call/js/index.js @@ +1,3 @@ > +'use strict'; > + > +window.addEventListener('load', function callSetup(evt) { nit: this is a bad name. Maybe `emergencyCallSetup`. ::: apps/emergency-call/js/keypad.js @@ +206,5 @@ > this.hideBarHangUpAction.addEventListener('mouseup', > this.hangUpCallFromKeypad); > } > > + var channel = this._onCall ? 'telephony' : 'notification'; Is this ever 'telephony'? On `init()`, `this._onCall` should always be false. I think we need to re-acquire the `channel` variable every time the 'visibilitychange' callback is fired. This might also explain the problem described in comment 33.

Comment on attachment 8506215 [details] [diff] [review] [PATCH 3/3] Make volume adjustments apply correctly to the callscreen and dialer touch tones Review of attachment 8506215 [details] [diff] [review]: ----------------------------------------------------------------- In general, I'd rather have us use and stay on the 'normal' channel. This seems like a decent workaround, but it may not be necessary. Etienne, what are your thoughts on this? Is there any relevant history here? Do you know who might know more about the channels, and in particular setting and maintaining the 'normal' channel? ::: apps/communications/dialer/test/unit/keypad_test.js @@ +490,5 @@ > assert.equal(lastCall.args[0], 'telephony'); > }); > > + test('should switch back to notification when it gets hidden', function() > + { nit: ```js test('should switch back to notification when it gets hidden', function() { // ... ``` ::: shared/js/dialer/keypad.js @@ +224,5 @@ > this.hangUpCallFromKeypad); > } > > + var channel = this._onCall ? 'telephony' : 'notification'; > + TonePlayer.init(channel); In the Callscreen app, shared/Keypad is initialized with oncall=true, so this would work correctly, unlike in attachment 8506214 [details] [diff] [review], where it's initialized with oncall=undefined/false.

Comment on attachment 8506215 [details] [diff] [review] [PATCH 3/3] Make volume adjustments apply correctly to the callscreen and dialer touch tones See comment 36.

Comment on attachment 8506215 [details] [diff] [review] [PATCH 3/3] Make volume adjustments apply correctly to the callscreen and dialer touch tones Review of attachment 8506215 [details] [diff] [review]: ----------------------------------------------------------------- I'd like somebody on the platform side to confirm that we're not abusing the notification channel here.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #36) > In general, I'd rather have us use and stay on the 'normal' channel. This > seems like a decent workaround, but it may not be necessary. I tried that and it causes an interesting issue. By default the mozAudioChannelManager.volumeControlChannel variable doesn't contain anything. If I set it to 'normal' (or '' for the matter) the volume control doesn't work anymore. It shows the normal volume control bar, just empty and without an icon for the channel and pushing up/down doesn't do anything. If I set it to 'content' however it works correctly playing on the normal channel and adjusting the volume accordingly. That brings another problem though: as per spec when in the contacts list or call log the volume should affect the notification channel; this means however switching the default volume control channel whenever we go in and out of the keypad and I couldn't figure how to do it (somehow using the 'hashchange' event doesn't cut it). So I figured we could just use the notification channel to play the tones instead. Not pretty but the alternatives aren't pretty either.

New patch with the DtmfTone code shared in a separate file and all traces of the voicemail functionality stripped from the emergency keypad (including the icon).

I'll wait before modifying the other patches until we have a consensus about which channel to use, the corner cases make this tricky :(

Oopsie, scratch my previous patch, I forgot the unit-tests, new patch coming.

(In reply to Gabriele Svelto [:gsvelto] from comment #39) > I tried that and it causes an interesting issue. By default the > mozAudioChannelManager.volumeControlChannel variable doesn't contain > anything. If I set it to 'normal' (or '' for the matter) the volume control > doesn't work anymore. It shows the normal volume control bar, just empty and > without an icon for the channel and pushing up/down doesn't do anything. If > I set it to 'content' however it works correctly playing on the normal > channel and adjusting the volume accordingly. That brings another problem > though: as per spec when in the contacts list or call log the volume should > affect the notification channel; this means however switching the default > volume control channel whenever we go in and out of the keypad and I > couldn't figure how to do it (somehow using the 'hashchange' event doesn't > cut it). So I figured we could just use the notification channel to play the > tones instead. Not pretty but the alternatives aren't pretty either. I investigated this a bit. I had the same findings for the 'normal' channel, and I suspect that `MozAudioChannelManager` combines it with the 'content' channel for the purposes of volume control. Interestingly, AOSP uses their equivalent of the 'notification' channel for keypad presses. However, all of their UI sound response goes through this channel, so it makes more sense in that case. Despite this, and I'm changing my mind here, I think it makes more sense for us to use the 'notification' channel. The reason being that it makes little sense to combine media (videos, music, etc.) with UI responses. (In reply to Gabriele Svelto [:gsvelto] from comment #33) > Comment on attachment 8506214 [details] [diff] [review] > [PATCH 2/3] Import fixes for audible tones, dialtones and tactile feedback > from the shared dialer > > One last comment about this patch: I did some testing and somehow switching > from the telephony channel to the notification channel works, but the other > way around doesn't seem to be working, I have to investigate further. I looked more into this as well. I'm not sure if I'm reproing the same issue or not, but it seems that the volume control no longer works after a call ends. I don't think we're getting stuck on the 'telephony' channel. It seems that what's happening is that there's a mismatch between the volume rocker controlling the 'content' channel while TonePlayer is outputting on the 'notification' channel. When I set `navigator.mozAudioChannelManager.volumeControlChannel = 'notification'`, it works as expected. So I think it might just be an issue of restoring this when the Callscreen closes.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #43) > I investigated this a bit. I had the same findings for the 'normal' channel, > and I suspect that `MozAudioChannelManager` combines it with the 'content' > channel for the purposes of volume control. > > Interestingly, AOSP uses their equivalent of the 'notification' channel for > keypad presses. However, all of their UI sound response goes through this > channel, so it makes more sense in that case. Despite this, and I'm changing > my mind here, I think it makes more sense for us to use the 'notification' > channel. The reason being that it makes little sense to combine media > (videos, music, etc.) with UI responses. OK, thanks for the investigation, let's stick with the notification channel then. I'll flag UX later just to be on the safe side. This also takes out the complexity of switching channels when switching panels in the dialer. > I looked more into this as well. I'm not sure if I'm reproing the same issue > or not, but it seems that the volume control no longer works after a call > ends. I don't think we're getting stuck on the 'telephony' channel. It seems > that what's happening is that there's a mismatch between the volume rocker > controlling the 'content' channel while TonePlayer is outputting on the > 'notification' channel. > > When I set `navigator.mozAudioChannelManager.volumeControlChannel = > 'notification'`, it works as expected. So I think it might just be an issue > of restoring this when the Callscreen closes. Good point, I hadn't thought about that.

Updated patch with unit-tests copied over from the dialer. Since I don't know if you like bugzilla's interdiff functionality I've pushed a separate commit on top of the old one on GitHub for an easier review.

Comment on attachment 8507872 [details] [diff] [review] [PATCH 1/3 v3] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer Review of attachment 8507872 [details] [diff] [review]: ----------------------------------------------------------------- Please file a bug that blocks dialer-keypad (bug 1037065), to combine the two keypads together. r- only because of the divergence of emergency-call/index.html and emergency-call/test/unit/mock_dialer_index.html. The rest of the problems are fairly trivial. ::: apps/emergency-call/js/keypad.js @@ +161,5 @@ > this.hangUpCallFromKeypad); > } > > + TonePlayer.init('normal'); > + var channel = this._onCall ? 'telephony' : 'normal'; It's kind of wonky to use this `this._onCall` mechanism here still, but it'll help us unify the keypads later. @@ +164,5 @@ > + TonePlayer.init('normal'); > + var channel = this._onCall ? 'telephony' : 'normal'; > + window.addEventListener('visibilitychange', function() { > + var telephony = navigator.mozTelephony; > + var callIsActive = telephony && telephony.calls.length; Why was this change made? Is there a scenario in which `navigator.mozTelephony` will be `null`? ::: apps/emergency-call/test/unit/mock_dialer_index.html @@ +1,1 @@ > + <li role="presentation" id="keyboard-view"> This file is significantly different from emergency-call/index.html. It has likely also diverged from dialer/index.html. ::: apps/emergency-call/test/unit/mock_dialer_index.html.js @@ +7,5 @@ > + req.open('GET', '/test/unit/mock_dialer_index.html', false); > + req.send(null); > + > + return req.responseText; > +})(); This mechanism is no longer needed. You can just use `loadBodyHTML()`. See here for an example: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/call_info_test.js#L61 ::: apps/emergency-call/test/unit/mock_lazy_loader.js @@ +1,5 @@ > +'use strict'; > + > +/* exported MockLazyLoader */ > + > +var MockLazyLoader = { We could just use the shared MockLazyLoader instead. ::: shared/js/dialer/dtmftone.js @@ +1,1 @@ > +/* exported DtmfTone */ nit: this file should be called `dtmf_tone.js`. @@ +48,5 @@ > + > +/** > + * Length of a short DTMF tone, currently 120ms. > + */ > +DtmfTone.kShortToneLength = 120; This should go before the prototype.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #46) > Please file a bug that blocks dialer-keypad (bug 1037065), to combine the > two keypads together. Done, it's bug 1085387. > It's kind of wonky to use this `this._onCall` mechanism here still, but > it'll help us unify the keypads later. Yeah, it's horrible but we'll get rid of it. > Why was this change made? Is there a scenario in which > `navigator.mozTelephony` will be `null`? Unit-tests and visibility handlers don't mix. The alternative is to remove the visibilitychange event handlers in a teardown phase but I'd keep that for the unified dialer. > This file is significantly different from emergency-call/index.html. It has > likely also diverged from dialer/index.html. I just copied it over, I'll verify what needs to be changed and fix it. > This mechanism is no longer needed. You can just use `loadBodyHTML()`. See > here for an example: > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/ > test/unit/call_info_test.js#L61 Yeah, I'm familiar with that but I just copied over the tests from the dialer. > We could just use the shared MockLazyLoader instead. Good point, I'll see if I can clean up the dialer tests too.

Here's an updated patch with the review comments addressed. As usual I've posted the incremental patch separately on GitHub for ease of review. I've spent most of today playing with the channels in the callscreen and dialer and I'm encountering some truly weird behavior. First of all we're switching channels based on visibility and I'm not really sure why, we might even possibly have a race there. The second issue I've encountered is that the default volume channel changes after a call has ended and only a further visibility change will restore it properly to what it was before the call. All in all this is looking a lot more complicated than I had anticipated.

Comment on attachment 8508778 [details] [diff] [review] [PATCH 1/2 v4] Import fixes for audible tones, dialtones and tactile feedback from the shared dialer Review of attachment 8508778 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. I'm rubber-stamping the changes to emergency-call/keypad_test.js. ::: apps/emergency-call/test/unit/keypad_test.js @@ +7,2 @@ > > +require('/shared/test/unit/load_body_html_helper.js'); This should go in setup.js.

Ugh, I just realized this issue is *real* nasty. It turns out that we have *multiple* visibility handlers mucking around with the AudioContext(s) and their channels: - callscreen https://github.com/mozilla-b2g/gaia/blob/549c754e9acb947c74e439a3d249e80f87979c3b/apps/callscreen/js/index.js#L18 - dialer https://github.com/mozilla-b2g/gaia/blob/831734552e25f2fed1bfa39d2861f32ea8981540/apps/communications/dialer/js/dialer.js#L486 - ... and of course the shared code which interacts with both of the above https://github.com/mozilla-b2g/gaia/blob/831734552e25f2fed1bfa39d2861f32ea8981540/shared/js/dialer/keypad.js#L229 As one might guess the order in which they're invoked might yield different results and makes it practically impossible to figure out which channel should be active at which time. And we're not done yet because the telephony_messages.js code feels compelled to also mess around with the channels when the line is busy: https://github.com/mozilla-b2g/gaia/blob/35854536ee75b7f3094a56bfcd5190e710ba610b/shared/js/dialer/telephony_messages.js#L124 Long story short, I have to figure out on which channel each sound is supposed to be played and stick that logic in the TonePlayer and the TonePlayer *alone* then hide all the methods used to muck around with the channels. My first guess is that the following behavior should be enforced: - The busy tone must always happen on the telephony channel - The audible touch tones should happen on the notification channel in the dialer and on the telephony channel in the callscreen - The AudioContext should always be removed when an application becomes hidden unless we're the callscreen and there's an ongoing call All in all this is going to be a lot longer than what I expected but this code *needs* to be cleaned up. It's fragile and convoluted.

Per bug 1085256.

[Blocking Requested - why for this release]:Impact normal usage seriously, it's definitely a blocker on carrier side.

Here's a patch that fixes the remaining issues: - Ensures that the dialer uses the notification channel and the callscreen the telephony one - Ensures that the emergency-call app switches between notification and telephony depending on the context (not yet fully tested) - Moved the busy tone into the callscreen, this ensures it's always played on the telephony channel, it will delay the callscreen closing procedure until it's playing - Move channel handling into the TonePlayer, now the apps will specify a policy (one channel or two) and let it handle which channel to use - Removed the 4 (!) visibilitychange handlers that were mucking around with the channels and/or audio context, no more races FTW! Fixes to the existing unit-tests & new ones are still missing so only asking for feedback for now, I'll also update the PR with this patch on top.

Comment on attachment 8510386 [details] [diff] [review] [PATCH 2/2] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8510386 [details] [diff] [review]: ----------------------------------------------------------------- Generally good. I like the move of the busy line code to the Callscreen app. ::: apps/callscreen/js/calls_handler.js @@ +89,5 @@ > } > > + // Make sure we play the busy tone when appropriate > + if (telephony.active) { > + telephony.active.addEventListener('error', handleBusyLine); This will be added every time we get an 'oncallschanged' event. We should only bind this once. @@ +250,5 @@ > + [480, 620, 500], [0, 0, 500]]; > + > + TonePlayer.playSequence(sequence); > + exitCallScreenIfNoCalls(sequence.reduce(function(prev, curr) { > + return prev + curr[2]; This really needs a comment explaining that it's calculating the duration of the `sequence` tones. @@ +306,5 @@ > > /** > * Checks now and also in CallScreen.callEndPromptTime seconds if there > + * are no currently handled calls and no tones being played, and if not, > + * exits the app. Resets this timer on each successive invocation. This comment is inaccurate. This function doesn't check that there are no tones being played. It just lets us reset the timer if we need more time for tones. @@ +321,5 @@ > if (handledCalls.length === 0) { > window.close(); > } > exitCallScreenTimeout = null; > + }, timeout || CallScreen.callEndPromptTime); I think it would be better to just require a `timeout` and pass in `CallScreen.callEndPromptTime` at the other calls site. ::: shared/js/dialer/keypad.js @@ +105,5 @@ > }, > > multiSimActionButton: null, > > + init: function kh_init(channel, oncall) { Maybe we could just infer 'oncall' from the passed in channel. In every case that we have, either: channel === 'telephony' and oncall === true or channel !== 'telephony' and oncall === false ::: shared/js/dialer/tone_player.js @@ +22,5 @@ > + * play sounds. > + * > + * @param channel {String} The default channel used to play sounds. > + * @param telephonyChannel {String} If present switch to this channel > + * automatically if there is an active call. This really seems like black magic until you read the code. This could use a better explanation. I also wonder if this is actually needed here or if it could live in just the Callscreen app.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #56) > This will be added every time we get an 'oncallschanged' event. We should > only bind this once. No, because the function passed is always the same so it's added only once: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener#Multiple_identical_event_listeners I had to slightly modify this code but I'm still ensuring that it always gets the same function. > This really needs a comment explaining that it's calculating the duration of > the `sequence` tones. Good point. > This comment is inaccurate. This function doesn't check that there are no > tones being played. It just lets us reset the timer if we need more time for > tones. Yes, it's a leftover from a previous change I had dropped in the meantime; I'll correct it. > I think it would be better to just require a `timeout` and pass in > `CallScreen.callEndPromptTime` at the other calls site. OK. > Maybe we could just infer 'oncall' from the passed in channel. In every case > that we have, either: > channel === 'telephony' and oncall === true > or > channel !== 'telephony' and oncall === false Yes, I'm doing some testing because it just occurred to me that emergency calls are handled by the callscreen (I mean, the call itself) so the switching logic might actually be useless. Meh. > This really seems like black magic until you read the code. This could use a > better explanation. I also wonder if this is actually needed here or if it > could live in just the Callscreen app. It might not be needed at all, I'm doing some further tests to verify this.

Here's a new version of the patch, this time I didn't push it on top of the previous one in the branch because it's fundamentally different and I'd like to avoid confusion. In short what I've figured out is that the channel switching logic was useless: we never need to switch channels with the current architecture. In the dialer and emergency call apps we never have an ongoing call so we stick to the notification channel and in the callscreen we're always on a call so we use the telephony one. Besides the above here's a list of my changes to speed up the review (naturally I've taken into account your previous comments in this patch where they still applied): - All the logic to handle audio contexts and channel is within the TonePlayer, I've removed some methods like setChannel() that are now useless and flagged a few more as internal by adding an underscore to the method name (we could do better here by actually hiding them but let's keep that for later). - All visibilitychange handlers that were dealing with trashing the audio context or changing the channels have been removed. - AudioContexts are created lazily as per your last patch and dropped if the app is hidden (unless we're on the telephony channel and a call is active). - To verify if there's an active call I'm checking mozTelephony.calls.length and mozTelephony.conferenceGroup.calls.length because mozTelephony.active might be null if there's only one (conference) call and it's held. - The busy tone is played in the callscreen (already done in the WIP) and its length has been trimmed to 3s. It will keep the callscreen alive while playing via exitCallScreenIfNoCalls' timeout. - The dialer and emergency call app now explicitly set the volume control channel they want (notification) and I've adjusted their permissions accordingly. - Plenty of changes to the unit-tests: I've removed the ones that were broken or non-relevant and added more to cover my changes. I've also modified some mocks accordingly. On the testing side I've tested this with a modified gecko to be able to fake emergency calls. All scenarios I tried - including the busy line and various visibility changes - work fine.

Hi Jack, we'll bring this into coming Triage and you can see Gabriele is working on that already. However my question is 1. is this a certification test case, and blocker? 2. I assume this issue existed since 1.3 and we already has product launched with 1.3, why you said it's blocker in carrier side (comment#54)?

(In reply to Wesly Huang from comment #59) > 2. I assume this issue existed since 1.3 and we already has product launched > with 1.3, why you said it's blocker in carrier side (comment#54)? I haven't checked but I think that this wasn't a problem in 1.3. The reason is that we didn't have a separate callscreen application in 1.3 and I suspect that the problem was introduced when we split the callscreen from the dialer and messed up most of our audio logic.

Comment on attachment 8511646 [details] [diff] [review] [PATCH 2/2 v2] Make the TonePlayer handle the channel sounds will be played on After double-checking I've verified that this should fix bug 1082876 too. My guess was that before we could call start() after having taken a visibilitychange event that trashed the audio context. This cannot happen anymore since we're always ensure the context is there when calling start(). For the rest of the info on this patch see comment 53.

Comment on attachment 8511646 [details] [diff] [review] [PATCH 2/2 v2] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8511646 [details] [diff] [review]: ----------------------------------------------------------------- We're almost there. I'm really happy with how the refactors and simplification here have gone. ::: apps/callscreen/js/index.js @@ +7,5 @@ > > CallsHandler.setup(); > AudioCompetingHelper.init('callscreen'); > CallScreen.init(); > + KeypadManager.init('telephony', /* oncall */ true); We should add a test for this. ::: apps/callscreen/test/unit/calls_handler_test.js @@ +155,5 @@ > MockNavigatorMozTelephony.mTriggerCallsChanged(); > sinon.assert.calledOnce(MockCallScreen.switchToDefaultOut); > }); > > + test('should add a listener for playing the busy tone', function() { This test doesn't seem necessary. Adding the listener is a prerequisite for the 'should play the busy tone if we found the line busy' test. @@ +159,5 @@ > + test('should add a listener for playing the busy tone', function() { > + MockNavigatorMozTelephony.active = mockCall; > + this.sinon.spy(mockCall, 'addEventListener'); > + MockNavigatorMozTelephony.mTriggerCallsChanged(); > + sinon.assert.calledWithMatch(mockCall.addEventListener, 'error'); s/calledWithMatch/calledWith/ @@ +2032,5 @@ > + setup(function() { > + this.sinon.spy(MockTonePlayer, 'playSequence'); > + }); > + > + setup(function() { You have two `setup()` functions here. ::: apps/communications/dialer/js/index.js @@ +13,5 @@ > + if (navigator.mozAudioChannelManager) { > + navigator.mozAudioChannelManager.volumeControlChannel = 'notification'; > + } > + > + KeypadManager.init('notification'); We should add tests for each of these. ::: apps/emergency-call/js/dialer.js @@ +72,5 @@ > + /* Tell the audio channel manager that we want to adjust the notification > + * channel when the user presses the volumeup/volumedown buttons. */ > + if (navigator.mozAudioChannelManager) { > + navigator.mozAudioChannelManager.volumeControlChannel = 'notification'; > + } We should add a test for this. ::: apps/emergency-call/js/keypad.js @@ +160,5 @@ > this.hideBarHangUpAction.addEventListener('mouseup', > this.hangUpCallFromKeypad); > } > > + TonePlayer.init('notification'); We should add a test for this. ::: shared/js/dialer/keypad.js @@ +185,5 @@ > this.hideBarHangUpAction.addEventListener('click', > this.hangUpCallFromKeypad); > } > > + TonePlayer.init(this._onCall ? 'telephony' : 'notification'); We should add a test for this. ::: shared/js/dialer/tone_player.js @@ +32,2 @@ > this._channel = channel; > + this._maybeTrashAudio = (function tp_maybeTrashAudio() { This is a wonky way to make this function private. Why not just stick this on `TonePlayer` with a `_` prefix? @@ +43,5 @@ > + }).bind(this); > + > + window.addEventListener('visibilitychange', this._maybeTrashAudio); > + navigator.mozTelephony > + .addEventListener('callschanged', this._maybeTrashAudio); ```js navigator.mozTelephony.addEventListener( 'callschanged', this._maybeTrashAudio); ``` @@ +50,5 @@ > + /** > + * Tears down the tone player and removes the registered event listeners, > + * mostly used for unit-testing. > + */ > + teardown: function tp_teardown() { Let's just move this to the unit tests. We can perform this during the root `teardown()` callback.

(In reply to Wesly Huang from comment #59) > Hi Jack, we'll bring this into coming Triage and you can see Gabriele is > working on that already. However my question is > > 1. is this a certification test case, and blocker? > 2. I assume this issue existed since 1.3 and we already has product launched > with 1.3, why you said it's blocker in carrier side (comment#54)? Hi Wesly, In fact, it's regression from 1.3, please check. Thanks.

Updated patch, pushed the changes separately on top of the PR, I've made the following changes: - Covered the callscreen's index.js file with unit-tests - Removed the redundant test for playing busy tones - Covered the dialer's index.js file with unit-tests - Added a unit-test covering the TonePlayer initializion in the emergency-app - Added a unit-test covering the TonePlayer initialization in the dialer - Small formatting tweaks Just a couple more comments: (In reply to Doug Sherk (:drs) (use needinfo?) from comment #62) > ::: shared/js/dialer/tone_player.js > @@ +32,2 @@ > > this._channel = channel; > > + this._maybeTrashAudio = (function tp_maybeTrashAudio() { > > This is a wonky way to make this function private. Why not just stick this > on `TonePlayer` with a `_` prefix? I'm not making it private, just storing the bound function for reuse later. Without it I wouldn't be able to remove the event listener. > @@ +50,5 @@ > > + /** > > + * Tears down the tone player and removes the registered event listeners, > > + * mostly used for unit-testing. > > + */ > > + teardown: function tp_teardown() { > > Let's just move this to the unit tests. We can perform this during the root > `teardown()` callback. I'd like to keep it if you don't mind: it's easier to keep it in sync with the init() function and I can use it without having to expose the TonePlayer's internals in the unit-tests. If I'll be able to refactor the TonePlayer so that init() can be called multiple times without side-effects (which is rather simple) I'll remove it altogether.

Comment on attachment 8512831 [details] [diff] [review] [PATCH 2/2 v3] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8512831 [details] [diff] [review]: ----------------------------------------------------------------- I'm reluctantly setting r- because there's a serious problem that was introduced in either this patch or the previous one. We're passing a channel string, e.g. 'telephony' or 'notification', into `KeypadManager.init()` in places that we shouldn't be. This means that we're using the 'telephony' channel when we shouldn't be, in the 'Dialer' app. We should instead be passing the `oncall` variable, which is a bool. ::: apps/callscreen/js/calls_handler.js @@ +87,5 @@ > highPriorityWakeLock.unlock(); > highPriorityWakeLock = null; > } > > + // Make sure we play the busy tone when appropriate Maybe we should make the `handleBusyLine()` function name more descriptive and remove this comment instead. ::: apps/callscreen/js/index.js @@ +4,5 @@ > > +/** > + * Function invoked at onload time to initialize the application. > + */ > +function loadCallScreen(evt) { `onLoadCallScreen()` or `initCallScreen()`. @@ +10,5 @@ > > CallsHandler.setup(); > AudioCompetingHelper.init('callscreen'); > CallScreen.init(); > + KeypadManager.init('telephony', /* oncall */ true); This is still passing the channel. It should only pass the `oncall` variable. This works because we're double-inverting the first parameter and getting back `true`. @@ +16,5 @@ > > +/** > + * Dummy function introduced in bug 1083729, see below. > + */ > +function unloadCallScreen(evt) { } I would suggest a name change here, but you're not adding this, and we'll be removing it soon anyways. ::: apps/callscreen/test/unit/index_test.js @@ +8,5 @@ > + > +require('/test/unit/mock_audio_competing_helper.js'); > +require('/test/unit/mock_call_screen.js'); > + > +var mocksHelperForIndexDotJs = new MocksHelper([ s/mocksHelperForIndexDotJs/mocksHelperForIndex/ @@ +33,5 @@ > + suite('loading', function() { > + test('onload/onunload handlers are properly registered', function() { > + sinon.assert.calledWith(window.addEventListener, 'load', loadCallScreen); > + sinon.assert.calledWith(window.addEventListener, 'unload', > + unloadCallScreen); I'm kind of lukewarm about adding these `addEventListener()` tests, but since they're already written, I don't think it'll hurt. ::: apps/communications/dialer/js/index.js @@ +1,4 @@ > /* globals KeypadManager, NavbarManager, LazyLoader, LazyL10n, CallHandler */ > 'use strict'; > > +function dialerSetup() { very very nit-picky: I prefer <verb><Noun>, but you can leave this as-is if you'd like. It would be better if this was consistent with the other 'index.js' files, though. @@ +13,5 @@ > + if (navigator.mozAudioChannelManager) { > + navigator.mozAudioChannelManager.volumeControlChannel = 'notification'; > + } > + > + KeypadManager.init('notification'); This should be passing the `oncall` variable instead. ::: apps/communications/dialer/test/unit/index_test.js @@ +17,5 @@ > + 'NavbarManager' > +]).init(); > + > +suite('index.js', function() { > + mocksHelperForIndexDotJs.attachTestHelpers(); s/mocksHelperForIndexDotJs/mocksHelperForIndex/ @@ +45,5 @@ > + > + test('all components are initialized with the proper parameters', > + function() { > + sinon.assert.calledWith(MockKeypadManager.init, 'notification'); > + assert.equal(navigator.mozAudioChannelManager.volumeControlChannel, These should be 2 separate tests. ::: apps/communications/dialer/test/unit/mock_navbar_manager.js @@ +1,1 @@ > +/* exported MockNavbarManager */ This file puts us in an awkward spot where the mock file for 'dialer.js' is 'mock_navbar_manager.js'. I agree with your decision here, though. ::: shared/js/dialer/keypad.js @@ +110,5 @@ > + * Initializes the keypad manager, registers all the appropriate event > + * handlers and instances the necessary sound infrastructure so that the > + * keypad will be fully functional once this method has been called. > + * > + * @param oncall {Boolean} A boolean indicating whetever they keypad will be s/whetever they/whether the/ @@ +112,5 @@ > + * keypad will be fully functional once this method has been called. > + * > + * @param oncall {Boolean} A boolean indicating whetever they keypad will be > + * used during a call or not. In the former case the keypad will use > + * the telephony channel to play sounds and in the latter the This comment could be written in case/point form instead for quicker scanning. I'd also suggest wrapping 'telephony' and 'notification' in quotes for better visibility when this is being glanced at. ::: shared/js/dialer/tone_player.js @@ +19,5 @@ > + _maybeTrashAudio: null, > + > + /** > + * Initializes the tone player by specifying which channel will be used to > + * play sounds. The TonePlayer will lazily create a an AudioContext to play s/a an/an/

Review comments addressed, good that you caught my misuses of KeypadManager.init(), I mixed it up with TonePlayer.init(). I've pushed the incremental patch on top of the PR as usual.

Comment on attachment 8513001 [details] [diff] [review] [PATCH 2/2 v4] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8513001 [details] [diff] [review]: ----------------------------------------------------------------- After placing and ending a call, in the Dialer, the audio channel that I adjust when hitting the volume rocker is the 'normal' channel. When I touch a keypad button, it plays on the 'notification' channel. So I have to keep tapping keypad buttons while adjusting the volume in order to affect any change. It works as expected before I place a call. Strangely, I'm having the opposite problem in the Emergency Call app. The volume rocker controls the 'notification' channel, which the tones are being played on. But when I tap on the keypad buttons and adjust the volume at the same time, the 'normal' channel gets adjusted. I'm not sure what's going on here. I suspect that we'll have to restore the `navigator.mozAudioChannelManager.volumeControlChannel` variable when exiting the Callscreen. Or we could restore this in the 'visibilitychange' handler. ::: apps/callscreen/js/calls_handler.js @@ +89,5 @@ > } > > + // Make sure we play the busy tone when appropriate > + if (telephony.active) { > + telephony.active.addEventListener('error', playBusyTone); Thanks for renaming this, but playing the busy tone isn't all this does. It also calls `exitCallScreenIfNoCalls()`. Maybe `handleBusyErrorAndPlayTone()`? It's verbose, but accurate. @@ +243,5 @@ > } > > + /** > + * Play the busy tone in response to the corresponding error being triggered > + * at the end of a call. This comment doesn't mention that this may close the Callscreen.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #67) > After placing and ending a call, in the Dialer, the audio channel that I > adjust when hitting the volume rocker is the 'normal' channel. When I touch > a keypad button, it plays on the 'notification' channel. So I have to keep > tapping keypad buttons while adjusting the volume in order to affect any > change. It works as expected before I place a call. I noticed that too. I think it's a bug in the audio channel manager though. I've tried working around it by resetting the `volumeControlChannel` property every time we become visible but the problem persists. I think we'll have to accept it as it is and file a follow up. I've also become moderately familiar with that part of the system app so I might take a stab at it myself but not as part of this bug.

I've addressed the last review comments you made. I've omitted re-setting the volume control channel depending on visibility because it doesn't solve the problem we're seeing. I'll investigate the system app later to see where that's coming from.

Comment on attachment 8513456 [details] [diff] [review] [PATCH 2/2 v5] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8513456 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/communications/dialer/js/index.js @@ +14,5 @@ > + navigator.mozAudioChannelManager.volumeControlChannel = 'notification'; > + } > + > + window.addEventListener('visibilitychange', function() { > + // HACK: XXX reset the volume control channel when shown comment 69 seemed to indicate that this wouldn't be included, but I went ahead and tested it anyways since it's here. I had the same findings as you; that this didn't fix the issue. I also tested how this worked before this patch. Unfortunately, this patch introduces this issue and it doesn't happen on master. I'm uncomfortable landing it with a known regression, especially one this bad. If we weren't going to uplift this, I'd be fine with doing followups. We also still have the issue I described with the Emergency Call app in comment 67.

(In reply to Doug Sherk (:drs) (use needinfo?) from comment #70) > comment 69 seemed to indicate that this wouldn't be included, but I went > ahead and tested it anyways since it's here. I had the same findings as you; > that this didn't fix the issue. I must have forgot it inside the patch, good that you could test it too. > I also tested how this worked before this patch. Unfortunately, this patch > introduces this issue and it doesn't happen on master. I'm uncomfortable > landing it with a known regression, especially one this bad. If we weren't > going to uplift this, I'd be fine with doing followups. OK, I agree that the need for uplifting this makes it unacceptable. I will investigate the issue which most likely is happening in the system app. Unfortunately all bets are off for the ETA because I have no idea how complicated the underlying issue may be. I only know that the audio channel manager underwent some intrusive surgery sometimes between 2.0 and now and this might be part of the problem. > We also still have the issue I described with the Emergency Call app in > comment 67. I think it's really the same issue.

Moving the dependency over from bug 1088668.

OK, my first investigation points towards Gecko for the default volume adjustment bug we're getting. The system app is receiving two `default-volume-channel-changed` events from gecko when switching back from the callscreen to the dialer after a call. The first carries the `notification` channel int its payload which is right, the second however comes with the `unknown` channel which causes the volume control to use the content channel instead. Now digging into gecko...

Thanks Gabriele and Doug's effort on it! As so many effort is spent already, plus it's a valid issue from my view, I plan to tag it in coming 2.0 Triage, how do you think?

(In reply to Wesly Huang from comment #75) > Thanks Gabriele and Doug's effort on it! As so many effort is spent already, > plus it's a valid issue from my view, I plan to tag it in coming 2.0 Triage, > how do you think? Considering the aggregate amount of issues this fixes I think it will be important to have it in 2.0. However bear in mind that the changes here are large and thus carry a certain risk. They're simple in the sense that they replace a lot of fragile code with more robust one, and they're accompanied with unit-tests, but the patch is still large so we'll have to be careful when uplifting.

Yes Gabriele I understand your point so actually a little hesitate to tag it for 2.0 (maybe put it to 2.1 or 2.2), but as partner mentioned this would be a blocker in carrier side I think it's worth trying to fix in 2.0 with care. (rather do it carefully than make a rush)

I've done a fairly extensive investigation of the issue that's causing the volume control channel to change erroneously when going back from the callscreen to the dialer and while I have understood why its happening I'm not sure where the problem lies. During the callscreen -> dialer transaction the AudioChannelManager in gecko is responsible for adjusting the current default volume control channel. The system app will be listening to these changes and adjust the volume slider accordingly. The AudioChannelManager adjusts the current volume control channel in response to visibility events so that - in theory - the most relevant control channel for the currently visible app(s) is set as the default. What happens in our situation is that during the transition between the two apps the dialer becomes visible triggering a change from the `telephony' channel to the `notification' one. Somehow, right afterwards another visibilitychange event is triggered from the system app and changes the channel again to `unknown' (which defaults to content) hence giving us the wrong scroller. The code responding to the visibilitychange events is here: http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/system/gonk/AudioChannelManager.cpp#l148 ... and this is where channels are being set: http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/system/gonk/AudioChannelManager.cpp#l126 To make things murkier the first default volume control channel change happens in the dialer content app and is passed via IPC to the main process, see here: http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/ipc/ContentParent.cpp#l2372 Now, I'm not sure who's at fault here. It might be that the visibilitychange events are being sent in the wrong order, or are arriving in the wrong order due to a race condition (the IPC part here makes me suspect of that) and it could also be that the AudioChannelManager is not doing the right thing. I'm not familiar enough with this design to figure out where the fix belongs so in the meantime I propose to use this patch which makes the dialer app use the `content' channel for playing audible key tones. This is not an optimal solution, but it works and make the volume scroller consistent with the tones in both the dialer and emergency-call app. Note that the emergency-call app is still using the `notification' channel because it suffers from the opposite problem: if we use the `content' channel the AudioChannelManager will switch us back to `notification' after ending a call.

Comment on attachment 8514934 [details] [diff] [review] [PATCH 2/2 v6] Make the TonePlayer handle the channel sounds will be played on Review of attachment 8514934 [details] [diff] [review]: ----------------------------------------------------------------- Some parts of this are still fairly fragile and we must take extra care when uplifting it. I recommend setting the 'NO_UPLIFT' whiteboard tag and doing the uplifts yourself. Please fix the requested permissions before landing as we discussed on IRC. With this patch, we are now mixing the channels that we play tones on for each app. The Emergency Call app is using the 'notification' channel, but the Callscreen and Dialer are using the 'content' channel. We should file a followup to fix this. ::: apps/communications/dialer/js/index.js @@ +10,5 @@ > > + /* XXX: Tell the audio channel manager that we want to adjust the "content" > + * channel when the user presses the volumeup/volumedown buttons. We should > + * be using the "notification" channel instead but we can't due to bug > + * TUVWXYZ */ Please file a bug for this before landing. ::: shared/js/dialer/keypad.js @@ +114,5 @@ > + * @param oncall {Boolean} True if the keypad manager will be used during a > + * call and will play sounds on the "telephony" channel. False if it > + * will be used outside of a call and should use the "content" channel > + * instead. We should be using the "notification" channel but we can't > + * due to bug TUVWXYZ. And add that bug number here too. @@ +187,5 @@ > this.hangUpCallFromKeypad); > } > > + /* XXX: We should be using the "notification" channel here instead of the > + * "content" one but we can't due to bug TUVWXYZ. */ And add that bug number here too.

I've filed bug 1092346 with an STR and a detailed explanation of the problem we've encountered. I've rebased the patches on top of master, squashed them and added the correct bug number. I'll be waiting for the try run to turn green before merging and will file a follow-up to revert the dialer to the `notification' channel whenever it will be possible to.

Filed the follow up in bug 1092362.

There's a bunch of orange in the integration tests, I don't feel confident in landing this since it seems to affect the dialer/contacts. I'll try to reproduce locally to figure out what's happening.

I've re-run all the integration tests on my machine and they all work fine. As much as I hate merging on a non-fully green tree these are genuine intermittent failures. Merged to gaia/master ba9fb2858c5740768ddf96f66324fa6a1f889789 https://github.com/mozilla-b2g/gaia/commit/ba9fb2858c5740768ddf96f66324fa6a1f889789

Gabriele, please add a demo of this to the current sprint page when you get a chance: https://wiki.mozilla.org/FirefoxOS/Comms/Dialer/Sprint/v2.1-S8#Demos

sorry had to revert this out by request from zac for failing tests. I guess zac can comment more here

It's a clear Gip test failure (not intermittent) on Gaia-Try because it failed on both platforms with identical traces. The Python functional test cases just need to be updated. These tests will fail on device testing too, when the Flame build is built this changeset.

(In reply to Zac C (:zac) from comment #86) > It's a clear Gip test failure (not intermittent) on Gaia-Try because it > failed on both platforms with identical traces. > > The Python functional test cases just need to be updated. > > These tests will fail on device testing too, when the Flame build is built > this changeset. I'm a bit puzzled, I run those tests locally and none of them failed. Maybe I'm missing something?

(In reply to Gabriele Svelto [:gsvelto] from comment #87) > (In reply to Zac C (:zac) from comment #86) > > It's a clear Gip test failure (not intermittent) on Gaia-Try because it > > failed on both platforms with identical traces. > > > > The Python functional test cases just need to be updated. > > > > These tests will fail on device testing too, when the Flame build is built > > this changeset. > > I'm a bit puzzled, I run those tests locally and none of them failed. Maybe > I'm missing something? Possibly not, sometimes Treeherder hardware test runs behave differently to local test runs (mainly speed of execution is to blame). The test is waiting for 'toolbar-option-selected' class to be added to the selected menu bar option at the bottom of the page. I ran the test locally (albeit on a Flame) and I can see the test is passing too so the class is being added at some point! I'll run it locally against desktopb2g too now.

I can replicate this locally on desktopb2g. After the dialer app is launched, this JS error is raised: JavaScript error: app://communications.gaiamobile.org/shared/js/dialer/tone_player.js, line 46: TypeError: navigator.mozTelephony is undefined After this error was raised I killed the test run and interacted with desktopb2g manually. In the dialer app only the keybad buttons would work and none others worked. I presume this means the dialer app failed to initialise after the JS error.

I've just launched b2g-bin using a `make` built profile: ./b2g-bin --profile /home/zac/Mozilla/gaia/profile It fails the same just launching it manually so this is nothing to do with the tests.

(In reply to Zac C (:zac) from comment #90) > It fails the same just launching it manually so this is nothing to do with > the tests. I think you just put me on the right track. I was unconditionally using the navigator.mozTelephony property in the TonePlayer initialization but naturally that's not going to work on b2g desktop. Still I don't understand why the UI tests were passing on my machine; I must have done something wrong when running them. Thanks for your help.

Here's a new PR with the modifications that should fix the UI tests as well as running in b2g desktop. I've run both and they're working correctly, waiting for try to (hopefully) agree with my local testing.

OK, this time try is unambiguously green: https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=265568452ded Merged to gaia/master 3c50520982560ccba301474d1ac43706138fc851 https://github.com/mozilla-b2g/gaia/commit/3c50520982560ccba301474d1ac43706138fc851

[Blocking Requested - why for this release]: [Triage] since the change is big so even 2.0 patch is promising we don't tag for 2.0, instead we nom. for 2.1. (partner can still take this patch by their own)

yw Gabriele, nice when we pick up real bugs like this :)

(In reply to Wesly Huang from comment #94) > [Triage] since the change is big so even 2.0 patch is promising we don't tag > for 2.0, instead we nom. for 2.1. (partner can still take this patch by > their own) After having analyzed a bit how to uplift this I think I will try to apply only the second patch to 2.1 - and possibly to 2.0 too. The reason is that the first part is not strictly needed to solve the issue here (and in related bugs 1085256, 1088668 and 1067219) and has a strong dependency on bug 1060451 which would be another large uplift. If I manage to adapt attachment 8514934 [details] [diff] [review] to 2.1 then there's a good chance we'll be able to apply it unmodified to 2.0 too.

(In reply to Jack Liu from comment #54) > [Blocking Requested - why for this release]:Impact normal usage seriously, > it's definitely a blocker on carrier side. This has been a long standing issue and we may not not be able to block a release on this at this point, looks like this is getting fixed and will be in 2.2.

Resetting ni for comment 84.

This pull request contains only the second patch of this series (attachment 8514934 [details] [diff] [review]) uplifted to work on v2.1. This avoids uplifting the refactoring we've done in master and which depends on a few other patches and which would carry a significant risk if uplifted. I had deliberately split this in a refactoring patch and one that would fix the actual problem to allow for easier, less risky uplift and this is the result. Note that even though this was de-prioritized the duplicates & related issues were not. The bug here is not only about volume regulation but also causes high power consumption so I wholeheartedly recommend that we take this for v2.1 and v2.0 (I'm preparing an uplifted patch for the latter too). [Approval Request Comment] [Bug caused by] (feature/regressing bug #): Too many to count, this was caused by a number of bugs that originated as fixes for side-effects of the callscreen split [User impact] if declined: Hi power-consumption at all times plus confusing behavior, see bug 1085256, bug 1088668 and bug 1067219 [Testing completed]: Tested all possible scenarios on a v2.1 device, unit-tests were imported from the master branch and adapted to ensure functionality [Risk to taking this patch] (and alternatives if risky): Dialer/callscreen audio functionality may be impacted, there's not many alternatives to this [String changes made]: None

Verified on: Gaia-Rev 7004ccfd16e2ad20739bd04b72fa1672ee58686f Gecko-Rev afea13fdb3de3abd9ece29d3da5b700abe450988 Build-ID 20141107145850 Version 36.0a1 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.jlorenzo.20141002.104456 FW-Date Thu Oct 2 10:45:09 CEST 2014 Bootloader L1TC00011880 Regarding the uplift patch, I checked the following: * The tones of the emergency dialer can be controlled by the volume buttons. * Emergency dialer has no effect on a current call. * You can't make a call with that dialer if you already have a call. And this issues are still occuring: * The emergency dialer and the regular dialer don't share the same channel (see bug 1092346). Hence, the volume is not the same between each other. * Dialer tones are not transmitted to other during conf call (see bug 1095503). It works when you make a conf call with your ICE contact in emergency dialer though. I couldn't test the busy tone because I'm redirected to the voicemail if one of my SIM card is busy in another call.

See comment 99. Just an additional note: this also makes the emergency app audible tones work; they were completely broken before. There's a small snag however: due to the workarounds we have in place for Web Audio issues we're forced to play them on the content channel. This means that while volume controls works correctly before a call it won't right after a call and until the default volume control channel is restored (for example by hiding the application). This is due to bug 1092346 and is unavoidable; but I feel it's a minor issue considering that before the tones didn't work at all.

Can we get qa to build with that patch on 2.1 and verify?

(In reply to Fabrice Desré [:fabrice] from comment #102) > Can we get qa to build with that patch on 2.1 and verify? I take that one.

I built 2.1 with the patch. One thing that I didn't notice when I verified the patch on master is bug 1098426 (Music stops brutally when you press a key on the keypad). As we use the content channel as a workaround in this patch, I don't think we should uplift this patch at the current state of the 2.1 release. We might find more cross-app regressions like bug 1098426. Moreover, each time you press one of the key in the keypad, you'll notice briefly the Play icon in the status bar. This may confuse an end-user. I think it's more prudent to keep the current behavior which is known for many releases.

Comment on attachment 8518384 [details] [PULL REQUEST gaia/v2.1] Improvements to the emergency and dialer keypad touch tones See comment 104.

Comment on attachment 8519251 [details] [PULL REQUEST gaia/v2.0] Improvements to the emergency and dialer keypad touch tones See comment 104.

Comment on attachment 8518384 [details] [PULL REQUEST gaia/v2.1] Improvements to the emergency and dialer keypad touch tones Thanks for testing, Johan.

Thanks for the thorough testing Johan, this means we need to solve bug 1092346 ASAP because it's a requirement for fixing bug 1092362. Let's hold off the uplifts until then.

Comment on attachment 8518384 [details] [PULL REQUEST gaia/v2.1] Improvements to the emergency and dialer keypad touch tones Re-requesting approval now that the workaround for bug 1092362 landed. The PR now includes that cherry-picked patch too which didn't require modifications for v2.1.

Bhavana, I saw you marked this bug as WONTFIX for v2.0 but the reason why I had asked for approval is that the request for it came from one of our partners even though they didn't set any blocking flag on the bug, see bug 1085256 comment 8. I've updated the 2.0 uplift PR with the fix for bug 1092362 which removes the only issue that stemmed from landing this so the uplift should be more robust now. I still think we should take this for v2.0 because the power consumption implications of this bug are significant even though it carries some risk. Note that the uplifted patches for v2.0 also apply on v2.0m so depending on which branch our partners are tracking they might be taken there too.

(In reply to Gabriele Svelto [:gsvelto] from comment #110) > Bhavana, I saw you marked this bug as WONTFIX for v2.0 but the reason why I > had asked for approval is that the request for it came from one of our > partners even though they didn't set any blocking flag on the bug, see bug > 1085256 comment 8. I've updated the 2.0 uplift PR with the fix for bug > 1092362 which removes the only issue that stemmed from landing this so the > uplift should be more robust now. I still think we should take this for v2.0 > because the power consumption implications of this bug are significant even > though it carries some risk. > > Note that the uplifted patches for v2.0 also apply on v2.0m so depending on > which branch our partners are tracking they might be taken there too. Thanks for pointing to the partner dependency :gsvelto. I'll flag Johan to re-test this on 2.1 and once he verifies we are fine to uplift it both on 2.0/2.1

Johan, seeting qa-approval back to ? so you can help test the updated patches! thanks :)

Quick note, I've just rebased both the v2.0 and v2.1 uplift PRs, I had to fix a couple of small conflicts in v2.1 but nothing major. Also clearing my needinfo as I've provided a demo for this in the sprint page.

Comment on attachment 8518384 [details] [PULL REQUEST gaia/v2.1] Improvements to the emergency and dialer keypad touch tones With this patch emergency tones can be regulated by the volume rocker. They don't share the same volume though. I haven't found anything major in this patch. A couple of minor issues: * Bug 1106076 - The music goes soft while using the keypad of the emergency dialer * Bug 1106074 - Audio output goes through speaker and earplugs in emergency dialer Testing done (with earplugs, then with BT headset, then without anything): * Use the in-call dialer * Use the regular dialer while and while not in call * Listen to some music (and FM radio when earplugs) while in call and use the dialer keypad. Same thing with emergency dialer. Bug 1021616 was already filed. * Watch a video while in call. As a consequence, this patch is mature enough to be uplifted to 2.1. I still need to check 2.0.

Comment on attachment 8519251 [details] [PULL REQUEST gaia/v2.0] Improvements to the emergency and dialer keypad touch tones I have different results here. The emergency and the regular dialer both use the same channel. I double checked the way I built 2.1 and the results I had, they are different. Do you have an idea of what can explain these changes, Gabriele? So, both issues found for 2.1 (and master) are not reproducible on 2.0. We also can't use the regular dialer while in call because the feature was added in 2.1. Testing done (with earplugs, then with BT headset, then without anything): * Use the in-call dialer * Use the regular dialer while in call * Listen to some music (and FM radio when earplugs) while in call and use the dialer keypad. Same thing with emergency dialer. Bug 1021616 was already filed. * Watch a video while in call.

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #115) > I have different results here. The emergency and the regular dialer both use > the same channel. I double checked the way I built 2.1 and the results I > had, they are different. Do you have an idea of what can explain these > changes, Gabriele? Good catch, it seems that on v2.0 I've used the content channel for the emergency dialer instead of the notification one. I'll address that and ping you again to re-test this particular scenario. Thanks for the help with this; it's a huge contribution to the final quality of the patch.

I've double-checked and what you're seeing in 2.0 is unfortunately an unavoidable problem. I had described it in comment 101 but forgot when I've written the previous comment. The issue in 2.0 is that gecko is missing an important Web Audio fix that makes touch-tones sound scratchy. We've got a workaround in the code that uses audio elements instead of playing tones via Web Audio. Since audio elements always play on the content channel we have to use that even though we should have been using the notification channel. If we take this patch in 2.0 then we have to accept the problem you described as removing it would yield an even worse problem (all touch tones would be scratchy).

The v2.1 Gaia Try run has failures. Also, did you want to take NO_UPLIFT off this bug now?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #118) > The v2.1 Gaia Try run has failures. The v2.1 uplift still had a couple of linter errors and a broken unit-test; I've fixed both and re-triggered the try run. Let's see what happens. > Also, did you want to take NO_UPLIFT off > this bug now? Yes indeed, thanks for pointing that out.

v2.1: https://github.com/mozilla-b2g/gaia/commit/473113cbdfb6ebe03dda3c0223bec3e138df9ff4

This issue has been verified successfully on Flame 2.1 See attachment: Verify_834530.MP4 Reproducing rate: 0/5 Reproducing Steps: Precondition: Have a passcode on your phone's lock screen. 1.Go to emergency dialer. (From lock screen, tap 'unlock' icon, and then hit "Emergency call" button at lower-left of the passcode keyboard) 2.Hit some of the dialer keys, to see how loud they are. 3.Try to use volume rocker to adjust dialer keys. ** The dialer keys’ volume can be adjusted successfully. 4.Unlock phone, go to normal dialer. 5.Rrepeat steps 2 & 3. ** The *normal* dialer works fine 6.Re-lock phone and return to emergency dialer. ** It's at its volume that adjusted in step3. 7.Long press Power key and select "silence incoming calls" in the power-button-menu. ** The emergency dialer's DTMF tones are quiet. Flame 2.1 build: Gaia-Rev 38e17b0219cbc50a4ad6f51101898f89e513a552 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a Build-ID 20141205001201 Version 34.0

Hi Gabriele, The patch might have something improper which cause a new bug1115304. I have checked your patch and find the things improper: >+ if (document.hidden && >+ !((this._channel === 'telephony') && callIsActive)) { >+ this._trashAudio(); >+ } 1.Why not add a this._ensureAudio(); for the else choice? 2.Why the init of TonePlayer's audio channel be 'telephony'?(this._onCall is true when CallScreen is inited) > TonePlayer.init(this._onCall ? 'telephony' : null); then it will register a wrong audio channel before a call is in. Codes modified as follows can help to fix the issue in bug1115304: >if (document.hidden && > !((this._channel === 'telephony') && callIsActive)) { > this._trashAudio(); >+ } else { >+ this._ensureAudio(); > } >+ /* XXX: We should be using the `notification' channel here but we can't due >+ * to bug 1092346 and pass `null' instead, forcing the use of the default >+ * audio channel. */ >- TonePlayer.init(this._onCall ? 'telephony' : null); >+ TonePlayer.init(null); Need your help to check :(

(In reply to jingmei.zhang from comment #122) > 1.Why not add a this._ensureAudio(); for the else choice? Because we call this._ensureAudio() lazily only when we need to play a sound. > 2.Why the init of TonePlayer's audio channel be 'telephony'?(this._onCall > is true when CallScreen is inited) > > TonePlayer.init(this._onCall ? 'telephony' : null); > then it will register a wrong audio channel before a call is in. When the call screen is opened we already know that there's a call so this._onCall will always be set to true and the callscreen will always use the `telephony' channel. > Codes modified as follows can help to fix the issue in bug1115304: > > >if (document.hidden && > > !((this._channel === 'telephony') && callIsActive)) { > > this._trashAudio(); > >+ } else { > >+ this._ensureAudio(); > > } What difference does it make with this code? > >+ /* XXX: We should be using the `notification' channel here but we can't due > >+ * to bug 1092346 and pass `null' instead, forcing the use of the default > >+ * audio channel. */ > >- TonePlayer.init(this._onCall ? 'telephony' : null); > >+ TonePlayer.init(null); > > Need your help to check :( Here you're switching the callscreen to use the default channel. First we'd need to fix bug 1092346 before doing that.

(In reply to Gabriele Svelto [:gsvelto] from comment #123) > (In reply to jingmei.zhang from comment #122) > > 1.Why not add a this._ensureAudio(); for the else choice? > > Because we call this._ensureAudio() lazily only when we need to play a sound. > But I find that when we a call in ,this._ensureAudio() isn't called,As far as I can know,this._ensureAudio() is used to update the current audiochannel,is that right? > > > > >if (document.hidden && > > > !((this._channel === 'telephony') && callIsActive)) { > > > this._trashAudio(); > > >+ } else { > > >+ this._ensureAudio(); > > > } > > What difference does it make with this code? It would make some difference.Modified the codes as above,recieve a call and then the ringtone will come out of earpiece other than the speaker. I guess the 'telephony' set for tone_player that causes issue in bug1115304.

Add Alastor for a further confirm. Hi Alastor, The 'telephony' set for tone_player may cause VOIP is connected（issue in bug1115304）. Can you help to check?

(In reply to jingmei.zhang from comment #124) > But I find that when we a call in ,this._ensureAudio() isn't called,As far > as I can know,this._ensureAudio() is used to update the current > audiochannel,is that right? No, this._ensureAudio() is used to create an audio context which is then used to play touch tones and similar sounds. It is not invoked unless the user taps on a key in the keypad or we need to play another sound such as the busy tone. The audio channel is chosen automatically by the system app's sound manager depending on what the application is doing. > It would make some difference.Modified the codes as above,recieve a call and > then the ringtone will come out of earpiece other than the speaker. > > I guess the 'telephony' set for tone_player that causes issue in bug1115304. If you don't tap keys in the keypad we will never create an audio context. What you're doing is forcing the creation of an audio context by calling this._ensureAudio() but we don't want to do this because we don't needed one.

Hi, JingMei, Sorry for my late response, I'm confused about why the 'telephony' is related with voip? Maybe the root cause of the bug1115304 is another reason.

Dear William, Could you please help check the following scenario with mozilla build between the two versions[1,2]: Step1) Play a music; Step2) Receive an incoming call and connect; Step3) Hang up the call and then music will resume to play. We should pay attention to check if the music will come out speaker quickly as expected or not. Thanks a lot. [1] <project name="gaia" remote="mozillaorg" revision="2052ad921917c237f72d1f91040c0fb933c93662" upstream="v2.1"/> [2] <project name="gaia" remote="mozillaorg" revision="fc85c28243d2d8baf90a76bbf36b8c1e608c190a" upstream="v2.1"/>

Hi Shawn, For comment128,Please help to check. Thank you~

Jingmei: Is there any regression caused by the patch of Bug 834530 (per comment 128)? If yes, we will try to allocate gaia RD to check this issue first, otherwise, please file a new bug for discussion!!

(In reply to shawn ku [:sku] from comment #130) > Is there any regression caused by the patch of Bug 834530 (per comment 128)? > > If yes, we will try to allocate gaia RD to check this issue first, Hi Shawn, Yes,patch in Bug 834530 cause issue in bug1115304(STR per comment128),we need your help to check this issue.

Hi gsvelto: Please help check STR in comment 128. Partner claims there is an side effect caused by the patch.

(In reply to helloarvin from comment #128) > Dear William, > > Could you please help check the following scenario with mozilla build > between the two versions[1,2]: > > Step1) Play a music; > Step2) Receive an incoming call and connect; > Step3) Hang up the call and then music will resume to play. > > We should pay attention to check if the music will come out speaker > quickly as expected or not. > Thanks a lot. > > [1] <project name="gaia" remote="mozillaorg" > revision="2052ad921917c237f72d1f91040c0fb933c93662" upstream="v2.1"/> > [2] <project name="gaia" remote="mozillaorg" > revision="fc85c28243d2d8baf90a76bbf36b8c1e608c190a" upstream="v2.1"/> I am not the v2.1 branch QA contact, so I cannot reply you immediately. Anyway, here are the test results. By the way, you didn't mentioned the test device and Gecko version. So, I only can try the similar test environment to do the test. Many thanks. Scenario 1. revision="2052ad921917c237f72d1f91040c0fb933c93662" Branch="v2.1" @ Test result: The music app resume to play quickly as expected. - https://www.youtube.com/watch?v=jnUTcm1WB7o @ Build information: (Flame v2.1) - Gaia-Rev 2052ad921917c237f72d1f91040c0fb933c93662 - Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/09fb60a37850 - Build-ID 20141023001201 - Version 34.0 - Device-Name flame - FW-Release 4.4.2 - FW-Incremental eng.cltbld.20141023.034829 - FW-Date Thu Oct 23 03:48:40 EDT 2014 - Bootloader L1TC00011880 Scenario 2. revision="fc85c28243d2d8baf90a76bbf36b8c1e608c190a" Branch="v2.1" @ Test result: The music app resume to play quickly as expected. - https://www.youtube.com/watch?v=qcoLwLoroTI @ Build information: (Flame v2.1) - Gaia-Rev fc85c28243d2d8baf90a76bbf36b8c1e608c190a - Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/4f8c0c021128 - Build-ID 20141026001201 - Version 34.0 - Device-Name flame - FW-Release 4.4.2 - FW-Incremental eng.cltbld.20141026.035227 - FW-Date Sun Oct 26 03:52:38 EDT 2014 - Bootloader L1TC00011880

Hi William, Could you please help check under the same environment as comment #133 on the dolphin 7715ea? Thanks a lot.

(In reply to shawn ku [:sku] from comment #132) > Please help check STR in comment 128. Partner claims there is an side > effect caused by the patch. I'll try and see if that's the case. If it is then the solution is most likely going to be bug 1102814. Leaving the NI until I verify this.

(In reply to helloarvin from comment #134) > Hi William, > > Could you please help check under the same environment as comment #133 on > the dolphin 7715ea? > Thanks a lot. @ Scenario 1. revision="2052ad921917c237f72d1f91040c0fb933c93662" Branch="v2.1" - Test result: The music app resumes to play music quickly. - Build information: (Flame v2.1) ~ Gaia-Rev 2052ad921917c237f72d1f91040c0fb933c93662 ~ Gecko-Rev 32ee81d6c8897c421880a2b9d3cc4ea4863e3170 ~ Build-ID 20150213140751 ~ Version 34.0 ~ Device-Name scx15_sp7715ea ~ FW-Release 4.4.2 ~ FW-Incremental 44 ~ FW-Date Tue Dec 30 13:56:10 CST 2014 @ Scenario 2. revision="fc85c28243d2d8baf90a76bbf36b8c1e608c190a" Branch="v2.1" - Test result: The music app resumes to play music quickly. - Build information: (Flame v2.1) ~ Gaia-Rev fc85c28243d2d8baf90a76bbf36b8c1e608c190a ~ Gecko-Rev 32ee81d6c8897c421880a2b9d3cc4ea4863e3170 ~ Build-ID 20150213140751 ~ Version 34.0 ~ Device-Name scx15_sp7715ea ~ FW-Release 4.4.2 ~ FW-Incremental 44 ~ FW-Date Tue Dec 30 13:56:10 CST 2014 By the way, please "DO NOT" trace regression bug on a fixed bug. I suggest to submit a new one.

Dear William, please help confirm from which channel the music will come out once resumed under scenario[1]? Speaker or earphone? Our test result locally with dolphin sp7715ea: After the call disconnected, the music indeed resume quickly but it will play via earphone about 4-5s first and then convert to play via speaker. Thanks. [1] @Scenario 1. revision="2052ad921917c237f72d1f91040c0fb933c93662" Branch="v2.1"

(In reply to helloarvin from comment #137) > Dear William, > > please help confirm from which channel the music will come out once resumed > under scenario[1]? Speaker or earphone? > > Our test result locally with dolphin sp7715ea: After the call disconnected, > the music indeed resume quickly but it will play via earphone about 4-5s > first and then convert to play via speaker. > > Thanks. > > [1] @Scenario 1. revision="2052ad921917c237f72d1f91040c0fb933c93662" > Branch="v2.1" Please provide the detailed test steps, prerequisite, and Gaia+Gecko information here if you need help. Many thanks.

(In reply to William Hsu [:whsu] from comment #138) > Please provide the detailed test steps, prerequisite, and Gaia+Gecko > information here if you need help. > Many thanks. Hi William, First to thank you for your effort to help us confirm the issue ,especially when this is a 2.1 branch issue. But I think we have not reached an agreement for the test result we need to pay attention to. For your questions mentioned in comment138,I list as follows: device:dolphin detailed test steps: Step1 )Play a music; Step2) Receive an incoming call and connect; Step3) Hang up the call. prerequisite:no Gaia+Gecko information: the same as the information you have used in comment136. pay attention: > What you have tested before is completely right,But the result you pay attention to is wrong.Though >music will resume quickly after the call,what we need to check is if the music is come out from >speaker quickly!!! Our test result locally with dolphin sp7715ea: >After the call disconnected, > the music indeed resume quickly but it will play via earphone about 4-5s(we could not hear music at >this time) and then convert to play via speaker(the music sound suddenly increased ). If any confusion,let us know. What's more,We have already filed a bug for this issue we mention,see in bug1115304. Thank you !!! If there are any question,let us know.

Hi, Jingmei, I would like to help you address this issue. But, you didn't provide clear information and the key point (like, earphone) while you requested me to do the test. So, I don't know how to help you address this problem. I only can guess what you thought. I think the test need earphone, right? If so, these are my reproduction steps. @ Prerequisite 1. Plug earphone @ Reproduction steps: 1. Launch music app 2. Play a song 3. Make a phone call to the device 4. Answer the phone call 5. Hang up the phone after 10 seconds 6. Check the sound @ Build 1. revision="2052ad921917c237f72d1f91040c0fb933c93662" Branch="v2.1" - Test result: The music app resumes to play music quickly, and the sound "always" come from earphone. - Build information: (Dolphin v2.1) ~ Gaia-Rev 2052ad921917c237f72d1f91040c0fb933c93662 ~ Gecko-Rev 32ee81d6c8897c421880a2b9d3cc4ea4863e3170 ~ Build-ID 20150213140751 ~ Version 34.0 ~ Device-Name scx15_sp7715ea ~ FW-Release 4.4.2 ~ FW-Incremental 44 ~ FW-Date Tue Dec 30 13:56:10 CST 2014 @ Build 2. revision="fc85c28243d2d8baf90a76bbf36b8c1e608c190a" Branch="v2.1" - Test result: The music app resumes to play music quickly, and the sound "always" come from earphone. - Build information: (Dolphin v2.1) ~ Gaia-Rev fc85c28243d2d8baf90a76bbf36b8c1e608c190a ~ Gecko-Rev 32ee81d6c8897c421880a2b9d3cc4ea4863e3170 ~ Build-ID 20150213140751 ~ Version 34.0 ~ Device-Name scx15_sp7715ea ~ FW-Release 4.4.2 ~ FW-Incremental 44 ~ FW-Date Tue Dec 30 13:56:10 CST 2014 Many thanks!

Dear William, Please help update the agreed conclusion on the scenario as offline talk when back,thanks. Merry Spring Festival!

(In reply to helloarvin from comment #141) > Dear William, > > Please help update the agreed conclusion on the scenario as offline talk > when back,thanks. > Merry Spring Festival! Hi, Arvin and Jingmei, I have updated result on Bug 1115304#c39. FYI. Happy New Year!

Clearing my NI as work on the aforementioned issue went ahead in another bug.