Store audio device's active state in preference

chromeos/audio/audio_devices_pref_handler_impl.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/audio/audio_devices_pref_handler_impl.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 
@@ skipping 16 matching lines @@
 // Gets the device id string for storing audio preference. The format of
 // device string is a string consisting of 3 parts.
 // |device_name| : |integer from lower 32 bit of device id| :
 // |0(output device) or 1(input device)|
 // If an audio device has both integrated input and output devices, the first 2
 // parts of the string could be identical, only the last part will differentiate
 // them.
 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.
   std::replace(device_id_string.begin(), device_id_string.end(), '.', ' ');
   return device_id_string;
 }
 
 }  // namespace
 
 namespace chromeos {
@@ skipping 32 matching lines @@
   return (mute == kPrefMuteOn);
 }
 
 void AudioDevicesPrefHandlerImpl::SetMuteValue(const AudioDevice& device,
                                                bool mute) {
   device_mute_settings_->SetInteger(GetDeviceIdString(device),
                                     mute ? kPrefMuteOn : kPrefMuteOff);
   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);
 }
 
 void AudioDevicesPrefHandlerImpl::AddAudioPrefObserver(
     AudioPrefObserver* observer) {
   observers_.AddObserver(observer);
 }
 
 void AudioDevicesPrefHandlerImpl::RemoveAudioPrefObserver(
@@ skipping 25 matching lines @@
   if (device.type == AUDIO_TYPE_HDMI)
     return kDefaultHdmiOutputVolumePercent;
   else
     return kDefaultOutputVolumePercent;
 }
 
 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();
 
   UpdateDevicesMutePref();
   UpdateDevicesVolumePref();
 }
 
 AudioDevicesPrefHandlerImpl::~AudioDevicesPrefHandlerImpl() {
 }
 
@@ skipping 36 matching lines @@
   base::DictionaryValue::Iterator it(*device_volume_settings_);
   while (!it.IsAtEnd()) {
     double volume = kDefaultOutputVolumePercent;
     bool success = it.value().GetAsDouble(&volume);
     DCHECK(success);
     dict_update->SetDouble(it.key(), volume);
     it.Advance();
   }
 }
 
+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);
   device_mute_settings_->SetInteger(active_device, old_mute);
   SaveDevicesMutePref();
 }
 
 void AudioDevicesPrefHandlerImpl::MigrateDeviceVolumeSettings(
     const std::string& active_device) {
   double old_volume = local_state_->GetDouble(prefs::kAudioVolumePercent);
   device_volume_settings_->SetDouble(active_device, old_volume);
   SaveDevicesVolumePref();
 }
 
 void AudioDevicesPrefHandlerImpl::NotifyAudioPolicyChange() {
   FOR_EACH_OBSERVER(AudioPrefObserver,
                     observers_,
                     OnAudioPolicyPrefChanged());
 }
 
 // static
 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
   // media system.
   registry->RegisterBooleanPref(prefs::kAudioOutputAllowed, true);
 
   // Register the legacy audio prefs for migration.
   registry->RegisterDoublePref(prefs::kAudioVolumePercent,
                                kDefaultOutputVolumePercent);
   registry->RegisterIntegerPref(prefs::kAudioMute, kPrefMuteOff);
 }
 
 }  // namespace chromeos