[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and battery.

BUG=668052
BUG=606766
Committed: https://crrev.com/6b071fe7dc3bd64a2914eadd5c67b483d064a6cb
Cr-Commit-Position: refs/heads/master@{#439467}

[Mikhail, 2016-12-05 21:08:25]

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

[commit-bot: I haz the power, 2016-12-05 21:08:59]

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

[Mikhail, 2016-12-05 21:14:33]

Description was changed from

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For sensors with periodic reporting mode (i.e. frequently updated sensors) new
reading values are read and 'onchange' notfication is send from rAF callbacks,
thus avoiding possible Critical Rendering Path interruption.

Before this change a periodic timer was used and this could unnecessarily drain
CPU and battery.

BUG=668052
BUG=606766
==========

to

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For sensors with periodic reporting mode (i.e. frequently updated sensors) new
reading values are read and 'onchange' notfication is send from rAF callbacks,
thus avoiding possible Critical Rendering Path interruption.

Before this change a periodic timer was used and this could unnecessarily drain
CPU and battery.

BUG=668052
BUG=606766
==========

[Mikhail, 2016-12-05 21:14:33]

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

[commit-bot: I haz the power, 2016-12-05 21:33:11]

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

[commit-bot: I haz the power, 2016-12-05 21:33:11]

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

[Mikhail, 2016-12-05 21:37:56]

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

[commit-bot: I haz the power, 2016-12-05 21:38:34]

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

[Mikhail, 2016-12-05 21:39:18]

Patchset #1 (id:1) has been deleted

[Mikhail, 2016-12-05 21:39:57]

PTAL

[commit-bot: I haz the power, 2016-12-05 23:24:15]

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

[commit-bot: I haz the power, 2016-12-05 23:24:16]

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_...)

[haraken, 2016-12-06 02:38:30]

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

https://codereview.chromium.org/2551223003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:144: WeakMember<Document>
m_document;

Is there any reason you want to make it a WeakMember? WeakMember looks okay but
we prefer using a normal Member unless we have a reason to make it weak (because
WeakMember adds some overhead).

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

https://codereview.chromium.org/2551223003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp:92:
std::unique_ptr<Function<void()>> func,

func => sensorUpdatingFunction

[Mikhail, 2016-12-07 12:05:56]

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

[commit-bot: I haz the power, 2016-12-07 12:06:23]

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-12-07 13:55:52]

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

[commit-bot: I haz the power, 2016-12-07 13:55:53]

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), 2016-12-08 00:56:57]

https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp (right):

https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp:60:
m_timer.startOneShot(waitingTime, BLINK_FROM_HERE);
Can we also use RAF here to deliver events no more frequently than once per
frame and avoid the timer?

[Mikhail, 2016-12-15 14:12:42]

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

[commit-bot: I haz the power, 2016-12-15 14:12:57]

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

[Mikhail, 2016-12-15 14:16:03]

Description was changed from

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For sensors with periodic reporting mode (i.e. frequently updated sensors) new
reading values are read and 'onchange' notfication is send from rAF callbacks,
thus avoiding possible Critical Rendering Path interruption.

Before this change a periodic timer was used and this could unnecessarily drain
CPU and battery.

BUG=668052
BUG=606766
==========

to

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send
from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and
battery.

BUG=668052
BUG=606766
==========

[Mikhail, 2016-12-15 14:17:40]

On 2016/12/08 00:56:57, Reilly Grant wrote:
>
https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp
(right):
> 
>
https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp:60:
> m_timer.startOneShot(waitingTime, BLINK_FROM_HERE);
> Can we also use RAF here to deliver events no more frequently than once per
> frame and avoid the timer?

Done.

The new PS has quite a different approach that covers 'onchange' sensors too.
PTAL.

[commit-bot: I haz the power, 2016-12-15 16:17:11]

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

[commit-bot: I haz the power, 2016-12-15 16:17:12]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2016-12-16 00:48:55]

reillyg@chromium.org changed reviewers:
+ foolip@chromium.org

[Reilly Grant (use Gerrit), 2016-12-16 00:48:56]

Adding foolip@ to check my reasoning here but after doing a bit more research I
think we should be using Document::requestAnimationFrameTask instead of
requestAnimationFrame because we want to deliver these events before
requestAnimationFrame callbacks registered by script are executed.

I also looked into using enqueueAnimationFrameEvent but since we need to do
computation every frame to determine whether it is the right time to dispatch
the event I think a task is more appropriate.

https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp (right):

https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:13: using
ReportingMode = device::mojom::blink::ReportingMode;
Move outside of the namespace blink block and change to:

using device::mojom::blink::ReportingMode;

https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:81:
m_sensorProxy->notifySensorChanged(timestampSec);
Should we be calling notifySensorChanged() twice here? I think this first call
is redundant.

