Updates the mic icon status based on the device's audio state.

chrome/browser/ui/app_list/start_page_service.cc

Index: chrome/browser/ui/app_list/start_page_service.cc
diff --git a/chrome/browser/ui/app_list/start_page_service.cc b/chrome/browser/ui/app_list/start_page_service.cc
index 1ee794255773efdb5ffccb4a582da489a8ec423c..a932bc7a9bdc3a255ba70f398310e32bd2aceebe 100644
--- a/chrome/browser/ui/app_list/start_page_service.cc
+++ b/chrome/browser/ui/app_list/start_page_service.cc
@@ -43,12 +43,27 @@ namespace app_list {
 
 namespace {
 
+bool IsVoiceSearchEnabled() {

[tapted, 2014/11/25 05:03:10]

It's a bit confusing having standalone `IsVoiceSearchEnabled()` functions here
and in app_list_switches.cc, both having #ifdef(OS_CHROMEOS) in them.

I think the meaning of this function is actually quite different now. WDYT about

virtual bool SpeechRecognizerDelegate::CanReadSpeech() or CanListen();

StartPageService would just return false.

StartPageServiceChromeOS can then override this once it's pulled in the
cras_audio_handler.h dependency

[Jun Mukai, 2014/11/26 01:39:21]

Done to rename it to CanListen.
Not so sure about implementing as SpeechRecognizerDelegate's method.
SpeechRecognizer is owned and controlled by StartPageService.
SpeechRecognizer::Start will never be called anyways.

Another related but more confusing concept is HotwordService, which is in
chrome/browser/search. This CL might better be there ,let me think a bit more...

+  if (!app_list::switches::IsVoiceSearchEnabled())
+    return false;
+#if defined(OS_CHROMEOS)
+  if (!chromeos::CrasAudioHandler::IsInitialized())
+    return false;
+  chromeos::CrasAudioHandler* audio_handler = chromeos::CrasAudioHandler::Get();
+  if (!audio_handler->GetPrimaryActiveInputNode())
+    return false;
+  if (audio_handler->IsInputMuted())
+    return false;
+#endif
+  return true;
+}
+
 bool InSpeechRecognition(SpeechRecognitionState state) {
   return state == SPEECH_RECOGNITION_RECOGNIZING ||
       state == SPEECH_RECOGNITION_IN_SPEECH;
 }
 
-}
+}  // namespace
 
 class StartPageService::ProfileDestroyObserver
     : public content::NotificationObserver {
@@ -119,8 +134,10 @@ StartPageService::StartPageService(Profile* profile)
   // If experimental hotwording is enabled, then we're always "ready".
   // Transitioning into the "hotword recognizing" state is handled by the
   // hotword extension.
-  if (HotwordService::IsExperimentalHotwordingEnabled())
+  if (IsVoiceSearchEnabled() &&
+      HotwordService::IsExperimentalHotwordingEnabled()) {
     state_ = app_list::SPEECH_RECOGNITION_READY;
+  }
 
   if (app_list::switches::IsExperimentalAppListEnabled())
     LoadContents();
@@ -148,6 +165,9 @@ void StartPageService::AppListShown() {
         "appList.startPage.onAppListShown",
         base::FundamentalValue(HotwordEnabled()));
   }
+#if defined(OS_CHROMEOS)
+  chromeos::CrasAudioHandler::Get()->AddAudioObserver(this);

[tapted, 2014/11/25 11:07:52]

Also, what are the implications for always-on-hotwording? This doesn't seem like
the right place to add the observer if the IsExperimentalHotwordingEnabled()
code paths are taken

[Jun Mukai, 2014/11/26 01:39:21]

If that flag is specified, indeed it attempts to run the hotwording without
knowing there's no audio input. That isn't great indeed.

Unfortunately, here is the common place to handle the hotwording state / result
for both experimental and non-experimental hotwording.

[tapted, 2014/11/26 02:05:58]

So long as always-on isn't worse with it like this, it might be OK. That is, it
might not matter that the button is in the wrong state for always-on since the
button isn't visible until the app list is shown anyway. So, if the always-on
part makes sense in a follow-up, that's fine by me :)

