[sensors] Introduce Generic Sensor API interfaces

[sensors] Introduce Generic Sensor API interfaces

This patch is result of collaboration work with mikhail.pozdnyakov@intel.com
New generic sensor mojo interfaces are introduced together with platform
independent implementations.

BUG=606766
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

[shalamov, 2016-06-16 10:54:50]

alexander.shalamov@intel.com changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org, timvolodine@chromium.org,
tsepez@chromium.org

[shalamov, 2016-06-16 10:54:50]

This is a part of https://crrev.com/2051083002 Generic Sensor API
implementation.

Intent to implement with proposed design can be found here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/TkfdVqYAYiE/4J7Z088M...

PTAL

[dcheng, 2016-06-16 20:04:14]

Sorry, but can we land the mojom changes with the bigger patch? For IPC reviews,
it doesn't make sense to just review the changes in the mojom files.

[shalamov, 2016-06-22 09:42:08]

Description was changed from

==========
[sensors] Introduce Generic Sensor API interfaces

This patch introduces generic sensor mojo interfaces
that are required to implement W3C Generic Sensor API.

BUG=606766
==========

to

==========
[sensors] Introduce Generic Sensor API interfaces

This patch introduces generic sensor mojo interfaces
that are required to implement W3C Generic Sensor API.

BUG=606766
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[shalamov, 2016-06-22 10:04:03]

Description was changed from

==========
[sensors] Introduce Generic Sensor API interfaces

This patch introduces generic sensor mojo interfaces
that are required to implement W3C Generic Sensor API.

BUG=606766
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
[sensors] Introduce Generic Sensor API interfaces

This patch is result of collaboration work with mikhail.pozdnyakov@intel.com
New generic sensor mojo interfaces are introduced together with platform
independent implementations.

BUG=606766
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[shalamov, 2016-06-22 10:09:29]

Patchset #2 (id:20001) has been deleted

[shalamov, 2016-06-22 12:11:06]

On 2016/06/16 20:04:14, dcheng wrote:
> Sorry, but can we land the mojom changes with the bigger patch? For IPC
reviews,
> it doesn't make sense to just review the changes in the mojom files.

@dcheng, We've added common part that implements mojo interfaces and adds
generic base classes that could be shared between platforms.

[Mikhail, 2016-06-28 10:01:58]

mikhail.pozdnyakov@intel.com changed reviewers:
+ reillyg@chromium.org, sammc@chromium.org

[Mikhail, 2016-06-28 10:03:14]

mikhail.pozdnyakov@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com

[Mikhail, 2016-06-28 10:03:15]

Gentle ping for review

