athena: Use mojo to provide the bindings for the virtual keyboard.

athena: Use mojo to provide the bindings for the virtual keyboard.

Remove the gin/v8 based bindings, in favour of bindings provided by mojo. Use
the same mechanism as chrome://omnibox to provide the bindings for the virtual
keyboard when the VK extension API is not available.

BUG=380215, 387276
R=sky@chromium.org

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

[sadrul, 2014-06-19 22:13:42]

Hi! This is not ready for formal review yet. But this is my current WIP patch
for providing the virtual-keyboard bindings using mojo. There is some code
duplication in here that needs to be refactored (between VKWebUIController and
MojoWebUIHandler). This also adds the code directly in the core keyboard. I
would like to avoid this, but I haven't figured out a way yet. Please let me
know if you have feedback.

[sadrul, 2014-06-24 22:38:03]

I have cleaned up the code in ui/keyboard/, and some refactoring so that
chrome://omnibox and chrome://keyboard can share some code. I followed the
example of WebUIController for MojoWebUIController, but I think it goes against
the policies set in
http://www.chromium.org/developers/content-module/content-api I considered
merging the code of MojoWebUIController into WebUIController; it avoids adding a
new non-interface class in content/public/ ... but it still feels a bit awkward.
Should this be a separate component in //components/ instead?

[sadrul, 2014-06-25 15:04:25]

On 2014/06/24 22:38:03, sadrul wrote:
> I have cleaned up the code in ui/keyboard/, and some refactoring so that
> chrome://omnibox and chrome://keyboard can share some code. I followed the
> example of WebUIController for MojoWebUIController, but I think it goes
against
> the policies set in
> http://www.chromium.org/developers/content-module/content-api I considered
> merging the code of MojoWebUIController into WebUIController; it avoids adding
a
> new non-interface class in content/public/ ... but it still feels a bit
awkward.
> Should this be a separate component in //components/ instead?

Looks like there has been some changes in related code (in
https://codereview.chromium.org/285333003) which may make it unnecessary to move
this MojoWebUIController into the content layer. I will update this CL, and ping
back for review.

[sky, 2014-06-25 15:59:15]

Ya, sorry, I forgot to mention sammc is changing webui-mojo bindings. You
may want to ping him to see how many more changes are going to happen.

  -Scott


On Wed, Jun 25, 2014 at 8:04 AM, <sadrul@chromium.org> wrote:

> On 2014/06/24 22:38:03, sadrul wrote:
>
>> I have cleaned up the code in ui/keyboard/, and some refactoring so that
>> chrome://omnibox and chrome://keyboard can share some code. I followed the
>> example of WebUIController for MojoWebUIController, but I think it goes
>>
> against
>
>> the policies set in
>> http://www.chromium.org/developers/content-module/content-api I
>> considered
>> merging the code of MojoWebUIController into WebUIController; it avoids
>> adding
>>
> a
>
>> new non-interface class in content/public/ ... but it still feels a bit
>>
> awkward.
>
>> Should this be a separate component in //components/ instead?
>>
>
> Looks like there has been some changes in related code (in
> https://codereview.chromium.org/285333003) which may make it unnecessary
> to move
> this MojoWebUIController into the content layer. I will update this CL,
> and ping
> back for review.
>
> https://codereview.chromium.org/328303008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2014-06-26 01:55:58]

+sammc@

On 2014/06/25 15:59:15, sky wrote:
> Ya, sorry, I forgot to mention sammc is changing webui-mojo bindings. You
> may want to ping him to see how many more changes are going to happen.

I have updated this CL to use the new service-registry. But yes, looks like
there are some more changes in progress
(https://codereview.chromium.org/302573002/). Although it looks like the changes
do not conflict a whole lot so far. sammc@ Do you mind taking a look? Thanks!

[Sam McNally, 2014-06-26 11:04:40]

On 2014/06/26 01:55:58, sadrul wrote:
> I have updated this CL to use the new service-registry. But yes, looks like
> there are some more changes in progress
> (https://codereview.chromium.org/302573002/). Although it looks like the
changes
> do not conflict a whole lot so far. sammc@ Do you mind taking a look? Thanks!

https://codereview.chromium.org/302573002/ changes two things that will conflict
with this:
- services are obtained using the auto-generated service names
- in the renderer, the Mojo handle is no longer passed into the JS

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
File ui/keyboard/resources/keyboard_mojo.js (right):

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
ui/keyboard/resources/keyboard_mojo.js:11: 'ui/keyboard/webui/keyboard.mojom',
You'll need to depend on 'content/public/renderer/service_provider' to get
access to the browser service.

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
ui/keyboard/resources/keyboard_mojo.js:29: return function(handle) {
The caller will no longer pass in the handle. You'll need to use a
serviceProvider to get the handle:
serviceProvider.connectToService(keyboard.KeyboardUIHandlerMojoProxy.NAME_)

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
File ui/keyboard/webui/vk_webui_controller.cc (right):

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
ui/keyboard/webui/vk_webui_controller.cc:65:
host->GetMainFrame()->GetServiceRegistry()->AddService(
You'll want to use AddService<KeyboardUIHandlerMojo> to avoid needing to
manually include the service name.

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
ui/keyboard/webui/vk_webui_controller.cc:72: mojo::ScopedMessagePipeHandle
handle_to_api) {
With the above change, this can turn into
mojo::InterfaceRequest<KeyboardUIHandlerMojo> and BindToPipe below can turn into
BindToRequest.

[sadrul, 2014-07-24 07:35:06]

Hi! Any update on https://codereview.chromium.org/302573002/? If that CL is
going to take more time to land, would it make sense to try to land this CL
first, and make the necessary fixes separately in subsequent CL (or with
302573002)?

[sadrul, 2014-08-01 22:41:47]

Now that http://crrev.com/286459 has landed, I have updated this CL as
suggested. PTAL

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
File ui/keyboard/resources/keyboard_mojo.js (right):

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
ui/keyboard/resources/keyboard_mojo.js:11: 'ui/keyboard/webui/keyboard.mojom',
On 2014/06/26 11:04:40, Sam McNally wrote:
> You'll need to depend on 'content/public/renderer/service_provider' to get
> access to the browser service.

Done.

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/resources/k...
ui/keyboard/resources/keyboard_mojo.js:29: return function(handle) {
On 2014/06/26 11:04:40, Sam McNally wrote:
> The caller will no longer pass in the handle. You'll need to use a
> serviceProvider to get the handle:
> serviceProvider.connectToService(keyboard.KeyboardUIHandlerMojoProxy.NAME_)

Done.

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
File ui/keyboard/webui/vk_webui_controller.cc (right):

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
ui/keyboard/webui/vk_webui_controller.cc:65:
host->GetMainFrame()->GetServiceRegistry()->AddService(
On 2014/06/26 11:04:40, Sam McNally wrote:
> You'll want to use AddService<KeyboardUIHandlerMojo> to avoid needing to
> manually include the service name.

Done.

https://codereview.chromium.org/328303008/diff/200001/ui/keyboard/webui/vk_we...
ui/keyboard/webui/vk_webui_controller.cc:72: mojo::ScopedMessagePipeHandle
handle_to_api) {
On 2014/06/26 11:04:40, Sam McNally wrote:
> With the above change, this can turn into
> mojo::InterfaceRequest<KeyboardUIHandlerMojo> and BindToPipe below can turn
into
> BindToRequest.

Done.

[sky, 2014-08-04 17:33:49]

LGTM

[sadrul, 2014-08-06 10:56:03]

Committed patchset #14 manually as 287755 (presubmit successful).