Initial native keyboard proof of concept.

Initial native keyboard proof of concept.

The mojo:keyboard_native service is written in C++ to be platform agnostic (as opposed to the mojo:keyboard service which is currently only  a java wrapper around the Android IME).

mojo:keyboard_client serves as a client to the mojo:keyboard_native service testing its implementation of the keyboard mojom APIs.

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7da883d63253688cc3e54a1ab81d7117dfc1b83d

[jamesr, 2015-05-08 20:55:16]

I've left a bunch of mostly style comments.  It can take a bit to get used to
our various style conventions.  As for behavior, it looks like there are some
compilation errors on the trybots.  If you click the red bubbles on the
codereview page you can see what they are - try to fix them and upload a new
patch then run 'git cl try' to try again.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
File examples/keyboard_client/BUILD.gn (right):

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:6:
import("//mojo/public/tools/bindings/mojom.gni")
you can remove this - you only need to import this in order to use the "mojom"
template to tell the build system how to handle foo.mojom files

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:10: "//base:base",
just "//base".  in GN, "//foo/bar/blah" is a shorthand for "//foo/bar/blah:blah"

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:17: sources = [
put sources before deps.  style guide for BUILD.gn files:

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
File examples/keyboard_client/keyboard_client.cc (right):

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:5: #include <stdio.h>
don't think you are using this

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:15: namespace mojo {
please put this in the namespace 'examples', not 'mojo::examples'. a lot of old
stuff is in the namespace mojo but we shouldn't do that for new code

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:19: public keyboard::KeyboardClient
{
this 'public' should be aligned with the previous one. run 'git cl format' and
it'll run clang-format over your code to format it up all pretty

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:22: keyboard::KeyboardClientPtr
mystery_;
put member vars after functions and in the private: section why "mystery"? looks
the same as keyboardClient_ lower down

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:26: void Initialize(ApplicationImpl*
app) override {
it's really useful to document what base class a member function is overriding
especially once you have multiple base classes. we have a few different patterns
for this in our codebase but a common one is to have comments like

  // mojo::ApplicationDelegate implementation.
  void Initialize(ApplicationImpl* app) override {
    ...
  }

  // keyboard::KeyboardClient implementation.
  void CommitCompletion(...) override {
    ...
  }

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:31: request_ =
mojo::GetProxy(&mystery_);
since |request_| is only initialized on this line and then passed away on the
next line you can just use a local

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:32: binding_ = new
mojo::Binding<keyboard::KeyboardClient>(this,
there's no need to heap allocate |binding_|. you can make it a member variable,
initialize it with |this| in the KeyboardDelegate constructor and then call
Bind() on it one you have the InterfaceRequest

https://codereview.chromium.org/1139453002/diff/1/services/BUILD.gn
File services/BUILD.gn (right):

https://codereview.chromium.org/1139453002/diff/1/services/BUILD.gn#newcode16
services/BUILD.gn:16: "//services/js",
looks like a bad merge, the //services/js dep is on line 45

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
File services/keyboard_native/keyboard_service_impl.cc (right):

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.cc:5: #include
<services/keyboard_native/keyboard_service_impl.h>
use "" style includes and put a blank line after this

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
File services/keyboard_native/keyboard_service_impl.h (right):

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.h:26: // From
InterfaceFactory<Echo>
I like the comments but please be consistent

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.h:42: class
StrongBindingKeyboardServiceImpl : public KeyboardServiceImpl {
I don't really understand what this class does - it doesn't appear to have any
overrides? what it is its purpose in life?

[jamesr, 2015-05-08 20:59:14]

Also please add more information in the patch description about what the patch
is doing and (more importantly) why.  You can edit that through the web
interface by clicking "Edit issue" in the top left. For this patch, please
describe what the service does and what the example does and what's different
about it from existing things.  To many people, saying "native" on Android means
using java so you probably want to clarify that.

[APW, 2015-05-08 22:09:28]

I've updated the review based on review comments.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
File examples/keyboard_client/BUILD.gn (right):

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:6:
import("//mojo/public/tools/bindings/mojom.gni")
On 2015/05/08 20:55:15, jamesr wrote:
> you can remove this - you only need to import this in order to use the "mojom"
> template to tell the build system how to handle foo.mojom files

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:10: "//base:base",
On 2015/05/08 20:55:15, jamesr wrote:
> just "//base".  in GN, "//foo/bar/blah" is a shorthand for
"//foo/bar/blah:blah"

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/BU...
examples/keyboard_client/BUILD.gn:17: sources = [
On 2015/05/08 20:55:15, jamesr wrote:
> put sources before deps.  style guide for BUILD.gn files:
> 
>
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g...

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
File examples/keyboard_client/keyboard_client.cc (right):

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:5: #include <stdio.h>
On 2015/05/08 20:55:15, jamesr wrote:
> don't think you are using this

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:15: namespace mojo {
On 2015/05/08 20:55:15, jamesr wrote:
> please put this in the namespace 'examples', not 'mojo::examples'. a lot of
old
> stuff is in the namespace mojo but we shouldn't do that for new code

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:19: public keyboard::KeyboardClient
{
On 2015/05/08 20:55:15, jamesr wrote:
> this 'public' should be aligned with the previous one. run 'git cl format' and
> it'll run clang-format over your code to format it up all pretty

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:22: keyboard::KeyboardClientPtr
mystery_;
On 2015/05/08 20:55:15, jamesr wrote:
> put member vars after functions and in the private: section why "mystery"?
looks
> the same as keyboardClient_ lower down

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:26: void Initialize(ApplicationImpl*
app) override {
On 2015/05/08 20:55:16, jamesr wrote:
> it's really useful to document what base class a member function is overriding
> especially once you have multiple base classes. we have a few different
patterns
> for this in our codebase but a common one is to have comments like
> 
>   // mojo::ApplicationDelegate implementation.
>   void Initialize(ApplicationImpl* app) override {
>     ...
>   }
> 
>   // keyboard::KeyboardClient implementation.
>   void CommitCompletion(...) override {
>     ...
>   }

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:31: request_ =
mojo::GetProxy(&mystery_);
On 2015/05/08 20:55:15, jamesr wrote:
> since |request_| is only initialized on this line and then passed away on the
> next line you can just use a local

Done.

https://codereview.chromium.org/1139453002/diff/1/examples/keyboard_client/ke...
examples/keyboard_client/keyboard_client.cc:32: binding_ = new
mojo::Binding<keyboard::KeyboardClient>(this,
On 2015/05/08 20:55:16, jamesr wrote:
> there's no need to heap allocate |binding_|. you can make it a member
variable,
> initialize it with |this| in the KeyboardDelegate constructor and then call
> Bind() on it one you have the InterfaceRequest

Done.

https://codereview.chromium.org/1139453002/diff/1/services/BUILD.gn
File services/BUILD.gn (right):

https://codereview.chromium.org/1139453002/diff/1/services/BUILD.gn#newcode16
services/BUILD.gn:16: "//services/js",
On 2015/05/08 20:55:16, jamesr wrote:
> looks like a bad merge, the //services/js dep is on line 45

Done.

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
File services/keyboard_native/keyboard_service_impl.cc (right):

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.cc:5: #include
<services/keyboard_native/keyboard_service_impl.h>
On 2015/05/08 20:55:16, jamesr wrote:
> use "" style includes and put a blank line after this

Done.

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
File services/keyboard_native/keyboard_service_impl.h (right):

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.h:26: // From
InterfaceFactory<Echo>
On 2015/05/08 20:55:16, jamesr wrote:
> I like the comments but please be consistent

Done.

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.h:42: class
StrongBindingKeyboardServiceImpl : public KeyboardServiceImpl {
On 2015/05/08 20:55:16, jamesr wrote:
> I don't really understand what this class does - it doesn't appear to have any
> overrides? what it is its purpose in life?

examples/echo/echo_server.c uses this pattern, that's where I got it from.  You
think it's unnecessary?

[APW, 2015-05-08 22:27:02]

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
File services/keyboard_native/keyboard_service_impl.h (right):

https://codereview.chromium.org/1139453002/diff/1/services/keyboard_native/ke...
services/keyboard_native/keyboard_service_impl.h:42: class
StrongBindingKeyboardServiceImpl : public KeyboardServiceImpl {
On 2015/05/08 22:09:28, anwilson wrote:
> On 2015/05/08 20:55:16, jamesr wrote:
> > I don't really understand what this class does - it doesn't appear to have
any
> > overrides? what it is its purpose in life?
> 
> examples/echo/echo_server.c uses this pattern, that's where I got it from. 
You
> think it's unnecessary?

Done.

[jamesr, 2015-05-08 23:45:24]

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
File examples/keyboard_client/keyboard_client.cc (right):

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:18: KeyboardDelegate() :
binding_(this) {}
whenever you have a type with a vtable (i.e. inherits from a virtual class) it's
good practice to define a d'tor explicitly so it's clear that destruction is
virtual. for this it'd be something like

~KeyboardDelegate() override {}

since this class is tiny it isn't all that helpful but classes that start out
tiny have a habit of not staying small

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:27:
mojo::GetProxy(&keyboardClient_);
i think you still want keyboardClient_ to be a local, although you should call
it keyboard_client or just client, i.e. something like

keyboard::KeyboardClientPtr client;
binding_.Bind(GetProxy(&client));
keyboard_->Show(client.Pass());

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:38: int32_t newCursorPosition)
override {}
by convention we use hacker_style_for_names in c++.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Naming

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:54: };
add DISALLOW_COPY_AND_ASSIGN(KeyboardDelegate) to suppress the default
assignment operator and copy constructor. they won't work for this type, but the
macro will be clearer

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/keyboard_service_impl.cc (right):

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.cc:26: void
KeyboardServiceImpl::Show(KeyboardClientPtr client) {
note: i think this is deliberate here but keep in mind you're destroying the
|client| here at the end of this function and thus closing the pipe to the
client. that's to start but it'd probably be more realistic to at least hold on
to this until Hide() so you can deliver more data as time goes on

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/keyboard_service_impl.h (right):

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:5: #include
"base/memory/scoped_ptr.h"
i think you aren't using most of these includes - mind taking a pass to clear
these out?

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:19: class
KeyboardServiceDelegate : public mojo::ApplicationDelegate,
i would probably move this class and the MojoMain definition into a main.cc and
have only the KeyboardServiceImpl class definition in this file.  we aren't
strictly one-type-per-header but having predictable location for things like
service impls is pretty handy

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:33:
KeyboardServiceImpl(mojo::InterfaceRequest<KeyboardService> handle);
add the 'explicit' keyword for one-argument constructors

i think it's slightly more common to name this parameter 'request'

[APW, 2015-05-09 00:45:59]

FIxed more style issues.

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
File examples/keyboard_client/keyboard_client.cc (right):

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:18: KeyboardDelegate() :
binding_(this) {}
On 2015/05/08 23:45:23, jamesr wrote:
> whenever you have a type with a vtable (i.e. inherits from a virtual class)
it's
> good practice to define a d'tor explicitly so it's clear that destruction is
> virtual. for this it'd be something like
> 
> ~KeyboardDelegate() override {}
> 
> since this class is tiny it isn't all that helpful but classes that start out
> tiny have a habit of not staying small

Done.

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:27:
mojo::GetProxy(&keyboardClient_);
On 2015/05/08 23:45:23, jamesr wrote:
> i think you still want keyboardClient_ to be a local, although you should call
> it keyboard_client or just client, i.e. something like
> 
> keyboard::KeyboardClientPtr client;
> binding_.Bind(GetProxy(&client));
> keyboard_->Show(client.Pass());

Done.

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:38: int32_t newCursorPosition)
override {}
On 2015/05/08 23:45:23, jamesr wrote:
> by convention we use hacker_style_for_names in c++.
> https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Naming

Done.

https://codereview.chromium.org/1139453002/diff/60001/examples/keyboard_clien...
examples/keyboard_client/keyboard_client.cc:54: };
On 2015/05/08 23:45:23, jamesr wrote:
> add DISALLOW_COPY_AND_ASSIGN(KeyboardDelegate) to suppress the default
> assignment operator and copy constructor. they won't work for this type, but
the
> macro will be clearer

Done.

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/keyboard_service_impl.cc (right):

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.cc:26: void
KeyboardServiceImpl::Show(KeyboardClientPtr client) {
On 2015/05/08 23:45:23, jamesr wrote:
> note: i think this is deliberate here but keep in mind you're destroying the
> |client| here at the end of this function and thus closing the pipe to the
> client. that's to start but it'd probably be more realistic to at least hold
on
> to this until Hide() so you can deliver more data as time goes on

Acknowledged.

I will hold on to it in a later change.

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
File services/keyboard_native/keyboard_service_impl.h (right):

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:5: #include
"base/memory/scoped_ptr.h"
On 2015/05/08 23:45:24, jamesr wrote:
> i think you aren't using most of these includes - mind taking a pass to clear
> these out?

Done.

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:19: class
KeyboardServiceDelegate : public mojo::ApplicationDelegate,
On 2015/05/08 23:45:24, jamesr wrote:
> i would probably move this class and the MojoMain definition into a main.cc
and
> have only the KeyboardServiceImpl class definition in this file.  we aren't
> strictly one-type-per-header but having predictable location for things like
> service impls is pretty handy

Done.

https://codereview.chromium.org/1139453002/diff/60001/services/keyboard_nativ...
services/keyboard_native/keyboard_service_impl.h:33:
KeyboardServiceImpl(mojo::InterfaceRequest<KeyboardService> handle);
On 2015/05/08 23:45:24, jamesr wrote:
> add the 'explicit' keyword for one-argument constructors
> 
> i think it's slightly more common to name this parameter 'request'

Done.

[APW, 2015-05-11 17:39:40]

PTAL

[APW, 2015-05-11 17:39:42]

PTAL

[jamesr, 2015-05-11 17:50:56]

lgtm

https://codereview.chromium.org/1139453002/diff/120001/examples/keyboard_clie...
File examples/keyboard_client/keyboard_client.cc (right):

https://codereview.chromium.org/1139453002/diff/120001/examples/keyboard_clie...
examples/keyboard_client/keyboard_client.cc:28:
mojo::GetProxy(&keyboard_client);
two tips (you don't have to change the code but FYI):

(1) auto is really useful for this call
(2) you don't have to say the "mojo::" part in "mojo::GetProxy" as that function
is designed to work with ADL
(http://en.wikipedia.org/wiki/Argument-dependent_name_lookup)

so you can write this

auto request = GetProxy(&keyboard_client);

https://codereview.chromium.org/1139453002/diff/120001/services/keyboard_nati...
File services/keyboard_native/main.cc (right):

https://codereview.chromium.org/1139453002/diff/120001/services/keyboard_nati...
services/keyboard_native/main.cc:33:
DISALLOW_COPY_AND_ASSIGN(KeyboardServiceDelegate);
for this macro to do what it wants to do it has to be in a private: section

[APW, 2015-05-11 18:12:47]

Committed patchset #8 (id:140001) manually as
7da883d63253688cc3e54a1ab81d7117dfc1b83d (presubmit successful).