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

Issue 2332903002: [sensors] [mac] Implement ambient light sensor for macOS (Closed)

Created:
4 years, 3 months ago by darktears
Modified:
4 years, 2 months ago
CC:
shalamov, chromium-reviews, lunalu1, lunalu, maksims (do not use this acc), Mikhail, Reilly Grant (use Gerrit)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[sensors] [mac] Implement ambient light sensor for macOS Use IOKit to get information from the platform and callback when the value of the sensor is changing. Make sure to provide the time stamp as well (which is required by the spec). The data is passed in a shared buffer using seqlock mechanism. The patch also moves around code in content/device_sensors because they are needed by this implementation. This will avoid device/ to depend on content/. The default frequency to pull the ambient light is now inside device/sensors/public/cpp (shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/sensors/public/cpp) as well. Finally the patch also add a smart pointer to handle IONotificationPortRef. There are other opportunities in the chromium codebase to use that new smart pointer, I will land a follow up CL. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xLGN2b1-AAAJ BUG=606766 Committed: https://crrev.com/a5d055a1bd9e2db124cd7e2ef13c827f765f3c38 Cr-Commit-Position: refs/heads/master@{#424479}

Patch Set 1 #

Total comments: 7

Patch Set 2 : [sensors] [mac] Implement ambient light sensor for macOS #

Total comments: 11

Patch Set 3 : Fixes for comments from Mikhail #

Patch Set 4 : Fix gn gen --check #

Patch Set 5 : fix content_unittests link #

Patch Set 6 : More style fixes and build fixes #

Total comments: 6

Patch Set 7 : Fix comments from Mikhail #

Total comments: 8

Patch Set 8 : Remove polling thread and use IOServiceAddInterestNotification #

Total comments: 6

Patch Set 9 : Fixes for comments from Robert #

Patch Set 10 : Fix gn --check #

Total comments: 5

Patch Set 11 : Fixes for Tim comments #

Total comments: 12

Patch Set 12 : Fixes from additional comments #

Total comments: 6

Patch Set 13 : Fixes for Reilly comments #

Patch Set 14 : Address Robert comment #

Total comments: 2

Patch Set 15 : few more nits #

Total comments: 2

Patch Set 16 : Fix Mikhail nits #

Patch Set 17 : Rebase #

Patch Set 18 : Update commit message #

