Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
934843002:
Implement DeviceMotionEvent API (Closed)
|
Created:
5 years, 10 months ago by jonross Modified:
5 years, 9 months ago CC:
chromium-reviews, riju_, mlamouri+watch-sensors_chromium.org, jam, timvolodine, darin-cc_chromium.org, oshima+watch_chromium.org, Michael van Ouwerkerk, stevenjb+watch_chromium.org, tdanderson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement DeviceMotionEvent API
Update SensorManagerChromeOS to implement the DeviceMotionEvent API. Begin
processing accelerometer events into device motion events.
Updated all previous unittests of orientations to also verify the according
device motion events.
TEST=SensorManagerChromeOSTest.MotionBuffer,
SensorManagerChromeOSTest.GravityFilterCompletesForStationaryDevice,
SensorManagerChromeOSTest.NonGravityAcceleration,
SensorManagerChromeOSTest.AccelerationAgainstGravity
BUG=427662
Committed: https://crrev.com/d6b77054d45d0192a1fbbb7f893b73be7fdb44cf
Cr-Commit-Position: refs/heads/master@{#320159}
Patch Set 1 : #
Total comments: 20
Patch Set 2 : Rebase #Patch Set 3 : Update Thread Safety #
Total comments: 8
Patch Set 4 : Change AccelerometerUpdate to be RefCounted #
Total comments: 4
Patch Set 5 : Rebase #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : #
Total comments: 22
Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 6
Patch Set 11 : #
Total comments: 4
Patch Set 12 : Rebase #Patch Set 13 : Remove changes to ObserverListThreadsafe #Patch Set 14 : Fix missed test #Messages
Total messages: 48 (13 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + flackr@chromium.org
https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:25: static_cast<DeviceMotionHardwareBuffer*>(buffer)); Looks like return value is always true. If there are no current failure cases, just call void StartFetching...(buffer) and return true. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:42: motion_buffer_->data.interval = kInertialSensorIntervalMicroseconds / 1000; But this is not actually the update interval. Eventually it should be though: http://crbug.com/459218 https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:101: void SensorManagerChromeOS::OnAccelerometerUpdated( Given the accelerometer source reader runs on a blocking thread, and the target receiving the events is on another thread, we should eventually use a mechanism which doesn't stepping over to the UI thread to deliver these updates. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:132: if (!chromeos::AccelerometerReader::GetInstance()->HasObserver(this)) It should always have an observer if StopObserving is called shouldn't it? DCHECK that this is the case. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:151: gravity_.set_x(kGravityFilterRatio * gravity_.x() + I'm not sure we should be using a low pass filter to compute gravity, as having only an accelerometer for measurement makes this extremely likely to have errors. Unless I'm misreading this or missing something, it looks like android uses at least two (accel + magnetometer or gyro) if not all three sensors to determine the gravity component in order to provide a more accurate reading (http://developer.android.com/guide/topics/sensors/sensors_motion.html). Instead, we could tell developers that they can approximate gravity if there is not a sensor for it using a low pass filter and hopefully that would allow each app to make the right decision whether to use a low pass filter or not. Take for example turning the device. I have no accelerated it in any significant way along any of the axes, but it will temporarily read as an acceleration. Similarly if the device is in freefall we would determine the acceleration due to gravity to be approaching (0, 0, 0), or if the device is in a vehicle turning at constant velocity it will eventually see all of that force as acceleration due to gravity whereas the spec suggests it should not: http://www.w3.org/TR/orientation-event/. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:54: // Process accelerometer data and update according buffers. Comment is a bit confusing, Perhaps "Updates |motion_buffer_| or |orientation_buffer_| accordingly."? https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:62: // Synchronize orientation_buffer_ across threads. nit: Two comments or convert into a single comment covering both like "Synchronize buffer updates across threads." On reading through the code, I don't understand why these locks are necessary. Is StartFetchingDeviceMotionData not called on the UI thread? If it is, OnAccelerometerUpdated is also called on the UI thread, so it seems that there is no risk of motion_buffer_ updating while we're using it. If it's not, I don't think it's safe to call AccelerometerReader::AddObserver from another thread. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:82: nit: No need to separate these.
https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:42: motion_buffer_->data.interval = kInertialSensorIntervalMicroseconds / 1000; On 2015/02/18 22:34:09, flackr wrote: > But this is not actually the update interval. Eventually it should be though: > http://crbug.com/459218 This is the interval with which events are sent to Javascript. The buffer is read at a continuous interval, each time sending the current values. So this is the interval that websites will receive. It is true that this is not the rate which updates are received here. I'll add a todo to track this https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:151: gravity_.set_x(kGravityFilterRatio * gravity_.x() + On 2015/02/18 22:34:09, flackr wrote: > I'm not sure we should be using a low pass filter to compute gravity, as having > only an accelerometer for measurement makes this extremely likely to have > errors. Unless I'm misreading this or missing something, it looks like android > uses at least two (accel + magnetometer or gyro) if not all three sensors to > determine the gravity component in order to provide a more accurate reading > (http://developer.android.com/guide/topics/sensors/sensors_motion.html). > Instead, we could tell developers that they can approximate gravity if there is > not a sensor for it using a low pass filter and hopefully that would allow each > app to make the right decision whether to use a low pass filter or not. > > Take for example turning the device. I have no accelerated it in any significant > way along any of the axes, but it will temporarily read as an acceleration. > Similarly if the device is in freefall we would determine the acceleration due > to gravity to be approaching (0, 0, 0), or if the device is in a vehicle turning > at constant velocity it will eventually see all of that force as acceleration > due to gravity whereas the spec suggests it should not: > http://www.w3.org/TR/orientation-event/. The idea for the filter actually came from Android documentation: http://developer.android.com/reference/android/hardware/SensorEvent.html#values This is likely for hardware devices in which only the accelerometer exists. If we remove this to be consistent with the w3c spec, then I would recommend that we document the filtering for any developer that wants to implement it themselves.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
@flackr: PTAL https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:25: static_cast<DeviceMotionHardwareBuffer*>(buffer)); On 2015/02/18 22:34:09, flackr wrote: > Looks like return value is always true. If there are no current failure cases, > just call void StartFetching...(buffer) and return true. Done. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:101: void SensorManagerChromeOS::OnAccelerometerUpdated( On 2015/02/18 22:34:09, flackr wrote: > Given the accelerometer source reader runs on a blocking thread, and the target > receiving the events is on another thread, we should eventually use a mechanism > which doesn't stepping over to the UI thread to deliver these updates. For the DeviceMotion API this will be partially addressed by the changes to the observer list. For the remainder you filed crbug.com/461433 which will be addressed separately. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:132: if (!chromeos::AccelerometerReader::GetInstance()->HasObserver(this)) On 2015/02/18 22:34:09, flackr wrote: > It should always have an observer if StopObserving is called shouldn't it? > DCHECK that this is the case. Done. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:54: // Process accelerometer data and update according buffers. On 2015/02/18 22:34:09, flackr wrote: > Comment is a bit confusing, Perhaps "Updates |motion_buffer_| or > |orientation_buffer_| accordingly."? Done. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:62: // Synchronize orientation_buffer_ across threads. On 2015/02/18 22:34:09, flackr wrote: > nit: Two comments or convert into a single comment covering both like > "Synchronize buffer updates across threads." > > On reading through the code, I don't understand why these locks are necessary. > Is StartFetchingDeviceMotionData not called on the UI thread? If it is, > OnAccelerometerUpdated is also called on the UI thread, so it seems that there > is no risk of motion_buffer_ updating while we're using it. If it's not, I don't > think it's safe to call AccelerometerReader::AddObserver from another thread. StartFetchingDeviceMotionData is called from the IO thread. OnAccelerometerUpdated is currently called from the UI thread. However currently AccelerometerReader::AddObserver is not thread safe. An upcoming patch to this review will address that. This will have the benefit of performing all notifications to observers on their calling threads. Thus making these locks redundant. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:82: On 2015/02/18 22:34:09, flackr wrote: > nit: No need to separate these. Done.
https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:42: motion_buffer_->data.interval = kInertialSensorIntervalMicroseconds / 1000; On 2015/02/24 00:02:47, jonross wrote: > On 2015/02/18 22:34:09, flackr wrote: > > But this is not actually the update interval. Eventually it should be though: > > http://crbug.com/459218 > > This is the interval with which events are sent to Javascript. > > The buffer is read at a continuous interval, each time sending the current > values. So this is the interval that websites will receive. > > It is true that this is not the rate which updates are received here. I'll add a > todo to track this But when I tested it, it seems that JS is only notified if the value actually changed (See deviceorientation rate test page I wrote here: http://www.dynprojects.com/dp/tests/deviceorientationrate.html), so from the consumer's point of view the rate is actually max(kInertialSensorIntervalMicroseconds / 1000, kDelayBetweenReadsMs) https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:151: gravity_.set_x(kGravityFilterRatio * gravity_.x() + On 2015/02/24 00:02:47, jonross wrote: > On 2015/02/18 22:34:09, flackr wrote: > > I'm not sure we should be using a low pass filter to compute gravity, as > having > > only an accelerometer for measurement makes this extremely likely to have > > errors. Unless I'm misreading this or missing something, it looks like android > > uses at least two (accel + magnetometer or gyro) if not all three sensors to > > determine the gravity component in order to provide a more accurate reading > > (http://developer.android.com/guide/topics/sensors/sensors_motion.html). > > Instead, we could tell developers that they can approximate gravity if there > is > > not a sensor for it using a low pass filter and hopefully that would allow > each > > app to make the right decision whether to use a low pass filter or not. > > > > Take for example turning the device. I have no accelerated it in any > significant > > way along any of the axes, but it will temporarily read as an acceleration. > > Similarly if the device is in freefall we would determine the acceleration due > > to gravity to be approaching (0, 0, 0), or if the device is in a vehicle > turning > > at constant velocity it will eventually see all of that force as acceleration > > due to gravity whereas the spec suggests it should not: > > http://www.w3.org/TR/orientation-event/. > > The idea for the filter actually came from Android documentation: > > http://developer.android.com/reference/android/hardware/SensorEvent.html#values > > This is likely for hardware devices in which only the accelerometer exists. My understanding from reading this was that it's a suggestion for developers if they want to infer gravity from the accelerometer updates, given that it's written in onSensorChanged as if it were a client app. > > If we remove this to be consistent with the w3c spec, then I would recommend > that we document the filtering for any developer that wants to implement it > themselves. I agree, it would be nice to document (or link to) the filter from http://www.html5rocks.com/en/tutorials/device/orientation/ where it mentions that "some devices might not have the hardware to exclude the effect of gravity". https://codereview.chromium.org/934843002/diff/100001/base/observer_list_thre... File base/observer_list_threadsafe.h (right): https://codereview.chromium.org/934843002/diff/100001/base/observer_list_thre... base/observer_list_threadsafe.h:169: ObserverList<ObserverType>* list = &context->list; nit: should be able to return context->list.HasObserver(obs) no? https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:269: observers_->Notify(FROM_HERE, &Observer::OnAccelerometerUpdated, update_); nit: Add TODO to move this to blocking thread (reference existing crbug). I think this will be really easy given the threadsafe observer list. https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_types.h (left): https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_types.h:71: DISALLOW_COPY_AND_ASSIGN(AccelerometerUpdate); If this needs to be copyable, style guide says that the copy constructor should be explicitly defined: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova... Alternately, this could be passed as a ref counted object which would keep it alive until all callbacks had consumed the value. This may be less expensive than copying. https://codereview.chromium.org/934843002/diff/100001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/100001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:29: static_cast<DeviceOrientationHardwareBuffer*>(buffer)); nit: same applies here right? StartFetchingDeviceOrientationData also only returns true.
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/934843002/diff/100001/base/observer_list_thre... File base/observer_list_threadsafe.h (right): https://codereview.chromium.org/934843002/diff/100001/base/observer_list_thre... base/observer_list_threadsafe.h:169: ObserverList<ObserverType>* list = &context->list; On 2015/02/25 23:15:16, flackr wrote: > nit: should be able to return context->list.HasObserver(obs) no? Done. https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:269: observers_->Notify(FROM_HERE, &Observer::OnAccelerometerUpdated, update_); On 2015/02/25 23:15:16, flackr wrote: > nit: Add TODO to move this to blocking thread (reference existing crbug). I > think this will be really easy given the threadsafe observer list. Done. https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_types.h (left): https://codereview.chromium.org/934843002/diff/100001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_types.h:71: DISALLOW_COPY_AND_ASSIGN(AccelerometerUpdate); On 2015/02/25 23:15:17, flackr wrote: > If this needs to be copyable, style guide says that the copy constructor should > be explicitly defined: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova... > > Alternately, this could be passed as a ref counted object which would keep it > alive until all callbacks had consumed the value. This may be less expensive > than copying. The threadsafe observer runs its notifies async, with no guarantees of completion time. Switched to using a scoped_refptr https://codereview.chromium.org/934843002/diff/100001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/100001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:29: static_cast<DeviceOrientationHardwareBuffer*>(buffer)); On 2015/02/25 23:15:17, flackr wrote: > nit: same applies here right? StartFetchingDeviceOrientationData also only > returns true. Done.
https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:206: if (update_.get()) I believe scoped_refptr is testable (i.e. can just say if (update) ). https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:53: virtual void OnAccelerometerUpdated(const AccelerometerUpdate* update) = 0; I think this should pass a scoped_refptr<const chromeos::AccelerometerUpdate> so that references to the update from the callbacks can be tracked and passed along easily if they need to be sent to other threads.
https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:206: if (update_.get()) On 2015/03/02 15:46:39, flackr wrote: > I believe scoped_refptr is testable (i.e. can just say if (update) ). Done. https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/934843002/diff/140001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:53: virtual void OnAccelerometerUpdated(const AccelerometerUpdate* update) = 0; On 2015/03/02 15:46:40, flackr wrote: > I think this should pass a scoped_refptr<const chromeos::AccelerometerUpdate> so > that references to the update from the callbacks can be tracked and passed along > easily if they need to be sent to other threads. Done.
I think you missed the followups on the earlier patchset: https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s...
I've addressed the missed comments. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:42: motion_buffer_->data.interval = kInertialSensorIntervalMicroseconds / 1000; On 2015/02/25 23:15:16, flackr wrote: > On 2015/02/24 00:02:47, jonross wrote: > > On 2015/02/18 22:34:09, flackr wrote: > > > But this is not actually the update interval. Eventually it should be > though: > > > http://crbug.com/459218 > > > > This is the interval with which events are sent to Javascript. > > > > The buffer is read at a continuous interval, each time sending the current > > values. So this is the interval that websites will receive. > > > > It is true that this is not the rate which updates are received here. I'll add > a > > todo to track this > > But when I tested it, it seems that JS is only notified if the value actually > changed (See deviceorientation rate test page I wrote here: > http://www.dynprojects.com/dp/tests/deviceorientationrate.html), so from the > consumer's point of view the rate is actually > max(kInertialSensorIntervalMicroseconds / 1000, kDelayBetweenReadsMs) Done. https://codereview.chromium.org/934843002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:151: gravity_.set_x(kGravityFilterRatio * gravity_.x() + On 2015/02/25 23:15:16, flackr wrote: > On 2015/02/24 00:02:47, jonross wrote: > > On 2015/02/18 22:34:09, flackr wrote: > > > I'm not sure we should be using a low pass filter to compute gravity, as > > having > > > only an accelerometer for measurement makes this extremely likely to have > > > errors. Unless I'm misreading this or missing something, it looks like > android > > > uses at least two (accel + magnetometer or gyro) if not all three sensors to > > > determine the gravity component in order to provide a more accurate reading > > > (http://developer.android.com/guide/topics/sensors/sensors_motion.html). > > > Instead, we could tell developers that they can approximate gravity if there > > is > > > not a sensor for it using a low pass filter and hopefully that would allow > > each > > > app to make the right decision whether to use a low pass filter or not. > > > > > > Take for example turning the device. I have no accelerated it in any > > significant > > > way along any of the axes, but it will temporarily read as an acceleration. > > > Similarly if the device is in freefall we would determine the acceleration > due > > > to gravity to be approaching (0, 0, 0), or if the device is in a vehicle > > turning > > > at constant velocity it will eventually see all of that force as > acceleration > > > due to gravity whereas the spec suggests it should not: > > > http://www.w3.org/TR/orientation-event/. > > > > The idea for the filter actually came from Android documentation: > > > > > http://developer.android.com/reference/android/hardware/SensorEvent.html#values > > > > This is likely for hardware devices in which only the accelerometer exists. > > My understanding from reading this was that it's a suggestion for developers if > they want to infer gravity from the accelerometer updates, given that it's > written in onSensorChanged as if it were a client app. > > > > > If we remove this to be consistent with the w3c spec, then I would recommend > > that we document the filtering for any developer that wants to implement it > > themselves. > > I agree, it would be nice to document (or link to) the filter from > http://www.html5rocks.com/en/tutorials/device/orientation/ where it mentions > that "some devices might not have the hardware to exclude the effect of > gravity". Acknowledged.
https://codereview.chromium.org/934843002/diff/200001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/934843002/diff/200001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:70: int DelayBetweenReadsMs() const; Until we resolve increasing the update rate, I think we should just expose the constant in the header like this: https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi... https://codereview.chromium.org/934843002/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:37: // be updated (crbug.com/459218) nit: nix the comment, while we will update the accelerometer the reported rate here is the update rate.
https://codereview.chromium.org/934843002/diff/200001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/934843002/diff/200001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:70: int DelayBetweenReadsMs() const; On 2015/03/05 19:59:45, flackr wrote: > Until we resolve increasing the update rate, I think we should just expose the > constant in the header like this: > https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/overview/wi... Done. https://codereview.chromium.org/934843002/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:37: // be updated (crbug.com/459218) On 2015/03/05 19:59:45, flackr wrote: > nit: nix the comment, while we will update the accelerometer the reported rate > here is the update rate. Done.
jonross@chromium.org changed reviewers: + timvolodine@chromium.org
timvolodine@chromium.org: Please review changes in content/browser/device_sensors/* I am adding support for the Device Motion event on Chrome OS. Thanks, Jon
thanks Jon for adding this functionality, some comments/questions below https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; is there any reason to ignore the return value of StartFetchingDeviceMotionData and always return true? https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:30: return true; same question here https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:29: DeviceMotionHardwareBuffer* buffer) { Do StartFetchingDeviceMotionData and OnAccelerometerUpdated get executed on the same thread? If not I think you'll need a lock for motion_buffer_ to make sure it's not updated concurrently. My suspicion is that StartFetchingDeviceMotionData is executed on IO thread while OnAccelerometerUpdated on some other thread? https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:36: motion_buffer_->data.interval = std::max( nit: maybe add a comment why this should be max(..) https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:47: return false; nit: maybe extra empty line for readability. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:115: return; it's a bit strange to have this kind of logic, i.e. if motion_buffer_ is not null I cannot stop observing accelerometer.. I don't think we use this approach anywhere else, would a simple removeObserver be sufficient here?
https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/09 15:55:57, timvolodine wrote: > is there any reason to ignore the return value of StartFetchingDeviceMotionData > and always return true? Oh, I had suggested since the function could not return anything but true to simply return true and make the function void. Looks like it still returns a bool though.
https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/09 16:21:19, flackr wrote: > On 2015/03/09 15:55:57, timvolodine wrote: > > is there any reason to ignore the return value of > StartFetchingDeviceMotionData > > and always return true? > > Oh, I had suggested since the function could not return anything but true to > simply return true and make the function void. Looks like it still returns a > bool though. I'll update the functions to be void https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:29: DeviceMotionHardwareBuffer* buffer) { On 2015/03/09 15:55:57, timvolodine wrote: > Do StartFetchingDeviceMotionData and OnAccelerometerUpdated get executed on the > same thread? If not I think you'll need a lock for motion_buffer_ to make sure > it's not updated concurrently. My suspicion is that > StartFetchingDeviceMotionData is executed on IO thread while > OnAccelerometerUpdated on some other thread? AccelerometerReader has been updated to use a threadsafe observer. All OnAccelerometerUpdated calls will be made on the same thread from which the observer joined. So for SensorManagerChromeOS joining is on IO thread, and OnAccelerometerUpdated will be on IO thread. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:115: return; On 2015/03/09 15:55:57, timvolodine wrote: > it's a bit strange to have this kind of logic, i.e. if motion_buffer_ is not > null I cannot stop observing accelerometer.. I don't think we use this approach > anywhere else, would a simple removeObserver be sufficient here? It is not sufficient if we have two pages currently listening to different events. One page starts Device Motion, another Device Orientation. If one page closes and stops listening to one event type, I don't want this to also shutdown updates to the other. Since both events are based on observing the same accelerometer updates I added this check to only stop listening when neither event is active.
https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/09 16:35:26, jonross wrote: > On 2015/03/09 16:21:19, flackr wrote: > > On 2015/03/09 15:55:57, timvolodine wrote: > > > is there any reason to ignore the return value of > > StartFetchingDeviceMotionData > > > and always return true? > > > > Oh, I had suggested since the function could not return anything but true to > > simply return true and make the function void. Looks like it still returns a > > bool though. > > I'll update the functions to be void Done. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:30: return true; On 2015/03/09 15:55:57, timvolodine wrote: > same question here Done. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:36: motion_buffer_->data.interval = std::max( On 2015/03/09 15:55:57, timvolodine wrote: > nit: maybe add a comment why this should be max(..) Done. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:47: return false; On 2015/03/09 15:55:57, timvolodine wrote: > nit: maybe extra empty line for readability. Done.
https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:31: if (motion_buffer_) Start should never be called twice without calling stop in between should it? Can we CHECK(!motion_buffer_)? https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:64: if (orientation_buffer_) ditto
thanks Jon, just a few more suggestions https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/09 20:46:24, jonross wrote: > On 2015/03/09 16:35:26, jonross wrote: > > On 2015/03/09 16:21:19, flackr wrote: > > > On 2015/03/09 15:55:57, timvolodine wrote: > > > > is there any reason to ignore the return value of > > > StartFetchingDeviceMotionData > > > > and always return true? > > > > > > Oh, I had suggested since the function could not return anything but true to > > > simply return true and make the function void. Looks like it still returns a > > > bool though. > > > > I'll update the functions to be void > > Done. curious: so there is always an accelerometer available on chromeOS? i.e. is it possible that there is simply no sensor and what would happen in that case? https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:29: DeviceMotionHardwareBuffer* buffer) { On 2015/03/09 16:35:26, jonross wrote: > On 2015/03/09 15:55:57, timvolodine wrote: > > Do StartFetchingDeviceMotionData and OnAccelerometerUpdated get executed on > the > > same thread? If not I think you'll need a lock for motion_buffer_ to make sure > > it's not updated concurrently. My suspicion is that > > StartFetchingDeviceMotionData is executed on IO thread while > > OnAccelerometerUpdated on some other thread? > > AccelerometerReader has been updated to use a threadsafe observer. All > OnAccelerometerUpdated calls will be made on the same thread from which the > observer joined. > > So for SensorManagerChromeOS joining is on IO thread, and OnAccelerometerUpdated > will be on IO thread. I see, in that case how about adding a ThreadChecker to this class and DCHECK(thread_checker_.CalledOnValidThread()) where appropriate? https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:115: return; On 2015/03/09 16:35:26, jonross wrote: > On 2015/03/09 15:55:57, timvolodine wrote: > > it's a bit strange to have this kind of logic, i.e. if motion_buffer_ is not > > null I cannot stop observing accelerometer.. I don't think we use this > approach > > anywhere else, would a simple removeObserver be sufficient here? > > It is not sufficient if we have two pages currently listening to different > events. > > One page starts Device Motion, another Device Orientation. > > If one page closes and stops listening to one event type, I don't want this to > also shutdown updates to the other. > > Since both events are based on observing the same accelerometer updates I added > this check to only stop listening when neither event is active. ok I see, IMHO it would be more readable to have something like if (!orientation_buffer_) StopObservingAccelerometer(); inside the StopFetchingDeviceMotionData() method. Otherwise it looks like StopObservingAccelerometer does not always actually stop observing.. (alternatively renaming this method to something like "StopObservingAccelerometerIfPossible" would also be more clear).
https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/10 13:03:08, timvolodine wrote: > On 2015/03/09 20:46:24, jonross wrote: > > On 2015/03/09 16:35:26, jonross wrote: > > > On 2015/03/09 16:21:19, flackr wrote: > > > > On 2015/03/09 15:55:57, timvolodine wrote: > > > > > is there any reason to ignore the return value of > > > > StartFetchingDeviceMotionData > > > > > and always return true? > > > > > > > > Oh, I had suggested since the function could not return anything but true > to > > > > simply return true and make the function void. Looks like it still returns > a > > > > bool though. > > > > > > I'll update the functions to be void > > > > Done. > > curious: so there is always an accelerometer available on chromeOS? > i.e. is it possible that there is simply no sensor and what would happen in that > case? It is possible for there to be no sensor on ChromeOS. Currently ChromeOS would just never fire an OnAccelerometerUpdate event. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:29: DeviceMotionHardwareBuffer* buffer) { On 2015/03/10 13:03:08, timvolodine wrote: > On 2015/03/09 16:35:26, jonross wrote: > > On 2015/03/09 15:55:57, timvolodine wrote: > > > Do StartFetchingDeviceMotionData and OnAccelerometerUpdated get executed on > > the > > > same thread? If not I think you'll need a lock for motion_buffer_ to make > sure > > > it's not updated concurrently. My suspicion is that > > > StartFetchingDeviceMotionData is executed on IO thread while > > > OnAccelerometerUpdated on some other thread? > > > > AccelerometerReader has been updated to use a threadsafe observer. All > > OnAccelerometerUpdated calls will be made on the same thread from which the > > observer joined. > > > > So for SensorManagerChromeOS joining is on IO thread, and > OnAccelerometerUpdated > > will be on IO thread. > > I see, in that case how about adding a ThreadChecker to this class and > DCHECK(thread_checker_.CalledOnValidThread()) where appropriate? Done. https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:115: return; On 2015/03/10 13:03:08, timvolodine wrote: > On 2015/03/09 16:35:26, jonross wrote: > > On 2015/03/09 15:55:57, timvolodine wrote: > > > it's a bit strange to have this kind of logic, i.e. if motion_buffer_ is not > > > null I cannot stop observing accelerometer.. I don't think we use this > > approach > > > anywhere else, would a simple removeObserver be sufficient here? > > > > It is not sufficient if we have two pages currently listening to different > > events. > > > > One page starts Device Motion, another Device Orientation. > > > > If one page closes and stops listening to one event type, I don't want this to > > also shutdown updates to the other. > > > > Since both events are based on observing the same accelerometer updates I > added > > this check to only stop listening when neither event is active. > > ok I see, IMHO it would be more readable to have something like > if (!orientation_buffer_) > StopObservingAccelerometer(); > > inside the StopFetchingDeviceMotionData() method. Otherwise it looks like > StopObservingAccelerometer does not always actually stop observing.. > (alternatively renaming this method to something like > "StopObservingAccelerometerIfPossible" would also be more clear). Done. Went with the check in each Stop*Buffer method. https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:31: if (motion_buffer_) On 2015/03/10 05:32:36, flackr wrote: > Start should never be called twice without calling stop in between should it? > Can we CHECK(!motion_buffer_)? It appears so. Once the buffer is created new pages requesting the event do not trigger subsequent starts. They just receive the buffer. Done. https://codereview.chromium.org/934843002/diff/240001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:64: if (orientation_buffer_) On 2015/03/10 05:32:36, flackr wrote: > ditto Done.
lgtm with nits https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/10 15:03:58, jonross wrote: > On 2015/03/10 13:03:08, timvolodine wrote: > > On 2015/03/09 20:46:24, jonross wrote: > > > On 2015/03/09 16:35:26, jonross wrote: > > > > On 2015/03/09 16:21:19, flackr wrote: > > > > > On 2015/03/09 15:55:57, timvolodine wrote: > > > > > > is there any reason to ignore the return value of > > > > > StartFetchingDeviceMotionData > > > > > > and always return true? > > > > > > > > > > Oh, I had suggested since the function could not return anything but > true > > to > > > > > simply return true and make the function void. Looks like it still > returns > > a > > > > > bool though. > > > > > > > > I'll update the functions to be void > > > > > > Done. > > > > curious: so there is always an accelerometer available on chromeOS? > > i.e. is it possible that there is simply no sensor and what would happen in > that > > case? > > It is possible for there to be no sensor on ChromeOS. Currently ChromeOS would > just never fire an OnAccelerometerUpdate event. If this *should* fail (return false) when there is no accelerometer we should probably follow up with this. I wonder though if we'll have a race if this is called at startup before we know if there's an accelerometer. https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:31: DCHECK(buffer); nit: This is probably an unnecessary check, since the buffer is used immediately within this method the browser will crash on line 35 if buffer is NULL anyways. https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:66: DCHECK(buffer); ditto
https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/220001/content/browser/device_... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:26: return true; On 2015/03/10 17:21:11, flackr wrote: > On 2015/03/10 15:03:58, jonross wrote: > > On 2015/03/10 13:03:08, timvolodine wrote: > > > On 2015/03/09 20:46:24, jonross wrote: > > > > On 2015/03/09 16:35:26, jonross wrote: > > > > > On 2015/03/09 16:21:19, flackr wrote: > > > > > > On 2015/03/09 15:55:57, timvolodine wrote: > > > > > > > is there any reason to ignore the return value of > > > > > > StartFetchingDeviceMotionData > > > > > > > and always return true? > > > > > > > > > > > > Oh, I had suggested since the function could not return anything but > > true > > > to > > > > > > simply return true and make the function void. Looks like it still > > returns > > > a > > > > > > bool though. > > > > > > > > > > I'll update the functions to be void > > > > > > > > Done. > > > > > > curious: so there is always an accelerometer available on chromeOS? > > > i.e. is it possible that there is simply no sensor and what would happen in > > that > > > case? > > > > It is possible for there to be no sensor on ChromeOS. Currently ChromeOS would > > just never fire an OnAccelerometerUpdate event. > > If this *should* fail (return false) when there is no accelerometer we should > probably follow up with this. I wonder though if we'll have a race if this is > called at startup before we know if there's an accelerometer. Yes so in case there is no sensor the API should actually fire a "null" event (as per specification). I am fine if this will be addressed in a follow-up patch. https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:32: CHECK(!motion_buffer_); I wouldn't use CHECK here, seems a bit drastic to crash the whole browser because of this. Think DCHECK is probably sufficient. Also not sure if motion_buffer_ can actually be not null in very special cases, like crash of a renderer..
I've updated the checks in sensor manager. I've also created crbug.com/465794 to track updating AccelerometerReader to provide a mechanism for determining if accelerometer sensors are available. https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:31: DCHECK(buffer); On 2015/03/10 17:21:11, flackr wrote: > nit: This is probably an unnecessary check, since the buffer is used immediately > within this method the browser will crash on line 35 if buffer is NULL anyways. Looking into the call path it isn't needed. There is an earlier check that I didn't noticed: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de... https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:32: CHECK(!motion_buffer_); On 2015/03/10 17:31:33, timvolodine wrote: > I wouldn't use CHECK here, seems a bit drastic to crash the whole browser > because of this. Think DCHECK is probably sufficient. Also not sure if > motion_buffer_ can actually be not null in very special cases, like crash of a > renderer.. Changed to DCHECK. That would be a rare case if renderer crashed and no stop message was sent. However the shared memory buffers are created and tracked from within content: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de... So it could be safe to continue to use a non-null buffer. https://codereview.chromium.org/934843002/diff/260001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:66: DCHECK(buffer); On 2015/03/10 17:21:11, flackr wrote: > ditto Done.
jonross@chromium.org changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in ash/content/display/* chromeos/accelerometer/* I've updated AccelerometerReader to handle observers on different threads. It will now notify the observers on the thread which they joined from. I've also updated existing references accordingly. Thanks
lgtm also it's probably worth considering adding some UMA for ChromeOS like we have for other platforms.
jonross@chromium.org changed reviewers: + thakis@chromium.org
thakis@chromium.org: Please review changes in base/observer_list_threadsafe.h I've added a method (HasObserver) to check if an observer has been added. Thanks, Jon
thakis@chromium.org: Please review changes in base/observer_list_threadsafe.h I've added a method (HasObserver) to check if an observer has been added. Thanks, Jon
On 2015/03/10 18:24:44, timvolodine wrote: > lgtm > > also it's probably worth considering adding some UMA for ChromeOS like we have > for other platforms. I will add UMA support in a following change.
lgtm with nit/q. https://codereview.chromium.org/934843002/diff/280001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/934843002/diff/280001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:200: const chromeos::AccelerometerUpdate* update) { any reason not to use scoped_refptr here?
https://codereview.chromium.org/934843002/diff/280001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/280001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:114: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); This is the only place I can see where this AddObserver() method is called. Can't you just keep the "added_observer_" state locally?
https://codereview.chromium.org/934843002/diff/280001/ash/wm/maximize_mode/ma... File ash/wm/maximize_mode/maximize_mode_controller.cc (right): https://codereview.chromium.org/934843002/diff/280001/ash/wm/maximize_mode/ma... ash/wm/maximize_mode/maximize_mode_controller.cc:200: const chromeos::AccelerometerUpdate* update) { On 2015/03/10 22:55:24, oshima wrote: > any reason not to use scoped_refptr here? Nope, I just missed it. Done. https://codereview.chromium.org/934843002/diff/280001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/934843002/diff/280001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:114: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); On 2015/03/11 14:17:06, Nico (traveling) wrote: > This is the only place I can see where this AddObserver() method is called. > Can't you just keep the "added_observer_" state locally? I've removed the HasObserver methods. To update here I've added checks at the call sites. Becoming an observer will only happen when no previous buffer exists. (Instead of an extra flag)
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, oshima@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/934843002/#ps320001 (title: "Remove changes to ObserverListThreadsafe")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/934843002/320001
The CQ bit was unchecked by jonross@chromium.org
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, oshima@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/934843002/#ps340001 (title: "Fix missed test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/934843002/340001
Message was sent while issue was closed.
Committed patchset #14 (id:340001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/d6b77054d45d0192a1fbbb7f893b73be7fdb44cf Cr-Commit-Position: refs/heads/master@{#320159} |
