Allow virtual keyboard window to access audio stream and speech api for voice input

Allow virtual keyboard window to access audio stream and speech api for voice input


BUG=238350
TEST=
1. navigate to chrome://flags
2. turn on virtual keyboard flag
3. login again. focus on any input box (virtual keyboard should show up)
4. press "mic" button, it should turn to green.
5. speak
6. result should be displayed in the input box

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202123

[bshe, 2013-05-13 17:59:55]

Hi Sadrul.

This CL is not intend for commit but only for discussion.

It seems keyboard window in chrome doesn't have access to audio stream (we want
it for speech recognition). And the root cause seems that we didn't override
function RequestMediaAccessPermission. Please see patchset 1 to see what we
basically need to get access. Most of the dependency is in chrome/* directory. I
am thinking to move the delegate implementation to
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc But I am not sure if
there are any implication with it. So I just want to check with you to see if
you have any opinion. I am not familiar with web contents and keyboard window
container. So I may totally wrong. In any case, any suggestion is welcome!

Thanks.

[sadrul, 2013-05-14 05:13:03]

https://codereview.chromium.org/14646041/diff/1/ui/keyboard/keyboard_controll...
File ui/keyboard/keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/1/ui/keyboard/keyboard_controll...
ui/keyboard/keyboard_controller_proxy.cc:43: const
content::MediaResponseCallback& callback) OVERRIDE {
I took a brief look at the code in ProcessMediaAccessRequest, and
WebContentsImpl::RequestMediaAccessPermission. It doesn't look like there is a
good way of doing all of the work from here without duplicating a lot of code
from browser/media/.

We don't want to move the entire KeyboardContentsDelegate into chrome. An
alternate solution for this would be to introduce
KeyboardControllerProxy::RequestAudioInput (or something like that). The chrome
implementation (in AshKeyboardControllerProxy) would interact with
MediaCaptureDevicesDispatcher. Note that the implementation should just accept
the request, without showing an infobar to the user. The default implementation
can deny the request.

KeyboardContentsDelegate should have a reference to the KCP, and call into its
RequestAudioInput from here.

[bshe, 2013-05-14 18:34:24]

Could you please take another look? THanks!

On 2013/05/14 05:13:03, sadrul wrote:
>
https://codereview.chromium.org/14646041/diff/1/ui/keyboard/keyboard_controll...
> File ui/keyboard/keyboard_controller_proxy.cc (right):
> 
>
https://codereview.chromium.org/14646041/diff/1/ui/keyboard/keyboard_controll...
> ui/keyboard/keyboard_controller_proxy.cc:43: const
> content::MediaResponseCallback& callback) OVERRIDE {
> I took a brief look at the code in ProcessMediaAccessRequest, and
> WebContentsImpl::RequestMediaAccessPermission. It doesn't look like there is a
> good way of doing all of the work from here without duplicating a lot of code
> from browser/media/.
> 
> We don't want to move the entire KeyboardContentsDelegate into chrome. An
> alternate solution for this would be to introduce
> KeyboardControllerProxy::RequestAudioInput (or something like that). The
chrome
> implementation (in AshKeyboardControllerProxy) would interact with
> MediaCaptureDevicesDispatcher. Note that the implementation should just accept
> the request, without showing an infobar to the user. The default
implementation
> can deny the request.
I have add the permission to access media device from the extension's manifest.
With the extension passed to ProcessMediaAccessRequest, it seems no infobar is
showing and the request is granted.

> 
> KeyboardContentsDelegate should have a reference to the KCP, and call into its
> RequestAudioInput from here.

[bshe, 2013-05-14 18:35:21]

Sorry. forget to include my inline comment.

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is because
speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in either of
them. Do we have plan to create a new view type for keyboard view? It seems very
different from all other view types from what I understand. It is currently a
VIEW_TYPE_INVALID.

[sadrul, 2013-05-16 19:34:07]

ui/keyboard LGTM

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
On 2013/05/14 18:35:21, bshe wrote:
> This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is
because
> speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in either
of
> them. Do we have plan to create a new view type for keyboard view? It seems
very
> different from all other view types from what I understand. It is currently a
> VIEW_TYPE_INVALID.

I am not really sure. bryeung@ or someone more familiar with extensions than I
should know.

https://codereview.chromium.org/14646041/diff/7001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller_proxy.cc:22:
KeyboardContentsDelegate(keyboard::KeyboardControllerProxy* proxy)
explicit

[bryeung, 2013-05-16 20:34:49]

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
On 2013/05/16 19:34:08, sadrul wrote:
> On 2013/05/14 18:35:21, bshe wrote:
> > This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is
> because
> > speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> > VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in either
> of
> > them. Do we have plan to create a new view type for keyboard view? It seems
> very
> > different from all other view types from what I understand. It is currently
a
> > VIEW_TYPE_INVALID.
> 
> I am not really sure. bryeung@ or someone more familiar with extensions than I
> should know.

Sadly I'm not really sure either.  You'd probably need to talk to someone like
mpcomplete@.

https://codereview.chromium.org/14646041/diff/7001/ui/keyboard/keyboard_contr...
File ui/keyboard/keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/ui/keyboard/keyboard_contr...
ui/keyboard/keyboard_controller_proxy.cc:38: virtual void
RequestMediaAccessPermission(content::WebContents* web_contents,
nit: move this argument to the next line as well

[bshe, 2013-05-17 18:38:37]

+mpcomplete for keyboard VIEW_TYPE_TAB_CONTENTS discussion.
Please see the context here:
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...

Thanks!

On 2013/05/16 20:34:49, bryeung wrote:
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):
> 
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
> On 2013/05/16 19:34:08, sadrul wrote:
> > On 2013/05/14 18:35:21, bshe wrote:
> > > This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is
> > because
> > > speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> > > VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in
either
> > of
> > > them. Do we have plan to create a new view type for keyboard view? It
seems
> > very
> > > different from all other view types from what I understand. It is
currently
> a
> > > VIEW_TYPE_INVALID.
> > 
> > I am not really sure. bryeung@ or someone more familiar with extensions than
I
> > should know.
> 
> Sadly I'm not really sure either.  You'd probably need to talk to someone like
> mpcomplete@.

[Matt Perry, 2013-05-17 19:19:53]

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):

https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
On 2013/05/16 20:34:49, bryeung wrote:
> On 2013/05/16 19:34:08, sadrul wrote:
> > On 2013/05/14 18:35:21, bshe wrote:
> > > This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is
> > because
> > > speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> > > VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in
either
> > of
> > > them. Do we have plan to create a new view type for keyboard view? It
seems
> > very
> > > different from all other view types from what I understand. It is
currently
> a
> > > VIEW_TYPE_INVALID.
> > 
> > I am not really sure. bryeung@ or someone more familiar with extensions than
I
> > should know.
> 
> Sadly I'm not really sure either.  You'd probably need to talk to someone like
> mpcomplete@.

I don't think I have enough context. AFAICT, this class represents a renderer
host that exists to display a virtual keyboard which is implemented in HTML/JS.
Is that right? And if so, that HTML needs access to some speech recognition
APIs? If so, where is this HTML served from? Is it a component extension?

[bshe, 2013-05-17 19:30:54]

On 2013/05/17 19:19:53, Matt Perry wrote:
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):
> 
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
> On 2013/05/16 20:34:49, bryeung wrote:
> > On 2013/05/16 19:34:08, sadrul wrote:
> > > On 2013/05/14 18:35:21, bshe wrote:
> > > > This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS is
> > > because
> > > > speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> > > > VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in
> either
> > > of
> > > > them. Do we have plan to create a new view type for keyboard view? It
> seems
> > > very
> > > > different from all other view types from what I understand. It is
> currently
> > a
> > > > VIEW_TYPE_INVALID.
> > > 
> > > I am not really sure. bryeung@ or someone more familiar with extensions
than
> I
> > > should know.
> > 
> > Sadly I'm not really sure either.  You'd probably need to talk to someone
like
> > mpcomplete@.
> 
> I don't think I have enough context. AFAICT, this class represents a renderer
> host that exists to display a virtual keyboard which is implemented in
HTML/JS.
> Is that right? And if so, that HTML needs access to some speech recognition
> APIs? If so, where is this HTML served from? Is it a component extension?

Sorry, I should make it more clear. You are right. The virtual keyboard is a
component extension. Instead of using a regular extension window to host
html/js, it creates its own type of window.

I dont know exactly the view type is intended for. But the speech recognition
apis can only be accessed from VIEW_TYPE_TAB_CONTENTS and VIEW_TYPE_APP_SHELL.
So I was thinking to create a new type called VIEW_TYPE_KEYBOARD and inject
speech apis to that type. But I may totally miss the point of view type.

[Matt Perry, 2013-05-17 20:22:16]

On 2013/05/17 19:30:54, bshe wrote:
> On 2013/05/17 19:19:53, Matt Perry wrote:
> >
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> > File chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/14646041/diff/7001/chrome/browser/ui/ash/ash_...
> > chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc:71: this));
> > On 2013/05/16 20:34:49, bryeung wrote:
> > > On 2013/05/16 19:34:08, sadrul wrote:
> > > > On 2013/05/14 18:35:21, bshe wrote:
> > > > > This is probably hacky? The reason I set it to VIEW_TYPE_TAB_CONTENTS
is
> > > > because
> > > > > speech recognition are only enabled for VIEW_TYPE_TAB_CONTENTS and
> > > > > VIEW_TYPE_APP_SHELL now. But the keyboard view doesn't seem to fit in
> > either
> > > > of
> > > > > them. Do we have plan to create a new view type for keyboard view? It
> > seems
> > > > very
> > > > > different from all other view types from what I understand. It is
> > currently
> > > a
> > > > > VIEW_TYPE_INVALID.
> > > > 
> > > > I am not really sure. bryeung@ or someone more familiar with extensions
> than
> > I
> > > > should know.
> > > 
> > > Sadly I'm not really sure either.  You'd probably need to talk to someone
> like
> > > mpcomplete@.
> > 
> > I don't think I have enough context. AFAICT, this class represents a
renderer
> > host that exists to display a virtual keyboard which is implemented in
> HTML/JS.
> > Is that right? And if so, that HTML needs access to some speech recognition
> > APIs? If so, where is this HTML served from? Is it a component extension?
> 
> Sorry, I should make it more clear. You are right. The virtual keyboard is a
> component extension. Instead of using a regular extension window to host
> html/js, it creates its own type of window.
> 
> I dont know exactly the view type is intended for. But the speech recognition
> apis can only be accessed from VIEW_TYPE_TAB_CONTENTS and VIEW_TYPE_APP_SHELL.
> So I was thinking to create a new type called VIEW_TYPE_KEYBOARD and inject
> speech apis to that type. But I may totally miss the point of view type.

Yes, please add a VIEW_TYPE_VIRTUAL_KEYBOARD and use that, and allow speech APIs
for it. I actually don't understand why speech APIs are restricted by view type
at all, but this is at least uninvasive.

[bshe, 2013-05-21 15:25:13]

mpcomplete@ added a virtual_keyboard view type. Could you please take another
look?

+sky for OWNERS chrome/browser/ui/ash/*
+xians for OWNERS chrome/browser/speech/*

[Matt Perry, 2013-05-21 18:00:34]

lgtm

https://codereview.chromium.org/14646041/diff/20001/extensions/common/view_ty...
File extensions/common/view_type.h (right):

https://codereview.chromium.org/14646041/diff/20001/extensions/common/view_ty...
extensions/common/view_type.h:28: VIEW_TYPE_VIRTUAL_KEYBOARD
nit: leave a trailing comma

[bshe, 2013-05-23 13:59:23]

Done.

And actually add owners:

+sky for OWNERS chrome/browser/ui/ash/*
+xians for OWNERS chrome/browser/speech/*

https://codereview.chromium.org/14646041/diff/20001/extensions/common/view_ty...
File extensions/common/view_type.h (right):

https://codereview.chromium.org/14646041/diff/20001/extensions/common/view_ty...
extensions/common/view_type.h:28: VIEW_TYPE_VIRTUAL_KEYBOARD
On 2013/05/21 18:00:34, Matt Perry wrote:
> nit: leave a trailing comma

Done.

[no longer working on chromium, 2013-05-23 15:21:02]

rubber stamp for speech code

[no longer working on chromium, 2013-05-23 15:21:11]

On 2013/05/23 15:21:02, xians1 wrote:
> rubber stamp for speech code

lgtm

[sky, 2013-05-23 15:48:44]

LGTM

[commit-bot: I haz the power, 2013-05-23 15:49:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/14646041/31001

[commit-bot: I haz the power, 2013-05-23 16:13:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-24 14:46:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/14646041/62002

[commit-bot: I haz the power, 2013-05-24 17:51:22]

Change committed as 202123