Switch Audio Preferences to per device.

Switch Audio Preferences to per device.
Currently Chrome stores one global audio volume pref; if a user switches devices, this value remains the same. Change this so that we remember what volume (and mute setting) a user switched to when on a particular device, so when we switch to that device, we can set the volume/mute setting back to it again.


R=brettw@chromium.org, hshi@chromium.org, stevenjb@chromium.org
BUG=175798
TEST=Switched between speakers and USB headphones to verify that audio settings were rememebered and switched to correctly.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198011

[hshi1, 2013-05-02 00:57:05]

https://codereview.chromium.org/14801002/diff/3001/chrome/browser/chromeos/au...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/3001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:19: using
std::min;
I don't think you're using std::max and std::min in this file?

[rkc, 2013-05-02 01:53:36]

https://codereview.chromium.org/14801002/diff/3001/chrome/browser/chromeos/au...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/3001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:19: using
std::min;
On 2013/05/02 00:57:05, hshi1 wrote:
> I don't think you're using std::max and std::min in this file?

Done.

[rkc, 2013-05-02 02:02:11]

Added stevenjb@ and pam@ for OWNER's reviews.
Need owners reviews for,
src/chromeos/ (stevenjb@)
src/chrome/browser/chromeos (stevenjb@)
src/chrome/browser/prefs (pam@)

[hshi1, 2013-05-02 02:59:56]

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:114:
pref_change_registrar_.Add(::prefs::kAudioCaptureAllowed, callback);
why the "::prefs"?

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:187:
registry->RegisterBooleanPref(::prefs::kAudioCaptureAllowed, true);
ditto

https://codereview.chromium.org/14801002/diff/6001/chromeos/audio/cras_audio_...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14801002/diff/6001/chromeos/audio/cras_audio_...
chromeos/audio/cras_audio_handler.cc:130: }
is the NOTREACHED() no longer needed?

[rkc, 2013-05-02 06:34:21]

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:114:
pref_change_registrar_.Add(::prefs::kAudioCaptureAllowed, callback);
On 2013/05/02 02:59:56, hshi1 wrote:
> why the "::prefs"?

Not sure, it's not needed. Removed.
Done.

https://codereview.chromium.org/14801002/diff/6001/chrome/browser/chromeos/au...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:187:
registry->RegisterBooleanPref(::prefs::kAudioCaptureAllowed, true);
On 2013/05/02 02:59:56, hshi1 wrote:
> ditto

Done.

https://codereview.chromium.org/14801002/diff/6001/chromeos/audio/cras_audio_...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14801002/diff/6001/chromeos/audio/cras_audio_...
chromeos/audio/cras_audio_handler.cc:130: }
On 2013/05/02 02:59:56, hshi1 wrote:
> is the NOTREACHED() no longer needed?

Yes, the NOTREACHED is no longer relevant. Since we are doing the active device
switching in Chrome, not having an active device is a normal situation - for
example when the user plugs out the active device (in which case we should just
return false).

