Open the launcher when hotword is triggered in always-on mode.

chrome/browser/ui/app_list/app_list_view_delegate.cc

Index: chrome/browser/ui/app_list/app_list_view_delegate.cc
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.cc b/chrome/browser/ui/app_list/app_list_view_delegate.cc
index c8753f2e22db86b20683bcfc8ee6982fcfdcd210..65d24e5e2c463fac3209f6558cab2a878e0f7e4f 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.cc
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.cc
@@ -465,8 +465,18 @@ void AppListViewDelegate::ViewClosing() {
     if (service->HotwordEnabled()) {
       HotwordService* hotword_service =
           HotwordServiceFactory::GetForProfile(profile_);
-      if (hotword_service)
+      if (hotword_service) {
         hotword_service->StopHotwordSession(this);
+
+        // If we're in always-on mode, we always want to re-start hotwording

[Matt Giuca, 2014/10/09 18:07:11]

nit: "restart" does not need a hyphen.

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Done.

+        // after closing the launcher window. Use an empty client so that the
+        // launcher is opened. The previous call to StopHotwordSession is still

[Matt Giuca, 2014/10/09 18:07:11]

I'm a bit unclear on what this comment means. Are you saying that when the app
list is dismissed, you shut off the hotwording service and then start it again?
Why is it NULL? (Is that what "Use an empty client" means?) Is there a way to
just keep hotwording running instead of closing and reopening again?

I think this comment could be rewritten answering some of these questions.

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Yes.
So that a hotword trigger opens the launcher.
Um... The interactions here are non-trivial, and this is the simplest way to do
it. It also maintains compatibility with the existing extension until we're
ready to go live and delete the old stuff.
Done... kinda.

+        // needed to clear the client since you can't have two sessions running.
+        // TODO(amistry): This only works on ChromeOS since Chrome hides the

[Matt Giuca, 2014/10/09 18:07:11]

What happens on non-ChromeOS platforms in this case? I think Trent is planning
to refactor ChromeOS so that it behaves the same as on other platforms (the app
list gets torn down). Trent, is that the case? Then this will break on ChromeOS
as well.

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

For now, we're not targeting non-ChromeOS platform. It simply doesn't work. In
fact, the existing extension doesn't work on non-ChromeOS platforms either.

As for the refactor, I'd like to see that done before making any changes here.

[tapted, 2014/10/13 22:48:08]

sorry - missed this Q. Answer: maybe :).

The view uses a non-trivial amount of memory. I think ~20MB. calamity fixed a
big chunk of the latency in bringing up the view, so skipping the tear-down on
CrOS no longer as urgent. Long-term consistency is still one of the goals, so I
think it will happen that the view eventually persists on all platforms. But we
should tackle other things first - like lazily creating the compositor layers
required for drag & drop - to reduce resource usage.

+        // launcher instead of destroying it. Make this work on Chrome.
+        if (hotword_service->IsAlwaysOnEnabled())
+          hotword_service->RequestHotwordSession(NULL);
+      }
     }
   }
 }
@@ -512,6 +522,23 @@ void AppListViewDelegate::ToggleSpeechRecognition() {
       app_list::StartPageService::Get(profile_);
   if (service)
     service->ToggleSpeechRecognition();
+
+  // With the new hotword extension, stop the hotword session. With the launcher
+  // and NTP, this is unnecessary since the hotwording is implicitly stopped.
+  // However, for always on, hotword triggering launches the launcher which
+  // starts a session and hence starts the detector. This results in the
+  // detector running in parallel with the speech recognition. To get around

[Matt Giuca, 2014/10/09 18:07:11]

It's not clear what the difference is between "the detector" and "speech
recognition". Are you saying that you don't want the old detector and the new
one running at the same time, so you turn off the old one when the new one is
enabled?

How about: "Stop the old hotword detection service if the new service is running
(we do not want both services running concurrently)."

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

They're two different things. By "detector", I mean the hotword detector. The
"speech recognizer" refers to the speech-to-text recognizer, which is a
completely different thing. I've tries to clarify the comment.

+  // this, always stop the session when switching to speech recognition. An
+  // alternative is to launch the launcher with a "do not start hotword session"

[Matt Giuca, 2014/10/09 18:07:11]

Don't need to write alternatives in a comment (unless you have a specific
non-obvious reason why we shouldn't do that).

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Done.

+  // flag, but that seems messy compared to this.
+  if (HotwordService::IsExperimentalHotwordingEnabled() &&
+      service && service->HotwordEnabled()) {
+    HotwordService* hotword_service =
+        HotwordServiceFactory::GetForProfile(profile_);
+    if (hotword_service) {

[Matt Giuca, 2014/10/09 18:07:11]

nit: No curlies.

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Done.

+      hotword_service->StopHotwordSession(this);
+    }
+  }
 }
 
 void AppListViewDelegate::ShowForProfileByPath(
@@ -540,10 +567,13 @@ void AppListViewDelegate::OnSpeechRecognitionStateChanged(
   app_list::StartPageService* service =
       app_list::StartPageService::Get(profile_);
   // With the new hotword extension, we need to re-request hotwording after
-  // speech recognition has stopped.
+  // speech recognition has stopped. IsAppListVisible() is called because this

[Matt Giuca, 2014/10/09 18:07:11]

Just say "Do not request hotwording after the app list has already closed."

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Done.

+  // notification can happen after the app list is closed. In that case, we
+  // don't want to re-request hotwording.
   if (new_state == app_list::SPEECH_RECOGNITION_READY &&
       HotwordService::IsExperimentalHotwordingEnabled() &&
-      service && service->HotwordEnabled()) {
+      service && service->HotwordEnabled() &&
+      AppListService::Get(chrome::GetActiveDesktop())->IsAppListVisible()) {

[tapted, 2014/10/09 03:37:00]

There is an AppListViewDelegate for each desktop type. To get the "right" window
for this delegate, you can go through the |controller_| data member (i.e., here,
probably controller_->GetAppListWindow() which only returns a non-NULL pointer
when the app list is visible).

If you need more from AppListService, you could expose
AppListControllerDelegateImpl::service_ via the ControllerDelegate interface. Or
if you need access to Chrome-specific stuff, it's probably OK for the
AppListServiceImpl to be passed in to the ChromeAppListViewDelegate constructor.

[Anand Mistry (off Chromium), 2014/10/13 21:51:59]

Done.

     HotwordService* hotword_service =
         HotwordServiceFactory::GetForProfile(profile_);
     if (hotword_service) {