[Sensors] Consider maximum supported frequency

[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency, min(maximum supported frequency, 60 Hz))

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

[Mikhail, 2016-11-02 14:34:28]

Description was changed from

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

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

to

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

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

[Mikhail, 2016-11-02 14:34:28]

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

[Mikhail, 2016-11-02 14:39:31]

Patchset #1 (id:1) has been deleted

[shalamov, 2016-11-02 14:42:46]

https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp (right):

https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp:49: if
(maxSupportedFrequency && frequency > maxSupportedFrequency)
Can this be moved to Sensor or another place, so that all sensors can reuse this
code?

[darktears, 2016-11-02 22:55:22]

On 2016/11/02 at 14:42:46, alexander.shalamov wrote:
>
https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp (right):
> 
>
https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp:49: if
(maxSupportedFrequency && frequency > maxSupportedFrequency)
> Can this be moved to Sensor or another place, so that all sensors can reuse
this code?

Let's remember there is device/sensors/public/cpp/device_sensors_consts.h

[darktears, 2016-11-02 23:06:08]

alexis.menard@intel.com changed reviewers:
+ alexis.menard@intel.com

[darktears, 2016-11-02 23:06:09]

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:46: // If this information is
unavailable '0.0' is returned.
This sounds weird. I mean I would expect the default supported frequency to be
60hz unless the platform says different. Then your condition in
AmbientLightSensor.cpp becomes if (frequency > maxSupportedFrequency).

I even wonder if it could be put into PlatformSensorConfiguration instead so
when you get the default config you also get the max supported frequency.

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:47: // The default implemenmtation
returns '0.0'.
Typo here.

[Mikhail, 2016-11-03 12:09:55]

On 2016/11/02 14:42:46, shalamov wrote:
>
https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp (right):
> 
>
https://codereview.chromium.org/2465363004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp:49: if
> (maxSupportedFrequency && frequency > maxSupportedFrequency)
> Can this be moved to Sensor or another place, so that all sensors can reuse
this
> code?

Moved to the base class, so that concrete sensors do not have to repeat this
logic.

[Mikhail, 2016-11-03 12:13:17]

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:46: // If this information is
unavailable '0.0' is returned.
On 2016/11/02 23:06:09, darktears wrote:
> This sounds weird. I mean I would expect the default supported frequency to be
> 60hz unless the platform says different. Then your condition in
> AmbientLightSensor.cpp becomes if (frequency > maxSupportedFrequency).
> 
> I even wonder if it could be put into PlatformSensorConfiguration instead so
> when you get the default config you also get the max supported frequency.
I'm not sure we should confuse device capabilities and the limitations that come
from our framework at this level. IMO it should rather fall back to the default
frequency if maximum value is not available.

https://codereview.chromium.org/2465363004/diff/20001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:47: // The default implemenmtation
returns '0.0'.
On 2016/11/02 23:06:09, darktears wrote:
> Typo here.

Done.

[shalamov, 2016-11-03 12:54:05]

Could you also add implementation for win platform adaptation?

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/p...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/p...
device/generic_sensor/public/interfaces/sensor_provider.mojom:22: // Maximum
sampling frequency for the sensor: it considers both
|SensorConfiguration::kMaxAllowedFrequency| and actual sensor capabilities.
nit: format to 80 columns?

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/s...
File device/generic_sensor/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/s...
device/generic_sensor/sensor_provider_impl.cc:92: double maximum_frequency =
sensor->GetMaximumSupportedFrequency();
If platform returns 0, should we cap max to default?

https://codereview.chromium.org/2465363004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2465363004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:126: }, 'Test
that frequency is capped to the maximum supported frequency from the
platform.');
nit: format

[Mikhail, 2016-11-03 13:28:17]

thanks for your comments!

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/p...
File device/generic_sensor/public/interfaces/sensor_provider.mojom (right):

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/p...
device/generic_sensor/public/interfaces/sensor_provider.mojom:22: // Maximum
sampling frequency for the sensor: it considers both
|SensorConfiguration::kMaxAllowedFrequency| and actual sensor capabilities.
On 2016/11/03 12:54:05, shalamov wrote:
> nit: format to 80 columns?

