[Sensors] Handle default sensor configuration

[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at 'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g. 'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from 'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from 'SensorProxy::handleSensorError()'

BUG=606766
Committed: https://crrev.com/b49349c6d3c4e7875fbb3ed8e3535f71006e444c
Cr-Commit-Position: refs/heads/master@{#418528}

[Mikhail, 2016-09-12 09:53:28]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 09:53:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-09-12 09:59:56]

Description was changed from

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

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

to

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

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

[Mikhail, 2016-09-12 09:59:56]

mikhail.pozdnyakov@intel.com changed reviewers:
+ alexander.shalamov@intel.com, haraken@chromium.org, rockot@chromium.org,
timvolodine@chromium.org

[Mikhail, 2016-09-12 10:01:44]

PTAL

[commit-bot: I haz the power, 2016-09-12 11:37:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 11:37:46]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-12 11:39:24]

In terms of implementation, WebKit/Source/modules/ LGTM.

[timvolodine, 2016-09-12 16:37:08]

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
device/generic_sensor/public/interfaces/sensor_provider.mojom:20:
SensorConfiguration default_configuration;
curious how this is described in spec: does the platform decide on default
parameters when the user does not specify them?

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:200:
DCHECK(m_sensorProxy->defaultConfig());
should this check be in sensorProxy itself, perhaps when the defaultConfig is
first obtained?
i.e. this should probably automatically hold when proxy is in 'initialized'
state (independent of whether m_configuration is null or not)

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:202: if (!m_configuration) {
// Failed to create configuration.
when can this happen? maybe just DCHECK here?

[Mikhail, 2016-09-12 17:08:10]

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
device/generic_sensor/public/interfaces/sensor_provider.mojom:20:
SensorConfiguration default_configuration;
On 2016/09/12 16:37:08, timvolodine wrote:
> curious how this is described in spec: does the platform decide on default
> parameters when the user does not specify them?
That is not mentioned explicitly, however IMO it is natural to let platform to
define the default parameters so that they fit better to the underlying platform
and hardware

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:200:
DCHECK(m_sensorProxy->defaultConfig());
On 2016/09/12 16:37:08, timvolodine wrote:
> should this check be in sensorProxy itself, perhaps when the defaultConfig is
> first obtained?
> i.e. this should probably automatically hold when proxy is in 'initialized'
> state (independent of whether m_configuration is null or not)
right, the DCHECK can be omitted I just put it to underline the fact that
default config != nullptr here.

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:202: if (!m_configuration) {
// Failed to create configuration.
On 2016/09/12 16:37:08, timvolodine wrote:
> when can this happen? maybe just DCHECK here?

can be DCHECK() (given options checks can/should be done in ctor)

[Mikhail, 2016-09-12 18:52:46]

Please take a look

[Mikhail, 2016-09-12 21:27:34]

mikhail.pozdnyakov@intel.com changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2016-09-12 22:01:19]

mojom lgtm

[shalamov, 2016-09-13 10:52:59]

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2330943002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:65: const
device::mojom::blink::SensorConfiguration* defaultConfig() const { return
m_defaultConfig.get(); }
DCHECK(isInitialized()); ?

[shalamov, 2016-09-13 11:25:41]

On 2016/09/12 18:52:46, Mikhail wrote:
> Please take a look

lgtm

[timvolodine, 2016-09-13 13:17:11]

lgtm, thanks!

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
device/generic_sensor/public/interfaces/sensor_provider.mojom:20:
SensorConfiguration default_configuration;
On 2016/09/12 17:08:10, Mikhail wrote:
> On 2016/09/12 16:37:08, timvolodine wrote:
> > curious how this is described in spec: does the platform decide on default
> > parameters when the user does not specify them?
> That is not mentioned explicitly, however IMO it is natural to let platform to
> define the default parameters so that they fit better to the underlying
platform
> and hardware

sounds reasonable.. would you mind raising an issue on the spec github so that
the editors incorporate this at some stage (or at least know about this) ?

[Mikhail, 2016-09-13 13:56:48]

On 2016/09/13 13:17:11, timvolodine wrote:
> lgtm, thanks!
> 
>
https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
> File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):
> 
>
https://codereview.chromium.org/2330943002/diff/1/device/generic_sensor/publi...
> device/generic_sensor/public/interfaces/sensor_provider.mojom:20:
> SensorConfiguration default_configuration;
> On 2016/09/12 17:08:10, Mikhail wrote:
> > On 2016/09/12 16:37:08, timvolodine wrote:
> > > curious how this is described in spec: does the platform decide on default
> > > parameters when the user does not specify them?
> > That is not mentioned explicitly, however IMO it is natural to let platform
to
> > define the default parameters so that they fit better to the underlying
> platform
> > and hardware
> 
> sounds reasonable.. would you mind raising an issue on the spec github so that
> the editors incorporate this at some stage (or at least know about this) ?

Thanks for review! The issue is at https://github.com/w3c/sensors/issues/126.

[Mikhail, 2016-09-13 13:57:01]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-09-13 13:57:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2330943002/#ps40001
(title: "Comments from Alex")

[commit-bot: I haz the power, 2016-09-13 13:57:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 13:59:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 13:59:22]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mikhail, 2016-09-14 08:39:53]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 08:40:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 10:23:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 10:23:13]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-09-14 10:25:04]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2016-09-14 10:25:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, alexander.shalamov@intel.com, tsepez@chromium.org,
timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2330943002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-09-14 10:25:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 10:30:16]

Description was changed from

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

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

to

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

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

[commit-bot: I haz the power, 2016-09-14 10:30:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-14 10:31:50]

Description was changed from

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

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

to

==========
[Sensors] Handle default sensor configuration

This patch provides platform sensor's default configuration to the Blink side at
'SensorProxy' initialization stage.
Default configuration will be used for sensors construction by default, e.g.
'let sensor = new AmbientLightSensor();'.

A few related changes are added as well:
- Rename 'SensorReadBuffer' to 'SensorInitParams'
- Remove repeated code calling 'startListening()' from
'Sensor::onSensorInitialized()'
- Call 'reset()' for all 'SensorProxy' members from
'SensorProxy::handleSensorError()'

BUG=606766

Committed: https://crrev.com/b49349c6d3c4e7875fbb3ed8e3535f71006e444c
Cr-Commit-Position: refs/heads/master@{#418528}
==========

[commit-bot: I haz the power, 2016-09-14 10:31:51]

Patchset 4 (id:??) landed as
https://crrev.com/b49349c6d3c4e7875fbb3ed8e3535f71006e444c
Cr-Commit-Position: refs/heads/master@{#418528}