Store audio device's active state in preference

Store audio device's active state in preference

Use the stable device id provided by CRAS as key to
store the latest preference that user selects to or
away from an audio device.

BUG=308143
Committed: https://crrev.com/fe100d54b8455fdc706744a7f39f7d36653b2693
Cr-Commit-Position: refs/heads/master@{#368845}

[hychao, 2015-10-01 11:36:03]

hychao@chromium.org changed reviewers:
+ jennyz@chromium.org, rkc@chromium.org

[jennyz, 2015-12-17 22:50:22]

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_device.cc:134: std::string AudioDevice::ToString() const {
Please add stable_device_id.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler.h (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler.h:24: // as its default state value.
Any specific reason to use AUDIO_STATE_NOT_AVAILABL when an audio device got
plugged for the first time? It seems confusing without any further comments.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:94:
(int)AUDIO_STATE_NOT_AVAILABLE);
I guess this is what you mean in the .h comment for setting default device state
as not available if it got plugged in for the first time? A better comment might
be: The default value of the device state will be AUDIO_STATE_NOT_AVAILABLE if
the device is not found in the pref settings.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl_unittest.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl_unittest.cc:133: 
Would you please add a test case to test the write/read of the audio device
state pref?

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:240: void
CrasAudioHandler::ChangeActiveNodes(const NodeIdList& new_active_ids) {
What about multiple active audio nodes case?

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:912: // Otherwise, switch to a new active
audio device according to their priority.
The comments for how the active audio device is selected is kind of messy after
so many changes, mostly my fault. We should clean it up now. For this one, maybe
better be:
If audio nodes change is caused by unplugging some non-active audio devices, we 
wont't change the current active audio devices.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:927: // For all other cases, we will choose
the node with highest priority.
How about change this as:
If there is no current active node, or, no new hot plugged node, select the
active node by their priorities.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:930: } else {
How about comment like this:
If user has hot plugged any input nodes, look at the one with highest
priorty(normally, there is only one hot plugged input node), 1) If it was a new
device with higher priority, switch the active to this device. 2) If it was
device plugged in before, and was NOT selected as the active one last time
before it was unplugged, then don't change the current active device. 3) If it
was plugged in before and was selected as the active one before it was unplugged
last time, switch the active to this device.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:937: case AUDIO_STATE_NOT_AVAILABLE:
This is the case if the device is new, not plugged in before, right? If true,
please add a comment.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:941: case AUDIO_STATE_INACTIVE:
No need to add this line, it should fall into default case. If you would like to
add this line for drawing attention for this case, please add a comment.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:953: // internal audio output.
gitto input nodes logic.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:972: case AUDIO_STATE_INACTIVE:
ditto

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2059: TEST_F(CrasAudioHandlerTest,
UnselectActiveDeviceAndPlugAnother) {
The test case name can be changed to something more readable, for example,
ActiveDeviceSelectionWithStableDeviceId, anyway, the current name is not very
self-explaining.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2074:
cras_audio_handler_->GetPrimaryActiveOutputNode());
Add some comment will be more helpful to explain what you are testing. For
example, initially, the active device is selected based on priority, USB headset
is selected.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2077:
cras_audio_handler_->SwitchToDevice(speaker, true);
Please add a comment: Now change the active device to internal speaker, and usb
headset becomes inactive.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2087: // Plug the USB headset of
the same device stable id, but with different id
nits: add "." at the end. How about: Plug the same USB headset back, id is
different, but stable_device_id remains the same.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2093:
cras_audio_handler_->GetPrimaryActiveOutputNode());
Please add a comment: "Since usb headset was inactive before it is unplugged, it
won't be selected as the active device after it is plugged in again.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2101:
cras_audio_handler_->GetPrimaryActiveOutputNode());
Please add a comment: Since the second usb headset is a new device, it is
selected as the active device by its priority.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2110:
cras_audio_handler_->GetPrimaryActiveOutputNode());
When hotplug a new audio device, there are three cases: 1. The device is new; 2.
The device was plugged in before, inactive before it was unplugged last time; 3.
The device was plugged in before, active before it was unplugged. 

