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

media/audio/audio_system_impl.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_system_impl.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner_util.h"
 #include "media/audio/audio_manager.h"
@@ skipping 15 matching lines @@
     return AudioParameters();
 
   return audio_manager->GetInputStreamParameters(device_id);
 }
 
 }  // namespace
 
 AudioSystemImpl::AudioSystemImpl(AudioManager* audio_manager)
     : audio_manager_(audio_manager) {
   DCHECK(audio_manager_);
+  AudioSystem::SetInstance(this);
 }
 
-AudioSystemImpl::~AudioSystemImpl() {}
+AudioSystemImpl::~AudioSystemImpl() {
+  AudioSystem::ClearInstance(this);
+}
 
 // static
 std::unique_ptr<AudioSystem> AudioSystemImpl::Create(
     AudioManager* audio_manager) {
   return base::WrapUnique(new AudioSystemImpl(audio_manager));
 }
 
 void AudioSystemImpl::GetInputStreamParameters(
     const std::string& device_id,
     OnAudioParamsCallback on_params_cb) const {
   if (GetTaskRunner()->BelongsToCurrentThread()) {
     GetTaskRunner()->PostTask(
         FROM_HERE, base::Bind(on_params_cb, GetInputParametersOnDeviceThread(
                                                 audio_manager_, device_id)));
     return;
   }
   base::PostTaskAndReplyWithResult(
       GetTaskRunner(), FROM_HERE,
       base::Bind(&GetInputParametersOnDeviceThread,
                  base::Unretained(audio_manager_), device_id),
       std::move(on_params_cb));
 }
 
+void AudioSystemImpl::HasInputDevices(OnBoolCallback on_has_devices_cb) const {
+  if (GetTaskRunner()->BelongsToCurrentThread()) {

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

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:10]

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)
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?
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)

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

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.

+    GetTaskRunner()->PostTask(
+        FROM_HERE,
+        base::Bind(on_has_devices_cb, audio_manager_->HasAudioInputDevices()));
+    return;
+  }
+  base::PostTaskAndReplyWithResult(
+      GetTaskRunner(), FROM_HERE,
+      base::Bind(&AudioManager::HasAudioInputDevices,
+                 base::Unretained(audio_manager_)),
+      std::move(on_has_devices_cb));
+}
+
 AudioManager* AudioSystemImpl::GetAudioManager() const {
   return audio_manager_;
 }
 
 base::SingleThreadTaskRunner* AudioSystemImpl::GetTaskRunner() const {
   return audio_manager_->GetTaskRunner();
 }
 
 }  // namespace media