Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(59)

Issue 2551223003: [Sensors] Align sensor reading updates and 'onchange' notification with rAF. (Closed)

Created:
2 years, 6 months ago by Mikhail
Modified:
2 years, 6 months ago
CC:
blink-reviews, chromium-reviews, tdresser, timvolodine, wanming.lin_intel.com
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Comments from haraken@ #

Total comments: 1

Patch Set 3 : Re-implemented, rAF covers all sensors now #

Total comments: 2

Patch Set 4 : Comments from Reilly #

Total comments: 12

Patch Set 5 : Comments from Alex and Reilly #

Patch Set 6 : rebased #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+238 lines, -237 lines) Patch
M third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js View 1 2 1 chunk +4 lines, -1 line 1 comment Download
M third_party/WebKit/Source/modules/sensor/BUILD.gn View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/Sensor.h View 1 2 3 4 chunks +2 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/Sensor.cpp View 1 2 3 4 5 9 chunks +19 lines, -44 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorProxy.h View 1 2 3 4 7 chunks +24 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorProxy.cpp View 1 2 3 15 chunks +32 lines, -54 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.h View 1 2 3 4 1 chunk +39 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/SensorReadingUpdater.cpp View 1 2 3 4 1 chunk +112 lines, -0 lines 0 comments Download
D third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.h View 1 chunk +0 lines, -29 lines 0 comments Download
D third_party/WebKit/Source/modules/sensor/SensorUpdateNotificationStrategy.cpp View 1 chunk +0 lines, -85 lines 0 comments Download

Messages

