Repro: - Connect with a bluetooth headset - Press volume up/down Expected Behaviour: - Audio stream volume should be set to assigned value from bluetooth headset Actual Behaviour: - Wrong volume value

Comment on attachment 674125 [details] [diff] [review] Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume Review of attachment 674125 [details] [diff] [review]: ----------------------------------------------------------------- I'm not against the change but this code will break $GAIA/apps/system/js/hardware_buttons.js so r-. You may want to ask the r? to :djf next time too, he is the hardware button guy :)

CC'ing David Flanagan.

Comment on attachment 674126 [details] [diff] [review] Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js Review of attachment 674126 [details] [diff] [review]: ----------------------------------------------------------------- I guess this code will change depending on the mozChromeEvent you need to make hardware_buttons.js happy.

Comment on attachment 674124 [details] [diff] [review] Patch 1(v1): BluetoothHfpManager receive Vgs from headset Review of attachment 674124 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +143,5 @@ > StaticRefPtr<BluetoothHfpManagerObserver> sHfpObserver; > bool gInShutdown = false; > static nsCOMPtr<nsIThread> sHfpCommandThread; > static bool sStopSendingRingFlag = true; > + static bool sReceiveVgsFlag = false; Do we really need to keep making these static? This isn't accessed like sStopSendingRingFlag, which is becoming a mamber anyways as of bug 804687. Seems like this could just be a member too? @@ +225,5 @@ > { > + float volume; > + nsCOMPtr<nsIAudioManager> am = do_GetService("@mozilla.org/telephony/audiomanager;1"); > + if (!am) { > + NS_WARNING("Failed to get AudioManager Service!"); Don't fail in the constructor. Do this in a failable init function or something. @@ +489,3 @@ > newVgs *= 10; > + newVgs += tmp - '0'; > + } Why not just throw this into a ns*String and use ToInteger?

Comment on attachment 674125 [details] [diff] [review] Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume Please help to check. Thanks!

Comment on attachment 674126 [details] [diff] [review] Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js Please help to check. Thanks!

- No return in constructor - Make static variable into member variable (mReceiveVgsFlag) - Use nsACString.ToInteger()

Comment on attachment 674506 [details] [diff] [review] Patch 1(v2): BluetoothHfpManager receive Vgs from headset Review of attachment 674506 [details] [diff] [review]: ----------------------------------------------------------------- You made the nsCOMPtr a member, but I don't see a header file? Does this patch have everything? ::: dom/bluetooth/BluetoothHfpManager.cpp @@ +226,5 @@ > + float volume; > + nsCOMPtr<nsIAudioManager> am = do_GetService("@mozilla.org/telephony/audiomanager;1"); > + if (!am) { > + NS_WARNING("Failed to get AudioManager Service!"); > + return; This looks like it's still erroring in the constructor, instead of having an init function now? @@ +482,5 @@ > // HS volume range: [0, 15] > + nsCString vgsStr; > + while (length--) { > + vgsStr.Append(msg[index++]); > + } Nit: Can't we just substr the string based on the length to pull out the number, and append that?

Oops, my fault! All changes should be included in this patch now. Also move the initialization of mCurrentVgs to Init().

Comment on attachment 674125 [details] [diff] [review] Patch 2(v1): Send Chrome event of type 'volumeset' when bluetooth headset adjust volume Review of attachment 674125 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that this will break hardware_buttons.js as Vivien has suggested. You won't get any of the auto-repeat functionality from that module, of course, though I suppose that the headset itself might give you that. I think the ideal thing would be to send a volumeset chrome event when the bluetooth headset first connects with the phone, and use that event simply to set the audio.volume.master setting. Then, when the user alters the volume on their headset, it would be great if you could send the volume-up-press volume-up-release events as the current code does. But since I don't know anything about how the gecko bluetooth code works, I don't know if that is possible. It looks like this will work, though, so I'm going to give it an r+. Please note, though, that I don't have a bluetooth headset and have not tested this.

Comment on attachment 674126 [details] [diff] [review] Patch 3(v1): Add event handler for 'volumeset' in sound_manager.js Review of attachment 674126 [details] [diff] [review]: ----------------------------------------------------------------- I would have preferred to have this code that handles the volumeset event in the same patch as the code that generates the event, so we would be certain that they got merged together. But that is not a big deal. While reviewing this patch, I noticed a bug in sound_manager.js. On line 21, the default value for audio.volume.master is 5, when it should be 0.5. Would you fix that as part of this patch so we don't have to file, fix, and review a new bug for it? r+ with the bug fix and a comment about the volumeset event. ::: apps/system/js/sound_manager.js @@ +9,5 @@ > }); > window.addEventListener('volumedown', function() { > changeVolume(-1); > }); > + window.addEventListener('mozChromeEvent', function(e) { Please add a comment before this line explaining that this chrome event is generated in shell.js in response to the bluetooth headset. @@ +33,5 @@ > > var activeTimeout = 0; > function changeVolume(delta) { > if (currentVolume == 0 || > + ((currentVolume + delta) <= 0)) { I think this is correct, but without a bluetooth headset I can't actually test it. This part of sound_manager.js is really complex and ugly. If I was writing it, I'd just allow volume to be an integer between -1 and 10 instead of 0 and 10. -1 would me no sound and no vibration. 0 would mean vibration but no sound, and > 0 would be a volume. You don't have to fix that as part of this patch. I just want to note that it is confusing code, and this change makes it even more confusing.

(In reply to David Flanagan [:djf] from comment #13) > I think the ideal thing would be to send a volumeset chrome event when the > bluetooth headset first connects with the phone, and use that event simply > to set the audio.volume.master setting. Then, when the user alters the > volume on their headset, it would be great if you could send the > volume-up-press volume-up-release events as the current code does. But > since I don't know anything about how the gecko bluetooth code works, I > don't know if that is possible. Something different between volume button on bluetooth headset and volume button on the phone. When user press the volume button on bluetooth headset, we will get a new volume rather than +1 or -1. For example, the audio volume is set to 5 currently, when pressing volume-up button on bluetooth headset, it's volume will be set to 8 and send a message to notify the phone. If we press volume-up button again, it's volume will be set to 10 and send a message again. (The volume stream value is not fixed and depends on the implementation of bluetooth headsets. Two headsets may have different values when pressing volume buttons.) In this way, we have to (re)set audio volume whenever user press volume button from a bluetooth headset rather. On the other hand, some bluetooth headset will send its volume when it first connects with the phone, and we notify observers of "bluetooth-volume-change", and will dispatch a mozChromeEvent typed 'volumeset' from shell.js to sound_manager.js. In sound_maanger.js, we'll show volume bar and update audio.volume.master setting.

Add comment and fix bug (initial value of currentVolume)

try server for patch 1 and patch 2: https://tbpl.mozilla.org/?tree=Try&rev=c435dff5e2c9 https://tbpl.mozilla.org/?tree=Try&rev=cb559dfafbd9

https://github.com/mozilla-b2g/gaia/commit/d46c4a62d4ed8cf640833ba5455c9ee2697 Patch 3 merged.

mercurial format patch

mercurial format patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/814a4bca12eb https://hg.mozilla.org/integration/mozilla-inbound/rev/04c6d4d7e578

https://hg.mozilla.org/mozilla-central/rev/814a4bca12eb https://hg.mozilla.org/mozilla-central/rev/04c6d4d7e578

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b7b9f972ea0 https://hg.mozilla.org/releases/mozilla-aurora/rev/531a1ad0ab94