Expose sky KeyboardService in android mojo_shell

Expose sky KeyboardService in android mojo_shell

R=eseidel@chromium.org, qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/272572846ef8db2211f0dcaa3f1135a0acc2cb0e

[jamesr, 2015-04-15 23:03:12]

jamesr@chromium.org changed reviewers:
+ eseidel@chromium.org, qsr@chromium.org

[jamesr, 2015-04-15 23:03:13]

eseidel - please review //sky
qsr - please review //shell
davemoore - FYI

This wires things up enough so that show/hide work within mojo_shell on the
stocks app.  The keyboard itself doesn't work correctly (typing does nothing)
which likely indicates an issue in the Java-side initialization.

[eseidel, 2015-04-15 23:09:04]

lgtm

https://codereview.chromium.org/1088793003/diff/40001/sky/framework/shell.dart
File sky/framework/shell.dart (right):

https://codereview.chromium.org/1088793003/diff/40001/sky/framework/shell.dar...
sky/framework/shell.dart:15: if (proxyHandle.isValid) serviceProvider =
Really?  This is dart style?

https://codereview.chromium.org/1088793003/diff/40001/sky/framework/shell.dar...
sky/framework/shell.dart:23: if (embedder.shell == null)
_connection.requestService(proxy);
Why are one-line ifs a good thing? :p

[hansmuller1, 2015-04-15 23:13:34]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-04-15 23:13:34]

https://codereview.chromium.org/1088793003/diff/40001/sky/framework/shell.dart
File sky/framework/shell.dart (right):

https://codereview.chromium.org/1088793003/diff/40001/sky/framework/shell.dar...
sky/framework/shell.dart:23: if (embedder.shell == null)
_connection.requestService(proxy);
On 2015/04/15 23:09:04, eseidel wrote:
> Why are one-line ifs a good thing? :p

According to the Dart Style Guide, if statements should use curly braces and
span multiple lines except:

"short if statements with no else clause that fit on one line may omit the
braces.

if (arg == null) return defaultValue;"

[jamesr, 2015-04-15 23:29:22]

Hmm, this breaks these sky tests:

  editing/backspace.sky [ Timeout ]
  editing/replace.sky [ Timeout ]
  editing/selection.sky [ Timeout ]
  editing/typing.sky [ Timeout ]
  services/network.sky [ Timeout ]


presumably the test framework was mocking these out somehow?

[eseidel, 2015-04-16 00:02:20]

Those all run inside test_harness, which is run inside mojo shell.  So it's
using the real mojo network service, etc.

[eseidel, 2015-04-16 00:04:27]

https://github.com/domokit/mojo/tree/master/sky/tools/tester
To see how its driving mojo shell:
https://github.com/domokit/mojo/blob/master/sky/tools/webkitpy/layout_tests/p...

[qsr, 2015-04-16 02:05:38]

LGTM with the dependency fixed as a way to move forward.

The fact that this never issue any key is probably because native viewport
service is eating all key event to send that to the input manager, so you
probably need to rip this code apart.

Also in the long term, I don't think application should get this service from
the mojo:keyboard app, because we now have a security issue -> any app can get
this service and receive all key information, so they can steal information from
other app. Only the window manager should get access to this service directly,
other application should get it from the SP from their embedder and will only
receive event when they are focused.

https://codereview.chromium.org/1088793003/diff/40001/shell/BUILD.gn
File shell/BUILD.gn (right):

https://codereview.chromium.org/1088793003/diff/40001/shell/BUILD.gn#newcode302
shell/BUILD.gn:302: "//sky/services/keyboard",
That seems wrong. The shell should not depend on sky.

[jamesr, 2015-04-16 18:25:53]

On 2015/04/16 02:05:38, qsr wrote:
> LGTM with the dependency fixed as a way to move forward.
> 
> The fact that this never issue any key is probably because native viewport
> service is eating all key event to send that to the input manager, so you
> probably need to rip this code apart.
> 
> Also in the long term, I don't think application should get this service from
> the mojo:keyboard app, because we now have a security issue -> any app can get
> this service and receive all key information, so they can steal information
from
> other app. Only the window manager should get access to this service directly,
> other application should get it from the SP from their embedder and will only
> receive event when they are focused.

True.  We currently also let any app talk to mojo:native_viewport, so this isn't
novel.

> 
> https://codereview.chromium.org/1088793003/diff/40001/shell/BUILD.gn
> File shell/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1088793003/diff/40001/shell/BUILD.gn#newcode302
> shell/BUILD.gn:302: "//sky/services/keyboard",
> That seems wrong. The shell should not depend on sky.

Good point. I'll move the impl code elsewhere

[jamesr, 2015-04-16 22:26:55]

Now it actually works, PTAL

https://codereview.chromium.org/1088793003/diff/60001/sky/tools/deploy_sdk.py
File sky/tools/deploy_sdk.py (right):

https://codereview.chromium.org/1088793003/diff/60001/sky/tools/deploy_sdk.py...
sky/tools/deploy_sdk.py:174: sdk_path('packages/keyboard/lib'), gen_filter)
this is a hack around the lack of a good place to put generated mojom.dart
files. we've had discussions about how to improve this but i think this is OK
for now to be able to type

[qsr, 2015-04-16 22:34:03]

LGTM with nit.

https://codereview.chromium.org/1088793003/diff/60001/services/keyboard/org/d...
File services/keyboard/org/domokit/keyboard/KeyboardServiceImpl.java (right):

https://codereview.chromium.org/1088793003/diff/60001/services/keyboard/org/d...
services/keyboard/org/domokit/keyboard/KeyboardServiceImpl.java:49: public void
close() {}
This should close the client if it is not null.

[jamesr, 2015-04-17 00:11:19]

Committed patchset #5 (id:80001) manually as
272572846ef8db2211f0dcaa3f1135a0acc2cb0e (presubmit successful).