Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
471853002:
Use the new hotword extension in the launcher when it is enabled. (Closed)
|
Created:
6 years, 4 months ago by Anand Mistry (off Chromium) Modified:
6 years, 4 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, skanuj+watch_chromium.org, melevin+watch_chromium.org, tfarina, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, rlp+watch_chromium.org, Jered, Matt Giuca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse the new hotword extension in the launcher when it is enabled.
This stops the launcher from using its hotwording functionality, and
adds support for the new extension.
BUG=397019
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291031
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address review comments. #Patch Set 3 : Rebase. #
Messages
Total messages: 26 (0 generated)
I haven't really looked (just found a typo). I'm not sure I'm very qualified to do these reviews. I'm not familiar with any of the code you're touching. Do the other reviewers cover it? https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.h (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.h:42: // Returns true is the "enable-experimental-hotwording" flag is set. s/is/if/
Added xiyuan as reviewer. @xiyuan: Feel free to re-direct to someone you feel is more appropriate to review this.
I don't know this code so I just pointed out some obvious things. I assume one of the other reviewers knows about HotWording? https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || (client_ && client_ != client)) This looks suspicious. Why are we exiting if the passed in client is different from client_? Why are we assigning the client_ below? Why are we passing in client? https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:108: // Trasitioning into the "hotword recognizing" state is handled by the hotword typo https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:157: if (HotwordService::IsExperimentalHotwordingEnabled()) needs {}
mukai@ is a better reviewer for this.
https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || (client_ && client_ != client)) On 2014/08/14 15:48:06, arv wrote: > This looks suspicious. Why are we exiting if the passed in client is different > from client_? Why are we assigning the client_ below? Why are we passing in > client? IIRC there is only one client instance right now. Do we add them? As arv points out, the current code of client handling does not care so much about the scenario of multiple clients. https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.h (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.h:42: // Returns true is the "enable-experimental-hotwording" flag is set. No one explains what is experimental hotwording, and how it's different from the normal hotwording. Please explain briefly on the switches.cc or here. https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:131: bool hotwordEnabled = HotwordEnabled(); variable names are in style of hotword_enabled Also, why HotwordEnabled() doesn't use HotwordService::IsExperimentalHotwordingEnabled() directly? https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:135: hotwordEnabled = false; You can simply hotword_enabled = HotwordEnabled() && !HotwordService::IsExperimentalHotwordingEnabled(); https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/webui/app_... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/webui/app_... chrome/browser/ui/webui/app_list/start_page_handler.cc:197: bool hotwordEnabled = service->HotwordEnabled(); variable name style
https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || (client_ && client_ != client)) On 2014/08/14 22:10:08, Jun Mukai wrote: > On 2014/08/14 15:48:06, arv wrote: > > This looks suspicious. Why are we exiting if the passed in client is different > > from client_? Why are we assigning the client_ below? Why are we passing in > > client? > > IIRC there is only one client instance right now. Do we add them? As arv points > out, the current code of client handling does not care so much about the > scenario of multiple clients. I have no plans to add additional clients. My only requirement is that an existing client be able to start another session since the current one will be implicitly stopped when the hotword is triggered. Maybe I need a "resume" method instead, but that seems unnecessary. https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.h (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.h:42: // Returns true is the "enable-experimental-hotwording" flag is set. On 2014/08/14 07:45:23, Matt Giuca wrote: > s/is/if/ Done. https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.h:42: // Returns true is the "enable-experimental-hotwording" flag is set. On 2014/08/14 22:10:08, Jun Mukai wrote: > No one explains what is experimental hotwording, and how it's different from the > normal hotwording. Please explain briefly on the switches.cc or here. Added a small blurb to chrome/common/chrome_switches.cc https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:108: // Trasitioning into the "hotword recognizing" state is handled by the hotword On 2014/08/14 15:48:06, arv wrote: > typo Done. https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:131: bool hotwordEnabled = HotwordEnabled(); On 2014/08/14 22:10:08, Jun Mukai wrote: > variable names are in style of hotword_enabled Done. > Also, why HotwordEnabled() doesn't use > HotwordService::IsExperimentalHotwordingEnabled() directly? Because HotwordEnabled() is used in a number of other places where I don't want it to take into account IsExperimentalHotwordingEnabled() in this way. Here, I don't just want to know whether hotwording is enabled or not, but also whether or not to use the hotwording built into the start page. Two orthogonal concepts. https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:135: hotwordEnabled = false; On 2014/08/14 22:10:08, Jun Mukai wrote: > You can simply > hotword_enabled = HotwordEnabled() && > !HotwordService::IsExperimentalHotwordingEnabled(); Done. https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:157: if (HotwordService::IsExperimentalHotwordingEnabled()) On 2014/08/14 15:48:06, arv wrote: > needs {} Done. https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/webui/app_... File chrome/browser/ui/webui/app_list/start_page_handler.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/ui/webui/app_... chrome/browser/ui/webui/app_list/start_page_handler.cc:197: bool hotwordEnabled = service->HotwordEnabled(); On 2014/08/14 22:10:08, Jun Mukai wrote: > variable name style Done.
https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... File chrome/browser/search/hotword_service.cc (right): https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || (client_ && client_ != client)) On 2014/08/15 00:14:51, Anand Mistry wrote: > On 2014/08/14 22:10:08, Jun Mukai wrote: > > On 2014/08/14 15:48:06, arv wrote: > > > This looks suspicious. Why are we exiting if the passed in client is > different > > > from client_? Why are we assigning the client_ below? Why are we passing in > > > client? > > > > IIRC there is only one client instance right now. Do we add them? As arv > points > > out, the current code of client handling does not care so much about the > > scenario of multiple clients. > > I have no plans to add additional clients. My only requirement is that an > existing client be able to start another session since the current one will be > implicitly stopped when the hotword is triggered. Maybe I need a "resume" method > instead, but that seems unnecessary. You seems not answering to our concerns. If we can still assume single client, why do you have to add the condition of client_ != client?
On 2014/08/15 00:30:07, Jun Mukai wrote: > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > File chrome/browser/search/hotword_service.cc (right): > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || > (client_ && client_ != client)) > On 2014/08/15 00:14:51, Anand Mistry wrote: > > On 2014/08/14 22:10:08, Jun Mukai wrote: > > > On 2014/08/14 15:48:06, arv wrote: > > > > This looks suspicious. Why are we exiting if the passed in client is > > different > > > > from client_? Why are we assigning the client_ below? Why are we passing > in > > > > client? > > > > > > IIRC there is only one client instance right now. Do we add them? As arv > > points > > > out, the current code of client handling does not care so much about the > > > scenario of multiple clients. > > > > I have no plans to add additional clients. My only requirement is that an > > existing client be able to start another session since the current one will be > > implicitly stopped when the hotword is triggered. Maybe I need a "resume" > method > > instead, but that seems unnecessary. > > You seems not answering to our concerns. If we can still assume single client, > why do you have to add the condition of client_ != client? Ah, sorry. I misread. Adding that condition allows a client to call into RequestHotwordSession() multiple times and still require a single client. Without it, the method will return immediately on subsequent calls since client_ is set during the first call to RequestHotwordSession().
On 2014/08/15 00:57:43, Anand Mistry wrote: > On 2014/08/15 00:30:07, Jun Mukai wrote: > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > File chrome/browser/search/hotword_service.cc (right): > > > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || > > (client_ && client_ != client)) > > On 2014/08/15 00:14:51, Anand Mistry wrote: > > > On 2014/08/14 22:10:08, Jun Mukai wrote: > > > > On 2014/08/14 15:48:06, arv wrote: > > > > > This looks suspicious. Why are we exiting if the passed in client is > > > different > > > > > from client_? Why are we assigning the client_ below? Why are we passing > > in > > > > > client? > > > > > > > > IIRC there is only one client instance right now. Do we add them? As arv > > > points > > > > out, the current code of client handling does not care so much about the > > > > scenario of multiple clients. > > > > > > I have no plans to add additional clients. My only requirement is that an > > > existing client be able to start another session since the current one will > be > > > implicitly stopped when the hotword is triggered. Maybe I need a "resume" > > method > > > instead, but that seems unnecessary. > > > > You seems not answering to our concerns. If we can still assume single > client, > > why do you have to add the condition of client_ != client? > > Ah, sorry. I misread. > > Adding that condition allows a client to call into RequestHotwordSession() > multiple times and still require a single client. Without it, the method will > return immediately on subsequent calls since client_ is set during the first > call to RequestHotwordSession(). Ah, I think I get your point. So, the following situation: 1. HowordClient calls RequestHotwordSession 2. Hotword extension calls back OnHotwordStateChanged(true), start listening... 3. some system state has changed so the extension stops listening, then OnHotwordStateChanged(false) is called 4. later, HotwordClient wants to resume the hotword session... Is that right? If so, however, my suggestion is to change the HotwordClient to call StopHotwordSesion() at 3? Then resume is essentially requesting yet another hotword session. How about that?
On 2014/08/15 01:40:15, Jun Mukai wrote: > On 2014/08/15 00:57:43, Anand Mistry wrote: > > On 2014/08/15 00:30:07, Jun Mukai wrote: > > > > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > > File chrome/browser/search/hotword_service.cc (right): > > > > > > > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > > chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || > > > (client_ && client_ != client)) > > > On 2014/08/15 00:14:51, Anand Mistry wrote: > > > > On 2014/08/14 22:10:08, Jun Mukai wrote: > > > > > On 2014/08/14 15:48:06, arv wrote: > > > > > > This looks suspicious. Why are we exiting if the passed in client is > > > > different > > > > > > from client_? Why are we assigning the client_ below? Why are we > passing > > > in > > > > > > client? > > > > > > > > > > IIRC there is only one client instance right now. Do we add them? As > arv > > > > points > > > > > out, the current code of client handling does not care so much about the > > > > > scenario of multiple clients. > > > > > > > > I have no plans to add additional clients. My only requirement is that an > > > > existing client be able to start another session since the current one > will > > be > > > > implicitly stopped when the hotword is triggered. Maybe I need a "resume" > > > method > > > > instead, but that seems unnecessary. > > > > > > You seems not answering to our concerns. If we can still assume single > > client, > > > why do you have to add the condition of client_ != client? > > > > Ah, sorry. I misread. > > > > Adding that condition allows a client to call into RequestHotwordSession() > > multiple times and still require a single client. Without it, the method will > > return immediately on subsequent calls since client_ is set during the first > > call to RequestHotwordSession(). > > Ah, I think I get your point. So, the following situation: > 1. HowordClient calls RequestHotwordSession > 2. Hotword extension calls back OnHotwordStateChanged(true), start listening... > 3. some system state has changed so the extension stops listening, then > OnHotwordStateChanged(false) is called > 4. later, HotwordClient wants to resume the hotword session... > > Is that right? Almost. The event that stops the detector is internal to the extension. Detecting the hotword is one, and disabling it via settings (which notifies using the hotwordPrivate API) is another. > > If so, however, my suggestion is to change the HotwordClient to call > StopHotwordSesion() at 3? > Then resume is essentially requesting yet another hotword session. > How about that? I think that will make the current code much more complicated. Right now, the "session" in HotwordClient is managed by calling Request/StopHotwordSession() which is controlled by AppListViewDelegate, and requests the extension start/stop the session. What you're suggesting is that the extension itself request the session be stopped, but that would call Request/StopHotwordSession() which calls back into the extension. There's also the issue of how this will interact with the SpeechRecognitionState. In principle, I like your suggestion. In practice, I think it requires more fudging than I want to do in this CL.
On 2014/08/15 06:41:01, Anand Mistry wrote: > On 2014/08/15 01:40:15, Jun Mukai wrote: > > On 2014/08/15 00:57:43, Anand Mistry wrote: > > > On 2014/08/15 00:30:07, Jun Mukai wrote: > > > > > > > > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > > > File chrome/browser/search/hotword_service.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/471853002/diff/1/chrome/browser/search/hotwor... > > > > chrome/browser/search/hotword_service.cc:458: if (!IsServiceAvailable() || > > > > (client_ && client_ != client)) > > > > On 2014/08/15 00:14:51, Anand Mistry wrote: > > > > > On 2014/08/14 22:10:08, Jun Mukai wrote: > > > > > > On 2014/08/14 15:48:06, arv wrote: > > > > > > > This looks suspicious. Why are we exiting if the passed in client is > > > > > different > > > > > > > from client_? Why are we assigning the client_ below? Why are we > > passing > > > > in > > > > > > > client? > > > > > > > > > > > > IIRC there is only one client instance right now. Do we add them? As > > arv > > > > > points > > > > > > out, the current code of client handling does not care so much about > the > > > > > > scenario of multiple clients. > > > > > > > > > > I have no plans to add additional clients. My only requirement is that > an > > > > > existing client be able to start another session since the current one > > will > > > be > > > > > implicitly stopped when the hotword is triggered. Maybe I need a > "resume" > > > > method > > > > > instead, but that seems unnecessary. > > > > > > > > You seems not answering to our concerns. If we can still assume single > > > client, > > > > why do you have to add the condition of client_ != client? > > > > > > Ah, sorry. I misread. > > > > > > Adding that condition allows a client to call into RequestHotwordSession() > > > multiple times and still require a single client. Without it, the method > will > > > return immediately on subsequent calls since client_ is set during the first > > > call to RequestHotwordSession(). > > > > Ah, I think I get your point. So, the following situation: > > 1. HowordClient calls RequestHotwordSession > > 2. Hotword extension calls back OnHotwordStateChanged(true), start > listening... > > 3. some system state has changed so the extension stops listening, then > > OnHotwordStateChanged(false) is called > > 4. later, HotwordClient wants to resume the hotword session... > > > > Is that right? > > Almost. The event that stops the detector is internal to the extension. > Detecting the hotword is one, and disabling it via settings (which notifies > using the hotwordPrivate API) is another. > > > > > If so, however, my suggestion is to change the HotwordClient to call > > StopHotwordSesion() at 3? > > Then resume is essentially requesting yet another hotword session. > > How about that? > > I think that will make the current code much more complicated. Right now, the > "session" in HotwordClient is managed by calling Request/StopHotwordSession() > which is controlled by AppListViewDelegate, and requests the extension > start/stop the session. What you're suggesting is that the extension itself > request the session be stopped, but that would call Request/StopHotwordSession() > which calls back into the extension. There's also the issue of how this will > interact with the SpeechRecognitionState. In principle, I like your suggestion. > In practice, I think it requires more fudging than I want to do in this CL. okay, you made the point. lgtm.
Ping!
I am good as long as Mukai is happy. LGTM in case you are waiting for me.
Still need an lgtm for hotword_service.*
On 2014/08/20 06:48:01, Anand Mistry wrote: > Still need an lgtm for hotword_service.* Sorry, didn't realize you were waiting on that. LGTM for hotword_service*
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amistry@chromium.org/471853002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by amistry@chromium.org
The CQ bit was unchecked by amistry@chromium.org
The CQ bit was checked by amistry@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amistry@chromium.org/471853002/40001
Message was sent while issue was closed.
Committed patchset #3 (40001) as 291031 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