You have covered 1 & 2 cases. Can you add the #3 case?

https://codereview.chromium.org/1380103003/diff/20001/chromeos/chromeos_pref_...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:36: const char kAudioDevicesState[] =
"setting.audio.device_state";
what about "settings.audio.devices.state"?

[jennyz, 2015-12-17 22:54:34]

By the way, we will need to pass stabel_device_id in the extension audio api for
hotrod use. I can add that in after you land this cl.

[hychao, 2015-12-29 10:57:25]

Thanks for the detailed review!

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_device.cc:134: std::string AudioDevice::ToString() const {
On 2015/12/17 22:50:20, jennyz wrote:
> Please add stable_device_id.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler.h (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler.h:24: // as its default state value.
On 2015/12/17 22:50:21, jennyz wrote:
> Any specific reason to use AUDIO_STATE_NOT_AVAILABL when an audio device got
> plugged for the first time? It seems confusing without any further comments.

Updated as you suggested in another comment. Thanks!

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:94:
(int)AUDIO_STATE_NOT_AVAILABLE);
On 2015/12/17 22:50:21, jennyz wrote:
> I guess this is what you mean in the .h comment for setting default device
state
> as not available if it got plugged in for the first time? A better comment
might
> be: The default value of the device state will be AUDIO_STATE_NOT_AVAILABLE if
> the device is not found in the pref settings.

Updated the header file.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl_unittest.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl_unittest.cc:133: 
On 2015/12/17 22:50:21, jennyz wrote:
> Would you please add a test case to test the write/read of the audio device
> state pref?

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:240: void
CrasAudioHandler::ChangeActiveNodes(const NodeIdList& new_active_ids) {
On 2015/12/17 22:50:21, jennyz wrote:
> What about multiple active audio nodes case?

actually I forgot about the multiple active node path when I worked on this
commit. By current implementation, only the primary (1st added) active node will
have its state stored into preference. And potentially affect which node to
switch to, at the next time nodes changed.

Other options for multiple active nodes are:
(1) store device state for every node
(2) exclude multiple active node path from storing device state

For me it's not trivial to tell which option makes the most sense. It's a
complicated scenario when using both extension API and UI to change active node
at the same time, so it's hard to say how user would expect the behavior to be.
Plus that after stable_device_id is exposed through extension API, Hotrod app is
going to maintain its own device selection preference. So I'd prefer not adding
more code to implement (1) or (2).

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:912: // Otherwise, switch to a new active
audio device according to their priority.
On 2015/12/17 22:50:21, jennyz wrote:
> The comments for how the active audio device is selected is kind of messy
after
> so many changes, mostly my fault. We should clean it up now. For this one,
maybe
> better be:
> If audio nodes change is caused by unplugging some non-active audio devices,
we 
> wont't change the current active audio devices.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:927: // For all other cases, we will choose
the node with highest priority.
On 2015/12/17 22:50:21, jennyz wrote:
> How about change this as:
> If there is no current active node, or, no new hot plugged node, select the
> active node by their priorities.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:930: } else {
On 2015/12/17 22:50:21, jennyz wrote:
> How about comment like this:
> If user has hot plugged any input nodes, look at the one with highest
> priorty(normally, there is only one hot plugged input node), 1) If it was a
new
> device with higher priority, switch the active to this device. 2) If it was
> device plugged in before, and was NOT selected as the active one last time
> before it was unplugged, then don't change the current active device. 3) If it
> was plugged in before and was selected as the active one before it was
unplugged
> last time, switch the active to this device.

Thanks for the suggestion. I broke down and moved the comments of these device
states to each the switch case below, to make it more readable.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:937: case AUDIO_STATE_NOT_AVAILABLE:
On 2015/12/17 22:50:21, jennyz wrote:
> This is the case if the device is new, not plugged in before, right? If true,
> please add a comment.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:941: case AUDIO_STATE_INACTIVE:
On 2015/12/17 22:50:21, jennyz wrote:
> No need to add this line, it should fall into default case. If you would like
to
> add this line for drawing attention for this case, please add a comment.

