Implement Linux IME support

Implement Linux IME support

Create a Linux implementation of mojo:keyboard
Add a method to receive key events from native viewport
Add an interface to keyboard.mojom to allow binding the Linux
implementation as the native viewport's key event listener

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/2ccd4931ab53518ba596827825aff6fd92f22ce0

[Kris, 2015-11-19 21:19:58]

Description was changed from

==========
Implement Linux IME support

Create a Linux implementation of mojo:keyboard
Add a method to receive key events from native viewport
Add an interface to keyboard.mojom to allow binding the Linux
implementation as the native viewport's key event listener
==========

to

==========
Implement Linux IME support

Create a Linux implementation of mojo:keyboard
Add a method to receive key events from native viewport
Add an interface to keyboard.mojom to allow binding the Linux
implementation as the native viewport's key event listener
==========

[Kris, 2015-11-19 21:19:58]

kgiesing@google.com changed reviewers:
+ abarth@chromium.org, jamesr@chromium.org

[jamesr, 2015-11-19 23:20:18]

https://codereview.chromium.org/1453823005/diff/1/mojo/services/keyboard/inte...
File mojo/services/keyboard/interfaces/keyboard.mojom (right):

https://codereview.chromium.org/1453823005/diff/1/mojo/services/keyboard/inte...
mojo/services/keyboard/interfaces/keyboard.mojom:56: CreateKeyboardService(
this could use some documentation

https://codereview.chromium.org/1453823005/diff/1/mojo/services/native_viewpo...
File mojo/services/native_viewport/interfaces/native_viewport.mojom (right):

https://codereview.chromium.org/1453823005/diff/1/mojo/services/native_viewpo...
mojo/services/native_viewport/interfaces/native_viewport.mojom:37:
SetKeyEventDispatcher(NativeViewportEventDispatcher dispatcher);
The naming here is very strange - what's the difference between "SetEvent" and
"SetKeyEvent"? Are key events not events?  It's possible that the first method
should be called SetPointerEventDispatcher() but that raises another issue. 
Splitting different types of events at the NativeViewport level means that it's
impossible for things further on in the system to reconstruct the original order
(unless the event protocol provides additional information).  When key events
are dispatched to an IME abstraction the original order probably doesn't matter
that much but when they aren't the order definitely does matter and
NativeViewport is the wrong layer to make a decision about which matters.  I
think that splitting the stream must be done at a higher level of the stack that
has more information about the intended use case.  For now, that means in
flutter (or other apps that talk directly to native viewport).  When mozart is
further along the view manager system will be able to make this decision.

Concretely for this patch I think you should send key events down the current
(one) NativeViewportEventDispatcher interface and then forward the key events to
the IME from flutter.  This means having another pipe and copy of the events for
now, but once the view manager is performing the dispatching the extra cost will
go away (since the view manager will have to multiplex anyway)

[jamesr, 2015-11-19 23:47:33]

Rest of the changes lgtm with a number of style/nitpicky comments.

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
File services/keyboard/linux/keyboard_service_impl.cc (right):

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.cc:47: /*
don't check in commented out code

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.cc:78: case
mojo::KeyboardCode::TAB: // tab
nit: delete the "// tab" part

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.cc:82:
client_->Submit(::keyboard::SubmitAction::DONE);
omit :: in ::keyboard

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.cc:87: std::string s =
base::UTF16ToUTF8(character);
there's gotta be a better way to do this, although nothing sticks out. i think
the reason the source data is 16 bits is compat with win32 APIs which is a
pretty poor reason. we can fix it later

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
File services/keyboard/linux/keyboard_service_impl.h (right):

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.h:17: class
LinuxKeyboardServiceImpl : public ::keyboard::KeyboardService,
omit the :: in "::keyboard"

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.h:20: explicit
LinuxKeyboardServiceImpl(
only say 'explicit' if it's a one-argument constructor. implicit conversions are
much more difficult to accidentally trigger for multi-arg constructors

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.h:24: void
Show(::keyboard::KeyboardClientPtr client,
nit: put a blank line and a comment before these indicating what base class they
are overriding, i.e.
"// keyboard::KeyboardService implementation."

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/key...
services/keyboard/linux/keyboard_service_impl.h:31: void OnEvent(mojo::EventPtr
event,
ditto here: "// mojo::NativeViewportEventDispatcher implementation."

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/mai...
File services/keyboard/linux/main.cc (right):

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/mai...
services/keyboard/linux/main.cc:21: KeyboardServiceFactoryImpl(
explicit

https://codereview.chromium.org/1453823005/diff/1/services/keyboard/linux/mai...
services/keyboard/linux/main.cc:35: };
DISALLOW_... ?

[jamesr, 2015-11-20 00:09:02]

The linker error on the bots looks like the name change, you probably need to
add a name annotation to KeyboardServiceFactory to register the service (see
vtl's recent email to mojo-dev)

lgtm otherwise

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
File services/keyboard/linux/keyboard_service_impl.h (right):

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:35: const
mojo::Callback<void()>& callback) override;
FYI the C++ code generator produces a typedef for the callback type called
'<method name>Callback', so you can say 'const mojo::OnEventCallback& callback'
instead of tying out the function signature

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:39: mojo::ApplicationConnection*
connection_;
these two members appear unused, delete

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:40:
mojo::StrongBinding<keyboard::KeyboardService> binding_;
i think you should put the StrongBinding<> last in the member list since it
controls the lifetime of the object so you probably want it to be destructed
before other member variables. otherwise, if the object is still live while
destructors for other members run, they could end up calling into the binding_
object and it won't yet know that destruction is under way

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:51: #endif /*
defined(SERVICES_KEYBOARD_LINUX_KEYBOARD_SERVICE_IMPL_H_) */
#endif  // SERVICES_...

we basically never use /* */ style comments in c++ code

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
File services/keyboard/linux/main.cc (right):

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/main.cc:39: : public mojo::ApplicationDelegate,
indentation looks off here.  run 'git cl format' to fix up formatting

[Kris, 2015-11-20 01:37:35]

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
File services/keyboard/linux/keyboard_service_impl.h (right):

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:35: const
mojo::Callback<void()>& callback) override;
On 2015/11/20 00:09:02, jamesr wrote:
> FYI the C++ code generator produces a typedef for the callback type called
> '<method name>Callback', so you can say 'const mojo::OnEventCallback&
callback'
> instead of tying out the function signature

Done.

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:39: mojo::ApplicationConnection*
connection_;
On 2015/11/20 00:09:02, jamesr wrote:
> these two members appear unused, delete

Done.

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:40:
mojo::StrongBinding<keyboard::KeyboardService> binding_;
On 2015/11/20 00:09:02, jamesr wrote:
> i think you should put the StrongBinding<> last in the member list since it
> controls the lifetime of the object so you probably want it to be destructed
> before other member variables. otherwise, if the object is still live while
> destructors for other members run, they could end up calling into the binding_
> object and it won't yet know that destruction is under way

Done.

https://codereview.chromium.org/1453823005/diff/20001/services/keyboard/linux...
services/keyboard/linux/keyboard_service_impl.h:51: #endif /*
defined(SERVICES_KEYBOARD_LINUX_KEYBOARD_SERVICE_IMPL_H_) */
On 2015/11/20 00:09:02, jamesr wrote:
> #endif  // SERVICES_...
> 
> we basically never use /* */ style comments in c++ code

Done.

[Kris, 2015-11-20 18:07:54]

Committed patchset #3 (id:40001) manually as
2ccd4931ab53518ba596827825aff6fd92f22ce0 (presubmit successful).