[Hotword] Delay checking for audio devices until needed.

[Hotword] Delay checking for audio devices until needed.

Checking for audio devices right away is causing performance regressions on Mac. This CL delays
checking until needed, but does not include the availability of an audio capture device in the
error reporting until after it's known the check has been completed. Otherwise, the settings
page immediately displays an error upon opening.

In general, this should not be a problem as typically the audio device check will have completed
by the time the user goes to the settings page. There is a rare use case, where the user
installs Chrome and immediately goes to settings and in that case, we won't be able to
accurately report if there is an audio device preventing the user from starting hotwording.
However, by the time they reload, the error should be present which should be sufficient.

BUG=385514, 413104
Committed: https://crrev.com/78f60ef1ef5a8f4af3317383985955da4f1d4fdb
Cr-Commit-Position: refs/heads/master@{#300965}

[rpetterson, 2014-10-17 19:34:35]

rlp@chromium.org changed reviewers:
+ amistry@google.com

[Anand Mistry (off Chromium), 2014-10-17 20:32:41]

amistry@chromium.org changed reviewers:
+ amistry@chromium.org

[Anand Mistry (off Chromium), 2014-10-17 20:32:42]

https://codereview.chromium.org/654613004/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/654613004/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:401:
HotwordServiceFactory::GetInstance()->UpdateMicrophoneState();
If you call UpdateMicrophoneState() first, then IsAudioDeviceStateUpdated() will
always return true since the  former call will always update the audio state
(since it calls HotwordServiceFactory::OnUpdateAudioDevices()). Timing wise, the
ordering of these two statements won't matter. But moving
UpdateMicrophoneState() first will eliminate all the device state logic.

[Anand Mistry (off Chromium), 2014-10-17 21:41:43]

amistry@chromium.org changed reviewers:
- amistry@google.com

[rpetterson, 2014-10-20 06:25:50]

https://codereview.chromium.org/654613004/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service.cc (right):

https://codereview.chromium.org/654613004/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service.cc:401:
HotwordServiceFactory::GetInstance()->UpdateMicrophoneState();
On 2014/10/17 20:32:42, Anand Mistry wrote:
> If you call UpdateMicrophoneState() first, then IsAudioDeviceStateUpdated()
will
> always return true since the  former call will always update the audio state
> (since it calls HotwordServiceFactory::OnUpdateAudioDevices()). Timing wise,
the
> ordering of these two statements won't matter. But moving
> UpdateMicrophoneState() first will eliminate all the device state logic.

Thanks for your comment. It made me look a little deeper and I realized the
OnUpdateAudioDevices is actually called after devices have been enumerated. I've
removed the call to OnUpdateAudioDevices in UpdateMicrophoneState in
hotword_service_factory.cc and I think this logic here now makes more sense.
PTAL.

[Anand Mistry (off Chromium), 2014-10-20 07:09:05]

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:92:
MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices();
I think you've introduced a race here. If this factory is created after devices
have been enumerated (Singletons are lazy), then the microphone state won't be
updated until it changes (since the observer is registered after enumeration),
which may be never. I think you need the call to OnUpdateAudioDevices(). It also
looks like there's no way to distinguish between the "no devices" and "not yet
enumerated" states, so the whole state_updated_ thing is moot.

[rpetterson, 2014-10-20 17:25:07]

If this would be easier to discuss over chat, ping me when you get in :)

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:92:
MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices();
On 2014/10/20 07:09:05, Anand Mistry wrote:
> I think you've introduced a race here. If this factory is created after
devices
> have been enumerated (Singletons are lazy), then the microphone state won't be
> updated until it changes (since the observer is registered after enumeration),
> which may be never. I think you need the call to OnUpdateAudioDevices(). It
also
> looks like there's no way to distinguish between the "no devices" and "not yet
> enumerated" states, so the whole state_updated_ thing is moot.

I disagree. The factory is initialized on startup with a call to GetInstance()
from chrome_browser_main_extra_parts_profiles.cc which gets called from
PreMainMessageLoopRunImpl(). Since the factory is global, this will cover all
profiles. Because of the startup delay this call causes (the root of this bug),
the microphone state is not updated during startup so it must happen after
factory initialization.