Thanks for reminding. I think its useful to keep this line, so I can put the
comment right next to AUDIO_STATE_INACTIVE case.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:953: // internal audio output.
On 2015/12/17 22:50:21, jennyz wrote:
> gitto input nodes logic.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:972: case AUDIO_STATE_INACTIVE:
On 2015/12/17 22:50:21, jennyz wrote:
> ditto

Although the comments are not copied over, I keep this line for symmetry to the
input logic.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2059: TEST_F(CrasAudioHandlerTest,
UnselectActiveDeviceAndPlugAnother) {
On 2015/12/17 22:50:21, jennyz wrote:
> The test case name can be changed to something more readable, for example,
> ActiveDeviceSelectionWithStableDeviceId, anyway, the current name is not very
> self-explaining.

Done.
This do sounds better, thanks.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2074:
cras_audio_handler_->GetPrimaryActiveOutputNode());
On 2015/12/17 22:50:21, jennyz wrote:
> Add some comment will be more helpful to explain what you are testing. For
> example, initially, the active device is selected based on priority, USB
headset
> is selected.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2077:
cras_audio_handler_->SwitchToDevice(speaker, true);
On 2015/12/17 22:50:21, jennyz wrote:
> Please add a comment: Now change the active device to internal speaker, and
usb
> headset becomes inactive.

Done.
Also added a check below to make sure USB is no longer the active node.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2087: // Plug the USB headset of
the same device stable id, but with different id
On 2015/12/17 22:50:21, jennyz wrote:
> nits: add "." at the end. How about: Plug the same USB headset back, id is
> different, but stable_device_id remains the same.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2093:
cras_audio_handler_->GetPrimaryActiveOutputNode());
On 2015/12/17 22:50:21, jennyz wrote:
> Please add a comment: "Since usb headset was inactive before it is unplugged,
it
> won't be selected as the active device after it is plugged in again.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2101:
cras_audio_handler_->GetPrimaryActiveOutputNode());
On 2015/12/17 22:50:21, jennyz wrote:
> Please add a comment: Since the second usb headset is a new device, it is
> selected as the active device by its priority.

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler_unittest.cc:2110:
cras_audio_handler_->GetPrimaryActiveOutputNode());
On 2015/12/17 22:50:21, jennyz wrote:
> When hotplug a new audio device, there are three cases: 1. The device is new;
2.
> The device was plugged in before, inactive before it was unplugged last time;
3.
> The device was plugged in before, active before it was unplugged. 
> 
> You have covered 1 & 2 cases. Can you add the #3 case?

Done.

https://codereview.chromium.org/1380103003/diff/20001/chromeos/chromeos_pref_...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:36: const char kAudioDevicesState[] =
"setting.audio.device_state";
On 2015/12/17 22:50:21, jennyz wrote:
> what about "settings.audio.devices.state"?

Done.

[jennyz, 2016-01-05 23:46:55]

LGTM with nits. Thanks for putting this together!

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:240: void
CrasAudioHandler::ChangeActiveNodes(const NodeIdList& new_active_ids) {
On 2015/12/29 10:57:25, hychao wrote:
> On 2015/12/17 22:50:21, jennyz wrote:
> > What about multiple active audio nodes case?
> 
> actually I forgot about the multiple active node path when I worked on this
> commit. By current implementation, only the primary (1st added) active node
will
> have its state stored into preference. And potentially affect which node to
> switch to, at the next time nodes changed.
> 
> Other options for multiple active nodes are:
> (1) store device state for every node
> (2) exclude multiple active node path from storing device state
> 
> For me it's not trivial to tell which option makes the most sense. It's a
> complicated scenario when using both extension API and UI to change active
node
> at the same time, so it's hard to say how user would expect the behavior to
be.
> Plus that after stable_device_id is exposed through extension API, Hotrod app
is
> going to maintain its own device selection preference. So I'd prefer not
adding
> more code to implement (1) or (2).

Yes, it makes sense to exclude the new active node preference logic from this
code path, since this is only used by hotrod, who maintains its own selection
preference. Please add a comment for the exclusion reason.

https://codereview.chromium.org/1380103003/diff/40001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/1380103003/diff/40001/chromeos/audio/audio_de...
chromeos/audio/audio_device.cc:161: stable_device_id);
nit: how about moving this after line 143 to keep the sequence of the properties
consistent with their declaration?

