|
|
Created:
4 years, 3 months ago by darktears Modified:
4 years, 2 months ago Reviewers:
Reilly Grant (use Gerrit), shalamov, timvolodine, Nico, dcheng, maksims (do not use this acc), Robert Sesek, jochen (gone - plz use gerrit), Mikhail 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 #Messages
Total messages: 124 (69 generated)
On 2016/09/12 19:14:38, darktears wrote: Of course it requires some bits in https://codereview.chromium.org/2284613002/ and the IDL bindings in Blink.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_polling_mac.h (right): https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... 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 ? https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_polling_mac.h:25: std::map<PlatformSensorMac*, std::unique_ptr<base::RepeatingTimer>>; Think it would be better if 'PlatformSensorMac' itself contained a timer. https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_provider_mac.h (right): https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_mac.h:20: void BeginPollingSensor(PlatformSensor* sensor, this is not smth Provider should do (it's just a factory) Can we instead pass polling thread to PlatformSensorMac (e.g. as a task runner instance) ?
alexander.shalamov@intel.com changed reviewers: + alexander.shalamov@intel.com
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#... device/generic_sensor/DEPS:2: "+content/common", I think this violates layering, /device cannot depend on /content layer. https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:17: DeviceLightData() : illuminance(-1) {} Should it be 0? https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:37: return mojom::ReportingMode::CONTINUOUS; Should it be mojom::ReportingMode::ON_CHANGE, since in UpdateReading you call NotifySensorReadingChanged. https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:65: uint64_t lux_value[2]; Is it possible to store previous value and fire NotifySensorReadingChanged only when reading is changed?
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#... > device/generic_sensor/DEPS:2: "+content/common", > > I think this violates layering, /device cannot depend on /content layer. OK, I can probably move the old code over into device/. Can content/ depends on device/ for the old code to work? common/ dependency is to be able to write into the shared buffer. If I don't use shared_memory_seqlock_buffer.h, what else is recommended to use? > > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:17: DeviceLightData() > : illuminance(-1) {} > > Should it be 0? Sure. > > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:37: return > mojom::ReportingMode::CONTINUOUS; > > Should it be mojom::ReportingMode::ON_CHANGE, since in UpdateReading you call > NotifySensorReadingChanged. Ah my understanding what the other way around (in the platform POV). I can update. > > https://codereview.chromium.org/2332903002/diff/1/device/generic_sensor/platf... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:65: uint64_t > lux_value[2]; > > Is it possible to store previous value and fire NotifySensorReadingChanged only > when reading is changed? I will look into that but yes.
Looked only at device/generic_sensor/ changes https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_mac.cc:15: struct DeviceLightData { why is that needed? https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_mac.cc:20: typedef SharedMemorySeqLockBuffer<DeviceLightData> DeviceLightHardwareBuffer; nit: using DeviceLightHardwareBuffer = .. https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_mac.cc:43: return configuration.frequency() > 0 && configuration.frequency() <= 60; PlatformSensorConfiguration::kMaxAllowedFrequency instead of 60.0 https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_mac.cc (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_mac.cc:22: timer_.reset(); won't it get reset automatically when deleted? https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_mac.cc:34: const PlatformSensorConfiguration& configuration) { DCHECK to make sure this is a polling thread would be advisable here https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_mac.cc:45: base::Bind(&PlatformSensorMac::StopPoll, base::Unretained(this))); base::Unretained(this) can lead to raise condition if scheduled task is called after 'this' gets deleted. https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_mac.h (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_mac.h:22: const scoped_refptr<base::SingleThreadTaskRunner>& pls pass scoped_refptr by value https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_mac.h:24: virtual ~PlatformSensorMac(); nit: override https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_mac.cc (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_mac.cc:27: PlatformSensorProviderMac::PlatformSensorProviderMac() {} nit: = default https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_mac.cc:40: if (!polling_thread_->Start()) { For the record: in future we can add smth like 'protected virtual void PlatformSensorProviderBase::AllSensorsRemoved() {}' and call it from 'PlatformSensorProviderBase::RemoveSensor' when there are no sensors left. It'll be overridden in this class so that polling thread is terminated when it is not needed. https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_util_mac.h (right): https://codereview.chromium.org/2332903002/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_util_mac.h:12: double LMUvalueToLux(uint64_t raw_value); could it be placed to ambient_light_mac.h ?
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [sensors] [mac] Implement ambient light sensor for macOS macOS requires a mechanism to poll the sensors so this patch implements a polling thread for this purpose. At the moment the platform sensor uses the code in content/browser to fetch the value of the sensor, this code can probably be moved over at some point. BUG= ========== to ========== [sensors] [mac] Implement ambient light sensor for macOS macOS requires a mechanism to poll the sensors so this patch implements a polling thread for this purpose. 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/. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
alexis.menard@intel.com changed reviewers: + timvolodine@chromium.org
@timvolodine @mikhail : any feedback on the CL? Thanks.
Looks good overall https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:67: base::Unretained(this))); GetWeakPtr() to solve the potential raise condition. https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_mac.cc (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_mac.cc:17: polling_thread_task_runner_(polling_thread_task_runner), nit: polling_thread_task_runner_(std::move(polling_thread_task_runner)) https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_mac.h (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_mac.h:17: class PlatformSensorMac : public PlatformSensor { this class actually does not have any deps to Mac, on the other hand implementations on Linux/CrOS would also need a polling sensor primitive (maksim@ isn't it?) so maybe it's worth renaming it to 'PollingPlatformSensor' or smth like that. https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_mac.h:24: bool StartSensor(const PlatformSensorConfiguration& configuration) override; can all these methods be protected (as accessed either via PlatformSensor interface or from inside the object itself)? https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_mac.h:33: std::unique_ptr<base::RepeatingTimer> timer_; base::RepeatingTimer timer_; // like in example from timer.h, thus we can also obviate re-creating it on heap every StartSensor call.
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/19 06:25:18, Mikhail wrote: > Looks good overall > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:67: > base::Unretained(this))); > GetWeakPtr() to solve the potential raise condition. > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > device/generic_sensor/platform_sensor_mac.cc:17: > polling_thread_task_runner_(polling_thread_task_runner), > nit: polling_thread_task_runner_(std::move(polling_thread_task_runner)) > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > device/generic_sensor/platform_sensor_mac.h:17: class PlatformSensorMac : public > PlatformSensor { > this class actually does not have any deps to Mac, on the other hand > implementations on Linux/CrOS would also need a polling sensor primitive > (maksim@ isn't it?) so maybe it's worth renaming it to 'PollingPlatformSensor' > or smth like that. > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > device/generic_sensor/platform_sensor_mac.h:24: bool StartSensor(const > PlatformSensorConfiguration& configuration) override; > can all these methods be protected (as accessed either via PlatformSensor > interface or from inside the object itself)? > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > device/generic_sensor/platform_sensor_mac.h:33: > std::unique_ptr<base::RepeatingTimer> timer_; > base::RepeatingTimer timer_; // like in example from timer.h, thus we can also > obviate re-creating it on heap every StartSensor call. Alright feedback incorporated.
@timvolodine : I would appreciate your review. On 2016/09/19 20:00:28, darktears wrote: > On 2016/09/19 06:25:18, Mikhail wrote: > > Looks good overall > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:67: > > base::Unretained(this))); > > GetWeakPtr() to solve the potential raise condition. > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_mac.cc (right): > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_mac.cc:17: > > polling_thread_task_runner_(polling_thread_task_runner), > > nit: polling_thread_task_runner_(std::move(polling_thread_task_runner)) > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_mac.h (right): > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_mac.h:17: class PlatformSensorMac : > public > > PlatformSensor { > > this class actually does not have any deps to Mac, on the other hand > > implementations on Linux/CrOS would also need a polling sensor primitive > > (maksim@ isn't it?) so maybe it's worth renaming it to 'PollingPlatformSensor' > > or smth like that. > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_mac.h:24: bool StartSensor(const > > PlatformSensorConfiguration& configuration) override; > > can all these methods be protected (as accessed either via PlatformSensor > > interface or from inside the object itself)? > > > > > https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_mac.h:33: > > std::unique_ptr<base::RepeatingTimer> timer_; > > base::RepeatingTimer timer_; // like in example from timer.h, thus we can also > > obviate re-creating it on heap every StartSensor call. > > Alright feedback incorporated.
maksim.sisov@intel.com changed reviewers: + maksim.sisov@intel.com
https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_mac.h (right): https://codereview.chromium.org/2332903002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_mac.h:17: class PlatformSensorMac : public PlatformSensor { On 2016/09/19 06:25:18, Mikhail wrote: > this class actually does not have any deps to Mac, on the other hand > implementations on Linux/CrOS would also need a polling sensor primitive > (maksim@ isn't it?) so maybe it's worth renaming it to 'PollingPlatformSensor' > or smth like that. Ouch, I've just noticed this. Yes, right. I have almost the same implementation, but without timers. We can have only this one then!
lgtm
https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:26: current_lux(0) { I think it should be still -1, because 0 is valid lux value. https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:58: current_lux = lux; nit: current_lux -> current_lux_ https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:35: double current_lux; nit: current_lux -> current_lux_
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Drive-by comments: Are you sure polling is required here? I believe you should be able to use IOServiceAddInterestNotification() to get kIOGeneralInterest notifications when the sensors change. You can then use IOConnectCallMethod() to get the actual values. https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_mac.cc (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_mac.cc:39: polling_thread_.reset(new base::Thread("Sensor poller Mac")); If we do use polling polling, does this really need to be on its own dedicated thread? Why can't this be on a worker pool task runner?
On 2016/09/19 23:26:06, darktears wrote: > @timvolodine : I would appreciate your review. > ok I'll try to have a look today..
pinkerton@chromium.org changed reviewers: + pinkerton@chromium.org
+shrike as FYI. I concur with rsesek that this shouldn't poll. I'm concerned about the impact on battery life. Has that been measured? Is there a design doc? I didn't see anything in the intent-to-impl thread.
Aside from rsesek@'s and pinkerton@'s comments, some high level considerations below. Can we split this CL up into smaller logical pieces? i.e. - We should try to limit dependencies on device/generic_sensors and depend more selectively where possible. - If we are going to move seqlock related files out of content it should probably best go into base/ maybe base/synchronization.. This can be done in a separate patch. - I think it would be good to have a dedicated directory either under //device/generic_sensor/ or //device/sensors for files 'shared' between content/browser/device_sensors and device/generic_sensor (if any are needed) to avoid having a too tight coupling. https://codereview.chromium.org/2332903002/diff/120001/content/common/BUILD.gn File content/common/BUILD.gn (right): https://codereview.chromium.org/2332903002/diff/120001/content/common/BUILD.g... content/common/BUILD.gn:366: "//device/generic_sensor:generic_sensor", don't think this should depend on the whole generic_sensor https://codereview.chromium.org/2332903002/diff/120001/content/common/DEPS File content/common/DEPS (right): https://codereview.chromium.org/2332903002/diff/120001/content/common/DEPS#ne... content/common/DEPS:5: "+device/generic_sensor", same here https://codereview.chromium.org/2332903002/diff/120001/content/common/device_... File content/common/device_sensors/DEPS (right): https://codereview.chromium.org/2332903002/diff/120001/content/common/device_... content/common/device_sensors/DEPS:2: "+device/generic_sensor", also too general
+cc:lunalu@ : who is also looking into refactoring device_sensors.
Hi, Thanks for the review. I will split it into several pieces. On 2016/09/20 18:05:24, timvolodine wrote: > Aside from rsesek@'s and pinkerton@'s comments, some high level considerations > below. I will look at IOServiceAddInterestNotification and see if I can avoid the polling. For further consideration btw the generic sensor implementation is designed so that it leverages the PageVisibility API so when the page is not visible the sensor is "shut down" and the polling stops. Battery would be impacted only when the user is actually interacting with the page but at that point it's already impacted by the rest of what's happening on the page. > > Can we split this CL up into smaller logical pieces? i.e. > > - We should try to limit dependencies on device/generic_sensors and depend more > selectively where possible. Ok. > - If we are going to move seqlock related files out of content it should > probably best go into base/ maybe base/synchronization.. This can be done in a > separate patch. Agree. > - I think it would be good to have a dedicated directory either under > //device/generic_sensor/ or //device/sensors for files 'shared' between > content/browser/device_sensors and device/generic_sensor (if any are needed) to > avoid having a too tight coupling. > > https://codereview.chromium.org/2332903002/diff/120001/content/common/BUILD.gn > File content/common/BUILD.gn (right): > > https://codereview.chromium.org/2332903002/diff/120001/content/common/BUILD.g... > content/common/BUILD.gn:366: "//device/generic_sensor:generic_sensor", > don't think this should depend on the whole generic_sensor > > https://codereview.chromium.org/2332903002/diff/120001/content/common/DEPS > File content/common/DEPS (right): > > https://codereview.chromium.org/2332903002/diff/120001/content/common/DEPS#ne... > content/common/DEPS:5: "+device/generic_sensor", > same here > > https://codereview.chromium.org/2332903002/diff/120001/content/common/device_... > File content/common/device_sensors/DEPS (right): > > https://codereview.chromium.org/2332903002/diff/120001/content/common/device_... > content/common/device_sensors/DEPS:2: "+device/generic_sensor", > also too general
On 2016/09/20 22:15:17, darktears wrote: > I will look at IOServiceAddInterestNotification and see if I can avoid the > polling. Thank you. > For further consideration btw the generic sensor implementation is designed so > that it leverages the PageVisibility API so when the page is not visible the > sensor is "shut down" and the polling stops. Battery would be impacted only when > the user is actually interacting with the page but at that point it's already > impacted by the rest of what's happening on the page. That's good. But if we can avoid polling at all, that is preferable. We really want to minimize the number of wakeups we're generating (which figures into macOS's "apps using excessive energy" calculation).
https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... File device/generic_sensor/polling_platform_sensor.cc (right): https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... device/generic_sensor/polling_platform_sensor.cc:33: timer_.Start(FROM_HERE, base::TimeDelta::FromMicroseconds( You cannot create timer on one thread and kill it on another. PollingPlatformSensor::~PollingPlatformSensor() kill the timer on another thread. It's better not to use PostTask here. Just PostTask your actual task in UpdateReading().
On 2016/09/21 08:20:15, maksims wrote: > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... > File device/generic_sensor/polling_platform_sensor.cc (right): > > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... > device/generic_sensor/polling_platform_sensor.cc:33: timer_.Start(FROM_HERE, > base::TimeDelta::FromMicroseconds( > You cannot create timer on one thread and kill it on another. > PollingPlatformSensor::~PollingPlatformSensor() kill the timer on another > thread. It's better not to use PostTask here. Just PostTask your actual task in > UpdateReading(). For the record, starting splitting the patch : https://codereview.chromium.org/2358123005/ I'm investigating the IOServiceAddInterestNotification while I'm waiting review on the CL above.
Description was changed from ========== [sensors] [mac] Implement ambient light sensor for macOS macOS requires a mechanism to poll the sensors so this patch implements a polling thread for this purpose. 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/. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [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. 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/. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
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/... > > File device/generic_sensor/polling_platform_sensor.cc (right): > > > > > https://codereview.chromium.org/2332903002/diff/120001/device/generic_sensor/... > > device/generic_sensor/polling_platform_sensor.cc:33: timer_.Start(FROM_HERE, > > base::TimeDelta::FromMicroseconds( > > You cannot create timer on one thread and kill it on another. > > PollingPlatformSensor::~PollingPlatformSensor() kill the timer on another > > thread. It's better not to use PostTask here. Just PostTask your actual task > in > > UpdateReading(). > > For the record, starting splitting the patch : > https://codereview.chromium.org/2358123005/ > > I'm investigating the IOServiceAddInterestNotification while I'm waiting review > on the CL above. While https://codereview.chromium.org/2358123005/ is progressing, here is a new version using IOServiceAddInterestNotification instead of a polling thread. Please fire your comments.
Thanks, this is definitely a big improvement! Is it possible to write a unittest for this code as well? (Or is it tested by something cross-platform?) https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... File device/base/device_util_mac.h (right): https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... device/base/device_util_mac.h:12: double LMUvalueToLux(uint64_t raw_value); Give this a comment. https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:12: #include <IOKit/IOMessage.h> This goes after the .h for the .cc, but before the other includes. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: ui_task_runner_(base::MessageLoop::current()->task_runner()), I think this is deprecated MessageLoop method. Also, it would probably be better to have the task runner injected as an argument. https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: if (light_sensor_service_) Use ScopedIOObject for these. https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:105: dispatch_get_main_queue()); This will be the UI thread, which is probably undesirable for calling IOConnectCallMethod on. You could use dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0) instead. https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:11: #include <IOKit/IOKitLib.h> System includes go first.
On 2016/09/29 22:51:27, Robert Sesek wrote: > Thanks, this is definitely a big improvement! Thanks for the review. > > Is it possible to write a unittest for this code as well? (Or is it tested by > something cross-platform?) > IDL Bindings are added and tested here : https://codereview.chromium.org/2356133002/ A generic platform sensor test is added here : https://codereview.chromium.org/2306333002/ Testing the actual platform_sensor_ambient_light_mac.cc implementation require the hardware, I'm not sure how I could practically test that. > https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... > File device/base/device_util_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... > device/base/device_util_mac.h:12: double LMUvalueToLux(uint64_t raw_value); > Give this a comment. Done > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:12: #include > <IOKit/IOMessage.h> > This goes after the .h for the .cc, but before the other includes. > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: > ui_task_runner_(base::MessageLoop::current()->task_runner()), > I think this is deprecated MessageLoop method. Also, it would probably be better > to have the task runner injected as an argument. Done. base::MessageLoop::current() doesn't seem to be deprecated, https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... and neither task_runner() https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... Anyhow I'm not sure what would be the best way to access the UI task runner from the code here. Maybe through Thread? Not sure that's easier/cleaner than this. > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: if > (light_sensor_service_) > Use ScopedIOObject for these. I used it for light_sensor_service_ but for light_sensor_object_ as I need to pass it as a & to IOServiceOpen it doesn't seem possible for me to use the scoped object. > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:105: > dispatch_get_main_queue()); > This will be the UI thread, which is probably undesirable for calling > IOConnectCallMethod on. You could use > dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0) instead. Changed. > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:11: #include > <IOKit/IOKitLib.h> > System includes go first. Fixed
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/30 17:48:45, darktears wrote: > On 2016/09/29 22:51:27, Robert Sesek wrote: > > Thanks, this is definitely a big improvement! > > Thanks for the review. > > > > > Is it possible to write a unittest for this code as well? (Or is it tested by > > something cross-platform?) > > > > IDL Bindings are added and tested here : > https://codereview.chromium.org/2356133002/ > A generic platform sensor test is added here : > https://codereview.chromium.org/2306333002/ > > Testing the actual platform_sensor_ambient_light_mac.cc implementation require > the hardware, I'm not sure how I could practically test that. > > > > https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... > > File device/base/device_util_mac.h (right): > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/base/device_uti... > > device/base/device_util_mac.h:12: double LMUvalueToLux(uint64_t raw_value); > > Give this a comment. > > Done > > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:12: #include > > <IOKit/IOMessage.h> > > This goes after the .h for the .cc, but before the other includes. > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: > > ui_task_runner_(base::MessageLoop::current()->task_runner()), > > I think this is deprecated MessageLoop method. Also, it would probably be > better > > to have the task runner injected as an argument. > > Done. base::MessageLoop::current() doesn't seem to be deprecated, > https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... > and neither task_runner() > https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... > > Anyhow I'm not sure what would be the best way to access the UI task runner from > the code here. Maybe through Thread? Not sure that's easier/cleaner than this. > > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: if > > (light_sensor_service_) > > Use ScopedIOObject for these. > > I used it for light_sensor_service_ but for light_sensor_object_ as I need to > pass it as a & to IOServiceOpen it doesn't seem possible for me to use the > scoped object. > > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:105: > > dispatch_get_main_queue()); > > This will be the UI thread, which is probably undesirable for calling > > IOConnectCallMethod on. You could use > > dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0) instead. > > Changed. > > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.h:11: #include > > <IOKit/IOKitLib.h> > > System includes go first. > > Fixed @timvolodine : you may want to have a look on that new version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Didn't go through the generic_sensor/ mac implementation in much detail, but assume the mac experts on this patch did. In general looks good to me, thanks for producing a non-polling version! Obviously needs approval from owner for device/base. Could you also update the description to reflect the refactoring into device/base? https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn File device/base/BUILD.gn (right): https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... device/base/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2016? https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... device/base/BUILD.gn:5: import("//build/config/features.gni") not needed? https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... device/base/BUILD.gn:14: "//base", is this needed? https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... device/generic_sensor/BUILD.gn:49: sources += [] ? https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:71: default_configuration.set_frequency(5); I think this should be a constant somewhere more centralized, with a comment explaining the default value and when it is used.
alexis.menard@intel.com changed reviewers: + reillyg@chromium.org
Description was changed from ========== [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. 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/. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [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. 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/base (and is shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/base as well so it can be shared. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
On 2016/10/03 18:18:06, timvolodine wrote: > Didn't go through the generic_sensor/ mac implementation in much detail, but > assume the mac experts on this patch did. In general looks good to me, thanks > for producing a non-polling version! Obviously needs approval from owner for > device/base. Could you also update the description to reflect the refactoring > into device/base? > > https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn > File device/base/BUILD.gn (right): > > https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... > device/base/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights > reserved. > 2016? > > https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... > device/base/BUILD.gn:5: import("//build/config/features.gni") > not needed? > > https://codereview.chromium.org/2332903002/diff/180001/device/base/BUILD.gn#n... > device/base/BUILD.gn:14: "//base", > is this needed? > > https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... > File device/generic_sensor/BUILD.gn (right): > > https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... > device/generic_sensor/BUILD.gn:49: sources += [] > ? > > https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/180001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:71: > default_configuration.set_frequency(5); > I think this should be a constant somewhere more centralized, with a comment > explaining the default value and when it is used. @timvolodine : Fixed everything. @reillyg : please have a look. Thanks.
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... File device/base/device_sensors_consts.h (right): https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... device/base/device_sensors_consts.h:11: const int kDefaultAmbientLightFrequencyHz = 5; I recommend putting this in //device/generic_sensors/sensors_consts.h. https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... File device/base/device_util_mac.h (right): https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... device/base/device_util_mac.h:14: double LMUvalueToLux(uint64_t raw_value); This seems to be specific to light sensors. Any reason it isn't in //device/generic_sensor? https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: IONotificationPortDestroy(light_sensor_port_); Consider adding a specialization of ScopedGeneric (like ScopedFD) for this type in //base/mac or //device/base. There are two other places in Chromium where we use these and only one of them properly destroys the port. https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:54: IOObjectRelease(light_sensor_object_); Use ScopedIOObject for this field. If you need to pass &light_sensor_object_ to a method that assigns into it create a temporary and then copy the value into light_sensor_object_. https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:89: sensor->UpdateReading(lux_values); I would just write this as: if (kr == KERN_SUCCESS) sensor->UpdateReading(lux_values); https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:44: base::mac::ScopedIOObject<io_service_t> light_sensor_service_; nit: space between methods and fields https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:45: IONotificationPortRef light_sensor_port_; Initialize this to nullptr. https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:49: DISALLOW_COPY_AND_ASSIGN(PlatformSensorAmbientLightMac); nit: space between fields and this macro https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_mac.cc (right): https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_mac.cc:37: base::MessageLoop::current()->task_runner()); This method is deprecated, use base::ThreadTaskRunnerHandle::Get().
On 2016/10/04 07:11:27, Reilly Grant wrote: > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... > File device/base/device_sensors_consts.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... > device/base/device_sensors_consts.h:11: const int > kDefaultAmbientLightFrequencyHz = 5; > I recommend putting this in //device/generic_sensors/sensors_consts.h. Then I make content/renderer depends on generic_sensors/. Unless I create a generic_sensors/common target which would then be used. > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... > File device/base/device_util_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... > device/base/device_util_mac.h:14: double LMUvalueToLux(uint64_t raw_value); > This seems to be specific to light sensors. Any reason it isn't in > //device/generic_sensor? Same as above. Please note that I will add at least one function in here when I'll do the accelerometer for mac. Same reason as above thus the generic name. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: > IONotificationPortDestroy(light_sensor_port_); > Consider adding a specialization of ScopedGeneric (like ScopedFD) for this type > in //base/mac or //device/base. There are two other places in Chromium where we > use these and only one of them properly destroys the port. Ok. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:54: > IOObjectRelease(light_sensor_object_); > Use ScopedIOObject for this field. If you need to pass &light_sensor_object_ to > a method that assigns into it create a temporary and then copy the value into > light_sensor_object_. Good idea. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:89: > sensor->UpdateReading(lux_values); > I would just write this as: > > if (kr == KERN_SUCCESS) > sensor->UpdateReading(lux_values); > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:44: > base::mac::ScopedIOObject<io_service_t> light_sensor_service_; > nit: space between methods and fields > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:45: > IONotificationPortRef light_sensor_port_; > Initialize this to nullptr. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:49: > DISALLOW_COPY_AND_ASSIGN(PlatformSensorAmbientLightMac); > nit: space between fields and this macro > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider_mac.cc:37: > base::MessageLoop::current()->task_runner()); > This method is deprecated, use base::ThreadTaskRunnerHandle::Get(). I will fix but the .h file of MessageLoop doesn't say it.
On 2016/09/30 17:48:45, darktears wrote: > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: > > ui_task_runner_(base::MessageLoop::current()->task_runner()), > > I think this is deprecated MessageLoop method. Also, it would probably be > better > > to have the task runner injected as an argument. > > Done. base::MessageLoop::current() doesn't seem to be deprecated, > https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... > and neither task_runner() > https://chromium.googlesource.com/chromium/src/base/+/master/message_loop/mes... > > Anyhow I'm not sure what would be the best way to access the UI task runner from > the code here. Maybe through Thread? Not sure that's easier/cleaner than this. Generally the class should accept a TaskRunner interface and then the caller can use ThreadTaskRunnerHandle::Get(). > > > > > https://codereview.chromium.org/2332903002/diff/140001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: if > > (light_sensor_service_) > > Use ScopedIOObject for these. > > I used it for light_sensor_service_ but for light_sensor_object_ as I need to > pass it as a & to IOServiceOpen it doesn't seem possible for me to use the > scoped object. Use ScopedIOObject::InitializeInto(). https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:78: void*) { All parameters should be named. https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:106: io_object_t object; What is |object| and is it leaked here? https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:26: scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner); There's no reason to explicitly name this for the ui_task_runner. This could be used to receive callbacks on any thread. (Same with the member variable name).
On 2016/10/04 07:11:27, Reilly Grant wrote: > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... > File device/base/device_sensors_consts.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_sen... > device/base/device_sensors_consts.h:11: const int > kDefaultAmbientLightFrequencyHz = 5; > I recommend putting this in //device/generic_sensors/sensors_consts.h. > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... > File device/base/device_util_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/base/device_uti... > device/base/device_util_mac.h:14: double LMUvalueToLux(uint64_t raw_value); > This seems to be specific to light sensors. Any reason it isn't in > //device/generic_sensor? > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:51: > IONotificationPortDestroy(light_sensor_port_); > Consider adding a specialization of ScopedGeneric (like ScopedFD) for this type > in //base/mac or //device/base. There are two other places in Chromium where we > use these and only one of them properly destroys the port. Let's do that in a separate patch, so I can fix all in one shot. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:54: > IOObjectRelease(light_sensor_object_); > Use ScopedIOObject for this field. If you need to pass &light_sensor_object_ to > a method that assigns into it create a temporary and then copy the value into > light_sensor_object_. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:89: > sensor->UpdateReading(lux_values); > I would just write this as: > > if (kr == KERN_SUCCESS) > sensor->UpdateReading(lux_values); > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:44: > base::mac::ScopedIOObject<io_service_t> light_sensor_service_; > nit: space between methods and fields > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:45: > IONotificationPortRef light_sensor_port_; > Initialize this to nullptr. > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.h:49: > DISALLOW_COPY_AND_ASSIGN(PlatformSensorAmbientLightMac); > nit: space between fields and this macro > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/200001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider_mac.cc:37: > base::MessageLoop::current()->task_runner()); > This method is deprecated, use base::ThreadTaskRunnerHandle::Get().
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, though (I forgot this directory existed) consider putting the common utility functions in //device/sensors/public/cpp.
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
alexis.menard@intel.com changed reviewers: + boliu@chromium.org, esprehn@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/05 03:28:40, Reilly Grant wrote: > lgtm, though (I forgot this directory existed) consider putting the common > utility functions in //device/sensors/public/cpp. Changed to that location in latest changeset. @Mikhail : LGTM? @rsesek : Fixed your comments, PTAL @esprehn : Need a LGTM for the content/ changes. Thanks.
Last few comments, mostly requests for documentation. https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:23: struct Reading { Move the anon namespace into device and put this in it as well? https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:52: light_sensor_service_.reset(IOServiceGetMatchingService( Any reason to not do this in StartSensor()? https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:101: light_sensor_port_ = IONotificationPortCreate(kIOMasterPortDefault); Can StartSensor be called twice? If so, then light_sensor_port_ can get leaked. https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.h (right): https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:19: class PlatformSensorAmbientLightMac : public PlatformSensor { This class should have some documentation. Especially the ctor with the number of arguments it takes. https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:43: void*); name here too https://codereview.chromium.org/2332903002/diff/220001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.h:45: base::mac::ScopedIOObject<io_service_t> light_sensor_service_; And these fields should be documented too.
what do you want me to review?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/05 21:30:39, boliu wrote: > what do you want me to review? Nothing actually git cl owners suggested you but in fact you are listed for Android. Sorry for the noise.
alexis.menard@intel.com changed reviewers: - boliu@chromium.org, pinkerton@chromium.org
On 2016/10/06 04:53:50, darktears wrote: > On 2016/10/05 21:30:39, boliu wrote: > > what do you want me to review? > > Nothing actually git cl owners suggested you but in fact you are listed for > Android. Sorry for the noise. @rsesek : please have a look
Description was changed from ========== [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. 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/base (and is shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/base as well so it can be shared. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [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). Because of using seqlock we need to adapt the mojo definitions to take it into account. This will require an update in Android and Blink which will be done in a separate patch (I will defer the landing of this one). 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/generic_sensor/shared (shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/generic_sensor/shared 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/xL... BUG=606766 ==========
Description was changed from ========== [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). Because of using seqlock we need to adapt the mojo definitions to take it into account. This will require an update in Android and Blink which will be done in a separate patch (I will defer the landing of this one). 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/generic_sensor/shared (shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/generic_sensor/shared 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/xL... BUG=606766 ========== to ========== [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). Because of using seqlock we need to adapt the mojo definitions to take it into account. This will require an update in Android and Blink which will be done in a separate patch (I will defer the landing of this one). 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/generic_sensor/shared (shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/generic_sensor/shared 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/xL... BUG=606766 ==========
LGTM! https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionoti... File base/mac/scoped_ionotificationportref.h (right): https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionoti... 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/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/260001/device/generic_sensor/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: CHECK(type == mojom::SensorType::AMBIENT_LIGHT); DCHECK_EQ is probably sufficient since this should only catch developer error.
alexis.menard@intel.com changed reviewers: + dcheng@chromium.org, thakis@chromium.org
On 2016/10/06 18:58:53, Robert Sesek wrote: > LGTM! > > https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionoti... > File base/mac/scoped_ionotificationportref.h (right): > > https://codereview.chromium.org/2332903002/diff/260001/base/mac/scoped_ionoti... > 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/... > File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): > > https://codereview.chromium.org/2332903002/diff/260001/device/generic_sensor/... > device/generic_sensor/platform_sensor_ambient_light_mac.cc:30: CHECK(type == > mojom::SensorType::AMBIENT_LIGHT); > DCHECK_EQ is probably sufficient since this should only catch developer error. @thakis or dcheng : I need the LGTM for the base/ addition. @esprehn : I need the LGTM for the content/ bits. @Mikhail, @timvolodine : Final LGTM?
lgtm https://codereview.chromium.org/2332903002/diff/280001/device/generic_sensor/... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2332903002/diff/280001/device/generic_sensor/... 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/... device/generic_sensor/platform_sensor_ambient_light_mac.cc:113: current_lux_ = 0; nit: = 0.0;
alexis.menard@intel.com changed reviewers: + jam@chromium.org, jochen@chromium.org - esprehn@chromium.org
you added a number of reviewers, so please specify which files you want them to look at.
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?
rs lgtm for //base
lgtm
jam@chromium.org changed reviewers: - jam@chromium.org
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [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). Because of using seqlock we need to adapt the mojo definitions to take it into account. This will require an update in Android and Blink which will be done in a separate patch (I will defer the landing of this one). 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/generic_sensor/shared (shared amongst platforms). The method to convert the value returned by the LMU sensor (on mac machines) to lux is moved to device/generic_sensor/shared 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/xL... BUG=606766 ========== to ========== [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/xL... BUG=606766 ==========
The CQ bit was checked by alexis.menard@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, rsesek@chromium.org, mikhail.pozdnyakov@intel.com, dcheng@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2332903002/#ps340001 (title: "Update commit message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by alexis.menard@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by alexis.menard@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by alexis.menard@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [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/xL... BUG=606766 ========== to ========== [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/xL... BUG=606766 Committed: https://crrev.com/a5d055a1bd9e2db124cd7e2ef13c827f765f3c38 Cr-Commit-Position: refs/heads/master@{#424479} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/a5d055a1bd9e2db124cd7e2ef13c827f765f3c38 Cr-Commit-Position: refs/heads/master@{#424479}
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! |