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

Issue 2332323002: [sensors] Ambient light sensor bindings implementation (Closed)

Created:
4 years, 3 months ago by shalamov
Modified:
4 years, 3 months ago
CC:
blink-reviews, chromium-reviews, haraken
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[sensors] Ambient light sensor bindings implementation This patch implements AmbientLightSensor [1] blink bindings and adds LayoutTest helpers for testing sensors that are based on Generic Sensor API. Following layout tests added to test new functionality: IDL tests. - third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html - third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensorReading.html AmbientLightSensor tests. - third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xLGN2b1-AAAJ [1] ED specification for Ambient Light Sensor http://w3c.github.io/ambient-light/ BUG=606766 Committed: https://crrev.com/c457d06894e8f01b24ae83750203fb5d8819c560 Cr-Commit-Position: refs/heads/master@{#419438}

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fixes for comments from Haraken #

Total comments: 2

Patch Set 3 : Fixes for comments from Riju #

Patch Set 4 : Rebase and improve tests #

Patch Set 5 : Add AmbientLightSensor to the list of expected global interfaces #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+770 lines, -2 lines) Patch
A third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html View 1 2 3 1 chunk +189 lines, -0 lines 1 comment Download
A third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html View 1 chunk +9 lines, -0 lines 2 comments Download
A third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensorReading.html View 1 chunk +9 lines, -0 lines 1 comment Download
A third_party/WebKit/LayoutTests/sensor/mock-sensor.html View 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js View 1 2 3 1 chunk +302 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/modules_idl_files.gni View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h View 1 2 3 4 1 chunk +34 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp View 1 1 chunk +54 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensor.idl View 1 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.h View 1 1 chunk +48 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.cpp View 1 1 chunk +47 lines, -0 lines 1 comment Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.idl View 1 chunk +13 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/sensor/AmbientLightSensorReadingInit.idl View 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/BUILD.gn View 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorReading.h View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/sensor/SensorReading.cpp View 1 2 chunks +9 lines, -1 line 0 comments Download

Messages

Total messages: 40 (22 generated)
shalamov
Could you please take a look. Thanks.
4 years, 3 months ago (2016-09-13 09:30:36 UTC) #2
haraken
In terms of implementation LGTM https://codereview.chromium.org/2332323002/diff/1/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp (right): https://codereview.chromium.org/2332323002/diff/1/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp#newcode19 third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp:19: return sensor; return new ...
4 years, 3 months ago (2016-09-13 09:45:04 UTC) #3
shalamov
https://codereview.chromium.org/2332323002/diff/1/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp (right): https://codereview.chromium.org/2332323002/diff/1/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp#newcode19 third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp:19: return sensor; On 2016/09/13 09:45:04, haraken wrote: > > ...
4 years, 3 months ago (2016-09-13 10:34:17 UTC) #4
riju_
https://codereview.chromium.org/2332323002/diff/20001/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h (right): https://codereview.chromium.org/2332323002/diff/20001/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h#newcode9 third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h:9: #include "platform/heap/Handle.h" maybe not needed
4 years, 3 months ago (2016-09-13 11:06:53 UTC) #5
shalamov
https://codereview.chromium.org/2332323002/diff/20001/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h File third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h (right): https://codereview.chromium.org/2332323002/diff/20001/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h#newcode9 third_party/WebKit/Source/modules/sensor/AmbientLightSensor.h:9: #include "platform/heap/Handle.h" On 2016/09/13 11:06:53, riju_ wrote: > maybe ...
4 years, 3 months ago (2016-09-13 11:17:10 UTC) #6
jochen (gone - plz use gerrit)
can you add a link to the intent to implement to the CL Description please? ...
4 years, 3 months ago (2016-09-14 08:28:14 UTC) #11
Mikhail
lgtm
4 years, 3 months ago (2016-09-16 06:27:47 UTC) #17
shalamov
timvolodine@ do you have any comments for this CL?
4 years, 3 months ago (2016-09-16 09:58:03 UTC) #22
shalamov
On 2016/09/16 09:58:03, shalamov wrote: > timvolodine@ do you have any comments for this CL? ...
4 years, 3 months ago (2016-09-19 07:15:57 UTC) #23
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/2332323002/80001
4 years, 3 months ago (2016-09-19 07:16:17 UTC) #26
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/32651)
4 years, 3 months ago (2016-09-19 08:37:17 UTC) #28
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/2332323002/80001
4 years, 3 months ago (2016-09-19 09:31:52 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/32665)
4 years, 3 months ago (2016-09-19 10:31:34 UTC) #32
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/2332323002/80001
4 years, 3 months ago (2016-09-19 12:06:17 UTC) #34
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 3 months ago (2016-09-19 12:33:06 UTC) #36
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/c457d06894e8f01b24ae83750203fb5d8819c560 Cr-Commit-Position: refs/heads/master@{#419438}
4 years, 3 months ago (2016-09-19 12:34:58 UTC) #38
iclelland
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2348333002/ by iclelland@chromium.org. ...
4 years, 3 months ago (2016-09-19 14:41:10 UTC) #39
timvolodine
4 years, 3 months ago (2016-09-19 16:37:32 UTC) #40
Message was sent while issue was closed.
looks good thanks! impressive 'mocking' :). just some thoughts, for this or
follow-up patches

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html (right):

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html:188: 
Just a couple of ideas for future consideration:
- seems like nice to add some 'classical' layout test at some point. e.g.
DeviceOrientation/window-property.html or
WebKit/LayoutTests/battery-status/api-defined.html
- maybe something trivial that shows how to use the API (thinking of e.g.
WebKit/LayoutTests/battery-status/promise-with-eventlisteners.html). I
understand it's covered by the tests here but seem a bit harder to read.
- is there a test for special cases e.g. if there is no sensor, or when onerror
is expected..?

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html (right):

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html:1: <!DOCTYPE
html>
no expectations txt files anywhere?

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensor.html:7:
assert_true(new AmbientLightSensor() instanceof AmbientLightSensor);
nit: or as alternative maybe AmbientLightSensor in window

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensorReading.html
(right):

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/sensor/idl-AmbientLightSensorReading.html:6: //
Test that AmbientLightSensorReading interface exists
you could put this test in idl-AmbientLightSensor.html, they are very similar

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.cpp
(right):

https://codereview.chromium.org/2332323002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/sensor/AmbientLightSensorReading.cpp:31:
return 0.0;
is this according to spec?

Powered by Google App Engine
This is Rietveld 408576698