Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417643006:
[sensors][mac] Make sure to update the sensor value when initializing the sensor. (Closed)
Description[sensors][mac] Make sure to update the sensor value when initializing the sensor.
IOServiceAddInterestNotification will only notify when the hardware sensor
value changes however when calling StartSensor we need to make
sure to query the value of the sensor right away to return the correct
value to JavaScript. The reason is that after calling StartSensor
the light conditions may never change and the IOServiceAddInterestNotification
callback will never be called and we return the default 0.0 sensor value. We
can fix this by making sure to initialize the sensor value in
PlatformSensorAmbientLightMac StartSensor.
BUG=606766
Committed: https://crrev.com/75daa868751b28b3bad9dcc033198c201129ae7d
Cr-Commit-Position: refs/heads/master@{#426806}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix Tim comments #
Total comments: 2
Patch Set 3 : Last comments from Tim #
Messages
Total messages: 25 (13 generated)
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...
alexis.menard@intel.com changed reviewers: + rsesek@chromium.org, timvolodine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
On 2016/10/14 at 20:07:20, rsesek wrote: > LGTM @timvolodine : appreciate your review :)
https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:57: sensor->ReadSensorValue(); would it make sense to make ReadSensorValue return void and return true in StartSensor below? https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:99: return ReadSensorValue(); is it possible that ReadSensorValue returns false but later the callback is still executed. Also, in that case do you need to unregister the callback? https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:109: bool PlatformSensorAmbientLightMac::ReadSensorValue() { maybe better name something like: ReadAndUpdate.. or ReadAndNotify?
https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:57: sensor->ReadSensorValue(); On 2016/10/18 at 19:26:13, timvolodine wrote: > would it make sense to make ReadSensorValue return void and return true in StartSensor below? The idea here was that at some point we will need to report the error back in JavaScript. So here I would get the return value and do what's needed to throw it in JavaScript. https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:99: return ReadSensorValue(); On 2016/10/18 at 19:26:13, timvolodine wrote: > is it possible that ReadSensorValue returns false but later the callback is still executed. Also, in that case do you need to unregister the callback? Yes then we can clean it up. https://codereview.chromium.org/2417643006/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_ambient_light_mac.cc:109: bool PlatformSensorAmbientLightMac::ReadSensorValue() { On 2016/10/18 at 19:26:13, timvolodine wrote: > maybe better name something like: ReadAndUpdate.. or ReadAndNotify? I will change.
The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run
On 2016/10/19 at 22:33:08, darktears wrote: > The CQ bit was checked by alexis.menard@intel.com to run a CQ dry run @timvolodine : Fixed your comments. Final review?
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.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
lgtm
On 2016/10/20 at 18:20:20, mikhail.pozdnyakov wrote: > lgtm Thanks, @timvolodine : I need your blessing.
blessing (aka lgtm) % two comments ;) https://codereview.chromium.org/2417643006/diff/20001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_ambient_light_mac.cc (right): https://codereview.chromium.org/2417643006/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_mac.cc:58: sensor->NotifySensorError(); if the callback is executed often would that be a problem? i.e. too many sensor error notifications? would it make sense to call StopSensor() here as well? https://codereview.chromium.org/2417643006/diff/20001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_mac.cc:110: light_sensor_object_.reset(); reset light_sensor_service_?
The CQ bit was checked by alexis.menard@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, timvolodine@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2417643006/#ps40001 (title: "Last comments from Tim")
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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [sensors][mac] Make sure to update the sensor value when initializing the sensor. IOServiceAddInterestNotification will only notify when the hardware sensor value changes however when calling StartSensor we need to make sure to query the value of the sensor right away to return the correct value to JavaScript. The reason is that after calling StartSensor the light conditions may never change and the IOServiceAddInterestNotification callback will never be called and we return the default 0.0 sensor value. We can fix this by making sure to initialize the sensor value in PlatformSensorAmbientLightMac StartSensor. BUG=606766 ========== to ========== [sensors][mac] Make sure to update the sensor value when initializing the sensor. IOServiceAddInterestNotification will only notify when the hardware sensor value changes however when calling StartSensor we need to make sure to query the value of the sensor right away to return the correct value to JavaScript. The reason is that after calling StartSensor the light conditions may never change and the IOServiceAddInterestNotification callback will never be called and we return the default 0.0 sensor value. We can fix this by making sure to initialize the sensor value in PlatformSensorAmbientLightMac StartSensor. BUG=606766 Committed: https://crrev.com/75daa868751b28b3bad9dcc033198c201129ae7d Cr-Commit-Position: refs/heads/master@{#426806} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/75daa868751b28b3bad9dcc033198c201129ae7d Cr-Commit-Position: refs/heads/master@{#426806} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