[Mikhail, 2016-12-16 08:13:55]

Patchset #4 (id:80001) has been deleted

[foolip, 2016-12-16 10:24:15]

On 2016/12/16 00:48:56, Reilly Grant wrote:
> Adding foolip@ to check my reasoning here but after doing a bit more research
I
> think we should be using Document::requestAnimationFrameTask instead of
> requestAnimationFrame because we want to deliver these events before
> requestAnimationFrame callbacks registered by script are executed.
> 
> I also looked into using enqueueAnimationFrameEvent but since we need to do
> computation every frame to determine whether it is the right time to dispatch
> the event I think a task is more appropriate.

I recently introduced Document::enqueueAnimationFrameTask which I will use for
Fullscreen. There I also have to update some state and fire events at the same
time, so maybe it's what you need?

Does the spec for this say anything useful? It should probably use language (and
red warnings) similar to https://fullscreen.spec.whatwg.org/

[Mikhail, 2016-12-16 11:22:57]

On 2016/12/16 10:24:15, foolip wrote:
> On 2016/12/16 00:48:56, Reilly Grant wrote:
> > Adding foolip@ to check my reasoning here but after doing a bit more
research
> I
> > think we should be using Document::requestAnimationFrameTask instead of
> > requestAnimationFrame because we want to deliver these events before
> > requestAnimationFrame callbacks registered by script are executed.
> > 
> > I also looked into using enqueueAnimationFrameEvent but since we need to do
> > computation every frame to determine whether it is the right time to
dispatch
> > the event I think a task is more appropriate.
> 
> I recently introduced Document::enqueueAnimationFrameTask which I will use for
> Fullscreen. There I also have to update some state and fire events at the same
> time, so maybe it's what you need?

I think this method indeed fits better (thank you for implementing and thanks
Reilly for his proposal to use it here!), so that we can update sensor reading
before any script-scheduled callbacks are executed and it's still aligned with
Critical Rendering Path.
> 
> Does the spec for this say anything useful? It should probably use language
(and
> red warnings) similar to https://fullscreen.spec.whatwg.org/

Generic Sensor spec does not expose any 'request..' API, so I see this change
just as an implementation detail/optimization for 'onchange' event firing.

[foolip, 2016-12-16 12:30:56]

On 2016/12/16 11:22:57, Mikhail wrote:
> On 2016/12/16 10:24:15, foolip wrote:
> > On 2016/12/16 00:48:56, Reilly Grant wrote:
> > > Adding foolip@ to check my reasoning here but after doing a bit more
> research
> > I
> > > think we should be using Document::requestAnimationFrameTask instead of
> > > requestAnimationFrame because we want to deliver these events before
> > > requestAnimationFrame callbacks registered by script are executed.
> > > 
> > > I also looked into using enqueueAnimationFrameEvent but since we need to
do
> > > computation every frame to determine whether it is the right time to
> dispatch
> > > the event I think a task is more appropriate.
> > 
> > I recently introduced Document::enqueueAnimationFrameTask which I will use
for
> > Fullscreen. There I also have to update some state and fire events at the
same
> > time, so maybe it's what you need?
> 
> I think this method indeed fits better (thank you for implementing and thanks
> Reilly for his proposal to use it here!), so that we can update sensor reading
> before any script-scheduled callbacks are executed and it's still aligned with
> Critical Rendering Path.
> > 
> > Does the spec for this say anything useful? It should probably use language
> (and
> > red warnings) similar to https://fullscreen.spec.whatwg.org/
> 
> Generic Sensor spec does not expose any 'request..' API, so I see this change
> just as an implementation detail/optimization for 'onchange' event firing.

I see that https://w3c.github.io/sensors/#sensor-onchange says just "onchange is
an EventHandler which is called whenever a new reading is available." If that is
all that the spec says, then it's not enough to actually write strict tests from
or achieve interoperable implementations. Can you open a spec issue requesting
that the precise timing of when state changes and events are fired is defined?

[Mikhail, 2016-12-16 12:47:48]

On 2016/12/16 12:30:56, foolip wrote:
> I see that https://w3c.github.io/sensors/#sensor-onchange says just "onchange
is
> an EventHandler which is called whenever a new reading is available." If that
is
> all that the spec says, then it's not enough to actually write strict tests
from
> or achieve interoperable implementations. Can you open a spec issue requesting
> that the precise timing of when state changes and events are fired is defined?