Done.

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/s...
File device/generic_sensor/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2465363004/diff/40001/device/generic_sensor/s...
device/generic_sensor/sensor_provider_impl.cc:92: double maximum_frequency =
sensor->GetMaximumSupportedFrequency();
On 2016/11/03 12:54:05, shalamov wrote:
> If platform returns 0, should we cap max to default?

It should not return 0. Implementation must cap max to default.

https://codereview.chromium.org/2465363004/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2465363004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:126: }, 'Test
that frequency is capped to the maximum supported frequency from the
platform.');
On 2016/11/03 12:54:05, shalamov wrote:
> nit: format

Done.

[shalamov, 2016-11-03 13:32:55]

lgtm with nits

https://codereview.chromium.org/2465363004/diff/60001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right):

https://codereview.chromium.org/2465363004/diff/60001/device/generic_sensor/p...
device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:498: // Tests
that GetMaximumSupportedFrequency is returns fallback value.
nit: is returns => returns

[Mikhail, 2016-11-03 13:42:26]

CC'ing Tom for .mojom changes

[Mikhail, 2016-11-03 13:54:47]

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

[commit-bot: I haz the power, 2016-11-03 13:55:05]

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

[haraken, 2016-11-03 13:58:45]

modules/ implementation LGTM

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:155: frequency =
maximumFrequency;

Duplicated code.

[commit-bot: I haz the power, 2016-11-03 15:07:13]

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

[commit-bot: I haz the power, 2016-11-03 15:07:13]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-11-03 17:32:56]

