[Sensors] Allow Sensor API only on secure top-level browsing contexts and add frequency checks

[Sensors] Allow Sensor API only on secure top-level browsing contexts and add frequency checks

This patch introduces secure context and top-level browsing context checks as required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given frequency is negative exception is raised; if the given frequency is more than 60 Hz it is capped to 60 Hz.

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

[Mikhail, 2016-09-19 12:54:03]

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

[commit-bot: I haz the power, 2016-09-19 12:54:08]

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-19 12:56:34]

Description was changed from

==========
[Sensors] Introduce checks at sensor construction

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

to

==========
[Sensors] Introduce checks at sensor construction

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

[Mikhail, 2016-09-19 12:56:34]

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

[Mikhail, 2016-09-19 13:01:22]

PTAL

[haraken, 2016-09-19 13:07:51]

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl (right):

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl:11:
ConstructorCallWith=(ScriptState,ExecutionContext),

ConstructorCallWith=ScriptState would be enough. You can get the
ExecutionContext by scriptState->getExecutionContext().

[haraken, 2016-09-19 13:09:41]

In terms of implementation LGTM.

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

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:37: if
(!scriptState->domWindow()->frame() ||
!scriptState->domWindow()->frame()->isMainFrame()) {

You need to add '!scriptState->domWindow() ||' as well.

[shalamov, 2016-09-19 13:11:42]

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:20: test(() =>
assert_throws(new RangeError(), () => new AmbientLightSensor({frequency: -60})),
nit: Format to 80 columns?

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

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:46:
exceptionState.throwRangeError("Frequency must be positive");
nit: Dot at the end of error message.

[Mikhail, 2016-09-19 13:52:11]

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

[commit-bot: I haz the power, 2016-09-19 13:52:32]

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-19 14:18:03]

Thanks for your comments!

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:20: test(() =>
assert_throws(new RangeError(), () => new AmbientLightSensor({frequency: -60})),
On 2016/09/19 13:11:42, shalamov wrote:
> nit: Format to 80 columns?

Done.

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl (right):

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl:11:
ConstructorCallWith=(ScriptState,ExecutionContext),
On 2016/09/19 13:07:51, haraken wrote:
> 
> ConstructorCallWith=ScriptState would be enough. You can get the
> ExecutionContext by scriptState->getExecutionContext().

Done.

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

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:37: if
(!scriptState->domWindow()->frame() ||
!scriptState->domWindow()->frame()->isMainFrame()) {
On 2016/09/19 13:09:41, haraken wrote:
> 
> You need to add '!scriptState->domWindow() ||' as well.

Done.

https://codereview.chromium.org/2353493002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:46:
exceptionState.throwRangeError("Frequency must be positive");
On 2016/09/19 13:11:42, shalamov wrote:
> nit: Dot at the end of error message.

Done.

[commit-bot: I haz the power, 2016-09-19 15:19:09]

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

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

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-09-19 16:52:13]

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:22: () => new
AmbientLightSensor({frequency: -60})),
nit: also test for capping at 60?

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:26: let
ambientLightSensor = new AmbientLightSensor({frequency: 60});
just curious here: does the frequency here mean 'max frequency'. ambient light
normally does not have a constant 'frequency' right?

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:50: const double
kMaxAllowedFrequency = 60.0;
think this should be a more 'global' constant somewhere. perhaps a dedicated .h
file in modules/sensor or common..

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:53: }
don't know what the spec says here, maybe output some message to console for
developers to know there is capping..

[haraken, 2016-09-20 00:41:12]

LGTM on my side

[Mikhail, 2016-09-20 13:26:55]

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

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

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-20 13:27:40]

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

[Mikhail, 2016-09-20 13:29:36]

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

[Mikhail, 2016-09-20 13:29:37]

Thanks for your comments!

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:22: () => new
AmbientLightSensor({frequency: -60})),
On 2016/09/19 16:52:13, timvolodine wrote:
> nit: also test for capping at 60?

Done.

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:26: let
ambientLightSensor = new AmbientLightSensor({frequency: 60});
On 2016/09/19 16:52:13, timvolodine wrote:
> just curious here: does the frequency here mean 'max frequency'. ambient light
> normally does not have a constant 'frequency' right?

that is more 'desired polling frequency', frequency can be passed to AML sensor
constructor via SensorOptions:
https://w3c.github.io/ambient-light/#ambient-light-sensor-interface

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:50: const double
kMaxAllowedFrequency = 60.0;
On 2016/09/19 16:52:13, timvolodine wrote:
> think this should be a more 'global' constant somewhere. perhaps a dedicated
.h
> file in modules/sensor or common..