[hychao, 2016-01-06 06:34:12]

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/20001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:240: void
CrasAudioHandler::ChangeActiveNodes(const NodeIdList& new_active_ids) {
On 2016/01/05 23:46:55, jennyz wrote:
> On 2015/12/29 10:57:25, hychao wrote:
> > On 2015/12/17 22:50:21, jennyz wrote:
> > > What about multiple active audio nodes case?
> > 
> > actually I forgot about the multiple active node path when I worked on this
> > commit. By current implementation, only the primary (1st added) active node
> will
> > have its state stored into preference. And potentially affect which node to
> > switch to, at the next time nodes changed.
> > 
> > Other options for multiple active nodes are:
> > (1) store device state for every node
> > (2) exclude multiple active node path from storing device state
> > 
> > For me it's not trivial to tell which option makes the most sense. It's a
> > complicated scenario when using both extension API and UI to change active
> node
> > at the same time, so it's hard to say how user would expect the behavior to
> be.
> > Plus that after stable_device_id is exposed through extension API, Hotrod
app
> is
> > going to maintain its own device selection preference. So I'd prefer not
> adding
> > more code to implement (1) or (2).
> 
> Yes, it makes sense to exclude the new active node preference logic from this
> code path, since this is only used by hotrod, who maintains its own selection
> preference. Please add a comment for the exclusion reason.

Done.

https://codereview.chromium.org/1380103003/diff/40001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.cc (right):

https://codereview.chromium.org/1380103003/diff/40001/chromeos/audio/audio_de...
chromeos/audio/audio_device.cc:161: stable_device_id);
On 2016/01/05 23:46:55, jennyz wrote:
> nit: how about moving this after line 143 to keep the sequence of the
properties
> consistent with their declaration?

Done.

[hychao, 2016-01-06 06:34:51]

hychao@chromium.org changed reviewers:
+ derat@chromium.org

[jennyz, 2016-01-06 18:11:22]

lgtm

[hychao, 2016-01-07 02:12:11]

The CQ bit was checked by hychao@chromium.org

