Add sensors application to mojo

Add sensors application to mojo

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/b1efb142febc8da269cd598f302651c66cde11cb

[DaveMoore, 2015-04-20 23:29:57]

Make sensors be a mojo_java_application

[DaveMoore, 2015-04-20 23:50:30]

Only build sensors on Android

[DaveMoore, 2015-04-21 04:36:25]

Support both SkyDemo and mojo_shell

[DaveMoore, 2015-04-21 04:41:04]

Support both SkyDemo and mojo_shell

[DaveMoore, 2015-04-21 05:40:54]

davemoore@chromium.org changed reviewers:
+ jamesr@chromium.org

[qsr, 2015-04-21 08:28:44]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-04-21 08:28:45]

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
File mojo/services/sensors/public/interfaces/sensors.mojom (right):

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
mojo/services/sensors/public/interfaces/sensors.mojom:41: AddListener(SensorType
type, SensorListener listener);
I though we didn't want to use the observer pattern because of performance
issue.

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
File services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java (right):

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java:21: public
SensorServiceImpl(Context context, Core core, MessagePipeHandle pipe) {
Why does this constructor takes a Core? One you remove this (that you can access
through the pipe anyway), you can then reuse this constructor in Sensor.java,
and you do not need the other one.

https://codereview.chromium.org/1093033002/diff/80001/sky/framework/debug/sha...
File sky/framework/debug/shake-to-reload.sky (right):

https://codereview.chromium.org/1093033002/diff/80001/sky/framework/debug/sha...
sky/framework/debug/shake-to-reload.sky:21: print("onSensorChanged: $value");
Is that voluntary?

[DaveMoore, 2015-04-21 14:59:46]

Address review comments

[DaveMoore, 2015-04-21 15:00:08]

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
File mojo/services/sensors/public/interfaces/sensors.mojom (right):

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
mojo/services/sensors/public/interfaces/sensors.mojom:41: AddListener(SensorType
type, SensorListener listener);
On 2015/04/21 08:28:45, qsr wrote:
> I though we didn't want to use the observer pattern because of performance
> issue.

This was lifted as is from the Sky implementation. I can explore better
approaches, but I'd rather do that in another cl after we move it here.

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
File services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java (right):

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java:21: public
SensorServiceImpl(Context context, Core core, MessagePipeHandle pipe) {
On 2015/04/21 08:28:45, qsr wrote:
> Why does this constructor takes a Core? One you remove this (that you can
access
> through the pipe anyway), you can then reuse this constructor in Sensor.java,
> and you do not need the other one.

The Core can be dropped (again, lifted as is from the Sky impl). But doesn't
ApplicationRunner handle the pipe?

https://codereview.chromium.org/1093033002/diff/80001/sky/framework/debug/sha...
File sky/framework/debug/shake-to-reload.sky (right):

https://codereview.chromium.org/1093033002/diff/80001/sky/framework/debug/sha...
sky/framework/debug/shake-to-reload.sky:21: print("onSensorChanged: $value");
On 2015/04/21 08:28:45, qsr wrote:
> Is that voluntary?

Accident (was for testing). I'll remove.

[qsr, 2015-04-21 15:59:32]

LGTM with the new constructor removed.

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
File mojo/services/sensors/public/interfaces/sensors.mojom (right):

https://codereview.chromium.org/1093033002/diff/80001/mojo/services/sensors/p...
mojo/services/sensors/public/interfaces/sensors.mojom:41: AddListener(SensorType
type, SensorListener listener);
On 2015/04/21 15:00:07, DaveMoore wrote:
> On 2015/04/21 08:28:45, qsr wrote:
> > I though we didn't want to use the observer pattern because of performance
> > issue.
> 
> This was lifted as is from the Sky implementation. I can explore better
> approaches, but I'd rather do that in another cl after we move it here.

 Ok. Let's see that in another CL.

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
File services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java (right):

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java:21: public
SensorServiceImpl(Context context, Core core, MessagePipeHandle pipe) {
On 2015/04/21 15:00:07, DaveMoore wrote:
> On 2015/04/21 08:28:45, qsr wrote:
> > Why does this constructor takes a Core? One you remove this (that you can
> access
> > through the pipe anyway), you can then reuse this constructor in
Sensor.java,
> > and you do not need the other one.
> 
> The Core can be dropped (again, lifted as is from the Sky impl). But doesn't
> ApplicationRunner handle the pipe?

 I don't understand what you mean. The new code you use is calling the new
constructor, and then binding to a pipe. So instead you can just call this
constructor that will do the binding.

[DaveMoore, 2015-04-21 16:29:58]

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
File services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java (right):

https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java:21: public
SensorServiceImpl(Context context, Core core, MessagePipeHandle pipe) {
On 2015/04/21 15:59:32, qsr wrote:
> On 2015/04/21 15:00:07, DaveMoore wrote:
> > On 2015/04/21 08:28:45, qsr wrote:
> > > Why does this constructor takes a Core? One you remove this (that you can
> > access
> > > through the pipe anyway), you can then reuse this constructor in
> Sensor.java,
> > > and you do not need the other one.
> > 
> > The Core can be dropped (again, lifted as is from the Sky impl). But doesn't
> > ApplicationRunner handle the pipe?
> 
>  I don't understand what you mean. The new code you use is calling the new
> constructor, and then binding to a pipe. So instead you can just call this
> constructor that will do the binding.

There is a call to the SensorServiceImpl(Context) in Sensors.java. And there is
a call to SensorServiceImpl(Context, MessagePipeHandle) in 
SkyDemoApplication.java. Which are you saying I should get rid of?

[qsr, 2015-04-21 16:33:10]

On 2015/04/21 16:29:58, DaveMoore wrote:
>
https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
> File services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java
(right):
> 
>
https://codereview.chromium.org/1093033002/diff/80001/services/sensors/org/ch...
> services/sensors/org/chromium/mojo/sensors/SensorServiceImpl.java:21: public
> SensorServiceImpl(Context context, Core core, MessagePipeHandle pipe) {
> On 2015/04/21 15:59:32, qsr wrote:
> > On 2015/04/21 15:00:07, DaveMoore wrote:
> > > On 2015/04/21 08:28:45, qsr wrote:
> > > > Why does this constructor takes a Core? One you remove this (that you
can
> > > access
> > > > through the pipe anyway), you can then reuse this constructor in
> > Sensor.java,
> > > > and you do not need the other one.
> > > 
> > > The Core can be dropped (again, lifted as is from the Sky impl). But
doesn't
> > > ApplicationRunner handle the pipe?
> > 
> >  I don't understand what you mean. The new code you use is calling the new
> > constructor, and then binding to a pipe. So instead you can just call this
> > constructor that will do the binding.
> 
> There is a call to the SensorServiceImpl(Context) in Sensors.java. And there
is
> a call to SensorServiceImpl(Context, MessagePipeHandle) in 
> SkyDemoApplication.java. Which are you saying I should get rid of?

 I would get rid of the one in Sensor, replacing it with the one that takes the
Pipe. Mainly, right now you have:

SensorService.MANAGER.bind(new SensorServiceImpl(mContext), pipe);

That you can replace by:
new SensorServiceImpl(mContext, pipe)

[DaveMoore, 2015-04-21 16:42:26]

Remove extra constructor

[DaveMoore, 2015-04-21 16:56:09]

Committed patchset #7 (id:120001) manually as
b1efb142febc8da269cd598f302651c66cde11cb (presubmit successful).