There is an issue proposing some improvements to 'onchange' event behavior:
https://github.com/w3c/sensors/issues/152
(https://github.com/w3c/sensors/issues/152#issuecomment-259957946)
Do you think it covers the problem you described or is it worth creating a
separate issue?

[foolip, 2016-12-16 12:57:56]

On 2016/12/16 12:47:48, Mikhail wrote:
> On 2016/12/16 12:30:56, foolip wrote:
> > I see that https://w3c.github.io/sensors/#sensor-onchange says just
"onchange
> is
> > an EventHandler which is called whenever a new reading is available." If
that
> is
> > all that the spec says, then it's not enough to actually write strict tests
> from
> > or achieve interoperable implementations. Can you open a spec issue
requesting
> > that the precise timing of when state changes and events are fired is
defined?
> 
> There is an issue proposing some improvements to 'onchange' event behavior:
> https://github.com/w3c/sensors/issues/152
> (https://github.com/w3c/sensors/issues/152#issuecomment-259957946)
> Do you think it covers the problem you described or is it worth creating a
> separate issue?

I'll comment on that issue.

[Mikhail, 2016-12-16 13:27:00]

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

[commit-bot: I haz the power, 2016-12-16 13:27: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-12-16 13:29:04]

On 2016/12/16 00:48:56, Reilly Grant wrote:
> Adding foolip@ to check my reasoning here but after doing a bit more research
I
> think we should be using Document::requestAnimationFrameTask instead of
> requestAnimationFrame because we want to deliver these events before
> requestAnimationFrame callbacks registered by script are executed.
> 
> I also looked into using enqueueAnimationFrameEvent but since we need to do
> computation every frame to determine whether it is the right time to dispatch
> the event I think a task is more appropriate.

Done. Thanks!
> 
>
https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp
(right):
> 
>
https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:13: using
> ReportingMode = device::mojom::blink::ReportingMode;
> Move outside of the namespace blink block and change to:
> 
> using device::mojom::blink::ReportingMode;

Done.
> 
>
https://codereview.chromium.org/2551223003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp:81:
> m_sensorProxy->notifySensorChanged(timestampSec);
> Should we be calling notifySensorChanged() twice here? I think this first call
> is redundant.

Done.

[commit-bot: I haz the power, 2016-12-16 15:22:58]

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

[commit-bot: I haz the power, 2016-12-16 15:22:59]

Dry run: This issue passed the CQ dry run.

[shalamov, 2016-12-16 17:10:49]

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:284:
dispatchEvent(Event::create(EventTypeNames::change));
should it be reversed?

m_storedData = m_sensorProxy->sensorReading()->data();
dispatchEvent(Event::create(EventTypeNames::change));

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:15: #include
"platform/Timer.h"
I think this can be removed now.

[Reilly Grant (use Gerrit), 2016-12-16 22:09:37]

lgtm with nits

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:284:
dispatchEvent(Event::create(EventTypeNames::change));
On 2016/12/16 17:10:48, shalamov wrote:
> should it be reversed?
> 
> m_storedData = m_sensorProxy->sensorReading()->data();
> dispatchEvent(Event::create(EventTypeNames::change));

If this data were used by Sensor::reading() then it would matter. At the moment
I don't think it does but I still would favor the reversed logic.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:28: virtual void
handleAnimationFrameTask() = 0;
nit: onAnimationFrameInternal

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:31: bool
m_pendingAnimationFrameTask;
nit: m_hasPendingAnimationFrameTask

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:34: void
onAnimationFrameTask();
nit: onAnimationFrame

[Mikhail, 2016-12-19 07:07:36]

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

[commit-bot: I haz the power, 2016-12-19 07:07:45]

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

[Mikhail, 2016-12-19 07:07:56]

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:284:
dispatchEvent(Event::create(EventTypeNames::change));
On 2016/12/16 22:09:37, Reilly Grant wrote:
> On 2016/12/16 17:10:48, shalamov wrote:
> > should it be reversed?
> > 
> > m_storedData = m_sensorProxy->sensorReading()->data();
> > dispatchEvent(Event::create(EventTypeNames::change));
> 
> If this data were used by Sensor::reading() then it would matter. At the
moment
> I don't think it does but I still would favor the reversed logic.

Done.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/Sensor.cpp:284:
dispatchEvent(Event::create(EventTypeNames::change));
On 2016/12/16 17:10:48, shalamov wrote:
> should it be reversed?
> 
> m_storedData = m_sensorProxy->sensorReading()->data();
> dispatchEvent(Event::create(EventTypeNames::change));

Done.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorProxy.h:15: #include
"platform/Timer.h"
On 2016/12/16 17:10:48, shalamov wrote:
> I think this can be removed now.

Done.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h (right):

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:28: virtual void
handleAnimationFrameTask() = 0;
On 2016/12/16 22:09:37, Reilly Grant wrote:
> nit: onAnimationFrameInternal

Done.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:31: bool
m_pendingAnimationFrameTask;
On 2016/12/16 22:09:37, Reilly Grant wrote:
> nit: m_hasPendingAnimationFrameTask

Done.

https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h:34: void
onAnimationFrameTask();
On 2016/12/16 22:09:37, Reilly Grant wrote:
> nit: onAnimationFrame

Done.

[shalamov, 2016-12-19 07:24:31]

lgtm

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

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

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

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-12-19 09:20:51]

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

[Mikhail, 2016-12-19 09:20:51]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2551223003/#ps120001
(title: "Comments from Alex and Reilly")

[commit-bot: I haz the power, 2016-12-19 09:21:06]

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-12-19 09:26:40]

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