[Pam (message me for reviews), 2013-05-02 15:11:40]

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_pref_handler_impl.h (right):

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_pref_handler_impl.h:17: // NOTE: This class
will be removed once we remove the old Audio Handler code.
Please turn this into a TODO, so there's a username associated with the note.

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:247: if
(CommandLine::ForCurrentProcess()->
I'm not too excited by this dependency on the command line. It would be cleaner
to pull the common pref registration for the old and new handlers out into
shared code, or alternatively to  have the new handler check to see if the
shared prefs have already been registered and register only its new ones if so.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/audio_pref...
File chromeos/audio/audio_pref_handler.h (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/audio_pref...
chromeos/audio/audio_pref_handler.h:17: // NOTE: This class will be removed once
we remove the old Audio Handler code.
Ditto TODO

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:159: LOG(WARNING) << "Tried to set volume
with output mute locked";
I haven't looked at the caller too closely, but this error message and the
method name don't appear to match. I would have expected something like "Tried
to change output mute while it was locked". Am I just confused, or is it a typo?

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:177: LOG(WARNING) << "Tried to set gain
with input mute locked";
Ditto confusing message.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.h:56:
scoped_refptr<AudioDevicesPrefHandler>audio_pref_handler);
Please restore the space after the >

[rkc, 2013-05-02 18:46:09]

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_pref_handler_impl.h (right):

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_pref_handler_impl.h:17: // NOTE: This class
will be removed once we remove the old Audio Handler code.
On 2013/05/02 15:11:40, Pam wrote:
> Please turn this into a TODO, so there's a username associated with the note.

Done.

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/prefs/brow...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/14801002/diff/14001/chrome/browser/prefs/brow...
chrome/browser/prefs/browser_prefs.cc:247: if
(CommandLine::ForCurrentProcess()->
On 2013/05/02 15:11:40, Pam wrote:
> I'm not too excited by this dependency on the command line. It would be
cleaner
> to pull the common pref registration for the old and new handlers out into
> shared code, or alternatively to  have the new handler check to see if the
> shared prefs have already been registered and register only its new ones if
so.

Done.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/audio_pref...
File chromeos/audio/audio_pref_handler.h (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/audio_pref...
chromeos/audio/audio_pref_handler.h:17: // NOTE: This class will be removed once
we remove the old Audio Handler code.
On 2013/05/02 15:11:40, Pam wrote:
> Ditto TODO

Done.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:159: LOG(WARNING) << "Tried to set volume
with output mute locked";
On 2013/05/02 15:11:40, Pam wrote:
> I haven't looked at the caller too closely, but this error message and the
> method name don't appear to match. I would have expected something like "Tried
> to change output mute while it was locked". Am I just confused, or is it a
typo?

It was a typo but looking back at this, this is a thoroughly unnecessary warning
and can actually get quite spammy in certain situations. Removed them.

Done.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.cc:177: LOG(WARNING) << "Tried to set gain
with input mute locked";
On 2013/05/02 15:11:40, Pam wrote:
> Ditto confusing message.

Removed for the same reasons as above.
Done.

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/14801002/diff/14001/chromeos/audio/cras_audio...
chromeos/audio/cras_audio_handler.h:56:
scoped_refptr<AudioDevicesPrefHandler>audio_pref_handler);
On 2013/05/02 15:11:40, Pam wrote:
> Please restore the space after the >

Done.

[hshi1, 2013-05-02 18:55:40]

lgtm

[brettw, 2013-05-02 19:09:48]

owners lgtm rubberstamp

[stevenjb, 2013-05-02 19:29:47]

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:40: }
nit: no {}

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:41: double
volume;
Needs a default; GetDouble will not set |volume| if it doesn't exist

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:49:
device_volume_settings_->SetDouble(active_device_id, volume_percent);
Do we need to check/limit the range of volume_percent?

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:62: }
nit: no {}

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:63: int mute;
Set default

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:96: :
device_mute_settings_(new base::DictionaryValue()),
Align : with P

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:121:
device_mute_settings_ .reset(mute_prefs->DeepCopy());
no ' ' before .

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:128: int mute;
needs default (or handle GetAsInteger returning false)

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:139:
device_volume_settings_ .reset(volume_prefs->DeepCopy());
no ' ' before .

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:147: double
volume;
default

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h (right):

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h:55: // use
default values of mute=off and volume=75%.
nit: ' ' around =

[rkc, 2013-05-02 19:43:44]

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:40: }
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:41: double
volume;
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> Needs a default; GetDouble will not set |volume| if it doesn't exist
> 

It is should be impossible for this call to fail since we just check for this
key a few lines above and if the key doesn't exist, the Migrate call creates the
key with a default value (same with the other Get calls).

Added a default anyway (here and all places below).

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:49:
device_volume_settings_->SetDouble(active_device_id, volume_percent);
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> Do we need to check/limit the range of volume_percent?

I am not sure if cras handles it, so added a check.
Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:62: }
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:63: int mute;
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> Set default

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:96: :
device_mute_settings_(new base::DictionaryValue()),
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> Align : with P

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:121:
device_mute_settings_ .reset(mute_prefs->DeepCopy());
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> no ' ' before .

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:128: int mute;
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> needs default (or handle GetAsInteger returning false)

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:139:
device_volume_settings_ .reset(volume_prefs->DeepCopy());
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> no ' ' before .

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.cc:147: double
volume;
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> default

Done.

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h (right):

https://codereview.chromium.org/14801002/diff/22001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h:55: // use
default values of mute=off and volume=75%.
On 2013/05/02 19:29:47, stevenjb (chromium) wrote:
> nit: ' ' around =

Done.

[stevenjb, 2013-05-02 20:16:51]

lgtm

[commit-bot: I haz the power, 2013-05-02 20:24:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/14801002/31001

[commit-bot: I haz the power, 2013-05-02 21:18:22]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[dgreid, 2013-05-02 21:21:49]

https://codereview.chromium.org/14801002/diff/31001/chrome/browser/chromeos/a...
File chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h (right):

https://codereview.chromium.org/14801002/diff/31001/chrome/browser/chromeos/a...
chrome/browser/chromeos/audio/audio_devices_pref_handler_impl.h:62:
scoped_ptr<base::DictionaryValue> device_mute_settings_;
Is this keyed based on device ids from the audio server?  Those are only unique
until they wrap (probably OK).  They should be considered random across power
cycles.

[rkc, 2013-05-02 23:45:59]

Committed patchset #6 manually as r198011 (presubmit successful).

[Pam (message me for reviews), 2013-05-03 07:33:32]

Post-facto LGTM for browser/prefs/ (brettw's LGTM covered it from
chrome/OWNERS).

- Pam