Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
752253002:
Updates the mic icon status based on the device's audio state. (Closed)
DescriptionUpdates the mic icon status based on the device's audio state.
BUG=383624
R=amistry@chromium.org, tapted@chromium.org, oshima@chromium.org
TEST=manually
Committed: https://crrev.com/3f492b65105964526ca8dc5d6a66971c0a427cdd
Cr-Commit-Position: refs/heads/master@{#306296}
Patch Set 1 #
Total comments: 15
Patch Set 2 : fix #
Total comments: 8
Patch Set 3 : just TODO comment #Patch Set 4 : fix #
Total comments: 4
Patch Set 5 : fix comment #
Messages
Total messages: 24 (4 generated)
Please review this (cc:jennyz for the usage of CrasAudioHandler)
lgtm
Sorry for the delay - start_page_service.cc code isn't super familiar to me. A couple of things confused me - comments below. Main comment is the last one -- I think we should really have StartPageServiceChromeOS rather than a proliferation of #ifdefs. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:46: bool IsVoiceSearchEnabled() { 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 https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:251: if (IsVoiceSearchEnabled()) { 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. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:280: new_state = SPEECH_RECOGNITION_OFF; This can leave new_state == state_ -- should that suppress the FOR_EACH_OBSERVER(..)? (comment here?) https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.h:42: public SpeechRecognizerDelegate, does this trailing comma break things? I'm not really a fan of guarding parent classes with #ifdef - the code gets unmaintainable quite quickly. Getting my head around all the speech-recognition states is already quite hard. It actually would't be too hard to subclass StartPageService with a StartPageServiceChromeOS and just #ifdef around `new StartPageService(..)` and a #include in start_page_service_factory.cc -- Can we do that? Then make StartPageService::AppListShown virtual, or have AppListShown call OnAppListShown at the end. StartPageService::Create(Profile*) is another option. Then in start_page_service.cc you would just need #if !defined(OS_CHROMEOS) StartPageService* StartPageService::Create(Profile* profile) { return new StartPageService(profile); } #endif with StartPageServiceChromeOS being provided in start_page_service_chromeos.cc
https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); 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
Although I uploaded a new patchset and replying some comments, I don't think the uploaded patchset is final. I'll think a bit more about the code around hotwording. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:46: bool IsVoiceSearchEnabled() { On 2014/11/25 05:03:10, tapted wrote: > 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 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... https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); On 2014/11/25 11:07:52, tapted wrote: > 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 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. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:251: if (IsVoiceSearchEnabled()) { On 2014/11/25 05:03:10, tapted wrote: > 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. 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. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:280: new_state = SPEECH_RECOGNITION_OFF; On 2014/11/25 05:03:10, tapted wrote: > This can leave new_state == state_ -- should that suppress the > FOR_EACH_OBSERVER(..)? (comment here?) Done. https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.h:42: public SpeechRecognizerDelegate, On 2014/11/25 05:03:11, tapted wrote: > does this trailing comma break things? > > I'm not really a fan of guarding parent classes with #ifdef - the code gets > unmaintainable quite quickly. Getting my head around all the speech-recognition > states is already quite hard. > > It actually would't be too hard to subclass StartPageService with a > StartPageServiceChromeOS and just #ifdef around `new StartPageService(..)` and a > #include in start_page_service_factory.cc -- Can we do that? > > Then make StartPageService::AppListShown virtual, or have AppListShown call > OnAppListShown at the end. > > > StartPageService::Create(Profile*) is another option. Then in > start_page_service.cc you would just need > > #if !defined(OS_CHROMEOS) > StartPageService* StartPageService::Create(Profile* profile) { > return new StartPageService(profile); > } > #endif > > > with StartPageServiceChromeOS being provided in start_page_service_chromeos.cc You are right. I think StartPageServiceChromeOS is an overkill in this situation -- I factored out the audio handling code into an internal class, named AudioDeviceStatus. WDYT?
https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); On 2014/11/26 01:39:21, Jun Mukai wrote: > On 2014/11/25 11:07:52, tapted wrote: > > 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 > > 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. 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 :) https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.h:42: public SpeechRecognizerDelegate, On 2014/11/26 01:39:21, Jun Mukai wrote: > On 2014/11/25 05:03:11, tapted wrote: > > does this trailing comma break things? > > > > I'm not really a fan of guarding parent classes with #ifdef - the code gets > > unmaintainable quite quickly. Getting my head around all the > speech-recognition > > states is already quite hard. > > > > It actually would't be too hard to subclass StartPageService with a > > StartPageServiceChromeOS and just #ifdef around `new StartPageService(..)` and > a > > #include in start_page_service_factory.cc -- Can we do that? > > > > Then make StartPageService::AppListShown virtual, or have AppListShown call > > OnAppListShown at the end. > > > > > > StartPageService::Create(Profile*) is another option. Then in > > start_page_service.cc you would just need > > > > #if !defined(OS_CHROMEOS) > > StartPageService* StartPageService::Create(Profile* profile) { > > return new StartPageService(profile); > > } > > #endif > > > > > > with StartPageServiceChromeOS being provided in start_page_service_chromeos.cc > > You are right. > I think StartPageServiceChromeOS is an overkill in this situation -- I factored > out the audio handling code into an internal class, named AudioDeviceStatus. > WDYT? looks good! https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/search_resource_manager.cc (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/search_resource_manager.cc:48: } it looks like the whole if block above is redundant, since the call to OnSpeechRecognitionStateChanged below calls search_box_->SetSpeechRecognitionButton(..) unconditionally https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:129: if (CanListen()) { nit: perhaps start_page_service_->OnSpeechRecognitionStateChanged( CanListen() ? SPEECH_RECOGNITION_READY : SPEECH_RECOGNITION_OFF); https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:144: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.h:24: #include "chromeos/audio/cras_audio_handler.h" move this to cc file?
https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); On 2014/11/26 02:05:58, tapted wrote: > On 2014/11/26 01:39:21, Jun Mukai wrote: > > On 2014/11/25 11:07:52, tapted wrote: > > > 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 > > > > 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. > > 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 :) 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.
https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); On 2014/11/26 10:49:58, Anand Mistry wrote: > On 2014/11/26 02:05:58, tapted wrote: > > On 2014/11/26 01:39:21, Jun Mukai wrote: > > > On 2014/11/25 11:07:52, tapted wrote: > > > > 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 > > > > > > 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. > > > > 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 :) > > 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. 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.
PTAL https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/1/chrome/browser/ui/app_list/s... chrome/browser/ui/app_list/start_page_service.cc:169: chromeos::CrasAudioHandler::Get()->AddAudioObserver(this); On 2014/11/26 21:59:44, Jun Mukai wrote: > On 2014/11/26 10:49:58, Anand Mistry wrote: > > On 2014/11/26 02:05:58, tapted wrote: > > > On 2014/11/26 01:39:21, Jun Mukai wrote: > > > > On 2014/11/25 11:07:52, tapted wrote: > > > > > 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 > > > > > > > > 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. > > > > > > 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 :) > > > > 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. > > 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. 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. https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/search/search_resource_manager.cc (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/search/search_resource_manager.cc:48: } On 2014/11/26 02:05:59, tapted wrote: > it looks like the whole if block above is redundant, since the call to > OnSpeechRecognitionStateChanged below calls > search_box_->SetSpeechRecognitionButton(..) unconditionally Done. https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.cc (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:129: if (CanListen()) { On 2014/11/26 02:05:59, tapted wrote: > nit: perhaps > > start_page_service_->OnSpeechRecognitionStateChanged( > CanListen() ? SPEECH_RECOGNITION_READY : SPEECH_RECOGNITION_OFF); Done. https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.cc:144: }; On 2014/11/26 02:05:59, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/start_page_service.h (right): https://codereview.chromium.org/752253002/diff/20001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/start_page_service.h:24: #include "chromeos/audio/cras_audio_handler.h" On 2014/11/26 02:05:59, tapted wrote: > move this to cc file? Done.
lgtm!
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/752253002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mukai@chromium.org changed reviewers: + oshima@chromium.org
oops. +oshima for chrome/browser/chromeos changes.
oshima, could you take a look?
lgtm with a nit. https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:805: // Called after ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() to Shutdown after PostMainMessageLoopRun() which should destroy all observers. https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:806: // wait for removing AudioObservers in chrome/ "." at the end.
https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:805: // Called after ChromeBrowserMainPartsLinux::PostMainMessageLoopRun() to On 2014/12/01 21:31:42, oshima wrote: > Shutdown after PostMainMessageLoopRun() which should destroy all observers. Done. https://codereview.chromium.org/752253002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:806: // wait for removing AudioObservers in chrome/ On 2014/12/01 21:31:42, oshima wrote: > "." at the end. Done.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/752253002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3f492b65105964526ca8dc5d6a66971c0a427cdd Cr-Commit-Position: refs/heads/master@{#306296}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/774553002/ by kbr@chromium.org. The reason for reverting is: Caused assertion failures in debug Linux ChromiumOS browser_tests; see Issue 383624 for details. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