[Ken Rockot(use gerrit already), 2016-06-28 16:18:29]

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
File device/sensors/mojo/BUILD.gn (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
device/sensors/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
Please, no new mojo/ directories. It's just an implementation, this code can
live directly in device/sensors

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
File device/sensors/platform/BUILD.gn (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
device/sensors/platform/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
No need for this subdirectory, please just put the sources in device/sensors

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:5: module device.sensors;
See note in other mojom file about module names

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:35: // Sensor reading buffer is
putting fields of 64-bit floating point type in the
This description could be clearer and doesn't really make sense floating in the
middle of the mojom here. I would include this information near the interface
method which provides the shared buffer, and I would be explicit about field
sizes and layout. i.e., note that they are tightly packed 64-bit floating point
values, the full buffer has a fixed size of 8+n*8 bytes for n fields, etc.
Anything to clarify.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:65: Stop(SensorConfiguration
configuration) => (Result result);
How is an implementation supposed to disambiguate sensor monitoring instances
(i.e. calls to Start) by |configuration| only? Seems dubious to simply compare
configuration fields by value or something like that.

And is it even valid to call Start() (and thus Stop()) more than once on a
single Sensor interface? Should it not be?

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:72: OnSensorError();
I think this pattern ("On" method naming) has been adopted in many places for
client interfaces, but it seems redundant to me if every client interface has a
bunch of On* methods. How about just

  RaiseError()
  UpdateReading()

? Then the client interface methods are imperative verb phrases like other
interface methods and they make plenty of sense in the context of "SensorClient"

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
File device/sensors/public/interfaces/sensor_factory.mojom (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:5: module device.sensors;
No nested namespaces please (we have to fix some existing cases like
device.usb). The only exception is new mojom should nest an inner mojom
namespace to disambiguate from non-mojom symbols.

Please change this to

module device.mojom;

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:11: // @param type -
SensorType, type of the sensor that should be created.
This argument documentation syntax seems arbitrary (it kind of mimics other
styles but not quite) and doesn't match anything we use elsewhere.

We don't have a documentation syntax for mojom, just like we don't have one for
chromium C++ code, and I don't think we need to adopt one. Can you please just
document this like you would any other C++ class method?

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:19: interface
SensorFactory {
nit: Maybe SensorProvider? It's not really a factory, it doesn't really create
sensors so much as it provides sensor interfaces.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:26: };
If the only thing you can do is "Create" Sensor, and the only thing you can do
with a Sensor interface is Start or Stop or SetClient, maybe we can simply
everything a bit? Here's my strawman proposal:

// Combine the shm details in a struct for clarity.
// (documentation explaining buffer layout)
struct SensorReadBuffer {
  handle<shared_buffer> memory;
  uint64 offset;
  uint64 size;
};

// One Sensor interface per monitoring instance. Then it only needs Stop().
interface Sensor {
  Stop();
};

interface SensorClient {
  RaiseError();
  UpdateReading();
};

interface SensorProvider {
  // |read_buffer| is null on failure. If the sensor supports async
  // notifications rather than continuous updates, |client_request| will
  // be non-null and should be bound by the client.
  MonitorSensor(SensorType type, double frequency, Sensor& sensor_request)
      => (SensorReadBuffer? read_buffer, SensorClient&? client_request);
};


This covers a lot of things: it removes uncertainty about what seem like
potentially fragile interactions between Start/Stop/SetClient, it eliminates
Result, eliminates ReportingMode, eliminates a need for Stop() to return a
result, and eliminates the need for a "Configuration" struct since the
configuration is only specified in one place, the MonitorSensor args.

[Ken Rockot(use gerrit already), 2016-06-28 16:24:13]

On 2016/06/28 at 16:18:29, Ken Rockot wrote:
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
> File device/sensors/mojo/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
> device/sensors/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
> Please, no new mojo/ directories. It's just an implementation, this code can
live directly in device/sensors
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
> File device/sensors/platform/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
> device/sensors/platform/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
> No need for this subdirectory, please just put the sources in device/sensors
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> File device/sensors/public/interfaces/sensor.mojom (right):
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor.mojom:5: module device.sensors;
> See note in other mojom file about module names
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor.mojom:35: // Sensor reading buffer is
putting fields of 64-bit floating point type in the
> This description could be clearer and doesn't really make sense floating in
the middle of the mojom here. I would include this information near the
interface method which provides the shared buffer, and I would be explicit about
field sizes and layout. i.e., note that they are tightly packed 64-bit floating
point values, the full buffer has a fixed size of 8+n*8 bytes for n fields, etc.
Anything to clarify.
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor.mojom:65: Stop(SensorConfiguration
configuration) => (Result result);
> How is an implementation supposed to disambiguate sensor monitoring instances
(i.e. calls to Start) by |configuration| only? Seems dubious to simply compare
configuration fields by value or something like that.
> 
> And is it even valid to call Start() (and thus Stop()) more than once on a
single Sensor interface? Should it not be?
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor.mojom:72: OnSensorError();
> I think this pattern ("On" method naming) has been adopted in many places for
client interfaces, but it seems redundant to me if every client interface has a
bunch of On* methods. How about just
> 
>   RaiseError()
>   UpdateReading()
> 
> ? Then the client interface methods are imperative verb phrases like other
interface methods and they make plenty of sense in the context of "SensorClient"
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> File device/sensors/public/interfaces/sensor_factory.mojom (right):
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor_factory.mojom:5: module
device.sensors;
> No nested namespaces please (we have to fix some existing cases like
device.usb). The only exception is new mojom should nest an inner mojom
namespace to disambiguate from non-mojom symbols.
> 
> Please change this to
> 
> module device.mojom;
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor_factory.mojom:11: // @param type -
SensorType, type of the sensor that should be created.
> This argument documentation syntax seems arbitrary (it kind of mimics other
styles but not quite) and doesn't match anything we use elsewhere.
> 
> We don't have a documentation syntax for mojom, just like we don't have one
for chromium C++ code, and I don't think we need to adopt one. Can you please
just document this like you would any other C++ class method?
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor_factory.mojom:19: interface
SensorFactory {
> nit: Maybe SensorProvider? It's not really a factory, it doesn't really create
sensors so much as it provides sensor interfaces.
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor_factory.mojom:26: };
> If the only thing you can do is "Create" Sensor, and the only thing you can do
with a Sensor interface is Start or Stop or SetClient, maybe we can simply
everything a bit? Here's my strawman proposal:
> 
> // Combine the shm details in a struct for clarity.
> // (documentation explaining buffer layout)
> struct SensorReadBuffer {
>   handle<shared_buffer> memory;
>   uint64 offset;
>   uint64 size;
> };
> 
> // One Sensor interface per monitoring instance. Then it only needs Stop().
> interface Sensor {
>   Stop();
> };
> 
> interface SensorClient {
>   RaiseError();
>   UpdateReading();
> };
> 
> interface SensorProvider {
>   // |read_buffer| is null on failure. If the sensor supports async
>   // notifications rather than continuous updates, |client_request| will
>   // be non-null and should be bound by the client.
>   MonitorSensor(SensorType type, double frequency, Sensor& sensor_request)
>       => (SensorReadBuffer? read_buffer, SensorClient&? client_request);
> };
> 
> 
> This covers a lot of things: it removes uncertainty about what seem like
potentially fragile interactions between Start/Stop/SetClient, it eliminates
Result, eliminates ReportingMode, eliminates a need for Stop() to return a
result, and eliminates the need for a "Configuration" struct since the
configuration is only specified in one place, the MonitorSensor args.

In fact then you could even get rid of Stop() and let it be implied by Sensor
pipe closure, but then Sensor is an empty interface. A little weird but maybe OK
imho.

[Reilly Grant (use Gerrit), 2016-06-28 17:19:28]

On 2016/06/28 at 16:24:13, rockot wrote:
> In fact then you could even get rid of Stop() and let it be implied by Sensor
pipe closure, but then Sensor is an empty interface. A little weird but maybe OK
imho.

You could get rid of Sensor entirely and signal a request to stop by closing the
pipe from the SensorClient.

[Ken Rockot(use gerrit already), 2016-06-28 17:23:51]

On 2016/06/28 at 17:19:28, reillyg wrote:
> On 2016/06/28 at 16:24:13, rockot wrote:
> > In fact then you could even get rid of Stop() and let it be implied by
Sensor pipe closure, but then Sensor is an empty interface. A little weird but
maybe OK imho.
> 
> You could get rid of Sensor entirely and signal a request to stop by closing
the pipe from the SensorClient.

Not all sensors support notification events, so not all sensors have a
SensorClient request pipe.

I guess that could change, but then you still need a way to communicate that the
sensor is continuous update vs notification-based update. Guess there could just
be a "bool continuous" flag in the response though.

[timvolodine, 2016-06-28 18:19:53]

On 2016/06/28 17:23:51, Ken Rockot wrote:
> On 2016/06/28 at 17:19:28, reillyg wrote:
> > On 2016/06/28 at 16:24:13, rockot wrote:
> > > In fact then you could even get rid of Stop() and let it be implied by
> Sensor pipe closure, but then Sensor is an empty interface. A little weird but
> maybe OK imho.
> > 
> > You could get rid of Sensor entirely and signal a request to stop by closing
> the pipe from the SensorClient.
> 
> Not all sensors support notification events, so not all sensors have a
> SensorClient request pipe.
> 
> I guess that could change, but then you still need a way to communicate that
the
> sensor is continuous update vs notification-based update. Guess there could
just
> be a "bool continuous" flag in the response though.

Right, I think it could make sense to have two separate interfaces. One for
slow-updating sensors and one for fast sensors. The slow approach could be
implemented like device/battery, with a minimal interface which also avoids the
issue when clients cannot keep up. The 'fast' approach could be similar to what
we have for device orientation api using shared memory, and which sammc@
recently refactored to use mojo https://codereview.chromium.org/2037513002/.

[shalamov, 2016-06-29 14:35:13]

Patchset #3 (id:60001) has been deleted

[shalamov, 2016-06-29 15:02:17]

Thanks for review comments!

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
File device/sensors/mojo/BUILD.gn (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/mojo/BUI...
device/sensors/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/06/28 16:18:28, Ken Rockot wrote:
> Please, no new mojo/ directories. It's just an implementation, this code can
> live directly in device/sensors

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
File device/sensors/platform/BUILD.gn (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/platform...
device/sensors/platform/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/06/28 16:18:28, Ken Rockot wrote:
> No need for this subdirectory, please just put the sources in device/sensors

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:5: module device.sensors;
On 2016/06/28 16:18:29, Ken Rockot wrote:
> See note in other mojom file about module names

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:35: // Sensor reading buffer is
putting fields of 64-bit floating point type in the
On 2016/06/28 16:18:29, Ken Rockot wrote:
> This description could be clearer and doesn't really make sense floating in
the
> middle of the mojom here. I would include this information near the interface
> method which provides the shared buffer, and I would be explicit about field
> sizes and layout. i.e., note that they are tightly packed 64-bit floating
point
> values, the full buffer has a fixed size of 8+n*8 bytes for n fields, etc.
> Anything to clarify.

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:65: Stop(SensorConfiguration
configuration) => (Result result);
On 2016/06/28 16:18:29, Ken Rockot wrote:
> How is an implementation supposed to disambiguate sensor monitoring instances
> (i.e. calls to Start) by |configuration| only? Seems dubious to simply compare
> configuration fields by value or something like that.
> 
> And is it even valid to call Start() (and thus Stop()) more than once on a
> single Sensor interface? Should it not be?

We don't need to know anything about monitoring instances. Everytime new JS
instance is created, controller on blink side will add configuration that is
specific
for that instance (WIP: https://codereview.chromium.org/2051083002). On browser
side, optimal configuration is selected everytime new configuration added /
removed.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor.mojom:72: OnSensorError();
On 2016/06/28 16:18:29, Ken Rockot wrote:
> I think this pattern ("On" method naming) has been adopted in many places for
> client interfaces, but it seems redundant to me if every client interface has
a
> bunch of On* methods. How about just
> 
>   RaiseError()
>   UpdateReading()
> 
> ? Then the client interface methods are imperative verb phrases like other
> interface methods and they make plenty of sense in the context of
"SensorClient"

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
File device/sensors/public/interfaces/sensor_factory.mojom (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:5: module device.sensors;
On 2016/06/28 16:18:29, Ken Rockot wrote:
> No nested namespaces please (we have to fix some existing cases like
> device.usb). The only exception is new mojom should nest an inner mojom
> namespace to disambiguate from non-mojom symbols.
> 
> Please change this to
> 
> module device.mojom;

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:11: // @param type -
SensorType, type of the sensor that should be created.
On 2016/06/28 16:18:29, Ken Rockot wrote:
> This argument documentation syntax seems arbitrary (it kind of mimics other
> styles but not quite) and doesn't match anything we use elsewhere.
> 
> We don't have a documentation syntax for mojom, just like we don't have one
for
> chromium C++ code, and I don't think we need to adopt one. Can you please just
> document this like you would any other C++ class method?

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:19: interface
SensorFactory {
On 2016/06/28 16:18:29, Ken Rockot wrote:
> nit: Maybe SensorProvider? It's not really a factory, it doesn't really create
> sensors so much as it provides sensor interfaces.

Done.

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:26: };
On 2016/06/28 16:18:29, Ken Rockot wrote:
> If the only thing you can do is "Create" Sensor, and the only thing you can do
> with a Sensor interface is Start or Stop or SetClient, maybe we can simply
> everything a bit? Here's my strawman proposal:
> 
> // Combine the shm details in a struct for clarity.
> // (documentation explaining buffer layout)
> struct SensorReadBuffer {
>   handle<shared_buffer> memory;
>   uint64 offset;
>   uint64 size;
> };
> 
> // One Sensor interface per monitoring instance. Then it only needs Stop().
> interface Sensor {
>   Stop();
> };
> 
> interface SensorClient {
>   RaiseError();
>   UpdateReading();
> };
> 
> interface SensorProvider {
>   // |read_buffer| is null on failure. If the sensor supports async
>   // notifications rather than continuous updates, |client_request| will
>   // be non-null and should be bound by the client.
>   MonitorSensor(SensorType type, double frequency, Sensor& sensor_request)
>       => (SensorReadBuffer? read_buffer, SensorClient&? client_request);
> };
> 
> 
> This covers a lot of things: it removes uncertainty about what seem like
> potentially fragile interactions between Start/Stop/SetClient, it eliminates
> Result, eliminates ReportingMode, eliminates a need for Stop() to return a
> result, and eliminates the need for a "Configuration" struct since the
> configuration is only specified in one place, the MonitorSensor args.

Unfortunately, it is a more complex.

On browser side, we can have multiple Sensor interface instances
"Sensor(SensorType)" (one per frame). Each frame can have multiple users (JS
objects) of this interface and each JS instance has
it's own configuraion, which is added / removed when JS instance calls start /
stop method.

[Ken Rockot(use gerrit already), 2016-06-29 15:03:48]

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
File device/sensors/public/interfaces/sensor_factory.mojom (right):

https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
device/sensors/public/interfaces/sensor_factory.mojom:26: };
On 2016/06/29 at 15:02:17, shalamov wrote:
> On 2016/06/28 16:18:29, Ken Rockot wrote:
> > If the only thing you can do is "Create" Sensor, and the only thing you can
do
> > with a Sensor interface is Start or Stop or SetClient, maybe we can simply
> > everything a bit? Here's my strawman proposal:
> > 
> > // Combine the shm details in a struct for clarity.
> > // (documentation explaining buffer layout)
> > struct SensorReadBuffer {
> >   handle<shared_buffer> memory;
> >   uint64 offset;
> >   uint64 size;
> > };
> > 
> > // One Sensor interface per monitoring instance. Then it only needs Stop().
> > interface Sensor {
> >   Stop();
> > };
> > 
> > interface SensorClient {
> >   RaiseError();
> >   UpdateReading();
> > };
> > 
> > interface SensorProvider {
> >   // |read_buffer| is null on failure. If the sensor supports async
> >   // notifications rather than continuous updates, |client_request| will
> >   // be non-null and should be bound by the client.
> >   MonitorSensor(SensorType type, double frequency, Sensor& sensor_request)
> >       => (SensorReadBuffer? read_buffer, SensorClient&? client_request);
> > };
> > 
> > 
> > This covers a lot of things: it removes uncertainty about what seem like
> > potentially fragile interactions between Start/Stop/SetClient, it eliminates
> > Result, eliminates ReportingMode, eliminates a need for Stop() to return a
> > result, and eliminates the need for a "Configuration" struct since the
> > configuration is only specified in one place, the MonitorSensor args.
> 
> Unfortunately, it is a more complex.
> 
> On browser side, we can have multiple Sensor interface instances
"Sensor(SensorType)" (one per frame). Each frame can have multiple users (JS
objects) of this interface and each JS instance has
> it's own configuraion, which is added / removed when JS instance calls start /
stop method.

OK but why not just have each JS instance get its own Sensor interface?

[shalamov, 2016-06-30 08:36:16]

On 2016/06/29 15:03:48, Ken Rockot wrote:
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> File device/sensors/public/interfaces/sensor_factory.mojom (right):
> 
>
https://codereview.chromium.org/2078433002/diff/40001/device/sensors/public/i...
> device/sensors/public/interfaces/sensor_factory.mojom:26: };
> On 2016/06/29 at 15:02:17, shalamov wrote:
> > On 2016/06/28 16:18:29, Ken Rockot wrote:
> > > If the only thing you can do is "Create" Sensor, and the only thing you
can
> do
> > > with a Sensor interface is Start or Stop or SetClient, maybe we can simply
> > > everything a bit? Here's my strawman proposal:
> > > 
> > > // Combine the shm details in a struct for clarity.
> > > // (documentation explaining buffer layout)
> > > struct SensorReadBuffer {
> > >   handle<shared_buffer> memory;
> > >   uint64 offset;
> > >   uint64 size;
> > > };
> > > 
> > > // One Sensor interface per monitoring instance. Then it only needs
Stop().
> > > interface Sensor {
> > >   Stop();
> > > };
> > > 
> > > interface SensorClient {
> > >   RaiseError();
> > >   UpdateReading();
> > > };
> > > 
> > > interface SensorProvider {
> > >   // |read_buffer| is null on failure. If the sensor supports async
> > >   // notifications rather than continuous updates, |client_request| will
> > >   // be non-null and should be bound by the client.
> > >   MonitorSensor(SensorType type, double frequency, Sensor& sensor_request)
> > >       => (SensorReadBuffer? read_buffer, SensorClient&? client_request);
> > > };
> > > 
> > > 
> > > This covers a lot of things: it removes uncertainty about what seem like
> > > potentially fragile interactions between Start/Stop/SetClient, it
eliminates
> > > Result, eliminates ReportingMode, eliminates a need for Stop() to return a
> > > result, and eliminates the need for a "Configuration" struct since the
> > > configuration is only specified in one place, the MonitorSensor args.
> > 
> > Unfortunately, it is a more complex.
> > 
> > On browser side, we can have multiple Sensor interface instances
> "Sensor(SensorType)" (one per frame). Each frame can have multiple users (JS
> objects) of this interface and each JS instance has
> > it's own configuraion, which is added / removed when JS instance calls start
/
> stop method.
> 
> OK but why not just have each JS instance get its own Sensor interface?

According to the specification
(https://www.w3.org/TR/generic-sensor/#construct-sensor-object),
sensors are constructable, therefore, someone could create e.g. 1000 of them.
Each mojo proxy would allocate 2 message pipes (1 for control, 1 for error /
change notification), and we would need to send notification to all of them (X
JS instances * N Frames). This will increase latency and create unnecessary IPC
overhead.

[dcheng, 2016-07-01 09:17:24]

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
File device/sensors/platform_sensor_configuration.h (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_configuration.h:17: bool operator>(const
PlatformSensorConfiguration& other) const;
Is this operator used? I don't see it stored in an ordered map anywhere.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
File device/sensors/platform_sensor_provider.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_provider.cc:37: auto new_sensor =
CreateSensor(type, std::move(mapping), size);
Note the current guidance is not to use auto to hold smart pointer types. Also,
please #include <utility> for std::move (here and elsewhere)

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_provider.cc:45: return add_result.first->second;
return new_sensor is slightly more efficient here.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_i...
File device/sensors/sensor_impl.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_i...
device/sensors/sensor_impl.cc:15: : sensor_(sensor), binding_(this,
std::move(request)) {
Nit: std::move(sensor)

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_p...
File device/sensors/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_p...
device/sensors/sensor_provider_impl.cc:27: if (s_provider_) {
Shouldn't this theoretically be impossible if we actually implement
PlatformSensorProvider::Create()? Should we DCHECK() this instead, since this
should never be a valid condition in the real world?

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_s...
File device/sensors/sensor_service.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_s...
device/sensors/sensor_service.cc:20: base::Passed(std::move(request))));
Nit: base::Passed(&request)

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_t...
File device/sensors/sensor_type_converters.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_t...
device/sensors/sensor_type_converters.cc:21: return
device::PlatformSensorConfiguration(sensor_configuration->frequency);
Instead of adding new type converters, please use type mapping:
https://www.chromium.org/developers/design-documents/mojo/type-mapping

Something similar also works for enums, which use EnumTraits instead.

[shalamov, 2016-07-01 14:22:47]

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
File device/sensors/platform_sensor_configuration.h (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_configuration.h:17: bool operator>(const
PlatformSensorConfiguration& other) const;
On 2016/07/01 09:17:24, dcheng wrote:
> Is this operator used? I don't see it stored in an ordered map anywhere.

We will be using it to find optimal configuration, in following patches. You are
right, this can be removed for now.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
File device/sensors/platform_sensor_provider.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_provider.cc:37: auto new_sensor =
CreateSensor(type, std::move(mapping), size);
On 2016/07/01 09:17:24, dcheng wrote:
> Note the current guidance is not to use auto to hold smart pointer types.
Also,
> please #include <utility> for std::move (here and elsewhere)

Done.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/platform...
device/sensors/platform_sensor_provider.cc:45: return add_result.first->second;
On 2016/07/01 09:17:24, dcheng wrote:
> return new_sensor is slightly more efficient here.

Done.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_i...
File device/sensors/sensor_impl.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_i...
device/sensors/sensor_impl.cc:15: : sensor_(sensor), binding_(this,
std::move(request)) {
On 2016/07/01 09:17:24, dcheng wrote:
> Nit: std::move(sensor)

Done.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_p...
File device/sensors/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_p...
device/sensors/sensor_provider_impl.cc:27: if (s_provider_) {
On 2016/07/01 09:17:24, dcheng wrote:
> Shouldn't this theoretically be impossible if we actually implement
> PlatformSensorProvider::Create()? Should we DCHECK() this instead, since this
> should never be a valid condition in the real world?

Actually, this codepath would be default for platforms that haven't implemented
PlatformSensorProvider class. On those platforms,
mojo service request will fail and blink side can handle failure gracefully and
report error according to the specification.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_s...
File device/sensors/sensor_service.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_s...
device/sensors/sensor_service.cc:20: base::Passed(std::move(request))));
On 2016/07/01 09:17:24, dcheng wrote:
> Nit: base::Passed(&request)

Done.

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_t...
File device/sensors/sensor_type_converters.cc (right):

https://codereview.chromium.org/2078433002/diff/80001/device/sensors/sensor_t...
device/sensors/sensor_type_converters.cc:21: return
device::PlatformSensorConfiguration(sensor_configuration->frequency);
On 2016/07/01 09:17:24, dcheng wrote:
> Instead of adding new type converters, please use type mapping:
> https://www.chromium.org/developers/design-documents/mojo/type-mapping
> 
> Something similar also works for enums, which use EnumTraits instead.

Done.

[timvolodine, 2016-07-04 11:32:46]

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:9: FIRST = 1,
"first" is not really a sensor, why not just start with AMBIENT_LIGHT=1 (or 0)?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:41: // the platform (and updates
the shared buffer) at the maxiumum frequency
on the implementation side: would this mean that the hardware sensor potentially
needs to be restarted with a higher frequency when a higher frequency config is
added?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:14: uint64 offset;
why is this needed? isn't the handle to shared buffer above sufficient?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:20: // point fields, its
layout is: { double timestamp; double values[3] }.
I am probably missing something, but where is this info stored and why
values[3]?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:36: GetSensor(SensorType
type, Sensor& sensor_request) => (
why not have something like RegisterListener(SensorType, SensorConfig,
SensorClient) => SensorReadBuffer ?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:114: }
NOTREACHED?

[shalamov, 2016-07-04 12:11:46]

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:9: FIRST = 1,
On 2016/07/04 11:32:45, timvolodine wrote:
> "first" is not really a sensor, why not just start with AMBIENT_LIGHT=1 (or
0)?

We use FIRST and LAST enum values to calculate sensor reading buffer offset for
concrete sensor.

SensorProvider allocates one SharedBuffer that is used by all sensors. By
default, on most of the systems, smallest shared buffer that can be allocated by
the kernel is 4 kilobytes, therefore, since the sensor readings are quite small,
we decided to allocate buffer only once. Each sensor will map it's readings
according to the offset, which is calculated using FIRST and LAST enum values.

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:41: // the platform (and updates
the shared buffer) at the maxiumum frequency
On 2016/07/04 11:32:45, timvolodine wrote:
> on the implementation side: would this mean that the hardware sensor
potentially
> needs to be restarted with a higher frequency when a higher frequency config
is
> added?

Yes, you are correct.

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:14: uint64 offset;
On 2016/07/04 11:32:46, timvolodine wrote:
> why is this needed? isn't the handle to shared buffer above sufficient?

Please check comment above, we use one sharedbuffer for all sensors.

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:20: // point fields, its
layout is: { double timestamp; double values[3] }.
On 2016/07/04 11:32:46, timvolodine wrote:
> I am probably missing something, but where is this info stored and why
> values[3]?

For the sensors that are defined in SensorType enum and the data that is
provided by platform APIs(Android, Windows), timestamp + 3 sensor reading values
should be enough. We can change it later if required, when we start adding
concrete sensor implementations.

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:36: GetSensor(SensorType
type, Sensor& sensor_request) => (
On 2016/07/04 11:32:46, timvolodine wrote:
> why not have something like RegisterListener(SensorType, SensorConfig,
> SensorClient) => SensorReadBuffer ?

Then we will need to add unregisterListener, in addition to that, we would need
to send updates to all (SensorClient<->SensorConfig) instances.
That would add unneeded IPC, as I mentioned before in
https://codereview.chromium.org/2078433002/#msg19

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:114: }
On 2016/07/04 11:32:46, timvolodine wrote:
> NOTREACHED?

Thanks, missed that.

[timvolodine, 2016-07-05 11:45:08]

thanks for the clarifications, some more questions below

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:41: // the platform (and updates
the shared buffer) at the maxiumum frequency
On 2016/07/04 12:11:46, shalamov wrote:
> On 2016/07/04 11:32:45, timvolodine wrote:
> > on the implementation side: would this mean that the hardware sensor
> potentially
> > needs to be restarted with a higher frequency when a higher frequency config
> is
> > added?
> 
> Yes, you are correct.

hmm, this frequency configuration makes the implementation (much) more
complicated. What happens if I register two javascript sensors of same type, one
with freq=43 other with freq=61?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:20: // point fields, its
layout is: { double timestamp; double values[3] }.
On 2016/07/04 12:11:46, shalamov wrote:
> On 2016/07/04 11:32:46, timvolodine wrote:
> > I am probably missing something, but where is this info stored and why
> > values[3]?
> 
> For the sensors that are defined in SensorType enum and the data that is
> provided by platform APIs(Android, Windows), timestamp + 3 sensor reading
values
> should be enough. We can change it later if required, when we start adding
> concrete sensor implementations.

I see, so is your plan to allocate the size of the 'largest' sensor for all
other sensors in the buffer? looking at various sensors like magnetometer and
rotation vector they may actually require more than 3 values. e.g.
https://developer.android.com/reference/android/hardware/SensorEvent.html#values

also, how scalable is using a single shared memory buffer? imagine that a
certain sensor receives an extra value in the future, will that be an issue?

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_provider.mojom:36: GetSensor(SensorType
type, Sensor& sensor_request) => (
On 2016/07/04 12:11:46, shalamov wrote:
> On 2016/07/04 11:32:46, timvolodine wrote:
> > why not have something like RegisterListener(SensorType, SensorConfig,
> > SensorClient) => SensorReadBuffer ?
> 
> Then we will need to add unregisterListener, in addition to that, we would
need
> to send updates to all (SensorClient<->SensorConfig) instances.
> That would add unneeded IPC, as I mentioned before in
> https://codereview.chromium.org/2078433002/#msg19

ok I am trying to understand how it would actually work on the renderer side, as
it is not immediately clear why all other SensorClient instances would need to
be updated. If there are two sensors of same type but different frequencies
would they have separate clients? if so, would e.g. GetSensor(SensorType,
SensorConfig) => .. work here as well?

Looks like depending on the choice how to treat various configs, i.e.
frequencies, the design could be different. And we should try to not
'overcomplicate' things where possible. My understanding was that frequency was
an indication not a hard constraint, e.g. that's the case on android which uses
the register/unregister approach (and I think windows has something similar).

How would this design compare e.g. with having a mojo service to just expose the
actual platform functionality i.e. one sensor of each type with one client in
the renderer and do the 'filtering' of configs on the renderer side?

[shalamov, 2016-07-05 12:20:34]

On 2016/07/05 11:45:08, timvolodine wrote:
> thanks for the clarifications, some more questions below
> 
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> File device/sensors/public/interfaces/sensor.mojom (right):
> 
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> device/sensors/public/interfaces/sensor.mojom:41: // the platform (and updates
> the shared buffer) at the maxiumum frequency
> On 2016/07/04 12:11:46, shalamov wrote:
> > On 2016/07/04 11:32:45, timvolodine wrote:
> > > on the implementation side: would this mean that the hardware sensor
> > potentially
> > > needs to be restarted with a higher frequency when a higher frequency
config
> > is
> > > added?
> > 
> > Yes, you are correct.
> 
> hmm, this frequency configuration makes the implementation (much) more
> complicated. What happens if I register two javascript sensors of same type,
one
> with freq=43 other with freq=61?
> 

On platform side, sensor will use highest required frequency (61Hz). On blink
side, each JS sensor instance will
poll using its own frequency (for sensors with CONTINUOUS reporting mode).

>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> File device/sensors/public/interfaces/sensor_provider.mojom (right):
> 
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> device/sensors/public/interfaces/sensor_provider.mojom:20: // point fields,
its
> layout is: { double timestamp; double values[3] }.
> On 2016/07/04 12:11:46, shalamov wrote:
> > On 2016/07/04 11:32:46, timvolodine wrote:
> > > I am probably missing something, but where is this info stored and why
> > > values[3]?
> > 
> > For the sensors that are defined in SensorType enum and the data that is
> > provided by platform APIs(Android, Windows), timestamp + 3 sensor reading
> values
> > should be enough. We can change it later if required, when we start adding
> > concrete sensor implementations.
> 
> I see, so is your plan to allocate the size of the 'largest' sensor for all
> other sensors in the buffer? looking at various sensors like magnetometer and
> rotation vector they may actually require more than 3 values. e.g.
>
https://developer.android.com/reference/android/hardware/SensorEvent.html#values
>
> also, how scalable is using a single shared memory buffer? imagine that a
> certain sensor receives an extra value in the future, will that be an issue?
> 

Indeed, some sensors, e.g. uncalibrated magnetometer require 7 sensor reading
values.
At the moment, we are not planning to provide support for uncalibrated
magnetometer, but if required
we can easily extend reading buffer. Even if we had 8 sensor reading values, we
would be able
to allocate buffers for 64 sensors types in one memory page.

>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> device/sensors/public/interfaces/sensor_provider.mojom:36:
GetSensor(SensorType
> type, Sensor& sensor_request) => (
> On 2016/07/04 12:11:46, shalamov wrote:
> > On 2016/07/04 11:32:46, timvolodine wrote:
> > > why not have something like RegisterListener(SensorType, SensorConfig,
> > > SensorClient) => SensorReadBuffer ?
> > 
> > Then we will need to add unregisterListener, in addition to that, we would
> need
> > to send updates to all (SensorClient<->SensorConfig) instances.
> > That would add unneeded IPC, as I mentioned before in
> > https://codereview.chromium.org/2078433002/#msg19
> 
> ok I am trying to understand how it would actually work on the renderer side,
as
> it is not immediately clear why all other SensorClient instances would need to
> be updated. If there are two sensors of same type but different frequencies
> would they have separate clients? if so, would e.g. GetSensor(SensorType,
> SensorConfig) => .. work here as well?
> 

All JS sensor instances of the same sensor type within one render frame share
same SensorClient instance.
Sensor configuration management is encapsulated on the browser side.

> Looks like depending on the choice how to treat various configs, i.e.
> frequencies, the design could be different. And we should try to not
> 'overcomplicate' things where possible. My understanding was that frequency
was
> an indication not a hard constraint, e.g. that's the case on android which
uses
> the register/unregister approach (and I think windows has something similar).
> 
> How would this design compare e.g. with having a mojo service to just expose
the
> actual platform functionality i.e. one sensor of each type with one client in
> the renderer and do the 'filtering' of configs on the renderer side?

Here is WIP CL for the initial prototype
https://codereview.chromium.org/2051083002/ from where you could check blink
side design.
It is out of sync with this CL, Mikhail will upload new CL for the blink part
shortly.

[dcheng, 2016-07-06 02:07:29]

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/platfor...
File device/sensors/platform_sensor_configuration.h (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/platfor...
device/sensors/platform_sensor_configuration.h:18: void SetFrequency(double
frequency);
Note that usually simple getters / setters like this are inlined in the header.
I'm not sure if it's too late / too much work to change this though (and it can
be done in a followup)

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:25:
out->SetFrequency(data.frequency());
Is there an upper limit on frequency that to enforce here? Or is that enforced
somewhere else in the stack?

https://codereview.chromium.org/2078433002/diff/120001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/120001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:17: return
input.GetFrequency();
It's OK to inline this one, since it's short and simple.

[shalamov, 2016-07-06 07:15:10]

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/platfor...
File device/sensors/platform_sensor_configuration.h (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/platfor...
device/sensors/platform_sensor_configuration.h:18: void SetFrequency(double
frequency);
On 2016/07/06 02:07:29, dcheng wrote:
> Note that usually simple getters / setters like this are inlined in the
header.
> I'm not sure if it's too late / too much work to change this though (and it
can
> be done in a followup)

Done.

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:25:
out->SetFrequency(data.frequency());
On 2016/07/06 02:07:29, dcheng wrote:
> Is there an upper limit on frequency that to enforce here? Or is that enforced
> somewhere else in the stack?

Configuration is validated by concrete sensor implementation using bool
PlatformSensor::CheckSensorConfiguration(const PlatformSensorConfiguration&
configuration); method. Used in platform_sensor.cc
PlatformSensor::StartListening

https://codereview.chromium.org/2078433002/diff/120001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/120001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:17: return
input.GetFrequency();
On 2016/07/06 02:07:29, dcheng wrote:
> It's OK to inline this one, since it's short and simple.

Done.

[shalamov, 2016-07-06 07:28:12]

alexander.shalamov@intel.com changed reviewers:
+ jochen@chromium.org

[shalamov, 2016-07-06 07:28:13]

Jochen could you please take a look at content/browser/* changes.

[dcheng, 2016-07-06 09:12:04]

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
File device/sensors/public/interfaces/sensor_struct_traits.cc (right):

https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
device/sensors/public/interfaces/sensor_struct_traits.cc:25:
out->SetFrequency(data.frequency());
On 2016/07/06 07:15:10, shalamov wrote:
> On 2016/07/06 02:07:29, dcheng wrote:
> > Is there an upper limit on frequency that to enforce here? Or is that
enforced
> > somewhere else in the stack?
> 
> Configuration is validated by concrete sensor implementation using bool
> PlatformSensor::CheckSensorConfiguration(const PlatformSensorConfiguration&
> configuration); method. Used in platform_sensor.cc
> PlatformSensor::StartListening

I think we can still provide an upper limit to clamp it here: then we don't need
to duplicate the validation in each PlatformSensor implementation. For example,
I think we clamp to 60Hz to reduce the risk of info leaks:
https://crbug.com/421691

[shalamov, 2016-07-06 09:54:57]

On 2016/07/06 09:12:04, dcheng wrote:
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> File device/sensors/public/interfaces/sensor_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> device/sensors/public/interfaces/sensor_struct_traits.cc:25:
> out->SetFrequency(data.frequency());
> On 2016/07/06 07:15:10, shalamov wrote:
> > On 2016/07/06 02:07:29, dcheng wrote:
> > > Is there an upper limit on frequency that to enforce here? Or is that
> enforced
> > > somewhere else in the stack?
> > 
> > Configuration is validated by concrete sensor implementation using bool
> > PlatformSensor::CheckSensorConfiguration(const PlatformSensorConfiguration&
> > configuration); method. Used in platform_sensor.cc
> > PlatformSensor::StartListening
> 
> I think we can still provide an upper limit to clamp it here: then we don't
need
> to duplicate the validation in each PlatformSensor implementation. For
example,
> I think we clamp to 60Hz to reduce the risk of info leaks:
> https://crbug.com/421691

Thanks for info, we will clamp frequency for high frequency sensors e.g.
Gyroscope, Accelerometer and Magnetometer on blink side, so that we can throw
exception immediately, without asking browser process to update sensor
configuration.

[jochen (gone - plz use gerrit), 2016-07-06 11:29:22]

will stamp the content parts once the main bits are reviewed

https://codereview.chromium.org/2078433002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2078433002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2076: // TODO(Mikhail): Add
cmd line switch for sensors.
please add the full email address here, so we know who is supposed to follow-up
on this

https://codereview.chromium.org/2078433002/diff/140001/device/sensors/OWNERS
File device/sensors/OWNERS (right):

https://codereview.chromium.org/2078433002/diff/140001/device/sensors/OWNERS#...
device/sensors/OWNERS:1: alexander.shalamov@intel.com
only chromium committers can be in owners files, sorry

[shalamov, 2016-07-06 11:58:38]

Patchset #6 (id:140001) has been deleted

[dcheng, 2016-07-08 02:10:58]

On 2016/07/06 09:54:57, shalamov wrote:
> On 2016/07/06 09:12:04, dcheng wrote:
> >
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> > File device/sensors/public/interfaces/sensor_struct_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> > device/sensors/public/interfaces/sensor_struct_traits.cc:25:
> > out->SetFrequency(data.frequency());
> > On 2016/07/06 07:15:10, shalamov wrote:
> > > On 2016/07/06 02:07:29, dcheng wrote:
> > > > Is there an upper limit on frequency that to enforce here? Or is that
> > enforced
> > > > somewhere else in the stack?
> > > 
> > > Configuration is validated by concrete sensor implementation using bool
> > > PlatformSensor::CheckSensorConfiguration(const
PlatformSensorConfiguration&
> > > configuration); method. Used in platform_sensor.cc
> > > PlatformSensor::StartListening
> > 
> > I think we can still provide an upper limit to clamp it here: then we don't
> need
> > to duplicate the validation in each PlatformSensor implementation. For
> example,
> > I think we clamp to 60Hz to reduce the risk of info leaks:
> > https://crbug.com/421691
> 
> Thanks for info, we will clamp frequency for high frequency sensors e.g.
> Gyroscope, Accelerometer and Magnetometer on blink side, so that we can throw
> exception immediately, without asking browser process to update sensor
> configuration.

We still need some sort of filtering in the browser process: a renderer can be
compromised and not bother doing the check.

[shalamov, 2016-07-08 08:06:53]

On 2016/07/08 02:10:58, dcheng wrote:
> On 2016/07/06 09:54:57, shalamov wrote:
> > On 2016/07/06 09:12:04, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> > > File device/sensors/public/interfaces/sensor_struct_traits.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2078433002/diff/100001/device/sensors/public/...
> > > device/sensors/public/interfaces/sensor_struct_traits.cc:25:
> > > out->SetFrequency(data.frequency());
> > > On 2016/07/06 07:15:10, shalamov wrote:
> > > > On 2016/07/06 02:07:29, dcheng wrote:
> > > > > Is there an upper limit on frequency that to enforce here? Or is that
> > > enforced
> > > > > somewhere else in the stack?
> > > > 
> > > > Configuration is validated by concrete sensor implementation using bool
> > > > PlatformSensor::CheckSensorConfiguration(const
> PlatformSensorConfiguration&
> > > > configuration); method. Used in platform_sensor.cc
> > > > PlatformSensor::StartListening
> > > 
> > > I think we can still provide an upper limit to clamp it here: then we
don't
> > need
> > > to duplicate the validation in each PlatformSensor implementation. For
> > example,
> > > I think we clamp to 60Hz to reduce the risk of info leaks:
> > > https://crbug.com/421691
> > 
> > Thanks for info, we will clamp frequency for high frequency sensors e.g.
> > Gyroscope, Accelerometer and Magnetometer on blink side, so that we can
throw
> > exception immediately, without asking browser process to update sensor
> > configuration.
> 
> We still need some sort of filtering in the browser process: a renderer can be
> compromised and not bother doing the check.

I've capped frequency to 60Hz on browser side. In case when higher sensor
polling frequency is requested, error would be returned to the blink side.

[dcheng, 2016-07-08 14:22:00]

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
File device/sensors/platform_sensor.cc (right):

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0;
Can we do this check in the StructTraits? You can DCHECK() in the serialization
side that frequency <= 60, and deserialization side can just fail if frequency >
60.

[shalamov_, 2016-07-08 14:50:37]

On 2016/07/08 14:22:00, dcheng wrote:
>
https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
> File device/sensors/platform_sensor.cc (right):
> 
>
https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
> device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0;
> Can we do this check in the StructTraits? You can DCHECK() in the
serialization
> side that frequency <= 60, and deserialization side can just fail if frequency
>
> 60.

Would it be better design to keep type conversion and validation separately?
Also, DCHECK in (de)serializer would not fix the problem you've referred to, we
need to cap frequency on release builds as well and report to blink side that
requested configuration is not supported. What do you think?

[Tom Sepez, 2016-07-11 17:40:03]

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/public/...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:32: double frequency;
Are there better limits for this than just the range of a double?  What happens
if we ask to poll for example, at GHz or above or conversely at extremely low
rates?  What does a negative frequency mean?  Do all implementations have to
check these?

[Mikhail, 2016-07-12 08:26:50]

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/public/...
File device/sensors/public/interfaces/sensor.mojom (right):

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/public/...
device/sensors/public/interfaces/sensor.mojom:32: double frequency;
On 2016/07/11 17:40:03, Tom Sepez wrote:
> Are there better limits for this than just the range of a double?  What
happens
> if we ask to poll for example, at GHz or above or conversely at extremely low
> rates?  What does a negative frequency mean?  Do all implementations have to
> check these?

Here we cap frequency to 60Hz
https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
Also an inappropriate configuration will raise an error on platform side.

[Tom Sepez, 2016-07-12 16:53:52]

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
File device/sensors/platform_sensor.cc (right):

https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0;
On 2016/07/08 14:22:00, dcheng wrote:
> Can we do this check in the StructTraits? You can DCHECK() in the
serialization
> side that frequency <= 60, and deserialization side can just fail if frequency
>
> 60.

You also want to check that frequency > 0, right?  Its a double and could be 0
or negative.

[Mikhail, 2016-07-12 20:14:47]

On 2016/07/12 16:53:52, Tom Sepez wrote:
>
https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
> File device/sensors/platform_sensor.cc (right):
> 
>
https://codereview.chromium.org/2078433002/diff/200001/device/sensors/platfor...
> device/sensors/platform_sensor.cc:18: return config.frequency() <= 60.0;
> On 2016/07/08 14:22:00, dcheng wrote:
> > Can we do this check in the StructTraits? You can DCHECK() in the
> serialization
> > side that frequency <= 60, and deserialization side can just fail if
frequency
> >
> > 60.
> 
> You also want to check that frequency > 0, right?  Its a double and could be 0
> or negative.

Right, that should be checked as well. Thanks.

Alex is OOO now and I do not have permissions modify this CL, so I had to create
a clone CL https://codereview.chromium.org/2144623003 in order not to interrupt
the review process. The uploaded patch includes 'frequency > 0' check. PTAL.

[Tom Sepez, 2016-07-13 17:42:54]

> 
> Alex is OOO now and I do not have permissions modify this CL, so I had to
create
> a clone CL https://codereview.chromium.org/2144623003 in order not to
interrupt
> the review process. The uploaded patch includes 'frequency > 0' check. PTAL.

When cloning a patch in this manner, we would strongly suggest that the initial
upload be the unmodified version of the patch that you are cloning, and that
subsequent modifications be made as incremental patch sets, so that one can
easily diff the changes.  Lumping them together means we'll have to hunt to
figure out what changed.

[Mikhail, 2016-07-13 18:59:03]

On 2016/07/13 17:42:54, Tom Sepez wrote:
> 
> When cloning a patch in this manner, we would strongly suggest that the
initial
> upload be the unmodified version of the patch that you are cloning, and that
> subsequent modifications be made as incremental patch sets, so that one can
> easily diff the changes.  Lumping them together means we'll have to hunt to
> figure out what changed.

Got it. I've modified the CL accordingly.