Total messages: 72 (43 generated)
Mikhail
PTAL
2 years, 6 months ago (2016-12-05 21:39:57 UTC) #10
haraken
https://codereview.chromium.org/2551223003/diff/20001/third_party/WebKit/Source/modules/sensor/SensorProxy.h File third_party/WebKit/Source/modules/sensor/SensorProxy.h (right): https://codereview.chromium.org/2551223003/diff/20001/third_party/WebKit/Source/modules/sensor/SensorProxy.h#newcode144 third_party/WebKit/Source/modules/sensor/SensorProxy.h:144: WeakMember<Document> m_document; Is there any reason you want to ...
2 years, 6 months ago (2016-12-06 02:38:30 UTC) #13
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp File third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp (right): https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp#newcode60 third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp:60: m_timer.startOneShot(waitingTime, BLINK_FROM_HERE); Can we also use RAF here to ...
2 years, 6 months ago (2016-12-08 00:56:57 UTC) #18
Mikhail
On 2016/12/08 00:56:57, Reilly Grant wrote: > https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp > File third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp (right): > > https://codereview.chromium.org/2551223003/diff/40001/third_party/WebKit/Source/modules/sensor/SensorUpdateNotifier.cpp#newcode60 ...
2 years, 6 months ago (2016-12-15 14:17:40 UTC) #22
Reilly Grant (use Gerrit)
Adding foolip@ to check my reasoning here but after doing a bit more research I ...
2 years, 6 months ago (2016-12-16 00:48:56 UTC) #26
foolip
On 2016/12/16 00:48:56, Reilly Grant wrote: > Adding foolip@ to check my reasoning here but ...
2 years, 6 months ago (2016-12-16 10:24:15 UTC) #28
Mikhail
On 2016/12/16 10:24:15, foolip wrote: > On 2016/12/16 00:48:56, Reilly Grant wrote: > > Adding ...
2 years, 6 months ago (2016-12-16 11:22:57 UTC) #29
foolip
On 2016/12/16 11:22:57, Mikhail wrote: > On 2016/12/16 10:24:15, foolip wrote: > > On 2016/12/16 ...
2 years, 6 months ago (2016-12-16 12:30:56 UTC) #30
Mikhail
On 2016/12/16 12:30:56, foolip wrote: > I see that https://w3c.github.io/sensors/#sensor-onchange says just "onchange is > ...
2 years, 6 months ago (2016-12-16 12:47:48 UTC) #31
foolip
On 2016/12/16 12:47:48, Mikhail wrote: > On 2016/12/16 12:30:56, foolip wrote: > > I see ...
2 years, 6 months ago (2016-12-16 12:57:56 UTC) #32
Mikhail
On 2016/12/16 00:48:56, Reilly Grant wrote: > Adding foolip@ to check my reasoning here but ...
2 years, 6 months ago (2016-12-16 13:29:04 UTC) #35
shalamov
https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp#newcode284 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)); ...
2 years, 6 months ago (2016-12-16 17:10:49 UTC) #38
Reilly Grant (use Gerrit)
lgtm with nits https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp#newcode284 third_party/WebKit/Source/modules/sensor/Sensor.cpp:284: dispatchEvent(Event::create(EventTypeNames::change)); On 2016/12/16 17:10:48, shalamov wrote: ...
2 years, 6 months ago (2016-12-16 22:09:37 UTC) #39
Mikhail
https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp File third_party/WebKit/Source/modules/sensor/Sensor.cpp (right): https://codereview.chromium.org/2551223003/diff/100001/third_party/WebKit/Source/modules/sensor/Sensor.cpp#newcode284 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 ...
2 years, 6 months ago (2016-12-19 07:07:56 UTC) #42
shalamov
lgtm
2 years, 6 months ago (2016-12-19 07:24:31 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2551223003/120001
2 years, 6 months ago (2016-12-19 09:21:06 UTC) #48
commit-bot: I haz the power
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_presubmit/builds/328783)
2 years, 6 months ago (2016-12-19 09:26:41 UTC) #50
haraken
On 2016/12/19 09:26:41, commit-bot: I haz the power wrote: > Try jobs failed on following ...
2 years, 6 months ago (2016-12-19 09:28:04 UTC) #51
Mikhail
On 2016/12/19 09:28:04, haraken wrote: > On 2016/12/19 09:26:41, commit-bot: I haz the power wrote: ...
2 years, 6 months ago (2016-12-19 09:42:26 UTC) #52
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2551223003/120001
2 years, 6 months ago (2016-12-19 09:42:41 UTC) #54
commit-bot: I haz the power
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_presubmit/builds/328791)
2 years, 6 months ago (2016-12-19 09:48:17 UTC) #56
foolip
Unchecked CQ because I see no updated tests in this CL. It's not intended to ...
2 years, 6 months ago (2016-12-19 12:19:34 UTC) #60
Mikhail
On 2016/12/19 12:19:34, foolip wrote: > Unchecked CQ because I see no updated tests in ...
2 years, 6 months ago (2016-12-19 12:49:36 UTC) #61
foolip
Sorry, I overlooked the actual test change. https://codereview.chromium.org/2551223003/diff/140001/third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js File third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js (right): https://codereview.chromium.org/2551223003/diff/140001/third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js#newcode310 third_party/WebKit/LayoutTests/sensor/resources/generic-sensor-tests.js:310: fastSensorNotifiedCounter == ...
2 years, 6 months ago (2016-12-19 13:23:47 UTC) #62
foolip
After discussion with Mikhail on IRC it seems like it should be possible to test ...
2 years, 6 months ago (2016-12-19 13:33:37 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2551223003/140001
2 years, 6 months ago (2016-12-19 13:55:03 UTC) #66
commit-bot: I haz the power
Committed patchset #6 (id:140001)
2 years, 6 months ago (2016-12-19 14:00:11 UTC) #69
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/6b071fe7dc3bd64a2914eadd5c67b483d064a6cb Cr-Commit-Position: refs/heads/master@{#439467}
2 years, 6 months ago (2016-12-19 14:03:46 UTC) #71
wjmaclean
2 years, 6 months ago (2016-12-19 16:25:41 UTC) #72
Message was sent while issue was closed.
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....

Powered by Google App Engine
This is Rietveld 408576698