Support for showing/hiding status area volume controls in desktop devices

chrome/browser/chromeos/audio/audio_handler.cc

Index: chrome/browser/chromeos/audio/audio_handler.cc
===================================================================
--- chrome/browser/chromeos/audio/audio_handler.cc	(revision 119043)
+++ chrome/browser/chromeos/audio/audio_handler.cc	(working copy)
@@ -49,10 +49,16 @@
   return g_audio_handler;
 }
 
-bool AudioHandler::IsInitialized() {
+bool AudioHandler::IsMixerInitialized() {
   return mixer_->IsInitialized();
 }
 
+// static
+AudioHandler* AudioHandler::GetInitialized() {
+  return g_audio_handler && g_audio_handler->IsMixerInitialized() ?
+         g_audio_handler : NULL;
+}
+
 double AudioHandler::GetVolumePercent() {
   return VolumeDbToPercent(mixer_->GetVolumeDb());
 }
@@ -60,6 +66,7 @@
 void AudioHandler::SetVolumePercent(double volume_percent) {
   volume_percent = min(max(volume_percent, 0.0), 100.0);
   mixer_->SetVolumeDb(PercentToVolumeDb(volume_percent));
+  VolumeChanged();

[Daniel Erat, 2012/01/26 17:55:21]

nit: unless you anticipate adding more code to VolumeChanged() in the near
future, please just remove it and call FOR_EACH_OBSERVER() directly here

[xiyuan, 2012/01/26 18:07:10]

nit: If you want to keep this function, please rename it to NotifyVolumeChanged
to be consistent with other code.

[achuithb, 2012/01/27 01:48:05]

Done.

 }
 
 void AudioHandler::AdjustVolumeByPercent(double adjust_by_percent) {
@@ -74,6 +81,7 @@
 
 void AudioHandler::SetMuted(bool mute) {
   mixer_->SetMuted(mute);
+  VolumeChanged();
 }
 
 AudioHandler::AudioHandler()
@@ -113,4 +121,26 @@
       (max_volume_db - min_volume_db) + min_volume_db;
 }
 
+void AudioHandler::AddVolumeObserver(VolumeObserver* observer) {

[Daniel Erat, 2012/01/26 17:55:21]

please match the order from the header file

[achuithb, 2012/01/27 01:48:05]

Done. These files have been moved to the bottom in the header file.

+  volume_observers_.AddObserver(observer);
+}
+
+void AudioHandler::RemoveVolumeObserver(VolumeObserver* observer) {
+  volume_observers_.RemoveObserver(observer);
+}
+
+void AudioHandler::VolumeChanged() {
+  FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnVolumeChanged());
+}
+
+AudioHandler::VolumeObserver::VolumeObserver() {
+  if (AudioHandler::GetInstance())  // Mixer may be uninitialized here.

[Daniel Erat, 2012/01/26 17:55:21]

so after instantiating an object derived from VolumeObserver, it may or may not
have automatically registered itself in the AudioHandler singleton depending on
when it was created, and if it didn't, i need to call AddVolumeObserver() myself
at some point in the future?  please remove this and make the implementation
responsible for registering and unregistering itself.

[achuithb, 2012/01/26 23:19:34]

The comment refers to the fact that we are using GetInstance and not
GetInitialized. Even if the audio mixer is not initialized (which it is not when
VolumeMenuButton is constructed), we still want to add ourselves as an observer.
In all other uses of AudioHandler in this file, we only want AudioHandler* that
has a properly initialized mixer (ie, result of GetAudioHandler or
AudioHandler::GetInitialized()).

The check for GetInstance() is a guard for chromeos-chrome on linux, where
AudioHandler is not initialized. If GetInstance() is false here, it's not going
to be true in the future.

I had it the way you want it (register/deregister done in the ctors/dtors of
VolumeMenuButton and VolumeControView), and I think this way is better.

It makes more sense to register ourselves in the ctor of the Observer, than in
the ctor of the class that derives from it. This reduces boiler-plate cut n
paste in all the derived classes as well as unnecessary clutter in the ctor/dtor
of those classes. VolumeMenuButton/VolumeControView just care about doing
something useful in the OnVolumeChanged callback. 

[Daniel Erat, 2012/01/26 23:53:28]

My comment wasn't related to the comment in the code; it was related to the fact
that after instantiating a VolumeObserver, it may or may not be registered
depending on whether the AudioHandler singleton has been created or not.
VolumeObservers get instantiated on platforms where we don't instantiate
AudioHandler?  That seems wrong.
If you want to do the registration automatically, you should:
- add a DCHECK(AudioHandler::GetInstance())
- make AddVolumeObserver() and RemoveVolumeObserver() be private methods so
users can't double-register
- document in the header that registration and unregistration are automatic

[achuithb, 2012/01/27 00:16:54]

We create the AudioHandler singleton before the status area, and the code does
not try to handle a re-ordering of the initialization sequence of these two
components. I'll add a DCHECK like: DCHECK(AudioHandler::GetInstance() ||
!system::runtime_environment::IsRunningOnChromeOS());

FWIW, caps_lock_menu_button.cc has a similar situation - SystemKeyEventListener
is not initialized on chromeos-chrome for linux and the initialization
dependency is guarded by a similar DCHECK.
The status area volume icon VolumeMenuButton derives from VolumeObserver, and
gets created on chromeos-chrome on linux. It's been useful for testing to have
it available on linux...
Will do. The DCHECK will assert that we're either not on the device, or the
audiohandler instance exists, as noted earlier.
I thought of doing this - it would require me to make VolumeObserver a friend of
AudioHandler so it could access those private methods. I can do this, but I was
under the impression that friend classes were generally frowned on.
will do.

+    AudioHandler::GetInstance()->AddVolumeObserver(this);
+}
+
+AudioHandler::VolumeObserver::~VolumeObserver() {
+  if (AudioHandler::GetInstance())
+    AudioHandler::GetInstance()->RemoveVolumeObserver(this);
+}
+
 }  // namespace chromeos