Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
680383007:
DeviceOrientation API on ChromeOS (Closed)
|
Created:
6 years, 1 month ago by jonross Modified:
5 years, 10 months ago CC:
chromium-reviews, sadrul, riju_, timvolodine, darin-cc_chromium.org, Michael van Ouwerkerk, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDeviceOrientation API on ChromeOS
Create a public SensorManager in content, so that platforms like ChromeOS can pass along sensor information. Currently only accelerometers are available. This manager will be used in the future for the DeviceMotion api as well.
Create an accelerometer observer in ash to pass sensor data to the content shell.
TEST=SensorManagerTest.OrientationBuffer, SensorManagerTest.NeutralOrientation, SensorManagerTest.UpsideDown, SensorManagerTest.BeforeUpsideDownBoundary, SensorManagerTest.LeftEdge, SensorManagerTest.RightEdge, SensorManagerTest.BeforeRightEdgeBoundary
BUG=342908
Patch Set 1 : #
Total comments: 18
Patch Set 2 : Isolate Chrome OS changes #
Total comments: 19
Patch Set 3 : Remove implementation details from content/public/browser #
Total comments: 22
Patch Set 4 : #Patch Set 5 : Move ash files using content into ash/content #
Total comments: 2
Patch Set 6 : Rebase and Rob's comment #
Total comments: 6
Patch Set 7 : Reduce Singleton Usage #
Total comments: 20
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Total comments: 7
Patch Set 10 : Rebase to Updated Accelerometer #Patch Set 11 : Rebase #Messages
Total messages: 41 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
jonross@chromium.org changed reviewers: + flackr@chromium.org, timvolodine@chromium.org
Hi Rob and Tim, In this review I create a public sensor manager in content, so that ash is able to send accelerometer data. I then implement the Screen Orientation API. Could you provide a review? Thanks, Jon https://codereview.chromium.org/680383007/diff/40001/content/public/browser/DEPS File content/public/browser/DEPS (right): https://codereview.chromium.org/680383007/diff/40001/content/public/browser/D... content/public/browser/DEPS:3: ] I intend for this change to DEPS to be temporary. I believe that: content/common/device_sensor/* should be moved to: content/public/common/device_sensor/* But wanted others opinions on that move. I would prepare a separate CL to move the files, and land it first.
Hi Jon, some initial comments below. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_default.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_default.cc:67: SensorManager::GetInstance()->StartFetchingDeviceOrientationData( this line shouldn't be here. the purpose of _default is to provide "null" values on platforms where the api is not supported. looks like this should be in a file called data_fetcher_shared_memory_chromeos.cc instead. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_unittest.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. same here I think this should be sensor_manager_chromeos_unittest https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:22: class SensorManagerTest : public ContentBrowserTest { is this a browser or unittest? browser tests are usually end-to-end, if you only want to test sensor_manager then a unittest seems sufficient. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:28: return SensorManager::GetInstance()->GetOrientationBufferForTest(); for unit testing you can create a buffer manually as e.g. in sensor_manager_android_unittest.cc https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:39: DeviceInertialSensorService::GetInstance(); if the purpose is to test sensor_manager then there is no need to involve DeviceInertialSensorService. Creation of a shared memory buffer is covered in the device_inertial_sensor_browsertest. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... File content/public/browser/sensor_manager.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. can you call this file sensor_manager_chromeos.cc? this would be in line with how other platforms are implemented. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. also I don't understand why this needs to be in content/public/... can this file simply live in content/browser/device_sensors/? Looks like there is some analogy with mac implementation where a sudden_motion_sensor_mac.cc is used from a third_party/. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:23: double z) { on which thread is this method called? do you need an autolock(orientation_buffer_lock_) here? https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:74: // Null message to signify that sensora are no longer available. sensora -> sensors
https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_default.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_default.cc:67: SensorManager::GetInstance()->StartFetchingDeviceOrientationData( On 2014/11/06 02:53:47, timvolodine wrote: > this line shouldn't be here. the purpose of _default is to provide "null" values > on platforms where the api is not supported. > looks like this should be in a file called > data_fetcher_shared_memory_chromeos.cc instead. Done. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_unittest.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/06 02:53:47, timvolodine wrote: > same here I think this should be sensor_manager_chromeos_unittest Done. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:22: class SensorManagerTest : public ContentBrowserTest { On 2014/11/06 02:53:47, timvolodine wrote: > is this a browser or unittest? > browser tests are usually end-to-end, if you only want to test sensor_manager > then a unittest seems sufficient. Just habit from development in ash. I'll switch this to a standalone unittest. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:28: return SensorManager::GetInstance()->GetOrientationBufferForTest(); On 2014/11/06 02:53:47, timvolodine wrote: > for unit testing you can create a buffer manually as e.g. in > sensor_manager_android_unittest.cc Done. https://codereview.chromium.org/680383007/diff/40001/content/browser/device_s... content/browser/device_sensors/sensor_manager_unittest.cc:39: DeviceInertialSensorService::GetInstance(); On 2014/11/06 02:53:47, timvolodine wrote: > if the purpose is to test sensor_manager then there is no need to involve > DeviceInertialSensorService. Creation of a shared memory buffer is covered in > the device_inertial_sensor_browsertest. Done. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... File content/public/browser/sensor_manager.cc (right): https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/06 02:53:47, timvolodine wrote: > also I don't understand why this needs to be in content/public/... > can this file simply live in content/browser/device_sensors/? > Looks like there is some analogy with mac implementation where a > sudden_motion_sensor_mac.cc is used from a third_party/. This file is also called from the ash library. So I placed it in content/public/ so that it is exposed. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:23: double z) { On 2014/11/06 02:53:47, timvolodine wrote: > on which thread is this method called? > do you need an autolock(orientation_buffer_lock_) here? This will be called from the UI thread. I'm adding the lock for safety/consistency. https://codereview.chromium.org/680383007/diff/40001/content/public/browser/s... content/public/browser/sensor_manager.cc:74: // Null message to signify that sensora are no longer available. On 2014/11/06 02:53:47, timvolodine wrote: > sensora -> sensors Done.
thanks Jon, some more comments: https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:11: #include "ui/gfx/geometry/vector3d_f.h" is this needed? https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:16: Shell::GetInstance()->accelerometer_controller()->AddObserver(this); related to comment in shell.cc: unless I am missing something this looks like the OnAccelerometerUpdated will be called even when Device Orientation is not used? https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:38: z); I think a callback (or maybe shared memory) would be more flexible here in general. https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc#newcode1066 ash/shell.cc:1066: sensor_manager_delegate_.reset(new SensorManagerDelegate); does this mean we are continuously observing sensors even when the Device Orientation API is not used? the general idea of Device Orientation is only to 'activate' the sensors on demand. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... File content/public/browser/sensor_manager.cc (right): https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.cc:21: void SensorManager::OnAccelerationIncludingGravity(double x, you could probably also implement Device Motion in one go using this, unless you want to do that in a separate patch.. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... File content/public/browser/sensor_manager.h (right): https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. is there any chance we can put it in content/browser/device_sensors/ with the other platform implementations? if you need to provide updates you can use base::Callback<..> instead of exposing the whole class. Alternatively you could pass the shared memory handle to the delegate in ash/. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.h:21: class CONTENT_EXPORT SensorManager { this is chrome OS specific right? so probably should be called SensorManagerChromeOS.
https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:30: if (!update.has(source)) nit, I think this would be a bit cleaner as an if/else if/else return or if you want to avoid repeating the enum value a local array of sources in preferred order. https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.h:15: virtual ~SensorManagerDelegate(); nit: I think the pattern we use now is ~SensorManagerDelegate() override; https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc#newcode1066 ash/shell.cc:1066: sensor_manager_delegate_.reset(new SensorManagerDelegate); On 2014/11/07 03:27:39, timvolodine wrote: > does this mean we are continuously observing sensors even when the Device > Orientation API is not used? > the general idea of Device Orientation is only to 'activate' the sensors on > demand. Agreed, we should try to limit the lifetime of this, and eventually tell AccelerometerReader to increase the rate to 60Hz while the API is being used. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... File content/public/browser/sensor_manager.cc (right): https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.cc:30: // accelerometer values have been reversed. Can you file a bug for this?
https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:11: #include "ui/gfx/geometry/vector3d_f.h" On 2014/11/07 03:27:39, timvolodine wrote: > is this needed? Nope, will be removed in next patch. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... File content/public/browser/sensor_manager.cc (right): https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.cc:21: void SensorManager::OnAccelerationIncludingGravity(double x, On 2014/11/07 03:27:39, timvolodine wrote: > you could probably also implement Device Motion in one go using this, unless you > want to do that in a separate patch.. I was planning to use this patch to settle on how we want content/ and ash/ to communicate the accelerometer updates. I plan to implement Device Motion in a follow up patch. https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.cc:30: // accelerometer values have been reversed. On 2014/11/07 19:30:46, flackr wrote: > Can you file a bug for this? crbug.com/431391 filed https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... File content/public/browser/sensor_manager.h (right): https://codereview.chromium.org/680383007/diff/60001/content/public/browser/s... content/public/browser/sensor_manager.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/07 03:27:39, timvolodine wrote: > is there any chance we can put it in content/browser/device_sensors/ with the > other platform implementations? > if you need to provide updates you can use base::Callback<..> instead of > exposing the whole class. Alternatively you could pass the shared memory handle > to the delegate in ash/. I'm going to go with the Callback suggestion. content/ cannot depend on ash/, so I will need a small class in content/public/browser. However to minimize it I think a delegate which ash can register would be appropriate. Delegate { GetInstance() SetInstance(Delegate) StartOrientation(Callback) = 0 StopOrientation() = 0 } This will let the logic of parsing the values move to content/browser/device_sensors. It will also allow ash/ to only start listening to the accelerometer once told to start. I prefer this to the SharedMemory approach, as that would require exposing all of the hardware buffers to ash/
I have removed the implementation details from content/public/browser/ content/public/browser/device_sensor_delegate.h is now used solely as an intermediary between content/ and ash/ The communication has been switched to make use of a Callback, which is provided to ash/ Ash/ no longer constantly listens to accelerometer data, instead it waits to be told to start by content/ https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:30: if (!update.has(source)) On 2014/11/07 19:30:46, flackr wrote: > nit, I think this would be a bit cleaner as an if/else if/else return or if you > want to avoid repeating the enum value a local array of sources in preferred > order. Done. https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.cc:38: z); On 2014/11/07 03:27:39, timvolodine (ooo 18Nov) wrote: > I think a callback (or maybe shared memory) would be more flexible here in > general. Switched to using a callback https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/60001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate.h:15: virtual ~SensorManagerDelegate(); On 2014/11/07 19:30:46, flackr wrote: > nit: I think the pattern we use now is ~SensorManagerDelegate() override; Done. https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/680383007/diff/60001/ash/shell.cc#newcode1066 ash/shell.cc:1066: sensor_manager_delegate_.reset(new SensorManagerDelegate); On 2014/11/07 19:30:46, flackr wrote: > On 2014/11/07 03:27:39, timvolodine wrote: > > does this mean we are continuously observing sensors even when the Device > > Orientation API is not used? > > the general idea of Device Orientation is only to 'activate' the sensors on > > demand. > > Agreed, we should try to limit the lifetime of this, and eventually tell > AccelerometerReader to increase the rate to 60Hz while the API is being used. Observing will now only occur after the delegate has been told to commence listening
https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:21: if (observing_accelerometer_) Call StopFetchingDeviceOrientationData? https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:40: double z = update.get(source).z(); Variables only used once, just call update.get(source).[xyz]() in .Run(...) https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:47: orientation_callback_ = callback; maybe D?CHECK(orientation_callback_.is_null()) to be sure we never overwrite this. https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:53: observing_accelerometer_ = false; orientation_callback_.Reset(); https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.h:37: bool observing_accelerometer_; This should be the same as orientation_callback_.is_null() shouldn't it? https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:79: // Create a unit vector to remove gravity. Did you mean for a TODO? It doesn't look like gravity's being removed. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:82: gfx::Vector3dF data(x, y ? -y : 0.0f, z ? -z : 0.0f); Isn't this just the following? (i.e. why the ternary's?) gfx::Vector3dF data(x, -y, -z); https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:93: // Have the interval boundaries comply with the specification. Beta is nit: Convert beta and gamma to fit the intervals in specification. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:94: // [-180,180) and gamma is [-90, 90). nit: [-180, 180) https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:99: orientation_buffer_->seqlock.WriteBegin(); Should we be moving this to accelerometer_reader.cc to avoid passing values through the UI thread? Maybe a TODO. https://codereview.chromium.org/680383007/diff/80001/content/public/browser/s... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/80001/content/public/browser/s... content/public/browser/sensor_manager_delegate.h:37: const base::Callback<void(double, double, double)>& callback) = 0; Can you typedef this callback type? Include x, y, z var names in typedef.
https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:21: if (observing_accelerometer_) On 2014/11/13 00:07:34, flackr wrote: > Call StopFetchingDeviceOrientationData? Done. https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:40: double z = update.get(source).z(); On 2014/11/13 00:07:34, flackr wrote: > Variables only used once, just call update.get(source).[xyz]() in .Run(...) Done. https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:47: orientation_callback_ = callback; On 2014/11/13 00:07:34, flackr wrote: > maybe D?CHECK(orientation_callback_.is_null()) to be sure we never overwrite > this. Done. https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.cc:53: observing_accelerometer_ = false; On 2014/11/13 00:07:34, flackr wrote: > orientation_callback_.Reset(); Done. https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... File ash/accelerometer/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/80001/ash/accelerometer/sensor... ash/accelerometer/sensor_manager_delegate_chromeos.h:37: bool observing_accelerometer_; On 2014/11/13 00:07:34, flackr wrote: > This should be the same as orientation_callback_.is_null() shouldn't it? Done. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:79: // Create a unit vector to remove gravity. On 2014/11/13 00:07:34, flackr wrote: > Did you mean for a TODO? It doesn't look like gravity's being removed. Poor comment, as this smooths out all acceleration. The unit vector was to simply the trig. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:82: gfx::Vector3dF data(x, y ? -y : 0.0f, z ? -z : 0.0f); On 2014/11/13 00:07:34, flackr wrote: > Isn't this just the following? (i.e. why the ternary's?) > gfx::Vector3dF data(x, -y, -z); Comment added, as -0.0f messed up trig https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:93: // Have the interval boundaries comply with the specification. Beta is On 2014/11/13 00:07:34, flackr wrote: > nit: Convert beta and gamma to fit the intervals in specification. Done. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:94: // [-180,180) and gamma is [-90, 90). On 2014/11/13 00:07:34, flackr wrote: > nit: [-180, 180) Done. https://codereview.chromium.org/680383007/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:99: orientation_buffer_->seqlock.WriteBegin(); On 2014/11/13 00:07:34, flackr wrote: > Should we be moving this to accelerometer_reader.cc to avoid passing values > through the UI thread? Maybe a TODO. TODO added https://codereview.chromium.org/680383007/diff/80001/content/public/browser/s... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/80001/content/public/browser/s... content/public/browser/sensor_manager_delegate.h:37: const base::Callback<void(double, double, double)>& callback) = 0; On 2014/11/13 00:07:34, flackr wrote: > Can you typedef this callback type? Include x, y, z var names in typedef. Done.
LGTM https://codereview.chromium.org/680383007/diff/120001/content/public/browser/... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/120001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:17: typedef base::Callback<void(double, double, double)> OrientationDataXYZCallback; nit: double x, double y, double z Also s/OrientationDataXYZCallback/OrientationDataCallback
https://codereview.chromium.org/680383007/diff/120001/content/public/browser/... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/120001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:17: typedef base::Callback<void(double, double, double)> OrientationDataXYZCallback; On 2014/11/13 21:09:22, flackr wrote: > nit: double x, double y, double z > Also s/OrientationDataXYZCallback/OrientationDataCallback Done.
Hi Tim, Would you be able to take another look at this review for the Device Orientation API Thanks, Jon
looks much better thanks! below some more comments let me know what you think. https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. also should this file be called _chromeos, since it's chromeOS specific? https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:23: class CONTENT_EXPORT SensorManagerDelegate { The implementation uses lots of singletons (e.g. both for sensor_manager_chromeos and sensor_manager_delegate), can we get rid of them? Here is an idea: 1. Have the sensor_manager_chromeos own a delegate as member (e.g. as scoped_ptr) 2. In sensor_manager_delegate.h: class SensorManagerDelegate { public: static scoped_ptr<SensorManagerDelegate> Create(const OrientationDataCallback& callback); virtual StartFetchingDeviceOrientationData() = 0; virtual StopFetchingDeviceOrientationData() = 0; }; sensor_manager_delegate_chromeos.cc would then implement a definition for Create() using "new SensorManagerDelegateChromeOS". to init its delegate the sensor_manager_chromeos would do: sensor_manager_delegate_ = SensorManagerDelegate::Create(callback); Does that make sense? We did a similar thing with Battery Status API (currently in /device/battery/) I wonder if we can apply that pattern here as well. https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:29: static SensorManagerDelegate* GetInstance(); you have both GetInstance() and public constructor methods which is somewhat unsafe and unclear what should be used..
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Updated with clean up of SensorManagerDelegateChromeOS. This class is no longer a singleton. I did not pursue the static Create method approach. This causes issues when trying to export implementation from different libraries. Instead I have a added static SensorManagerDelegateChromnOS::SetDelegate which acts as a pass through to content/browser/device_sensors/. Thus leaving SensorManagerChromeOS as the singleton, holding a reference to the given delegate. https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... File content/public/browser/sensor_manager_delegate.h (right): https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/11/24 14:51:05, timvolodine wrote: > also should this file be called _chromeos, since it's chromeOS specific? Done. https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:23: class CONTENT_EXPORT SensorManagerDelegate { This poses some problems with how the code is currently distributed between content and ash. I can get this to link, however it requires that ash implement and export the static Create method. I don't like having a cross library linking dependency. This also becomes trouble when the content_unittests tries to create a test stub. That executable is not able to export the implementation of Create to the content library. Currently crbug.com/431865 tracks an issue where we intend to move the reading of the hardware accelerometer to content/browser/device_sensors. With that change this delegate can be removed, and a public observer can be created for ash to listen for changes. I've switched this class to be not a singleton, but to pass the delegate to SensorManagerChromeOS. I'll add TODO to change this once the above issue is resolved. https://codereview.chromium.org/680383007/diff/140001/content/public/browser/... content/public/browser/sensor_manager_delegate.h:29: static SensorManagerDelegate* GetInstance(); On 2014/11/24 14:51:05, timvolodine wrote: > you have both GetInstance() and public constructor methods which is somewhat > unsafe and unclear what should be used.. I've moved the constructor to protected. GetInstance has been removed with this no longer being a singleton.
ok thanks for the explanation, I didn't realize you had a TODO and a crbug for this -- would be great to get rid of the pass-through delegate. a few more comments below. https://codereview.chromium.org/680383007/diff/200001/ash/content/acceleromet... File ash/content/accelerometer/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/200001/ash/content/acceleromet... ash/content/accelerometer/sensor_manager_delegate_chromeos.cc:21: // Content Shell will have closed itself, do not remove as delegate nit: "." and the end also not sure what this comment actually means? https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:27: : public base::RefCountedThreadSafe<SensorManagerChromeOS> { are you sure about this? the class is a singleton why does it need to be ref-counted? https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:30: static SensorManagerChromeOS* GetInstance(); ideally we should get rid of this singleton as well. maybe add a todo to consider that once crbug.com/431865 is done. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:62: SensorManagerDelegateChromeOS* delegate_; I assume Shell owns (and destructs) this. it's probably fine, but to double-check: can the delegate_ be used after it is destructed by Shell? https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:64: SensorManagerChromeOS(); this should come before the members https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:68: void OnAccelerationIncludingGravity(double x, double y, double z); this also before the members https://codereview.chromium.org/680383007/diff/200001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/200001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.cc:20: // TODO(jonross): Once accelerometer reading has been moved to I think this comment could go in the header file above the class declaration, that way it's more visible and relates to the whole delegate.
I've provided clarifications to some of your questions. I will be OOO until Monday, December 1st. I will update the change to address your comments then. https://codereview.chromium.org/680383007/diff/200001/ash/content/acceleromet... File ash/content/accelerometer/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/200001/ash/content/acceleromet... ash/content/accelerometer/sensor_manager_delegate_chromeos.cc:21: // Content Shell will have closed itself, do not remove as delegate On 2014/11/26 19:32:19, timvolodine wrote: > nit: "." and the end > also not sure what this comment actually means? When this class is being destructed, content::SensorManagerDelegateChromeOS will have already been destructed. So calling SetDelegate(nullptr) to remove this class as the delegate will cause a crash. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:27: : public base::RefCountedThreadSafe<SensorManagerChromeOS> { On 2014/11/26 19:32:19, timvolodine wrote: > are you sure about this? the class is a singleton why does it need to be > ref-counted? This is based on the example from callback.h for binding a class method to be used across threads. https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=54 I did not test this without the RefCountedThreadSafe, so I am not sure if it would be fine. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:62: SensorManagerDelegateChromeOS* delegate_; On 2014/11/26 19:32:19, timvolodine wrote: > I assume Shell owns (and destructs) this. it's probably fine, but to > double-check: can the delegate_ be used after it is destructed by Shell? After the Shell destructs delegate_ it cannot be used. However content::SensorManagerChromeOS is destructed before Shell destructs the delegate_
https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:20: class SensorManagerChromeOSTest; no need for this? https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:27: : public base::RefCountedThreadSafe<SensorManagerChromeOS> { On 2014/11/26 23:17:56, jonross (OOO Nov 27-30) wrote: > On 2014/11/26 19:32:19, timvolodine wrote: > > are you sure about this? the class is a singleton why does it need to be > > ref-counted? > > This is based on the example from callback.h for binding a class method to be > used across threads. > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=54 > > I did not test this without the RefCountedThreadSafe, so I am not sure if it > would be fine. normally you can ensure the callback of a singleton will live long enough, i.e. during shutdown StopFetchingAllDeviceData() is guaranteed to be called (in device_inertial_sensor_service). It seems you could drop reference counting and init the callback at construction time to: base::Bind(&SensorManagerChromeOS::OnAccelerationIncludingGravity, base::Unretained(this)); https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:62: SensorManagerDelegateChromeOS* delegate_; On 2014/11/26 23:17:56, jonross (OOO Nov 27-30) wrote: > On 2014/11/26 19:32:19, timvolodine wrote: > > I assume Shell owns (and destructs) this. it's probably fine, but to > > double-check: can the delegate_ be used after it is destructed by Shell? > > After the Shell destructs delegate_ it cannot be used. > > However content::SensorManagerChromeOS is destructed before Shell destructs the > delegate_ ok, that's what I was wondering: the order of destruction. generally speaking I think it would be good to expose something from chromeOS to content instead of having this delegate. Something similar like we did for battery api?: e.g. device/battery/battery_status_manager_chromeos.cc chromeos::PowerManagerClient* power_client = chromeos::DBusThreadManager::Get()->GetPowerManagerClient(); power_client->AddObserver(this); that way the content::SensorManagerChromeOS doesn't have to be a singleton. But that's probably something you had in mind for the TODO already..
https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:20: class SensorManagerChromeOSTest; On 2014/11/28 17:46:06, timvolodine wrote: > no need for this? Currently used for calling OnAccelerationIncludingGravity. However I have no objections to making that method public. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:30: static SensorManagerChromeOS* GetInstance(); On 2014/11/26 19:32:19, timvolodine wrote: > ideally we should get rid of this singleton as well. maybe add a todo to > consider that once crbug.com/431865 is done. Done. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:62: SensorManagerDelegateChromeOS* delegate_; On 2014/11/28 17:46:06, timvolodine wrote: > On 2014/11/26 23:17:56, jonross (OOO Nov 27-30) wrote: > > On 2014/11/26 19:32:19, timvolodine wrote: > > > I assume Shell owns (and destructs) this. it's probably fine, but to > > > double-check: can the delegate_ be used after it is destructed by Shell? > > > > After the Shell destructs delegate_ it cannot be used. > > > > However content::SensorManagerChromeOS is destructed before Shell destructs > the > > delegate_ > > ok, that's what I was wondering: the order of destruction. > > generally speaking I think it would be good to expose something from chromeOS to > content instead of having this delegate. Something similar like we did for > battery api?: > > e.g. device/battery/battery_status_manager_chromeos.cc > chromeos::PowerManagerClient* power_client = > chromeos::DBusThreadManager::Get()->GetPowerManagerClient(); > power_client->AddObserver(this); > > that way the content::SensorManagerChromeOS doesn't have to be a singleton. But > that's probably something you had in mind for the TODO already.. Extra documentation and a TODO added. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:64: SensorManagerChromeOS(); On 2014/11/26 19:32:19, timvolodine wrote: > this should come before the members Done. https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:68: void OnAccelerationIncludingGravity(double x, double y, double z); On 2014/11/26 19:32:19, timvolodine wrote: > this also before the members Done. https://codereview.chromium.org/680383007/diff/200001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/200001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.cc:20: // TODO(jonross): Once accelerometer reading has been moved to On 2014/11/26 19:32:19, timvolodine wrote: > I think this comment could go in the header file above the class declaration, > that way it's more visible and relates to the whole delegate. Done.
thanks Jon, lgtm https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/680383007/diff/200001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.h:20: class SensorManagerChromeOSTest; On 2014/12/02 16:13:36, jonross wrote: > On 2014/11/28 17:46:06, timvolodine wrote: > > no need for this? > > Currently used for calling OnAccelerationIncludingGravity. > > However I have no objections to making that method public. I think you can drop this line, you already have a friend class declaration below. https://codereview.chromium.org/680383007/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:61: // Null message to signify that sensors are no longer available nit: is not really a null message, maybe better something like: "make sure to indicate that the sensor data is no longer available" https://codereview.chromium.org/680383007/diff/220001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/220001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. crbug.com/431865 nit: ". crbug.com/431865" -> "(crbug.com/431865)."
https://codereview.chromium.org/680383007/diff/220001/content/browser/device_... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/680383007/diff/220001/content/browser/device_... content/browser/device_sensors/sensor_manager_chromeos.cc:61: // Null message to signify that sensors are no longer available On 2014/12/03 13:19:55, timvolodine wrote: > nit: is not really a null message, maybe better something like: "make sure to > indicate that the sensor data is no longer available" Done. https://codereview.chromium.org/680383007/diff/220001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/220001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. crbug.com/431865 On 2014/12/03 13:19:55, timvolodine wrote: > nit: ". crbug.com/431865" -> "(crbug.com/431865)." Done.
jonross@chromium.org changed reviewers: + oshima@chromium.org
oshima@chromium.org: Please review changes in ash/ ash/content/accelerometer/ In this review I am implementing the DeviceOrientation API on Chrome OS. Where accelerometer data read in ash is sent to the content shell. Thanks, Jon
jonross@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: Please review changes in content/browser/BUILD.gn content/public/browser/ Thanks, Jon
ash lgtm https://codereview.chromium.org/680383007/diff/240001/ash/content/acceleromet... File ash/content/accelerometer/DEPS (right): https://codereview.chromium.org/680383007/diff/240001/ash/content/acceleromet... ash/content/accelerometer/DEPS:2: "+content/public/browser/sensor_manager_delegate_chromeos.h", or just "+content/public/browser" I'm fine with either way tho.
Hi Avi, Would you be able to provide the owners review to: content/browser/BUILD.gn content/public/browser/* Thanks, Jon
jonross@chromium.org changed reviewers: + creis@chromium.org - avi@chromium.org
Hi Charlie, Could you provide an owner review to: content/browser/BUILD.gn content/public/browser/* Thanks, Jon
creis@chromium.org changed reviewers: + jam@chromium.org
@jam: Can you weigh in on the new ChromeOS-specific delegate (and proposed observer) in content/public/browser? I'm not sure if there's a better class these might belong on, rather than introducing a new concept in the API. https://codereview.chromium.org/680383007/diff/240001/ash/content/acceleromet... File ash/content/accelerometer/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/240001/ash/content/acceleromet... ash/content/accelerometer/sensor_manager_delegate_chromeos.h:18: class SensorManagerDelegateChromeOS This class name should probably be prefixed with Ash, per the Content API guidelines: http://www.chromium.org/developers/content-module/content-api "when code in chrome implements an interface from content, usually the convention is to prefix the implementation with "Chrome" (i.e. ChromeContentBrowserClient derives from content::ContentBrowserClient)" https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:17: // For platforms with a push API for accelerometer sensors this delegate can be "For platforms with" vs "ChromeOS" in the class name seems inconsistent to me. I feel like we probably shouldn't mention ChromeOS in the class name if we keep this. https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. (crbug.com/431865) Avoiding this delegate in the public interface would be nice if possible. Is there a lot of work required for this?
https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. (crbug.com/431865) On 2014/12/10 01:02:41, Charlie Reis wrote: > Avoiding this delegate in the public interface would be nice if possible. Is > there a lot of work required for this? It's a decent amount of work. I had been planning to address it in m-42 as that move would directly conflict with some other in progress work.
https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. (crbug.com/431865) On 2014/12/10 13:58:27, jonross wrote: > On 2014/12/10 01:02:41, Charlie Reis wrote: > > Avoiding this delegate in the public interface would be nice if possible. Is > > there a lot of work required for this? > > It's a decent amount of work. I had been planning to address it in m-42 as that > move would directly conflict with some other in progress work. can you explain some more on what work is needed? why can't this be directly implemented inside content?
https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... File content/public/browser/sensor_manager_delegate_chromeos.h (right): https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... content/public/browser/sensor_manager_delegate_chromeos.h:22: // content/public/browser expose an observer for ChromeOS. (crbug.com/431865) On 2014/12/10 21:57:05, jam wrote: > On 2014/12/10 13:58:27, jonross wrote: > > On 2014/12/10 01:02:41, Charlie Reis wrote: > > > Avoiding this delegate in the public interface would be nice if possible. > Is > > > there a lot of work required for this? > > > > It's a decent amount of work. I had been planning to address it in m-42 as > that > > move would directly conflict with some other in progress work. > > can you explain some more on what work is needed? > > why can't this be directly implemented inside content? My end goal is to have the ChromeOS accelerometer read within content/ and for other libraries, such as ash/, that need the information to listen for changes. Work that needs to be done: chromeos/accelerometer/accelerometer_reader.h needs to be moved into content/browser/device_sensors/ ash/accelerometer/accelerometer_controller.h needs to be replaced. I was planning on having content/browser/device_sensors/sensor_manager_chromeos.h replace the portion responsible for initializing AccelerometerReader. For the portion that notifies observers, I was going to move that to a new class in content/public/browser/, as ash/ still needs to listen for accelerometer events/ Currently the other ash code that uses the accelerometer is undergoing some significant changes. I had planned to perform the accelerometer move after both changes had landed in order to have it be one change, rather than having to increase the complexity of either review. The accelerometer code is also currently used in athena/ and I had yet to review what changes are needed there, or if it can currently depend on content/
On 2014/12/10 23:06:22, jonross wrote: > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > File content/public/browser/sensor_manager_delegate_chromeos.h (right): > > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > content/public/browser/sensor_manager_delegate_chromeos.h:22: // > content/public/browser expose an observer for ChromeOS. (crbug.com/431865) > On 2014/12/10 21:57:05, jam wrote: > > On 2014/12/10 13:58:27, jonross wrote: > > > On 2014/12/10 01:02:41, Charlie Reis wrote: > > > > Avoiding this delegate in the public interface would be nice if possible. > > Is > > > > there a lot of work required for this? > > > > > > It's a decent amount of work. I had been planning to address it in m-42 as > > that > > > move would directly conflict with some other in progress work. > > > > can you explain some more on what work is needed? > > > > why can't this be directly implemented inside content? > > My end goal is to have the ChromeOS accelerometer read within content/ and for > other libraries, such as ash/, that need the information to listen for changes. > > Work that needs to be done: > chromeos/accelerometer/accelerometer_reader.h needs to be moved into > content/browser/device_sensors/ > > ash/accelerometer/accelerometer_controller.h needs to be replaced. I was > planning on having content/browser/device_sensors/sensor_manager_chromeos.h > replace the portion responsible for initializing AccelerometerReader. For the > portion that notifies observers, I was going to move that to a new class in > content/public/browser/, as ash/ still needs to listen for accelerometer events/ i'm curious, why not have that in src/chromeos? it seems like system-level code, i.e. what we would call windows/mac/linux OS code for, should be in src/chromeos > > Currently the other ash code that uses the accelerometer is undergoing some > significant changes. I had planned to perform the accelerometer move after both > changes had landed in order to have it be one change, rather than having to > increase the complexity of either review. > > The accelerometer code is also currently used in athena/ and I had yet to review > what changes are needed there, or if it can currently depend on content/
On 2014/12/10 23:08:38, jam wrote: > On 2014/12/10 23:06:22, jonross wrote: > > > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > > File content/public/browser/sensor_manager_delegate_chromeos.h (right): > > > > > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > > content/public/browser/sensor_manager_delegate_chromeos.h:22: // > > content/public/browser expose an observer for ChromeOS. (crbug.com/431865) > > On 2014/12/10 21:57:05, jam wrote: > > > On 2014/12/10 13:58:27, jonross wrote: > > > > On 2014/12/10 01:02:41, Charlie Reis wrote: > > > > > Avoiding this delegate in the public interface would be nice if > possible. > > > Is > > > > > there a lot of work required for this? > > > > > > > > It's a decent amount of work. I had been planning to address it in m-42 as > > > that > > > > move would directly conflict with some other in progress work. > > > > > > can you explain some more on what work is needed? > > > > > > why can't this be directly implemented inside content? > > > > My end goal is to have the ChromeOS accelerometer read within content/ and for > > other libraries, such as ash/, that need the information to listen for > changes. > > > > Work that needs to be done: > > chromeos/accelerometer/accelerometer_reader.h needs to be moved into > > content/browser/device_sensors/ > > > > ash/accelerometer/accelerometer_controller.h needs to be replaced. I was > > planning on having content/browser/device_sensors/sensor_manager_chromeos.h > > replace the portion responsible for initializing AccelerometerReader. For the > > portion that notifies observers, I was going to move that to a new class in > > content/public/browser/, as ash/ still needs to listen for accelerometer > events/ > > i'm curious, why not have that in src/chromeos? it seems like system-level code, > i.e. what we would call windows/mac/linux OS code for, should be in src/chromeos > > > > > Currently the other ash code that uses the accelerometer is undergoing some > > significant changes. I had planned to perform the accelerometer move after > both > > changes had landed in order to have it be one change, rather than having to > > increase the complexity of either review. > > > > The accelerometer code is also currently used in athena/ and I had yet to > review > > what changes are needed there, or if it can currently depend on content/ ash/accelerometer/accelerometer_controller.h was created to abstract the OS accelerometer form ash/ which is why it isn't in src/chromeos. ui/accelerometer/accelerometer_types.h was created to provide type definitions for accelerometer messages, without tieing them to the OS. chromeos/accelerometer/accelerometer_reader.h was created in the location for system-level code. However this abstraction has led to chromeos/ depending on ui/ which we have been asked to remove. (crbug.com/431865) content/browser/device_sensors/ could certainly just call into chromeos/. However AccelerometerReader is not setup to support multiple threads reading it, and its current owner is in ash/
On 2014/12/10 23:55:40, jonross wrote: > On 2014/12/10 23:08:38, jam wrote: > > On 2014/12/10 23:06:22, jonross wrote: > > > > > > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > > > File content/public/browser/sensor_manager_delegate_chromeos.h (right): > > > > > > > > > https://codereview.chromium.org/680383007/diff/240001/content/public/browser/... > > > content/public/browser/sensor_manager_delegate_chromeos.h:22: // > > > content/public/browser expose an observer for ChromeOS. (crbug.com/431865) > > > On 2014/12/10 21:57:05, jam wrote: > > > > On 2014/12/10 13:58:27, jonross wrote: > > > > > On 2014/12/10 01:02:41, Charlie Reis wrote: > > > > > > Avoiding this delegate in the public interface would be nice if > > possible. > > > > Is > > > > > > there a lot of work required for this? > > > > > > > > > > It's a decent amount of work. I had been planning to address it in m-42 > as > > > > that > > > > > move would directly conflict with some other in progress work. > > > > > > > > can you explain some more on what work is needed? > > > > > > > > why can't this be directly implemented inside content? > > > > > > My end goal is to have the ChromeOS accelerometer read within content/ and > for > > > other libraries, such as ash/, that need the information to listen for > > changes. > > > > > > Work that needs to be done: > > > chromeos/accelerometer/accelerometer_reader.h needs to be moved into > > > content/browser/device_sensors/ > > > > > > ash/accelerometer/accelerometer_controller.h needs to be replaced. I was > > > planning on having content/browser/device_sensors/sensor_manager_chromeos.h > > > replace the portion responsible for initializing AccelerometerReader. For > the > > > portion that notifies observers, I was going to move that to a new class in > > > content/public/browser/, as ash/ still needs to listen for accelerometer > > events/ > > > > i'm curious, why not have that in src/chromeos? it seems like system-level > code, > > i.e. what we would call windows/mac/linux OS code for, should be in > src/chromeos > > > > > > > > Currently the other ash code that uses the accelerometer is undergoing some > > > significant changes. I had planned to perform the accelerometer move after > > both > > > changes had landed in order to have it be one change, rather than having to > > > increase the complexity of either review. > > > > > > The accelerometer code is also currently used in athena/ and I had yet to > > review > > > what changes are needed there, or if it can currently depend on content/ > > ash/accelerometer/accelerometer_controller.h was created to abstract the OS > accelerometer form ash/ which is why it isn't in src/chromeos. > > ui/accelerometer/accelerometer_types.h was created to provide type definitions > for accelerometer messages, without tieing them to the OS. > > chromeos/accelerometer/accelerometer_reader.h was created in the location for > system-level code. > > However this abstraction has led to chromeos/ depending on ui/ which we have > been asked to remove. (crbug.com/431865) > > content/browser/device_sensors/ could certainly just call into chromeos/. > However AccelerometerReader is not setup to support multiple threads reading it, > and its current owner is in ash/ seems that should be fixed, instead of adding this layer |