just a few more comments below

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/a...
File
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java
(right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/a...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:25:
private static final double SECONDS_IN_MICROSECOND = 0.000001d;
why not use import java.util.concurrent.TimeUnit for conversions instead?

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:45: // value obtained from the platfrom
if it is available. If platfrom
platfrom->platform

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:49: virtual double
GetMaximumSupportedFrequency();
const? (or is that too restrictive?)

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:498: // Tests
that GetMaximumSupportedFrequency is returns fallback value.
-is
(think Alexander already mentioned it)

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
File device/generic_sensor/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
device/generic_sensor/sensor_provider_impl.cc:93: DCHECK(maximum_frequency);
>=0?

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:259:
DCHECK(m_configuration->frequency &&
nit: m_configuration->frequency > 0 ? (or maybe >= dunno)

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:70: double
maximumFrequency() const;
nit: maybe inline?

[Mikhail, 2016-11-04 20:10:58]

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

[commit-bot: I haz the power, 2016-11-04 20:11:42]

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

[Mikhail, 2016-11-04 21:17:45]

Thanks for your comments.

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/a...
File
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java
(right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/a...
device/generic_sensor/android/java/src/org/chromium/device/sensors/PlatformSensor.java:25:
private static final double SECONDS_IN_MICROSECOND = 0.000001d;
On 2016/11/03 17:32:55, timvolodine wrote:
> why not use import java.util.concurrent.TimeUnit for conversions instead?
That API operates with 'long' type and loses precision.
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/TimeUnit.html

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:45: // value obtained from the platfrom
if it is available. If platfrom
On 2016/11/03 17:32:56, timvolodine wrote:
> platfrom->platform

Done.

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor.h:49: virtual double
GetMaximumSupportedFrequency();
On 2016/11/03 17:32:56, timvolodine wrote:
> const? (or is that too restrictive?)

would not like to put such a limit for implementation, other virtual getters are
non-const as well.

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/p...
device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:498: // Tests
that GetMaximumSupportedFrequency is returns fallback value.
On 2016/11/03 17:32:56, timvolodine wrote:
> -is
> (think Alexander already mentioned it)

Done.

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
File device/generic_sensor/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
device/generic_sensor/sensor_provider_impl.cc:93: DCHECK(maximum_frequency);
On 2016/11/03 17:32:56, timvolodine wrote:
> >=0?
The implementation must return a positive value for the frequency limit.

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:155: frequency =
maximumFrequency;
On 2016/11/03 13:58:45, haraken wrote:
> 
> Duplicated code.
ah, thanks!

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:259:
DCHECK(m_configuration->frequency &&
On 2016/11/03 17:32:56, timvolodine wrote:
> nit: m_configuration->frequency > 0 ? (or maybe >= dunno)

Done.

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2465363004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:70: double
maximumFrequency() const;
On 2016/11/03 17:32:56, timvolodine wrote:
> nit: maybe inline?

indeed, thanks

[commit-bot: I haz the power, 2016-11-04 21:42:21]

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

[commit-bot: I haz the power, 2016-11-04 21:42:22]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-11-08 08:01:21]

Tim, do you have more comments for this CL?

[Mikhail, 2016-11-09 08:56:28]

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

[commit-bot: I haz the power, 2016-11-09 08:56:36]

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

[Mikhail, 2016-11-09 08:59:06]

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

[Mikhail, 2016-11-09 08:59:07]

CC'ing Reilly to have a look

[commit-bot: I haz the power, 2016-11-09 12:44:35]

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

[commit-bot: I haz the power, 2016-11-09 12:44:36]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-11-09 16:57:30]

lgtm

[timvolodine, 2016-11-09 20:31:52]

fine with me, one nit comment, otherwise lgtm thanks!

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
File device/generic_sensor/sensor_provider_impl.cc (right):

https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
device/generic_sensor/sensor_provider_impl.cc:93: DCHECK(maximum_frequency);
On 2016/11/04 21:17:44, Mikhail wrote:
> On 2016/11/03 17:32:56, timvolodine wrote:
> > >=0?
> The implementation must return a positive value for the frequency limit.

nit: it could be negative and pass DCHECK?

[Mikhail, 2016-11-10 12:06:13]

On 2016/11/09 20:31:52, timvolodine wrote:
> fine with me, one nit comment, otherwise lgtm thanks!
> 
>
https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
> File device/generic_sensor/sensor_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/2465363004/diff/80001/device/generic_sensor/s...
> device/generic_sensor/sensor_provider_impl.cc:93: DCHECK(maximum_frequency);
> On 2016/11/04 21:17:44, Mikhail wrote:
> > On 2016/11/03 17:32:56, timvolodine wrote:
> > > >=0?
> > The implementation must return a positive value for the frequency limit.
> 
> nit: it could be negative and pass DCHECK?

Thanks for the catch!

[Mikhail, 2016-11-10 12:08:05]

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

[Mikhail, 2016-11-10 12:08:06]

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

[commit-bot: I haz the power, 2016-11-10 12:08:20]

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-11-10 12:15:51]

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

[commit-bot: I haz the power, 2016-11-10 12:15:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mikhail, 2016-11-10 12:20:08]

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

[Mikhail, 2016-11-10 12:20:34]

On 2016/11/03 13:42:26, Mikhail wrote:
> CC'ing Tom for .mojom changes

Could you take a look?

[Tom Sepez, 2016-11-10 16:52:53]

mojom LGTM

[Mikhail, 2016-11-11 07:33:53]

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

[commit-bot: I haz the power, 2016-11-11 07:34:07]

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-11-11 08:00:29]

Description was changed from

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

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

to

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

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

[commit-bot: I haz the power, 2016-11-11 08:00:31]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-11-11 08:04:12]

Description was changed from

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

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

to

==========
[Sensors] Consider maximum supported frequency

This patch improves capping of the given sensor frequency hint, now it considers
the actual sensor capabilities obtained from the platform (if available).
So now the behavior has changed in the following way:
frequency = min(given frequency, 60 Hz) -> frequency = min(given frequency,
min(maximum supported frequency, 60 Hz))

BUG=606766

Committed: https://crrev.com/9d5b8269d088402d1d9c261d1c12585d81a4ac78
Cr-Commit-Position: refs/heads/master@{#431520}
==========

[commit-bot: I haz the power, 2016-11-11 08:04:13]

Patchset 7 (id:??) landed as
https://crrev.com/9d5b8269d088402d1d9c261d1c12585d81a4ac78
Cr-Commit-Position: refs/heads/master@{#431520}