Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
856123002:
Device Orientation API on Chrome OS (Closed)
|
Created:
5 years, 11 months ago by jonross Modified:
5 years, 10 months ago CC:
chromium-reviews, riju_, Michael van Ouwerkerk, jam, timvolodine, darin-cc_chromium.org, mlamouri+watch-sensors_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevice Orientation API on Chrome OS
Implement DataFetcherSharedMemory on Chrome OS to provide Device Orientation support. Create SensorManagerChromeOS to observe accelerometer events, and to generate updates to the Device Orientation API.
This is a follow up to: https://codereview.chromium.org/680383007/
With the accelerometer on Chrome OS now exposed to content/ from chromeos/accelerometer, no ash/ implementation is required. No public api in content/public/browser is needed either.
TEST=SensorManagerTest.OrientationBuffer, SensorManagerTest.NeutralOrientation, SensorManagerTest.UpsideDown, SensorManagerTest.BeforeUpsideDownBoundary, SensorManagerTest.LeftEdge, SensorManagerTest.RightEdge, SensorManagerTest.BeforeRightEdgeBoundary
BUG=342908
Committed: https://crrev.com/e717460964dd27a7239abec31462441eb971edbe
Cr-Commit-Position: refs/heads/master@{#314605}
Patch Set 1 : #
Total comments: 18
Patch Set 2 : #Patch Set 3 : #
Total comments: 25
Patch Set 4 : Rebase #Patch Set 5 : #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Update to reflect chromeos accelerometer changes #Messages
Total messages: 30 (10 generated)
Patchset #1 (id:1) has been deleted
jonross@chromium.org changed reviewers: + timvolodine@chromium.org
Hi Tim, Previously you reviewed my work on implementing the Device Orientation API on Chrome OS. After the discussion on that review I have separately updated how accelerometers are observer on Chrome OS. This has allowed me to change the review significantly from it's original state. I've uploaded a new review based on this. Could you review it? Thanks, Jon
Hi Jon, yes looks much better indeed thanks! I've dropped some comments below. thanks, Tim https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:19: if (!sensor_manager_.get()) nit: think you can drop ".get" which results in shorter statement https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:23: case CONSUMER_TYPE_MOTION: is there a todo needed here? any relevant crbug? https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:19: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); it looks a bit strange you are adding an observer on creation and removing only on destruction. it seems adding an observer on StartFetchingDeviceOrientation and removing it on StopFetchingDeviceOrientation would be a more logical approach? wdyt? https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:99: // TODO: move chromeos/accelerometer to /content/browser/device_sensors/ and is this still relevant? https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:15: struct DefaultSingletonTraits; is this needed? https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:19: class SensorManagerChromeOSTest; is this needed? https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:23: class CONTENT_EXPORT SensorManagerChromeOS you could probably put this class entirely inside an anonymous namespace in data_fetcher_shared_memory_chromeos.cc since it does not look like it needs to be exposed anywhere else.. however I am fine if you prefer to keep it in a separate file. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:41: sensor_manager_.reset(new SensorManagerChromeOS); you are using a real implementation here? does this actually hook into chromeOS? I would expect a fake sensor manager but maybe I am missing something..
https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:19: if (!sensor_manager_.get()) On 2015/01/21 13:16:57, timvolodine wrote: > nit: think you can drop ".get" which results in shorter statement Done. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:23: case CONSUMER_TYPE_MOTION: On 2015/01/21 13:16:57, timvolodine wrote: > is there a todo needed here? any relevant crbug? Added a TODO with relevant CR bug https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:19: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); On 2015/01/21 13:16:57, timvolodine wrote: > it looks a bit strange you are adding an observer on creation and removing only > on destruction. it seems adding an observer on StartFetchingDeviceOrientation > and removing it on StopFetchingDeviceOrientation would be a more logical > approach? wdyt? I had originally considered that. However when I implement Device Motion API this class will be handling separate buffers for the different APIs, all from the same accelerometer source. I thought that this would be cleaner than tracking which buffers are in use at each start/stop. Though I would not object to changing it. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:99: // TODO: move chromeos/accelerometer to /content/browser/device_sensors/ and On 2015/01/21 13:16:57, timvolodine wrote: > is this still relevant? Nope, missed removing it. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:15: struct DefaultSingletonTraits; On 2015/01/21 13:16:57, timvolodine wrote: > is this needed? Nope, missed removing it. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:19: class SensorManagerChromeOSTest; On 2015/01/21 13:16:57, timvolodine wrote: > is this needed? Done. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:23: class CONTENT_EXPORT SensorManagerChromeOS On 2015/01/21 13:16:57, timvolodine wrote: > you could probably put this class entirely inside an anonymous namespace in > data_fetcher_shared_memory_chromeos.cc since it does not look like it needs to > be exposed anywhere else.. however I am fine if you prefer to keep it in a > separate file. I had considered that. However keeping it separate makes testing easier. So I plan to keep separate. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:41: sensor_manager_.reset(new SensorManagerChromeOS); On 2015/01/21 13:16:57, timvolodine wrote: > you are using a real implementation here? does this actually hook into chromeOS? > I would expect a fake sensor manager but maybe I am missing something.. I am testing a real sensor manager here. However in unittests there is nothing generating accelerometer events, so the only tie to chromeos/accelerometer is the adding/removing of the observer. Accelerometer events are injected via OnAccelerationIncludingGravity (line 26). Thus bypassing the chromeos/accelerometer and letting me just test the results of SensorManagerChromeOS handling data.
https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:19: chromeos::AccelerometerReader::GetInstance()->AddObserver(this); On 2015/01/21 16:02:11, jonross wrote: > On 2015/01/21 13:16:57, timvolodine wrote: > > it looks a bit strange you are adding an observer on creation and removing > only > > on destruction. it seems adding an observer on StartFetchingDeviceOrientation > > and removing it on StopFetchingDeviceOrientation would be a more logical > > approach? wdyt? > > I had originally considered that. However when I implement Device Motion API > this class will be handling separate buffers for the different APIs, all from > the same accelerometer source. I thought that this would be cleaner than > tracking which buffers are in use at each start/stop. > > Though I would not object to changing it. I think the add/remove observer should generally correspond to the Start/Stop semantics. It does not make sense to have an observer when nobody is listening. Looking at your approach it seems that you could: 1. put the add/remove observer logic (and future logic) in sensor_manager corresponding to start/stop methods, or 2. remove the start/stop methods from sensor_manager and perform add/remove observer on construction/destruction as it is now. In this case the class would probably be better called sensor_manager_observer and the logic for creating/destroying would be handled by data_fetcher_shared_memory. (i.e. having a live sensor_manager means we are listening) The problem with 2 is that it can be tricky to implement properly when sensor_manager is destroyed. I am not sure whether AccelerometerReader is thread-safe in that case. This is assuming the Add/Remove observer is called on the IO-thread while the accelerometer events can come in on a different thread. https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:41: sensor_manager_.reset(new SensorManagerChromeOS); On 2015/01/21 16:02:11, jonross wrote: > On 2015/01/21 13:16:57, timvolodine wrote: > > you are using a real implementation here? does this actually hook into > chromeOS? > > I would expect a fake sensor manager but maybe I am missing something.. > > I am testing a real sensor manager here. However in unittests there is nothing > generating accelerometer events, so the only tie to chromeos/accelerometer is > the adding/removing of the observer. > > Accelerometer events are injected via OnAccelerationIncludingGravity (line 26). > Thus bypassing the chromeos/accelerometer and letting me just test the results > of SensorManagerChromeOS handling data. It seems you are relying on AccelerometerReader::Initialize not being called during unit testing and that feels dangerous to me. Also this is a unit test not an integration test. I think it is better to provide a fake manager where the calls to AccelerometerReader are mocked out/replaced by something else.
Patchset #3 (id:60001) has been deleted
On 2015/01/21 19:08:22, timvolodine wrote: > https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... > File content/browser/device_sensors/sensor_manager_chromeos.cc (right): > > https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... > content/browser/device_sensors/sensor_manager_chromeos.cc:19: > chromeos::AccelerometerReader::GetInstance()->AddObserver(this); > On 2015/01/21 16:02:11, jonross wrote: > > On 2015/01/21 13:16:57, timvolodine wrote: > > > it looks a bit strange you are adding an observer on creation and removing > > only > > > on destruction. it seems adding an observer on > StartFetchingDeviceOrientation > > > and removing it on StopFetchingDeviceOrientation would be a more logical > > > approach? wdyt? > > > > I had originally considered that. However when I implement Device Motion API > > this class will be handling separate buffers for the different APIs, all from > > the same accelerometer source. I thought that this would be cleaner than > > tracking which buffers are in use at each start/stop. > > > > Though I would not object to changing it. > > I think the add/remove observer should generally correspond to the Start/Stop > semantics. It does not make sense to have an observer when nobody is listening. > > Looking at your approach it seems that you could: > 1. put the add/remove observer logic (and future logic) in sensor_manager > corresponding to start/stop methods, or > 2. remove the start/stop methods from sensor_manager and perform add/remove > observer on construction/destruction as it is now. In this case the class would > probably be better called sensor_manager_observer and the logic for > creating/destroying would be handled by data_fetcher_shared_memory. (i.e. having > a live sensor_manager means we are listening) > > The problem with 2 is that it can be tricky to implement properly when > sensor_manager is destroyed. I am not sure whether AccelerometerReader is > thread-safe in that case. This is assuming the Add/Remove observer is called on > the IO-thread while the accelerometer events can come in on a different thread. > > https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... > File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): > > https://codereview.chromium.org/856123002/diff/20001/content/browser/device_s... > content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:41: > sensor_manager_.reset(new SensorManagerChromeOS); > On 2015/01/21 16:02:11, jonross wrote: > > On 2015/01/21 13:16:57, timvolodine wrote: > > > you are using a real implementation here? does this actually hook into > > chromeOS? > > > I would expect a fake sensor manager but maybe I am missing something.. > > > > I am testing a real sensor manager here. However in unittests there is nothing > > generating accelerometer events, so the only tie to chromeos/accelerometer is > > the adding/removing of the observer. > > > > Accelerometer events are injected via OnAccelerationIncludingGravity (line > 26). > > Thus bypassing the chromeos/accelerometer and letting me just test the results > > of SensorManagerChromeOS handling data. > > It seems you are relying on AccelerometerReader::Initialize not being called > during unit testing and that feels dangerous to me. Also this is a unit test not > an integration test. I think it is better to provide a fake manager where the > calls to AccelerometerReader are mocked out/replaced by something else. I decided to go with your first option. SensorManagerChromeOS will now start/stop listening to the accelerometer in accordance with calls to start/stop orientation buffer. I've also isolated it from the system AccelerometerReader inside the tests. It now does not attach as an observer at all.
ok thanks Jon, lgtm with comments addressed https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:24: // TODO(jonross): Implement Device Motion API. (crbug.com/427662) nit: add NOTIMPLEMENTED()? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:30: return false; nit: NOTIMPLEMENTED? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:31: default: nit: I think it's better to not have a default here, and simply have NOTREACHED() after the switch. without the default case the compiler will ensure all possible values are handled inside the switch. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:48: NOTREACHED(); nit: same here re no default case; also think you could drop the |stopped| variable and simply use "return" as in Start(). https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:26: DCHECK(buffer); what happens if StartFetchingDeviceOrientationData is called twice in succession? maybe return early when orientation_buffer_ is not null (like in Stop)? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:43: base::AutoLock autolock(orientation_buffer_lock_); nit: do you need a lock to hold over the whole body? i.e. can StopObservingAccelerometer be placed outside the lock? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:42: // Shared memory to to update. remove duplicate "to" https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015? https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:22: ~TestSensorManagerChromeOS() override{}; space after override https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:70: scoped_ptr<SensorManagerChromeOS> sensor_manager_; nit: SensorManagerChromeOS -> TestSensorManagerChromeOS?
https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/26 15:29:07, timvolodine wrote: > nit: 2015? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:30: return false; On 2015/01/26 15:29:07, timvolodine wrote: > nit: NOTIMPLEMENTED? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:31: default: On 2015/01/26 15:29:07, timvolodine wrote: > nit: I think it's better to not have a default here, and simply have > NOTREACHED() after the switch. without the default case the compiler will ensure > all possible values are handled inside the switch. Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/data_fetcher_shared_memory_chromeos.cc:48: NOTREACHED(); On 2015/01/26 15:29:07, timvolodine wrote: > nit: same here re no default case; also think you could drop the |stopped| > variable and simply use "return" as in Start(). Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/26 15:29:07, timvolodine wrote: > nit: 2015? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:26: DCHECK(buffer); On 2015/01/26 15:29:07, timvolodine wrote: > what happens if StartFetchingDeviceOrientationData is called twice in > succession? maybe return early when orientation_buffer_ is not null (like in > Stop)? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.cc:43: base::AutoLock autolock(orientation_buffer_lock_); On 2015/01/26 15:29:07, timvolodine wrote: > nit: do you need a lock to hold over the whole body? i.e. can > StopObservingAccelerometer be placed outside the lock? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos.h (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/26 15:29:07, timvolodine wrote: > nit: 2015? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos.h:42: // Shared memory to to update. On 2015/01/26 15:29:08, timvolodine wrote: > remove duplicate "to" Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... File content/browser/device_sensors/sensor_manager_chromeos_unittest.cc (right): https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/01/26 15:29:08, timvolodine wrote: > nit: 2015? Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:22: ~TestSensorManagerChromeOS() override{}; On 2015/01/26 15:29:08, timvolodine wrote: > space after override Done. https://codereview.chromium.org/856123002/diff/80001/content/browser/device_s... content/browser/device_sensors/sensor_manager_chromeos_unittest.cc:70: scoped_ptr<SensorManagerChromeOS> sensor_manager_; On 2015/01/26 15:29:08, timvolodine wrote: > nit: SensorManagerChromeOS -> TestSensorManagerChromeOS? Done.
jonross@chromium.org changed reviewers: + jam@chromium.org
jam@chromium.org: Could you provide an owners review for: content/browser/BUILD.gn This review is a replacement for one where you had raised objections to a class being created in content/public/browser/, to pass accelerometer events from ash/ to content/ Since then I have changed how accelerometers can be accessed on chromeos. The classes in content/browser/device_sensors/ can now directly access accelerometer data in chromeos/accelerometer/. Regards, Jon
jonross@chromium.org changed reviewers: + sadrul@chromium.org
Hi Sadrul, In this review I have added a dependency on ui/accelerometer in content_tests. This is to write the unittests in content/browser/device_sensors/sensor_manager_chromeos_unittest.cc Could you provide an owner review for this DEPS change? Thanks, Jon
On 2015/01/26 16:51:08, jonross wrote: > Hi Sadrul, > > In this review I have added a dependency on ui/accelerometer in content_tests. From what I can see, we need to do this because //chromeos/ currently has a dependency on //ui/accelerometer/ for AccelerometerUpdate? There's currently a TODO to remove this dependency and move AU into //chromeos. Can that be done first (unless it's non-trivial)? > > This is to write the unittests in > content/browser/device_sensors/sensor_manager_chromeos_unittest.cc > > Could you provide an owner review for this DEPS change? > > Thanks, > Jon
On 2015/01/26 18:25:23, sadrul wrote: > On 2015/01/26 16:51:08, jonross wrote: > > Hi Sadrul, > > > > In this review I have added a dependency on ui/accelerometer in content_tests. > > From what I can see, we need to do this because //chromeos/ currently has a > dependency on //ui/accelerometer/ for AccelerometerUpdate? There's currently a > TODO to remove this dependency and move AU into //chromeos. Can that be done > first (unless it's non-trivial)? > > > > > This is to write the unittests in > > content/browser/device_sensors/sensor_manager_chromeos_unittest.cc > > > > Could you provide an owner review for this DEPS change? > > > > Thanks, > > Jon I'll take care of that first.
lgtm
Patchset #8 (id:180001) has been deleted
Hi Sadrul, The dependency on ui/accelerometer has been removed from content_unittests. With AccelerometerUpdate now in chromeos/ this dependency is no longer needed. Could you take another look? Thanks, Jon
On 2015/02/04 14:36:24, jonross wrote: > Hi Sadrul, > > The dependency on ui/accelerometer has been removed from content_unittests. > > With AccelerometerUpdate now in chromeos/ this dependency is no longer needed. > > Could you take another look? > > Thanks, > Jon Thanks a lot! I don't have expertise in this code, but you already have approval from owners. And now that there's no added dependency on ui, you don't need my review!
sadrul@chromium.org changed reviewers: - sadrul@chromium.org
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/856123002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/856123002/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e717460964dd27a7239abec31462441eb971edbe Cr-Commit-Position: refs/heads/master@{#314605}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:200001) has been created in https://codereview.chromium.org/899973002/ by jonross@chromium.org. The reason for reverting is: Compilation failure on CQ: http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Linux%20C.... |