The alternative is to return the call to OnUpdateAudioDevices and leave my
existing logic in hotword_service. This will work as well because on the first
call (before we have updated services) |audio_device_state_updated_| will be
false and we won't include the audio device state in the logic calculation for
IsServiceAvailable. After that we will have kicked off the microphone state
update, so the next time the user loads chrome://settings, it should be fine.
The first time this is called -- before the update has occurred -- is the reason
for putting the check for IsAudioDeviceStateUpdated first and then kicking off
the update.

[Anand Mistry (off Chromium), 2014-10-20 21:22:54]

lgtm

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/654613004/diff/40001/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:92:
MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices();
On 2014/10/20 17:25:07, rpetterson wrote:
> On 2014/10/20 07:09:05, Anand Mistry wrote:
> > I think you've introduced a race here. If this factory is created after
> devices
> > have been enumerated (Singletons are lazy), then the microphone state won't
be
> > updated until it changes (since the observer is registered after
enumeration),
> > which may be never. I think you need the call to OnUpdateAudioDevices(). It
> also
> > looks like there's no way to distinguish between the "no devices" and "not
yet
> > enumerated" states, so the whole state_updated_ thing is moot.
> 
> I disagree. The factory is initialized on startup with a call to GetInstance()
> from chrome_browser_main_extra_parts_profiles.cc which gets called from
> PreMainMessageLoopRunImpl(). Since the factory is global, this will cover all
> profiles. Because of the startup delay this call causes (the root of this
bug),
> the microphone state is not updated during startup so it must happen after
> factory initialization.

Thanks for the explanation. This makes a lot more sense.

> 
> The alternative is to return the call to OnUpdateAudioDevices and leave my
> existing logic in hotword_service. This will work as well because on the first
> call (before we have updated services) |audio_device_state_updated_| will be
> false and we won't include the audio device state in the logic calculation for
> IsServiceAvailable. After that we will have kicked off the microphone state
> update, so the next time the user loads chrome://settings, it should be fine.
> The first time this is called -- before the update has occurred -- is the
reason
> for putting the check for IsAudioDeviceStateUpdated first and then kicking off
> the update.

[rpetterson, 2014-10-21 17:58:49]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-10-21 18:00:21]

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

[commit-bot: I haz the power, 2014-10-21 18:00:22]

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

[commit-bot: I haz the power, 2014-10-21 18:00:29]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rpetterson, 2014-10-21 18:02:58]

rlp@chromium.org changed reviewers:
+ jered@chromium.org

[rpetterson, 2014-10-21 18:02:59]

Jered, requesting an OWNERS stamp. Anand has already reviewed (see discussion)

[Jered, 2014-10-21 18:09:53]

On 2014/10/21 18:02:59, rpetterson wrote:
> Jered, requesting an OWNERS stamp. Anand has already reviewed (see discussion)

lgtm

I didn't review the code at all. I thought rlp had owners for hotword_service,
please follow up in a separate CL to add yourself if not...

[rpetterson, 2014-10-21 18:18:26]

On 2014/10/21 18:09:53, Jered wrote:
> On 2014/10/21 18:02:59, rpetterson wrote:
> > Jered, requesting an OWNERS stamp. Anand has already reviewed (see
discussion)
> 
> lgtm
> 
> I didn't review the code at all. I thought rlp had owners for hotword_service,
> please follow up in a separate CL to add yourself if not...

I do for hotword files. I realize the issue now -- Anand doesn't have committer
status so any committer could have given the LGTM (not that I don't appreciate
yours! :). I've nominated him so that we shouldn't run into this again.

[rpetterson, 2014-10-21 18:19:22]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-10-21 18:20:29]

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

[commit-bot: I haz the power, 2014-10-21 18:26:08]

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

[commit-bot: I haz the power, 2014-10-21 18:26:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[rpetterson, 2014-10-23 21:30:35]

The CQ bit was checked by rlp@chromium.org

[commit-bot: I haz the power, 2014-10-23 21:31:35]

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

[commit-bot: I haz the power, 2014-10-23 23:01:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-23 23:01:28]

Patchset 4 (id:??) landed as
https://crrev.com/78f60ef1ef5a8f4af3317383985955da4f1d4fdb
Cr-Commit-Position: refs/heads/master@{#300965}