Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(818)

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

Created:
4 years ago by Mikhail
Modified:
4 years ago
CC:
blink-reviews, chromium-reviews, tdresser, timvolodine, wanming.lin
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
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 ...
4 years 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 > ...
4 years 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 ...
4 years 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 ...
4 years 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)); ...
4 years 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: ...
4 years 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 ...
4 years ago (2016-12-19 07:07:56 UTC) #42
shalamov
lgtm
4 years 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
4 years 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)
4 years 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 ...
4 years 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: ...
4 years 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
4 years 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)
4 years 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 ...
4 years 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 ...
4 years 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 == ...
4 years 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 ...
4 years 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
4 years ago (2016-12-19 13:55:03 UTC) #66
commit-bot: I haz the power
Committed patchset #6 (id:140001)
4 years 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}
4 years ago (2016-12-19 14:03:46 UTC) #71
wjmaclean
4 years 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