[Anand Mistry (off Chromium), 2014/11/26 10:49:58]

If I understand this change correctly, under the new hotwording, which we plan
to turn on in M41, hotwording will still turn on when the user opens the
launcher, but the microphone icon will go away. Not ideal, but that's fine since
there's no way the hotword detector can trigger. I'd rather not see any deeper
changes since I'd like to refactor some of the speech/hotwording code here,
since we shouldn't need to rely on the WebContents any more for it.

[Jun Mukai, 2014/11/26 21:59:44]

That is right. But I guess the hotworder will stop listening because audio API
fails because of no devices, and it will never come up again.  So probably I
guess someone has to notify the hotworder to restart the recognition again.
I believe that's fine to leave that task for further CLs, but at least it's
better to take it into account.

[Jun Mukai, 2014/11/26 23:29:50]

After thinking again, I've realized that this is still a bit unclear (and some
universal solution could be an overkill). I hope this will be changed and
simplified when the experimental hotwording is launched.

I've left a TODO comment about restarting hotwording.

+#endif
 }
 
 void StartPageService::AppListHidden() {
@@ -162,6 +182,9 @@ void StartPageService::AppListHidden() {
       speech_recognizer_) {
     speech_recognizer_->Stop();
   }
+#if defined(OS_CHROMEOS)
+  chromeos::CrasAudioHandler::Get()->RemoveAudioObserver(this);
+#endif
 }
 
 void StartPageService::ToggleSpeechRecognition() {
@@ -225,7 +248,7 @@ content::WebContents* StartPageService::GetStartPageContents() {
 }
 
 content::WebContents* StartPageService::GetSpeechRecognitionContents() {
-  if (app_list::switches::IsVoiceSearchEnabled()) {
+  if (IsVoiceSearchEnabled()) {

[tapted, 2014/11/25 05:03:10]

Why this change? It doesn't seem robust to return null after previously
returning non-null, and still leave the webcontents loaded.

Although I had a look around... It looks like the web contents returned by
GetSpeechRecognitionContents and GetSpeechContents() is only ever treated as
non-null or null. Could these functions both be eliminated and replaced by the
same SpeechRecognizerDelegate::CanListen() I mentioned above?

StartPageService::AppListShown or the constructor look like they can take care
of the LoadContents call to ensure "stuff" is warmed up. The extra complexity in
here doesn't seem necessary, but I might be missing something.

[Jun Mukai, 2014/11/26 01:39:21]

Reverted back to the original.  The original motivation was,
SearchResourceManager controls mic icon by this method, but I changed that code
to see state_ instead.

     if (!contents_)
       LoadContents();
     return contents_.get();
@@ -252,6 +275,9 @@ void StartPageService::OnSpeechSoundLevelChanged(int16_t level) {
 
 void StartPageService::OnSpeechRecognitionStateChanged(
     SpeechRecognitionState new_state) {
+  // Sometimes this can be called even though there are no audio input devices.
+  if (!IsVoiceSearchEnabled())
+    new_state = SPEECH_RECOGNITION_OFF;

[tapted, 2014/11/25 05:03:10]

This can leave new_state == state_ -- should that suppress the
FOR_EACH_OBSERVER(..)? (comment here?)

[Jun Mukai, 2014/11/26 01:39:21]

Done.

 
   if (HotwordService::IsExperimentalHotwordingEnabled() &&
       new_state == SPEECH_RECOGNITION_READY &&
@@ -286,6 +312,19 @@ void StartPageService::Shutdown() {
   UnloadContents();
 }
 
+#if defined(OS_CHROMEOS)
+void StartPageService::OnInputMuteChanged() {
+  if (IsVoiceSearchEnabled())
+    OnSpeechRecognitionStateChanged(SPEECH_RECOGNITION_READY);
+  else
+    OnSpeechRecognitionStateChanged(SPEECH_RECOGNITION_OFF);
+}
+
+void StartPageService::OnActiveInputNodeChanged() {
+  OnInputMuteChanged();
+}
+#endif
+
 void StartPageService::WebUILoaded() {
   // There's a race condition between the WebUI loading, and calling its JS
   // functions. Specifically, calling LoadContents() doesn't mean that the page