[commit-bot: I haz the power, 2016-12-19 09:26:41]

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...)

[haraken, 2016-12-19 09:28:04]

On 2016/12/19 09:26:41, commit-bot: I haz the power wrote:
> 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...)

owner LGTM

[Mikhail, 2016-12-19 09:42:26]

On 2016/12/19 09:28:04, haraken wrote:
> On 2016/12/19 09:26:41, commit-bot: I haz the power wrote:
> > 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...)
> 
> owner LGTM

thanks!

[Mikhail, 2016-12-19 09:42:32]

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

[commit-bot: I haz the power, 2016-12-19 09:42:41]

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-12-19 09:48:16]

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

[commit-bot: I haz the power, 2016-12-19 09:48:17]

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

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

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

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

[foolip, 2016-12-19 12:18:45]

The CQ bit was unchecked by foolip@chromium.org

[foolip, 2016-12-19 12:19:34]

Unchecked CQ because I see no updated tests in this CL. It's not intended to be
refactoring only, so if tests aren't needed, please elaborate in the
description.

[Mikhail, 2016-12-19 12:49:36]

On 2016/12/19 12:19:34, foolip wrote:
> Unchecked CQ because I see no updated tests in this CL. It's not intended to
be
> refactoring only, so if tests aren't needed, please elaborate in the
> description.

From user perspective it should not bring much difference comparing to the
existing approach with timers, except that notification comes not that precisely
in terms of Hz (which is fine, since the given frequency is just a "hint").
There are already existing layout tests for 'frequency hint' functionality,
their expectations are updated in this CL
https://codereview.chromium.org/2551223003/diff/140001/third_party/WebKit/Lay...
.

[foolip, 2016-12-19 13:23:47]

Sorry, I overlooked the actual test change.

https://codereview.chromium.org/2551223003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js
(right):

https://codereview.chromium.org/2551223003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js:310:
fastSensorNotifiedCounter == 4);
Shouldn't this change have caused the number of update events to decrease only?

A useful test for this change would be a 100 Hz sensor, and verifying that at
most one event is fired per animation frame, if that's the intended behavior.

[foolip, 2016-12-19 13:33:37]

After discussion with Mikhail on IRC it seems like it should be possible to test
this in more detail. The spec really should define the behavior here, with no
need for interpretation. Tests in this CL or as a follow-up is fine.

[Mikhail, 2016-12-19 13:54:51]

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

[Mikhail, 2016-12-19 13:54:52]

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

[commit-bot: I haz the power, 2016-12-19 13:55:03]

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-12-19 13:59:43]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482155691995040,
"parent_rev": "22f58f166d4105564423326f7d6f26abb2d52d5c", "commit_rev":
"1258768cf336031e00c904ff6f129c43a9264130"}

[commit-bot: I haz the power, 2016-12-19 14:00:10]

Description was changed from

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send
from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and
battery.

BUG=668052
BUG=606766
==========

to

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send
from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and
battery.

BUG=668052
BUG=606766

Review-Url: https://codereview.chromium.org/2551223003
==========

[commit-bot: I haz the power, 2016-12-19 14:00:11]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-19 14:03:45]

Description was changed from

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send
from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and
battery.

BUG=668052
BUG=606766

Review-Url: https://codereview.chromium.org/2551223003
==========

to

==========
[Sensors] Align sensor reading updates and 'onchange' notification with rAF.

For all sensors new reading values are read and 'onchange' notfication is send
from rAF callbacks, thus avoiding possible Critical Rendering Path interruption.

Before this change a timers were used and this could unnecessarily drain CPU and
battery.

BUG=668052
BUG=606766

Committed: https://crrev.com/6b071fe7dc3bd64a2914eadd5c67b483d064a6cb
Cr-Commit-Position: refs/heads/master@{#439467}
==========

[commit-bot: I haz the power, 2016-12-19 14:03:46]

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

[wjmaclean, 2016-12-19 16:25:41]

A revert of this CL (patchset #6 id:140001) has been created in
https://codereview.chromium.org/2590513002/ by wjmaclean@chromium.org.

The reason for reverting is: Seems like a likely cause for failures on:

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=se....