[Sensors] Implement sensor data polling

[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

[Mikhail, 2016-09-08 09:02:10]

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

[Mikhail, 2016-09-08 09:02:54]

Description was changed from

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

to

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

[Mikhail, 2016-09-08 09:02:54]

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

[commit-bot: I haz the power, 2016-09-08 09:06:55]

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-08 09:07:24]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-08 09:24:08]

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

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

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-08 09:59:10]

PTAL

[haraken, 2016-09-08 11:07:13]

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

https://codereview.chromium.org/2323683002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:55:
SensorProxy* sensor = new SensorProxy(type, m_frame->page(), this);

How is this working? It looks like that no one is assigning a valid frame to
m_frame. So m_frame should be nullptr here.

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

https://codereview.chromium.org/2323683002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:47:
Member<LocalFrame> m_frame;

Instead of adding m_frame, I'd make SensorProviderProxy
LocalFrameLifecycleObserver. Then you can use frame().

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

https://codereview.chromium.org/2323683002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:24: , public
PageVisibilityObserver {

Sensor is already a PageVisibilityObserver. Can we avoid making SensorProxy a
PageVisibilityObserver?

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

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

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

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-09-08 11:24:48]

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

https://codereview.chromium.org/2323683002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:55:
SensorProxy* sensor = new SensorProxy(type, m_frame->page(), this);
On 2016/09/08 11:07:13, haraken wrote:
> 
> How is this working? It looks like that no one is assigning a valid frame to
> m_frame. So m_frame should be nullptr here.
> 

Ah I missed m_frame initialization when split the code, sorry for that

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

https://codereview.chromium.org/2323683002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:24: , public
PageVisibilityObserver {
On 2016/09/08 11:07:13, haraken wrote:
> 
> Sensor is already a PageVisibilityObserver. Can we avoid making SensorProxy a
> PageVisibilityObserver?

Tim raised the same concern at https://codereview.chromium.org/2121313002/. 
Looks like I indeed need to find an alternate solution..

[Mikhail, 2016-09-08 12:30:07]

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

[commit-bot: I haz the power, 2016-09-08 12:30:17]

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-08 12:35:30]

In the latest patch Sensor instance invokes SensorProxy::suspend()/resume()
methods from its 'pageVisibilityChanged' override.
In order to obviate extra IPCs (since SensorProxy is shared between multiple
Sensors)SensorProxy contains a local flag 'm_suspended' which is checked before
calling the actual mojo binding.

[haraken, 2016-09-08 13:18:44]

In terms of implementation, LGTM

[commit-bot: I haz the power, 2016-09-08 14:34:30]

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

[commit-bot: I haz the power, 2016-09-08 14:34:31]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-09-08 17:02:06]

also lgtm, with a few suggestions

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

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:16:
DCHECK(m_frequency);
DCHECK(m_frequency > 0)?

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:68:
Timer<OnChangeSensorPollingStrategy> m_timer;
If the polling strategies generally use timers you could move this timer to the
base class (and template it).

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:88: double
pollingPeriod = 1 / m_frequency;
maybe store it somewhere instead of recomputing on each sensor data change? in
fact you could consider replacing m_frequency by this variable in the base
class.

[timvolodine, 2016-09-08 17:06:45]

Additionally you could consider some tests for the SensorPollingStrategy at some
point since it contains concrete implementations.

[Mikhail, 2016-09-08 19:00:12]

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

[commit-bot: I haz the power, 2016-09-08 19:00:46]

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

thanks for the suggestions

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

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:16:
DCHECK(m_frequency);
On 2016/09/08 17:02:06, timvolodine wrote:
> DCHECK(m_frequency > 0)?
Style checker proposes DCHECK_GT

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:68:
Timer<OnChangeSensorPollingStrategy> m_timer;
On 2016/09/08 17:02:06, timvolodine wrote:
> If the polling strategies generally use timers you could move this timer to
the
> base class (and template it).

Done.

https://codereview.chromium.org/2323683002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorPollingStrategy.cpp:88: double
pollingPeriod = 1 / m_frequency;
On 2016/09/08 17:02:06, timvolodine wrote:
> maybe store it somewhere instead of recomputing on each sensor data change? in
> fact you could consider replacing m_frequency by this variable in the base
> class.

Done.

[commit-bot: I haz the power, 2016-09-08 23:03:07]

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

[commit-bot: I haz the power, 2016-09-08 23:03:09]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-09-09 09:13:23]

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

[Mikhail, 2016-09-09 09:13:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, timvolodine@chromium.org
Link to the patchset: https://codereview.chromium.org/2323683002/#ps40001
(title: "Applied Tim's suggestions")

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

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-09 09:16:49]

Description was changed from

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

to

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

[commit-bot: I haz the power, 2016-09-09 09:16:50]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-09 09:18:24]

Description was changed from

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

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

to

==========
[Sensors] Implement sensor data polling

This patch contains implementations of SensorPollingStrategy interface, defining
when sensor reading is updated from shared buffer.

Also it contains logic for Suspend/Resume mojo API calls based on the page
visibility. For this purpose 'SensorProxy' inherits 'PageVisibilityObserver'.

Specification: https://w3c.github.io/sensors

BUG=606766

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

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

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