Switching VirtualKeyboardPrivate keyboard config call stack to receive HasInputDevices() responce a…

Switching VirtualKeyboardPrivate keyboard config call stack to receive HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of content/browser and on any browser thread (approach is similar to AudioManager one).
* HasInputDevices call is moved from ui/keyboard/keyboard_util to ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now, as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not being deleted there because its message loop was shut down during unique_ptr destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2687853002
Cr-Commit-Position: refs/heads/master@{#449704}
Committed: https://chromium.googlesource.com/chromium/src/+/f60887063fcf9c142a478a46f3c052c51256d5ca

[o1ka, 2017-02-09 11:10:30]

Description was changed from

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.

BUG=688383, 672468
==========

to

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.

BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[o1ka, 2017-02-09 11:10:57]

The CQ bit was checked by olka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 11:11:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 11:13:05]

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

[commit-bot: I haz the power, 2017-02-09 11:13:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[o1ka, 2017-02-09 11:26:28]

The CQ bit was checked by olka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 11:26:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[o1ka, 2017-02-09 11:32:20]

Description was changed from

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.

BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[o1ka, 2017-02-09 11:32:20]

olka@chromium.org changed reviewers:
+ bshe@chromium.org, dalecurtis@chromium.org

[commit-bot: I haz the power, 2017-02-09 11:49:45]

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

[commit-bot: I haz the power, 2017-02-09 11:49:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[o1ka, 2017-02-09 14:53:55]

The CQ bit was checked by olka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 14:54:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[o1ka, 2017-02-09 14:58:05]

Description was changed from

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not
being deleted there because its message loop was shut down during unique_ptr
destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[o1ka, 2017-02-09 14:58:05]

olka@chromium.org changed reviewers:
+ tommi@chromium.org
- dalecurtis@chromium.org

[o1ka, 2017-02-09 14:59:29]

Description was changed from

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread.
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not
being deleted there because its message loop was shut down during unique_ptr
destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread (approach is similar to AudioManager
one).
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not
being deleted there because its message loop was shut down during unique_ptr
destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2017-02-09 15:18:21]

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

[commit-bot: I haz the power, 2017-02-09 15:18:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[o1ka, 2017-02-09 16:16:42]

The CQ bit was checked by olka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 16:17:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[o1ka, 2017-02-09 16:18:25]

tommi@ PTAL at content/browser/speech/speech_recognition_browsertest.cc and
media/audio,
bshe@ PTAL at everything else.
Thanks!

[tommi (sloooow) - chröme, 2017-02-09 16:39:34]

lgtm but feel free to ping me on IM if I'm missing something wrt the PostTask
below (or if you want me to take another look).

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
File media/audio/audio_system_impl.cc (right):

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
media/audio/audio_system_impl.cc:66: if
(GetTaskRunner()->BelongsToCurrentThread()) {
I guess this is necessary so that the on_has_devices_cb is called on the audio
thread?
If that's not a requirement and it's fine for the callback to be made on the
calling thread to HasInputDevices, then we can do this with one less PostTask -
right?

If we want to keep this as is, I think it would be good to document (and same
for GetInputStreamParameters I guess)

[o1ka, 2017-02-09 16:56:11]

Thank you, some clarifications:

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
File media/audio/audio_system_impl.cc (right):

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
media/audio/audio_system_impl.cc:66: if
(GetTaskRunner()->BelongsToCurrentThread()) {
On 2017/02/09 16:39:34, tommi (krómíum) wrote:
> I guess this is necessary so that the on_has_devices_cb is called on the audio
> thread?

It's never called on the audio thread unless it is called on audio thread. (I.e.
it is always called on the caller thread)


> If that's not a requirement and it's fine for the callback to be made on the
> calling thread to HasInputDevices, then we can do this with one less PostTask
-
> right?

Calling the callback synchronously is something I avoid intentionally:
1) It will result in different behavior on Mac vs other platforms, which means 2
times more debugging;
2) It also means that the callback must be able to run in the context of the
method which calls HasInputDevices(), i.e., for example (though it's an example
of a bad design): if HasInputDevices() is called under a lock, then the callback
must not take the same lock.
3) It may also result in nested calls to AudioSystem (if a callback also makes
AudioSystem call).

So, "Callback may be called synchronously or posted later" approach seems to
complicate things and reduce debugability.

WDYT?

> 
> If we want to keep this as is, I think it would be good to document (and same
> for GetInputStreamParameters I guess)

What exactly needs to be documented? Now it says that "// Replies are
asynchronously sent to the thread the call is issued on" (see AudioSystem
interface)

[o1ka, 2017-02-09 16:57:24]

