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 119229)
+++ chrome/browser/chromeos/audio/audio_handler.cc	(working copy)
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/chromeos/audio/audio_mixer_alsa.h"
+#include "chrome/browser/chromeos/system/runtime_environment.h"
 #include "content/public/browser/browser_thread.h"
 
 using std::max;
@@ -49,8 +50,10 @@
   return g_audio_handler;
 }
 
-bool AudioHandler::IsInitialized() {
-  return mixer_->IsInitialized();
+// static
+AudioHandler* AudioHandler::GetInstanceIfInitialized() {
+  return g_audio_handler && g_audio_handler->IsMixerInitialized() ?
+         g_audio_handler : NULL;
 }
 
 double AudioHandler::GetVolumePercent() {
@@ -60,6 +63,7 @@
 void AudioHandler::SetVolumePercent(double volume_percent) {
   volume_percent = min(max(volume_percent, 0.0), 100.0);
   mixer_->SetVolumeDb(PercentToVolumeDb(volume_percent));
+  FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnVolumeChanged());
 }
 
 void AudioHandler::AdjustVolumeByPercent(double adjust_by_percent) {
@@ -74,6 +78,7 @@
 
 void AudioHandler::SetMuted(bool mute) {
   mixer_->SetMuted(mute);
+  FOR_EACH_OBSERVER(VolumeObserver, volume_observers_, OnVolumeChanged());
 }
 
 AudioHandler::AudioHandler()
@@ -85,6 +90,10 @@
   mixer_.reset();
 };
 
+bool AudioHandler::IsMixerInitialized() {
+  return mixer_->IsInitialized();
+}
+
 // VolumeDbToPercent() and PercentToVolumeDb() conversion functions allow us
 // complete control over how the 0 to 100% range is mapped to actual loudness.
 //
@@ -113,4 +122,26 @@
       (max_volume_db - min_volume_db) + min_volume_db;
 }
 
+void AudioHandler::AddVolumeObserver(VolumeObserver* observer) {
+  volume_observers_.AddObserver(observer);
+}
+
+void AudioHandler::RemoveVolumeObserver(VolumeObserver* observer) {
+  volume_observers_.RemoveObserver(observer);
+}
+
+AudioHandler::VolumeObserver::VolumeObserver() {
+  // AudioHandler should be initialized here if we are on the device, though
+  // the mixer may not have finished initializing yet.
+  DCHECK(AudioHandler::GetInstance() ||
+      !system::runtime_environment::IsRunningOnChromeOS());
+  if (AudioHandler::GetInstance())

[Daniel Erat, 2012/01/27 18:03:57]

remove the if statement; you already have a DCHECK()

[achuithb, 2012/01/27 18:53:44]

AudioHandler::GetInstance() is NULL on chrome-chromeos for linux. I need the
NULL check (or I could alternately use IsRunningOnChromeOS, but this is
clearer).

[stevenjb, 2012/01/27 19:45:02]

I'm not sure that this is clearer; how about at the top of the file:
if (!system::runtime_environment::IsRunningOnChromeOS())
  return;
Or, if it's possible that AudioHandler::GetInstance() might be true on non
chromeos:
if (!system::runtime_environment::IsRunningOnChromeOS() &&
!AudioHandler::GetInstance())
  return;
This way it's clear that when not running on chromeos, we expect that the audio
handler may not be initialized and just want to exit. Then we do the DCHECK and
call AddVolimeObserver unconditionally.

[Daniel Erat, 2012/01/27 23:06:54]

yeah, i'd also prefer:

  if (system::runtime_environment::IsRunningOnChromeOS() {
    DCHECK(AudioHandler::GetInstance());
    AudioHandler::GetInstance()->AddVolumeObserver(this);
  }

DCHECK() is more-or-less optional at this point, since we're going to crash on a
null deref otherwise.  the code in the d'tor should be similar.

[achuithb, 2012/01/31 00:58:43]

Well, I was following the pattern in:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/sta...
Steven - since you wrote that code, I'd like to understand why your opinion has
changed? I like the way it's written and I copied it exactly here.

If we add a mock audio handler for chromeos-chrome for linux (something that we
should do), testing for runtime_environment will no longer be correct. 
Null-checking a pointer that could be NULL will always be correct. 

I'll switch this to using the runtime_environment check for now, using Dan's
arrangement (leaving out the DCHECK since we're about to crash anyway).

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