Done.

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:53: }
On 2016/09/19 16:52:13, timvolodine wrote:
> don't know what the spec says here, maybe output some message to console for
> developers to know there is capping..

Done.

[Tom Sepez, 2016-09-20 16:41:06]

Messages LGTM

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

https://codereview.chromium.org/2353493002/diff/40001/device/generic_sensor/p...
device/generic_sensor/public/interfaces/sensor.mojom:34: const double
kMaxAllowedFrequency = 60.0;
nit: If we named this kMaxAllowedFrequencyHz, we could lose the comment entirel.

[timvolodine, 2016-09-20 17:46:37]

thanks, lgtm % comments

you could also change the title to something more descriptive, e.g.
"Allow Sensor API only on secure top-level browsing contexts and add frequency
checks"

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:26: let
ambientLightSensor = new AmbientLightSensor({frequency: 60});
On 2016/09/20 13:29:36, Mikhail wrote:
> On 2016/09/19 16:52:13, timvolodine wrote:
> > just curious here: does the frequency here mean 'max frequency'. ambient
light
> > normally does not have a constant 'frequency' right?
> 
> that is more 'desired polling frequency', frequency can be passed to AML
sensor
> constructor via SensorOptions:
> https://w3c.github.io/ambient-light/#ambient-light-sensor-interface

it's not exactly clear what 'desired polling frequency' even means in the
context of ambient light sensor, but that's probably a concern towards the spec
editors.

I was actually wondering if we are actively polling the shared memory buffer in
case of ambient light sensor (even if it remains constant)?

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

https://codereview.chromium.org/2353493002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:34: if
(ambientLightSensor.state === 'idle') {
why is this case needed?

[Mikhail, 2016-09-21 10:00:42]

Thanks for review!

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

https://codereview.chromium.org/2353493002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:26: let
ambientLightSensor = new AmbientLightSensor({frequency: 60});
On 2016/09/20 17:46:37, timvolodine wrote:
> it's not exactly clear what 'desired polling frequency' even means in the
> context of ambient light sensor, but that's probably a concern towards the
spec
> editors.

That is indeed being discussed in WG atm, however it might be easier to treat it
as something that is finally passed to
https://developer.android.com/reference/android/hardware/SensorManager.html#r...
as sampling polling period.
> 
> I was actually wondering if we are actively polling the shared memory buffer
in
> case of ambient light sensor (even if it remains constant)?

in this case the polling strategy will not poll the shared buffer until
'onreadingupdate' event is received from the platform.

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

https://codereview.chromium.org/2353493002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:34: if
(ambientLightSensor.state === 'idle') {
On 2016/09/20 17:46:37, timvolodine wrote:
> why is this case needed?

here we wait when the sensor gets actually stopped, thus keeping the environment
'clean' for the following test.

[timvolodine, 2016-09-21 14:01:24]

thanks! pls update the title in description for consistency as well

[Mikhail, 2016-09-21 14:06:29]

Description was changed from

==========
[Sensors] Introduce checks at sensor construction

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

to

==========
[Sensors] Allow Sensor API only on secure top-level browsing contexts and add
frequency checks 

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

[Mikhail, 2016-09-28 09:27:34]

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

[commit-bot: I haz the power, 2016-09-28 09:27:48]

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-28 11:12:48]

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

[commit-bot: I haz the power, 2016-09-28 11:12:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Mikhail, 2016-09-28 11:15:33]

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

[Mikhail, 2016-09-28 11:15:34]

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

[commit-bot: I haz the power, 2016-09-28 11:15:43]

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

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

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

[Mikhail, 2016-09-28 12:19:50]

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

[commit-bot: I haz the power, 2016-09-28 12:19:59]

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-28 12:25:14]

Description was changed from

==========
[Sensors] Allow Sensor API only on secure top-level browsing contexts and add
frequency checks 

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

to

==========
[Sensors] Allow Sensor API only on secure top-level browsing contexts and add
frequency checks 

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

[commit-bot: I haz the power, 2016-09-28 12:25:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 12:27:14]

Description was changed from

==========
[Sensors] Allow Sensor API only on secure top-level browsing contexts and add
frequency checks 

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

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

to

==========
[Sensors] Allow Sensor API only on secure top-level browsing contexts and add
frequency checks

This patch introduces secure context and top-level browsing context checks as
required at https://w3c.github.io/sensors/#construct-sensor-object.

Besides it introduces check of the given frequency option: if the given
frequency is negative exception is raised; if the given frequency is more than
60 Hz it is capped to 60 Hz.

BUG=606766

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

[commit-bot: I haz the power, 2016-09-28 12:27:15]

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