:)) "It's never called on the audio thread unless it is called on audio thread.
"
Clarification: "Callback is never called on the audio thread unless
HasInputDevices() is called on audio thread. "

[commit-bot: I haz the power, 2017-02-09 20:07:39]

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

[commit-bot: I haz the power, 2017-02-09 20:07:40]

Dry run: This issue passed the CQ dry run.

[tommi (sloooow) - chröme, 2017-02-10 10:51:07]

lgtm

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
File media/audio/audio_system_impl.cc (right):

https://codereview.chromium.org/2687853002/diff/60001/media/audio/audio_syste...
media/audio/audio_system_impl.cc:66: if
(GetTaskRunner()->BelongsToCurrentThread()) {
On 2017/02/09 16:56:10, o1ka (CET) wrote:
> On 2017/02/09 16:39:34, tommi (krómíum) wrote:
> > I guess this is necessary so that the on_has_devices_cb is called on the
audio
> > thread?
> 
> It's never called on the audio thread unless it is called on audio thread.
(I.e.
> it is always called on the caller thread)
> 
> 
> > If that's not a requirement and it's fine for the callback to be made on the
> > calling thread to HasInputDevices, then we can do this with one less
PostTask
> -
> > right?
> 
> Calling the callback synchronously is something I avoid intentionally:
> 1) It will result in different behavior on Mac vs other platforms, which means
2
> times more debugging;
> 2) It also means that the callback must be able to run in the context of the
> method which calls HasInputDevices(), i.e., for example (though it's an
example
> of a bad design): if HasInputDevices() is called under a lock, then the
callback
> must not take the same lock.
> 3) It may also result in nested calls to AudioSystem (if a callback also makes
> AudioSystem call).
> 
> So, "Callback may be called synchronously or posted later" approach seems to
> complicate things and reduce debugability.
> 
> WDYT?
> 
> > 
> > If we want to keep this as is, I think it would be good to document (and
same
> > for GetInputStreamParameters I guess)
> 
> What exactly needs to be documented? Now it says that "// Replies are
> asynchronously sent to the thread the call is issued on" (see AudioSystem
> interface)

Thanks  - now looking at this again, I see I misread the code.  I read it
incorrectly as |if (!BelongsToCurrentThread())|.  Now I see you're actually
saving a PostTask and not adding.

[o1ka, 2017-02-10 11:26:25]

Thanks!

bshe@ - PTAL

[bshe, 2017-02-10 16:55:45]

On 2017/02/10 11:26:25, o1ka (CET) wrote:
> Thanks!
> 
> bshe@ - PTAL

vk lgtm

[o1ka, 2017-02-10 18:23:46]

The CQ bit was checked by olka@chromium.org

[o1ka, 2017-02-10 18:24:21]

On 2017/02/10 16:55:45, bshe wrote:
> On 2017/02/10 11:26:25, o1ka (CET) wrote:
> > Thanks!
> > 
> > bshe@ - PTAL
> 
> vk lgtm
Thanks!

[commit-bot: I haz the power, 2017-02-10 18:24:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 19:52:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486751026947160,
"parent_rev": "d9bfc537eac0e031058b6df63b4290a645318cf8", "commit_rev":
"f60887063fcf9c142a478a46f3c052c51256d5ca"}

[commit-bot: I haz the power, 2017-02-10 19:52:51]

Description was changed from

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread (approach is similar to AudioManager
one).
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not
being deleted there because its message loop was shut down during unique_ptr
destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Switching VirtualKeyboardPrivate keyboard config call stack to receive
HasInputDevices() responce asyncrhonously

* AudioSystem::HasInputDevices() method added;
* AudioSystem::Get() introduced to access AudioSystem instance outside of
content/browser and on any browser thread (approach is similar to AudioManager
one).
* HasInputDevices call is moved from ui/keyboard/keyboard_util to
ChromeVirtualKeyboardDelegate;
* ChromeVirtualKeyboardDelegate::GetKeyboardConfig() replies asynchronously now,
as well as VirtualKeyboardPrivateGetKeyboardConfigFunction.
* Drive-by fix in SpeechRecognition browser tests (MocKAudioManager was not
being deleted there because its message loop was shut down during unique_ptr
destruction.)

TESTING=manually tested virtual keyboard on a chromebook.
BUG=688383, 672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2687853002
Cr-Commit-Position: refs/heads/master@{#449704}
Committed:
https://chromium.googlesource.com/chromium/src/+/f60887063fcf9c142a478a46f3c0...
==========

[commit-bot: I haz the power, 2017-02-10 19:52:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f60887063fcf9c142a478a46f3c0...