Store audio device's active state in preference

chromeos/audio/audio_devices_pref_handler_impl.cc

Index: chromeos/audio/audio_devices_pref_handler_impl.cc
diff --git a/chromeos/audio/audio_devices_pref_handler_impl.cc b/chromeos/audio/audio_devices_pref_handler_impl.cc
index 45aa433eb9ed1628cf19eb5f41a06158cf1e7fee..fa5d417b100db251ffbff49f764e83e1e9045b6f 100644
--- a/chromeos/audio/audio_devices_pref_handler_impl.cc
+++ b/chromeos/audio/audio_devices_pref_handler_impl.cc
@@ -34,7 +34,8 @@ const int kPrefMuteOn = 1;
 std::string GetDeviceIdString(const chromeos::AudioDevice& device) {
   std::string device_id_string =
       device.device_name + " : " +
-      base::Uint64ToString(device.id & static_cast<uint64_t>(0xffffffff)) +
+      base::Uint64ToString(device.stable_device_id &

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

is this going to break everyone's existing volume and mute prefs, or do the
stable ids initially match the old ids?

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

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.

+                           static_cast<uint64_t>(0xffffffff)) +
       " : " + (device.is_input ? "1" : "0");
   // Replace any periods from the device id string with a space, since setting
   // names cannot contain periods.
@@ -87,6 +88,27 @@ void AudioDevicesPrefHandlerImpl::SetMuteValue(const AudioDevice& device,
   SaveDevicesMutePref();
 }
 
+AudioDeviceState AudioDevicesPrefHandlerImpl::GetDeviceState(
+    const AudioDevice& device) {
+  UpdateDevicesStatePref();

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

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?

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

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

+  std::string device_id_str = GetDeviceIdString(device);
+  if (!device_state_settings_->HasKey(device_id_str)) {
+    device_state_settings_->SetInteger(device_id_str,
+                                       (int)AUDIO_STATE_NOT_AVAILABLE);

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

use c++ static_casts and/or reinterpret_casts rather than c casts here and
everywhere else

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

Done.

+    SaveDevicesStatePref();
+  }
+  int state = (int)AUDIO_STATE_NOT_AVAILABLE;
+  device_state_settings_->GetInteger(device_id_str, &state);
+  return (AudioDeviceState)state;
+}
+
+void AudioDevicesPrefHandlerImpl::SetDeviceState(const AudioDevice& device,
+                                                 AudioDeviceState state) {
+  device_state_settings_->SetInteger(GetDeviceIdString(device),
+                                     (int)state);
+  SaveDevicesStatePref();
+}
+
 bool AudioDevicesPrefHandlerImpl::GetAudioOutputAllowedValue() {
   return local_state_->GetBoolean(prefs::kAudioOutputAllowed);
 }
@@ -132,6 +154,7 @@ AudioDevicesPrefHandlerImpl::AudioDevicesPrefHandlerImpl(
     PrefService* local_state)
     : device_mute_settings_(new base::DictionaryValue()),
       device_volume_settings_(new base::DictionaryValue()),
+      device_state_settings_(new base::DictionaryValue()),
       local_state_(local_state) {
   InitializePrefObservers();
 
@@ -188,6 +211,26 @@ void AudioDevicesPrefHandlerImpl::SaveDevicesVolumePref() {
   }
 }
 
+void AudioDevicesPrefHandlerImpl::UpdateDevicesStatePref() {

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

ReadDevicesStatePref() or LoadDevicesStatePref() seem like they would be more
accurate names, since the pref isn't being updated here

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

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

+  const base::DictionaryValue* state_prefs =
+      local_state_->GetDictionary(prefs::kAudioDevicesState);
+  if (state_prefs)
+    device_state_settings_.reset(state_prefs->DeepCopy());
+}
+
+void AudioDevicesPrefHandlerImpl::SaveDevicesStatePref() {
+  DictionaryPrefUpdate dict_update(local_state_,
+                                   prefs::kAudioDevicesState);
+  base::DictionaryValue::Iterator it(*device_state_settings_);
+  while (!it.IsAtEnd()) {

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

can you just call DictionaryValue::MergeDictionary() here?

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

Done. Thanks!

+    int state = AUDIO_STATE_NOT_AVAILABLE;
+    bool success = it.value().GetAsInteger(&state);
+    DCHECK(success);
+    dict_update->SetInteger(it.key(), state);
+    it.Advance();
+  }
+}
+
 void AudioDevicesPrefHandlerImpl::MigrateDeviceMuteSettings(
     const std::string& active_device) {
   int old_mute = local_state_->GetInteger(prefs::kAudioMute);
@@ -212,6 +255,7 @@ void AudioDevicesPrefHandlerImpl::NotifyAudioPolicyChange() {
 void AudioDevicesPrefHandlerImpl::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kAudioDevicesVolumePercent);
   registry->RegisterDictionaryPref(prefs::kAudioDevicesMute);
+  registry->RegisterDictionaryPref(prefs::kAudioDevicesState);
 
   // Register the prefs backing the audio muting policies.
   // Policy for audio input is handled by kAudioCaptureAllowed in the Chrome