Unified diffs Side-by-side diffs Delta from patch set Stats (+367 lines, -33 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
A base/mac/scoped_ionotificationportref.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +33 lines, -0 lines 0 comments Download
M content/browser/device_sensors/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/device_sensors/data_fetcher_shared_memory_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -22 lines 0 comments Download
M content/renderer/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/device_sensors/device_light_event_pump.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +3 lines, -2 lines 0 comments Download
M device/generic_sensor/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +10 lines, -0 lines 0 comments Download
M device/generic_sensor/DEPS View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_ambient_light_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +66 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_ambient_light_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +127 lines, -0 lines 0 comments Download
A + device/generic_sensor/platform_sensor_provider_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -8 lines 0 comments Download
A device/generic_sensor/platform_sensor_provider_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +46 lines, -0 lines 0 comments Download
M device/sensors/public/cpp/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -0 lines 0 comments Download
A device/sensors/public/cpp/device_sensors_consts.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +15 lines, -0 lines 0 comments Download
A device/sensors/public/cpp/device_util_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +18 lines, -0 lines 0 comments Download
A device/sensors/public/cpp/device_util_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +32 lines, -0 lines 0 comments Download

Messages

Total messages: 124 (69 generated)
darktears
4 years, 3 months ago (2016-09-12 19:14:38 UTC) #1
darktears
On 2016/09/12 19:14:38, darktears wrote: Of course it requires some bits in https://codereview.chromium.org/2284613002/ and the ...
4 years, 3 months ago (2016-09-12 19:15:07 UTC) #2
Mikhail
https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platform_sensor_polling_mac.h File device/generic_sensor/platform_sensor_polling_mac.h (right): https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platform_sensor_polling_mac.h#newcode6 device/generic_sensor/platform_sensor_polling_mac.h:6: #define DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_POLLING_MAC_H_ shouldn't file be named as platform_sensor_polling_thread_mac.h ? ...
4 years, 3 months ago (2016-09-13 09:27:35 UTC) #4
shalamov
https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/DEPS File device/generic_sensor/DEPS (right): https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/DEPS#newcode2 device/generic_sensor/DEPS:2: "+content/common", I think this violates layering, /device cannot depend ...
4 years, 3 months ago (2016-09-13 09:52:34 UTC) #6
darktears
On 2016/09/13 09:52:34, shalamov wrote: > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/DEPS > File device/generic_sensor/DEPS (right): > > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/DEPS#newcode2 > ...
4 years, 3 months ago (2016-09-13 16:00:16 UTC) #7
Mikhail
Looked only at device/generic_sensor/ changes https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode15 device/generic_sensor/platform_sensor_ambient_light_mac.cc:15: struct DeviceLightData { why ...
4 years, 3 months ago (2016-09-16 16:23:54 UTC) #8
darktears
@timvolodine @mikhail : any feedback on the CL? Thanks.
4 years, 3 months ago (2016-09-17 02:31:40 UTC) #27
Mikhail
Looks good overall https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode67 device/generic_sensor/platform_sensor_ambient_light_mac.cc:67: base::Unretained(this))); GetWeakPtr() to solve the potential ...
4 years, 3 months ago (2016-09-19 06:25:18 UTC) #28
darktears
On 2016/09/19 06:25:18, Mikhail wrote: > Looks good overall > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/platform_sensor_ambient_light_mac.cc > File device/generic_sensor/platform_sensor_ambient_light_mac.cc ...
4 years, 3 months ago (2016-09-19 20:00:28 UTC) #33
darktears
@timvolodine : I would appreciate your review. On 2016/09/19 20:00:28, darktears wrote: > On 2016/09/19 ...
4 years, 3 months ago (2016-09-19 23:26:06 UTC) #34
maksims (do not use this acc)
https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/platform_sensor_mac.h File device/generic_sensor/platform_sensor_mac.h (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/platform_sensor_mac.h#newcode17 device/generic_sensor/platform_sensor_mac.h:17: class PlatformSensorMac : public PlatformSensor { On 2016/09/19 06:25:18, ...
4 years, 3 months ago (2016-09-20 09:51:36 UTC) #36
Mikhail
lgtm
4 years, 3 months ago (2016-09-20 09:57:42 UTC) #37
maksims (do not use this acc)
https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode26 device/generic_sensor/platform_sensor_ambient_light_mac.cc:26: current_lux(0) { I think it should be still -1, ...
4 years, 3 months ago (2016-09-20 11:41:15 UTC) #38
Robert Sesek
Drive-by comments: Are you sure polling is required here? I believe you should be able ...
4 years, 3 months ago (2016-09-20 14:25:21 UTC) #40
timvolodine
On 2016/09/19 23:26:06, darktears wrote: > @timvolodine : I would appreciate your review. > ok ...
4 years, 3 months ago (2016-09-20 14:57:48 UTC) #41
pink (ping after 24hrs)
+shrike as FYI. I concur with rsesek that this shouldn't poll. I'm concerned about the ...
4 years, 3 months ago (2016-09-20 15:08:12 UTC) #43
timvolodine
Aside from rsesek@'s and pinkerton@'s comments, some high level considerations below. Can we split this ...
4 years, 3 months ago (2016-09-20 18:05:24 UTC) #44
timvolodine
+cc:lunalu@ : who is also looking into refactoring device_sensors.
4 years, 3 months ago (2016-09-20 18:59:37 UTC) #45
darktears
Hi, Thanks for the review. I will split it into several pieces. On 2016/09/20 18:05:24, ...
4 years, 3 months ago (2016-09-20 22:15:17 UTC) #46
shrike
On 2016/09/20 22:15:17, darktears wrote: > I will look at IOServiceAddInterestNotification and see if I ...
4 years, 3 months ago (2016-09-20 22:51:55 UTC) #47
maksims (do not use this acc)
https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/polling_platform_sensor.cc File device/generic_sensor/polling_platform_sensor.cc (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/polling_platform_sensor.cc#newcode33 device/generic_sensor/polling_platform_sensor.cc:33: timer_.Start(FROM_HERE, base::TimeDelta::FromMicroseconds( You cannot create timer on one thread ...
4 years, 3 months ago (2016-09-21 08:20:15 UTC) #48
darktears
On 2016/09/21 08:20:15, maksims wrote: > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/polling_platform_sensor.cc > File device/generic_sensor/polling_platform_sensor.cc (right): > > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/polling_platform_sensor.cc#newcode33 > ...
4 years, 2 months ago (2016-09-26 21:34:01 UTC) #49
darktears
On 2016/09/26 21:34:01, darktears wrote: > On 2016/09/21 08:20:15, maksims wrote: > > > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/polling_platform_sensor.cc ...
4 years, 2 months ago (2016-09-29 21:27:58 UTC) #51
Robert Sesek
Thanks, this is definitely a big improvement! Is it possible to write a unittest for ...
4 years, 2 months ago (2016-09-29 22:51:27 UTC) #52
darktears
On 2016/09/29 22:51:27, Robert Sesek wrote: > Thanks, this is definitely a big improvement! Thanks ...
4 years, 2 months ago (2016-09-30 17:48:45 UTC) #53
darktears
On 2016/09/30 17:48:45, darktears wrote: > On 2016/09/29 22:51:27, Robert Sesek wrote: > > Thanks, ...
4 years, 2 months ago (2016-09-30 19:57:58 UTC) #60
timvolodine
Didn't go through the generic_sensor/ mac implementation in much detail, but assume the mac experts ...
4 years, 2 months ago (2016-10-03 18:18:06 UTC) #63
darktears
On 2016/10/03 18:18:06, timvolodine wrote: > Didn't go through the generic_sensor/ mac implementation in much ...
4 years, 2 months ago (2016-10-03 21:13:56 UTC) #66
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h File device/base/device_sensors_consts.h (right): https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h#newcode11 device/base/device_sensors_consts.h:11: const int kDefaultAmbientLightFrequencyHz = 5; I recommend putting this ...
4 years, 2 months ago (2016-10-04 07:11:27 UTC) #71
darktears
On 2016/10/04 07:11:27, Reilly Grant wrote: > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h > File device/base/device_sensors_consts.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h#newcode11 ...
4 years, 2 months ago (2016-10-04 14:46:46 UTC) #72
Robert Sesek
On 2016/09/30 17:48:45, darktears wrote: > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode30 > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: > > ui_task_runner_(base::MessageLoop::current()->task_runner()), > > ...
4 years, 2 months ago (2016-10-04 15:28:18 UTC) #73
darktears
On 2016/10/04 07:11:27, Reilly Grant wrote: > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h > File device/base/device_sensors_consts.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sensors_consts.h#newcode11 ...
4 years, 2 months ago (2016-10-04 18:31:50 UTC) #74
Reilly Grant (use Gerrit)
lgtm, though (I forgot this directory existed) consider putting the common utility functions in //device/sensors/public/cpp.
4 years, 2 months ago (2016-10-05 03:28:40 UTC) #79
darktears
On 2016/10/05 03:28:40, Reilly Grant wrote: > lgtm, though (I forgot this directory existed) consider ...
4 years, 2 months ago (2016-10-05 20:45:33 UTC) #83
Robert Sesek
Last few comments, mostly requests for documentation. https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode23 device/generic_sensor/platform_sensor_ambient_light_mac.cc:23: struct Reading ...
4 years, 2 months ago (2016-10-05 21:00:18 UTC) #84
boliu
what do you want me to review?
4 years, 2 months ago (2016-10-05 21:30:39 UTC) #85
darktears
On 2016/10/05 21:30:39, boliu wrote: > what do you want me to review? Nothing actually ...
4 years, 2 months ago (2016-10-06 04:53:50 UTC) #88
darktears
On 2016/10/06 04:53:50, darktears wrote: > On 2016/10/05 21:30:39, boliu wrote: > > what do ...
4 years, 2 months ago (2016-10-06 17:35:10 UTC) #90
Robert Sesek
LGTM! https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionotificationportref.h File base/mac/scoped_ionotificationportref.h (right): https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionotificationportref.h#newcode5 base/mac/scoped_ionotificationportref.h:5: #ifndef BASE_MAC_SCOPED_IONOTIFICATIONPORTREF_H nit: needs trailing _ https://codereview.chromium.org/2332903002/diff/260001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File ...
4 years, 2 months ago (2016-10-06 18:58:53 UTC) #93
darktears
On 2016/10/06 18:58:53, Robert Sesek wrote: > LGTM! > > https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionotificationportref.h > File base/mac/scoped_ionotificationportref.h (right): ...
4 years, 2 months ago (2016-10-06 19:35:47 UTC) #95
Mikhail
lgtm https://codereview.chromium.org/2332903002/diff/280001/device/generic_sensor/platform_sensor_ambient_light_mac.cc File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/280001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode29 device/generic_sensor/platform_sensor_ambient_light_mac.cc:29: current_lux_(0) { nit: 0.0 https://codereview.chromium.org/2332903002/diff/280001/device/generic_sensor/platform_sensor_ambient_light_mac.cc#newcode113 device/generic_sensor/platform_sensor_ambient_light_mac.cc:113: current_lux_ = ...
4 years, 2 months ago (2016-10-06 19:52:54 UTC) #96
jam
you added a number of reviewers, so please specify which files you want them to ...
4 years, 2 months ago (2016-10-06 22:57:01 UTC) #98
darktears
On 2016/10/06 22:57:01, jam wrote: > you added a number of reviewers, so please specify ...
4 years, 2 months ago (2016-10-06 23:18:52 UTC) #99
dcheng
rs lgtm for //base
4 years, 2 months ago (2016-10-07 01:46:24 UTC) #100
jochen (gone - plz use gerrit)
lgtm
4 years, 2 months ago (2016-10-10 12:46:40 UTC) #101
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/2332903002/340001
4 years, 2 months ago (2016-10-11 16:49:35 UTC) #108
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/84550)
4 years, 2 months ago (2016-10-11 16:59:44 UTC) #110
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/2332903002/340001
4 years, 2 months ago (2016-10-11 17:02:33 UTC) #112
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/84569)
4 years, 2 months ago (2016-10-11 17:16:27 UTC) #114
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/2332903002/340001
4 years, 2 months ago (2016-10-11 17:19:51 UTC) #116
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/84591)
4 years, 2 months ago (2016-10-11 17:30:17 UTC) #118
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/2332903002/340001
4 years, 2 months ago (2016-10-11 17:31:57 UTC) #120
commit-bot: I haz the power
Committed patchset #18 (id:340001)
4 years, 2 months ago (2016-10-11 17:51:01 UTC) #121
commit-bot: I haz the power
Patchset 18 (id:??) landed as https://crrev.com/a5d055a1bd9e2db124cd7e2ef13c827f765f3c38 Cr-Commit-Position: refs/heads/master@{#424479}
4 years, 2 months ago (2016-10-11 17:52:49 UTC) #123
timvolodine
4 years, 2 months ago (2016-10-11 17:53:14 UTC) #124
Message was sent while issue was closed.
On 2016/10/06 23:18:52, darktears wrote:
> On 2016/10/06 22:57:01, jam wrote:
> > you added a number of reviewers, so please specify which files you want them
> to
> > look at.
> 
> Sure.
> 
> @jam, jochen : the content/browser and content/renderer root bits
> 
> @dcheng : the base/ part
> 
> @timvolodine : final lgtm on the device_sensors?

device/sensors and device_sensors/ lgtm. thanks!

Powered by Google App Engine
This is Rietveld 408576698