[Sensors] Decouple sensor readings update from rAF

[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
Review-Url: https://codereview.chromium.org/2870073002
Cr-Commit-Position: refs/heads/master@{#471726}
Committed: https://chromium.googlesource.com/chromium/src/+/d0458b1c5e117db5cb8b63688ef39af43fd4fc97

[Mikhail, 2017-05-09 13:20:34]

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

[commit-bot: I haz the power, 2017-05-09 13:21:02]

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, 2017-05-09 18:23:58]

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

[commit-bot: I haz the power, 2017-05-09 18:23:59]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2017-05-10 09:33:46]

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

[commit-bot: I haz the power, 2017-05-10 09:34:18]

Dry run: CQ is trying da patch.

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

[Mikhail, 2017-05-10 09:38:36]

Description was changed from

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
BUG=606766
==========

to

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
BUG=606766
==========

[Mikhail, 2017-05-10 09:38:37]

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

[Mikhail, 2017-05-10 09:42:30]

PTAL

[shalamov, 2017-05-10 10:56:21]

lgtm

[commit-bot: I haz the power, 2017-05-10 11:32:49]

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

[commit-bot: I haz the power, 2017-05-10 11:32:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Reilly Grant (use Gerrit), 2017-05-10 14:53:28]

Remove BUG=606766 from the issue description. This resolves a specific issue and
that ALS launch bug is getting unreasonably long.

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
->PostTask(BLINK_FROM_HERE, std::move(sensor_reading_changed));
Why not call UpdateReading directly here?

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:274: // polling
frequency.
Please create an issue for this.

[Mikhail, 2017-05-11 08:26:39]

Description was changed from

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
BUG=606766
==========

to

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
==========

[Mikhail, 2017-05-11 10:46:20]

Thanks for your comments!

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
->PostTask(BLINK_FROM_HERE, std::move(sensor_reading_changed));
On 2017/05/10 14:53:28, Reilly Grant wrote:
> Why not call UpdateReading directly here?

It would synchronously invoke JS callbacks that might cause problems (like
modifying SensorProxy::observers_ container while it is being iterated through)

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:274: // polling
frequency.
On 2017/05/10 14:53:28, Reilly Grant wrote:
> Please create an issue for this.

Done https://bugs.chromium.org/p/chromium/issues/detail?id=721297

[Reilly Grant (use Gerrit), 2017-05-11 18:28:12]

lgtm with comments

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
->PostTask(BLINK_FROM_HERE, std::move(sensor_reading_changed));
On 2017/05/11 10:46:20, Mikhail wrote:
> On 2017/05/10 14:53:28, Reilly Grant wrote:
> > Why not call UpdateReading directly here?
> 
> It would synchronously invoke JS callbacks that might cause problems (like
> modifying SensorProxy::observers_ container while it is being iterated
through)

Can you add a comment explaining that here? I wish we had base::ObserverList in
Blink.

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:127: auto now =
WTF::MonotonicallyIncreasingTime();
nit: use double here as the type of the return value is not clear from the
function name.

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:274: // polling
frequency.
On 2017/05/11 10:46:20, Mikhail wrote:
> On 2017/05/10 14:53:28, Reilly Grant wrote:
> > Please create an issue for this.
> 
> Done https://bugs.chromium.org/p/chromium/issues/detail?id=721297

Please update the comment with the issue number. That is preferable to putting
people's names in the code.

[Mikhail, 2017-05-12 08:42:00]

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

[Mikhail, 2017-05-12 08:42:13]

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:197:
->PostTask(BLINK_FROM_HERE, std::move(sensor_reading_changed));
On 2017/05/11 18:28:12, Reilly Grant wrote:
> On 2017/05/11 10:46:20, Mikhail wrote:
> > On 2017/05/10 14:53:28, Reilly Grant wrote:
> > > Why not call UpdateReading directly here?
> > 
> > It would synchronously invoke JS callbacks that might cause problems (like
> > modifying SensorProxy::observers_ container while it is being iterated
> through)
> 
> Can you add a comment explaining that here? I wish we had base::ObserverList
in
> Blink.

Done.

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

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:127: auto now =
WTF::MonotonicallyIncreasingTime();
On 2017/05/11 18:28:12, Reilly Grant wrote:
> nit: use double here as the type of the return value is not clear from the
> function name.

Done.

https://codereview.chromium.org/2870073002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.cpp:274: // polling
frequency.
On 2017/05/11 18:28:12, Reilly Grant wrote:
> On 2017/05/11 10:46:20, Mikhail wrote:
> > On 2017/05/10 14:53:28, Reilly Grant wrote:
> > > Please create an issue for this.
> > 
> > Done https://bugs.chromium.org/p/chromium/issues/detail?id=721297
> 
> Please update the comment with the issue number. That is preferable to putting
> people's names in the code.

Done.

[commit-bot: I haz the power, 2017-05-12 08:42:28]

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, 2017-05-12 08:56:30]

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

[commit-bot: I haz the power, 2017-05-12 08:56:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mikhail, 2017-05-12 10:16:41]

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

[commit-bot: I haz the power, 2017-05-12 10:16:56]

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, 2017-05-12 10:26:33]

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

[commit-bot: I haz the power, 2017-05-12 10:26:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mikhail, 2017-05-12 12:16:43]

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

[commit-bot: I haz the power, 2017-05-12 12:17:03]

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, 2017-05-12 13:29:31]

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

[commit-bot: I haz the power, 2017-05-12 13:29:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mikhail, 2017-05-15 09:07:19]

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

[commit-bot: I haz the power, 2017-05-15 09:07:31]

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, 2017-05-15 09:16:46]

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

[commit-bot: I haz the power, 2017-05-15 09:16:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mikhail, 2017-05-15 09:28:38]

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

[commit-bot: I haz the power, 2017-05-15 09:28:50]

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, 2017-05-15 11:32:49]

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

[commit-bot: I haz the power, 2017-05-15 11:32:49]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2017-05-15 11:40:47]

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

[Mikhail, 2017-05-15 11:40:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
reillyg@chromium.org, alexander.shalamov@intel.com
Link to the patchset: https://codereview.chromium.org/2870073002/#ps100001
(title: "[Sensors] Decouple sensor readings update from rAF")

[commit-bot: I haz the power, 2017-05-15 11:41:00]

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, 2017-05-15 11:46:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494848447513590,
"parent_rev": "9f5f6b9548bc4cad0c64d3aa2b5b58ae8dad595b", "commit_rev":
"d0458b1c5e117db5cb8b63688ef39af43fd4fc97"}

[commit-bot: I haz the power, 2017-05-15 11:47:04]

Description was changed from

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872
==========

to

==========
[Sensors] Decouple sensor readings update from rAF

Before this change the implementation was using
Document::EnqueueAnimationFrameTask() which caused runs of the whole rendering
pipeline at up to 60Hz whether or not the sensor readings have been updated.

BUG=715872

Review-Url: https://codereview.chromium.org/2870073002
Cr-Commit-Position: refs/heads/master@{#471726}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0458b1c5e117db5cb8b63688ef3...
==========

[commit-bot: I haz the power, 2017-05-15 11:47:06]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d0458b1c5e117db5cb8b63688ef3...