[commit-bot: I haz the power, 2016-01-07 02:12:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/60001

[commit-bot: I haz the power, 2016-01-07 02:23:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-07 02:23:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2016-01-07 02:46:15]

https://codereview.chromium.org/1380103003/diff/60001/chromeos/chromeos_pref_...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/60001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:36: const char kAudioDevicesState[] =
"settings.audio.device_state";
please add a comment similar to the ones on the other prefs:

- what is it used for?
- what type of pref is this (int, bool)?
- what do the different values mean?

[hychao, 2016-01-07 03:14:16]

https://codereview.chromium.org/1380103003/diff/60001/chromeos/chromeos_pref_...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/60001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:36: const char kAudioDevicesState[] =
"settings.audio.device_state";
On 2016/01/07 02:46:15, Daniel Erat wrote:
> please add a comment similar to the ones on the other prefs:
> 
> - what is it used for?
> - what type of pref is this (int, bool)?
> - what do the different values mean?

Done.

[Daniel Erat, 2016-01-07 17:31:12]

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.h (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_device.h:61: uint64_t stable_device_id;
document the difference between this and id. why do we need both? is the plan to
eventually transition to just using one?

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler.h (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler.h:25: AUDIO_STATE_ACTIVE,
explicitly specify the numbers for these and document that these can't be
renumbered (since that would break existing prefs)

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:37:
base::Uint64ToString(device.stable_device_id &
is this going to break everyone's existing volume and mute prefs, or do the
stable ids initially match the old ids?

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:93: UpdateDevicesStatePref();
whey are you calling this every time in GetDeviceState() instead of just calling
it once in the c'tor as is done for mute and volume?

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:97:
(int)AUDIO_STATE_NOT_AVAILABLE);
use c++ static_casts and/or reinterpret_casts rather than c casts here and
everywhere else

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:214: void
AudioDevicesPrefHandlerImpl::UpdateDevicesStatePref() {
ReadDevicesStatePref() or LoadDevicesStatePref() seem like they would be more
accurate names, since the pref isn't being updated here

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:225: while (!it.IsAtEnd()) {
can you just call DictionaryValue::MergeDictionary() here?

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:939: case AUDIO_STATE_ACTIVE:
this should be indented two spaces beyond the previous line. please run
clang-format

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:974: case AUDIO_STATE_ACTIVE:
same here

https://codereview.chromium.org/1380103003/diff/80001/chromeos/chromeos_pref_...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:36: // An integer pref storing the latest state
of an audio device in Chrome OS UI.
this isn't correct, is it? it looks like you're registering this as a dictionary
pref, not an integer.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/chromeos_pref_...
chromeos/chromeos_pref_names.cc:38: // as the active one for audio I/O, or it's
a new plugged device.
if the different values are stored in an enum, please include the enum's name
here.

[hychao, 2016-01-11 07:02:32]

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_device.h (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_device.h:61: uint64_t stable_device_id;
On 2016/01/07 17:31:12, Daniel Erat wrote:
> document the difference between this and id. why do we need both? is the plan
to
> eventually transition to just using one?

Done. Added comment to describe the two IDs. There is no plan to deprecate any
one of them, but expect them to be used in a mixed way for different scenario.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler.h (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler.h:25: AUDIO_STATE_ACTIVE,
On 2016/01/07 17:31:12, Daniel Erat wrote:
> explicitly specify the numbers for these and document that these can't be
> renumbered (since that would break existing prefs)

Done.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
File chromeos/audio/audio_devices_pref_handler_impl.cc (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:37:
base::Uint64ToString(device.stable_device_id &
On 2016/01/07 17:31:12, Daniel Erat wrote:
> is this going to break everyone's existing volume and mute prefs, or do the
> stable ids initially match the old ids?

The device id doesn't work as it supposed to, due to that it doesn't persist
after plug/unplug or power off/on, and that is why we added stable_device_id to
fix the problem. 
That said, audio related preference is already broken. It is impossible to make
this backward compatible because there could be multiple key strings exists in
the dictionary all points to the same device.
I notice that the 'device_name' could be modified in system layer, so I remove
it from the string in the new patch set, to keep the uniqueness of the key.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:93: UpdateDevicesStatePref();
On 2016/01/07 17:31:12, Daniel Erat wrote:
> whey are you calling this every time in GetDeviceState() instead of just
calling
> it once in the c'tor as is done for mute and volume?

Done.
Yes you're right, call this once in constructor is sufficient.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:97:
(int)AUDIO_STATE_NOT_AVAILABLE);
On 2016/01/07 17:31:12, Daniel Erat wrote:
> use c++ static_casts and/or reinterpret_casts rather than c casts here and
> everywhere else

Done.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:214: void
AudioDevicesPrefHandlerImpl::UpdateDevicesStatePref() {
On 2016/01/07 17:31:12, Daniel Erat wrote:
> ReadDevicesStatePref() or LoadDevicesStatePref() seem like they would be more
> accurate names, since the pref isn't being updated here

Done. Thanks for the suggestion, modified other functions in this file. I think
LoadDevices*Pref() are the most accurate name here.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/audio_de...
chromeos/audio/audio_devices_pref_handler_impl.cc:225: while (!it.IsAtEnd()) {
On 2016/01/07 17:31:12, Daniel Erat wrote:
> can you just call DictionaryValue::MergeDictionary() here?

Done. Thanks!

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:939: case AUDIO_STATE_ACTIVE:
On 2016/01/07 17:31:12, Daniel Erat wrote:
> this should be indented two spaces beyond the previous line. please run
> clang-format

Done.

https://codereview.chromium.org/1380103003/diff/80001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.cc:974: case AUDIO_STATE_ACTIVE:
On 2016/01/07 17:31:12, Daniel Erat wrote:
> same here

Done.

[Daniel Erat, 2016-01-11 14:39:30]

lgtm

thanks. lgtm-ing since i'll be out for most of this week, but please address the
remaining comment and ask stevenjb@ to also take a look at the change if you
want additional feedback.

https://codereview.chromium.org/1380103003/diff/120001/chromeos/chromeos_pref...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/120001/chromeos/chromeos_pref...
chromeos/chromeos_pref_names.cc:36: // An integer pref storing the latest state
of an audio device in Chrome OS UI.
i think you missed the previous comments i left on this file. this isn't an
integer pref; it's a dictionary pref mapping (iirc) from stable device id to the
device state. please document that here and include the name of the enum

[hychao, 2016-01-12 04:21:14]

Thank you for the detailed review, Daniel!

https://codereview.chromium.org/1380103003/diff/120001/chromeos/chromeos_pref...
File chromeos/chromeos_pref_names.cc (right):

https://codereview.chromium.org/1380103003/diff/120001/chromeos/chromeos_pref...
chromeos/chromeos_pref_names.cc:36: // An integer pref storing the latest state
of an audio device in Chrome OS UI.
On 2016/01/11 14:39:30, Daniel Erat wrote:
> i think you missed the previous comments i left on this file. this isn't an
> integer pref; it's a dictionary pref mapping (iirc) from stable device id to
the
> device state. please document that here and include the name of the enum

Done.

[hychao, 2016-01-12 04:21:24]

The CQ bit was checked by hychao@chromium.org

[hychao, 2016-01-12 04:21:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org,
derat@chromium.org
Link to the patchset: https://codereview.chromium.org/1380103003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-01-12 04:21:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/140001

[commit-bot: I haz the power, 2016-01-12 04:37:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 04:37:53]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hychao, 2016-01-12 06:38:03]

The CQ bit was checked by hychao@chromium.org

[hychao, 2016-01-12 06:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/1380103003/#ps160001
(title: "fix audio_apitest")

[commit-bot: I haz the power, 2016-01-12 06:38:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/160001

[commit-bot: I haz the power, 2016-01-12 06:49:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 06:49:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rkc, 2016-01-12 07:07:32]

rubberstamp LGTM since derat and jennzy already have lgtm'ed

[hychao, 2016-01-12 07:24:23]

The CQ bit was checked by hychao@chromium.org

[commit-bot: I haz the power, 2016-01-12 07:24:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/160001

[commit-bot: I haz the power, 2016-01-12 07:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 07:51:54]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[hychao, 2016-01-12 08:02:56]

The CQ bit was checked by hychao@chromium.org

[hychao, 2016-01-12 08:02:56]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
derat@chromium.org, jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/1380103003/#ps180001
(title: "Fix string format")

[commit-bot: I haz the power, 2016-01-12 08:04:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/180001

[commit-bot: I haz the power, 2016-01-12 08:12:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-12 08:12:44]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hychao, 2016-01-12 09:41:37]

The CQ bit was checked by hychao@chromium.org

[commit-bot: I haz the power, 2016-01-12 09:41:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380103003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380103003/180001

[commit-bot: I haz the power, 2016-01-12 10:18:00]

Description was changed from

==========
Store audio device's active state in preference

Use the stable device id provided by CRAS as key to
store the latest preference that user selects to or
away from an audio device.

BUG=308143
==========

to

==========
Store audio device's active state in preference

Use the stable device id provided by CRAS as key to
store the latest preference that user selects to or
away from an audio device.

BUG=308143
==========

[commit-bot: I haz the power, 2016-01-12 10:18:01]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-12 10:18:43]

Description was changed from

==========
Store audio device's active state in preference

Use the stable device id provided by CRAS as key to
store the latest preference that user selects to or
away from an audio device.

BUG=308143
==========

to

==========
Store audio device's active state in preference

Use the stable device id provided by CRAS as key to
store the latest preference that user selects to or
away from an audio device.

BUG=308143

Committed: https://crrev.com/fe100d54b8455fdc706744a7f39f7d36653b2693
Cr-Commit-Position: refs/heads/master@{#368845}
==========

[commit-bot: I haz the power, 2016-01-12 10:18:44]

Patchset 10 (id:??) landed as
https://crrev.com/fe100d54b8455fdc706744a7f39f7d36653b2693
Cr-Commit-Position: refs/heads/master@{#368845}