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

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

BUG=397019
Committed: https://crrev.com/93a707e47019f209d6dfa5be204df06c886ef7e4
Cr-Commit-Position: refs/heads/master@{#299859}

[Anand Mistry (off Chromium), 2014-10-07 23:56:06]

amistry@chromium.org changed reviewers:
+ asargent@chromium.org, dbeam@chromium.org, mgiuca@chromium.org,
rlp@chromium.org

[asargent_no_longer_on_chrome, 2014-10-08 19:12:51]

c/b/e lgtm

[Dan Beam, 2014-10-09 01:09:39]

c/b/ui/webui/ lgtm

[tapted, 2014-10-09 03:37:01]

I'll leave the thorough review for Matt, but I noticed some subtle DesktopType
stuff when I had a quick skim.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:167:
chrome::HOST_DESKTOP_TYPE_NATIVE);
you might want AppListService::Get(GetCurrentBrowser()->host_desktop_type()); 
Or, if this can be called when there are no browser windows open,
chrome::GetActiveDesktop().

Otherwise, if the user is in Metro mode on Windows 8 and this triggers, it will
show the app launcher on the Windows 8 desktop, which they won't see.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_service_impl.cc:336:
implicit_cast<app_list::AppListViewDelegate*>(view_delegate_.get());
you shouldn't need the implicit cast after a rebase

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:576:
AppListService::Get(chrome::GetActiveDesktop())->IsAppListVisible()) {
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.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.cc:161: if
(webui_finished_loading_)
nit: needs curlies

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.h:12: #include
"base/callback_forward.h"
nit: pretty sure if you have a data member you "need" the whole header, so
something else is probably including it for you

[Matt Giuca, 2014-10-09 18:07:10]

mgiuca@chromium.org changed reviewers:
+ calamity@chromium.org

[Matt Giuca, 2014-10-09 18:07:12]

Also adding calamity as reviewer.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:471: // If we're in
always-on mode, we always want to re-start hotwording
nit: "restart" does not need a hyphen.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:473: // launcher is opened.
The previous call to StopHotwordSession is still
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.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:475: // TODO(amistry): This
only works on ChromeOS since Chrome hides the
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.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:530: // detector running in
parallel with the speech recognition. To get around
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)."

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:532: // alternative is to
launch the launcher with a "do not start hotword session"
Don't need to write alternatives in a comment (unless you have a specific
non-obvious reason why we shouldn't do that).

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:538: if (hotword_service) {
nit: No curlies.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:570: // speech recognition
has stopped. IsAppListVisible() is called because this
Just say "Do not request hotwording after the app list has already closed."

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.cc:246: for (const auto& cb :
pending_webui_callbacks_) {
Nit: no curlies.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.h:98: std::list<base::Closure>
pending_webui_callbacks_;
Is there a reason to use list over vector? I've never seen list used in Chromium
code; it's almost always a vector.

[calamity, 2014-10-09 20:01:26]

calamity@chromium.org changed reviewers:
- calamity@chromium.org

[calamity, 2014-10-09 20:01:27]

Removing myself as reviewer as I don't think I add much value to this review. I
will say that I don't see anything fundamentally wrong in the structure of the
app list service and delegate stuff.

[Anand Mistry (off Chromium), 2014-10-13 21:52:00]

https://codereview.chromium.org/631913004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/hotword_private/hotword_private_api.cc
(right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/hotword_private/hotword_private_api.cc:167:
chrome::HOST_DESKTOP_TYPE_NATIVE);
On 2014/10/09 03:37:00, tapted wrote:
> you might want AppListService::Get(GetCurrentBrowser()->host_desktop_type()); 
> Or, if this can be called when there are no browser windows open,
> chrome::GetActiveDesktop().
> 
> Otherwise, if the user is in Metro mode on Windows 8 and this triggers, it
will
> show the app launcher on the Windows 8 desktop, which they won't see.

Done. But note, priority is ChromeOS. We're not targeting Windows 8 (or Linux,
or OSX).

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_service_impl.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_service_impl.cc:336:
implicit_cast<app_list::AppListViewDelegate*>(view_delegate_.get());
On 2014/10/09 03:37:00, tapted wrote:
> you shouldn't need the implicit cast after a rebase

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:471: // If we're in
always-on mode, we always want to re-start hotwording
On 2014/10/09 18:07:11, Matt Giuca wrote:
> nit: "restart" does not need a hyphen.

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:473: // launcher is opened.
The previous call to StopHotwordSession is still
On 2014/10/09 18:07:11, Matt Giuca wrote:
> 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?
Yes.

> Why is it NULL?
So that a hotword trigger opens the launcher.

> (Is that what "Use an empty client" means?) Is there a way to
> just keep hotwording running instead of closing and reopening again?
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.

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

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:475: // TODO(amistry): This
only works on ChromeOS since Chrome hides the
On 2014/10/09 18:07:11, Matt Giuca wrote:
> 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.

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.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:530: // detector running in
parallel with the speech recognition. To get around
On 2014/10/09 18:07:11, Matt Giuca wrote:
> 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?

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.

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

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:532: // alternative is to
launch the launcher with a "do not start hotword session"
On 2014/10/09 18:07:11, Matt Giuca wrote:
> Don't need to write alternatives in a comment (unless you have a specific
> non-obvious reason why we shouldn't do that).

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:538: if (hotword_service) {
On 2014/10/09 18:07:11, Matt Giuca wrote:
> nit: No curlies.

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:570: // speech recognition
has stopped. IsAppListVisible() is called because this
On 2014/10/09 18:07:11, Matt Giuca wrote:
> Just say "Do not request hotwording after the app list has already closed."

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:576:
AppListService::Get(chrome::GetActiveDesktop())->IsAppListVisible()) {
On 2014/10/09 03:37:00, tapted wrote:
> 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.

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.cc:161: if
(webui_finished_loading_)
On 2014/10/09 03:37:00, tapted wrote:
> nit: needs curlies

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.cc:246: for (const auto& cb :
pending_webui_callbacks_) {
On 2014/10/09 18:07:11, Matt Giuca wrote:
> Nit: no curlies.

Done.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
File chrome/browser/ui/app_list/start_page_service.h (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.h:12: #include
"base/callback_forward.h"
On 2014/10/09 03:37:00, tapted wrote:
> nit: pretty sure if you have a data member you "need" the whole header, so
> something else is probably including it for you

Ah right. I put this in before I noticed that Callbacks work differently than
google code.

P.S. I prefer google-style callbacks.

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/s...
chrome/browser/ui/app_list/start_page_service.h:98: std::list<base::Closure>
pending_webui_callbacks_;
On 2014/10/09 18:07:12, Matt Giuca wrote:
> Is there a reason to use list over vector? I've never seen list used in
Chromium
> code; it's almost always a vector.

In this particular case, it doesn't matter since this will contain either 0 or 1
elements in the majority of cases. But vector does suck for some things compared
to a list.

[Matt Giuca, 2014-10-13 22:45:00]

c/b/ui/app_list lgtm.

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:483: // association between
the hotword service and 'this'.  When starting up
nit: Write |this| instead of 'this'.

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:489:
hotword_service->RequestHotwordSession(NULL);
nit: Use nullptr. Also change the above comment.

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

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_view_delegate.cc:475: // TODO(amistry): This
only works on ChromeOS since Chrome hides the
On 2014/10/09 18:07:11, Matt Giuca wrote:
> 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.

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.

[Anand Mistry (off Chromium), 2014-10-13 22:56:14]

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:483: // association between
the hotword service and 'this'.  When starting up
On 2014/10/13 22:45:00, Matt Giuca wrote:
> nit: Write |this| instead of 'this'.

Done.

https://codereview.chromium.org/631913004/diff/130001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:489:
hotword_service->RequestHotwordSession(NULL);
On 2014/10/13 22:45:00, Matt Giuca wrote:
> nit: Use nullptr. Also change the above comment.

Done.

[rpetterson, 2014-10-15 22:38:44]

LGTM

sorry it took me so long.

[Anand Mistry (off Chromium), 2014-10-16 04:38:34]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-10-16 04:39:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631913004/350001

[commit-bot: I haz the power, 2014-10-16 04:58:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 04:58:41]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Anand Mistry (off Chromium), 2014-10-16 05:36:43]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-10-16 05:37:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631913004/370001

[commit-bot: I haz the power, 2014-10-16 06:28:04]

Committed patchset #6 (id:370001)

[commit-bot: I haz the power, 2014-10-16 06:29:09]

Patchset 6 (id:??) landed as
https://crrev.com/93a707e47019f209d6dfa5be204df06c886ef7e4
Cr-Commit-Position: refs/heads/master@{#299859}