Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
200643005:
Read and expose accelerometer values from cros-ec-accel trigger. (Closed)
|
Created:
6 years, 9 months ago by flackr Modified:
6 years, 8 months ago CC:
chromium-reviews, kalyank, stevenjb+watch_chromium.org, sadrul, oshima+watch_chromium.org, ben+ash_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRead and expose accelerometer values from cros-ec-accel trigger.
BUG=342907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261317
Patch Set 1 #Patch Set 2 : Comments, update to use udev symlink to determine device name. #
Total comments: 76
Patch Set 3 : Address review comments. #
Total comments: 2
Patch Set 4 : Updated check for invalid field index to log error and not use accelerometer. #Patch Set 5 : Pass in TaskRunner from ash. #Patch Set 6 : Move content dep to ash/accelerometer/DEPS #Patch Set 7 : Read the accelerometer synchronously. #Patch Set 8 : Virtual dtor in observer, ASH_EXPORT for tests in dependent CLs. #
Total comments: 2
Patch Set 9 : Pass TaskRunner from chrome through to ash to AccelerometerReader. #Patch Set 10 : Remove content DEP from ash. #Patch Set 11 : Change dep to just ui/gfx/geometry. #Patch Set 12 : Link in gfx_geometry. #Patch Set 13 : Don't bind to AccelerometerReader, post to standalone function with scoped_refptr to data structure. #
Total comments: 10
Patch Set 14 : Fix alphabetization. #
Total comments: 2
Patch Set 15 : Always define accelerometer_controller_ in shell. #Patch Set 16 : Sort for real. #Patch Set 17 : Add CHROMEOS if defined for HandleAccelerometerReading. #
Messages
Total messages: 48 (0 generated)
Hey, this is the first chrome side patch to handle accelerometer updates. I decided that we probably want a class to observe which is always available so that we don't have to sprinkle if defined(OS_CHROMEOS) everywhere we want to do something based on accelerometer readings. I'm not sure about hardcoding the observer to have lid and base values. This would be easy to update later as needed though. Take a look and let me know what you think, thanks again for offering to review! https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:89: BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior( Is there a better way to post a non-blocking task for the AccelerometerReader from the UI thread, we of course don't want it to slow down the UI but putting it on an IO thread would mean that it would jank while doing heavy operations right?
some nits, but this looks generally fine to me! https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... File ash/accelerometer/accelerometer_controller.h (right): https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... ash/accelerometer/accelerometer_controller.h:24: // notifications if an accelerometer was detected. (just wondering; might not be necessary yet): do observers need to be able to tell whether an accelerometer is present or not? https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... ash/accelerometer/accelerometer_controller.h:40: virtual void OnAccelerometerRead(const gfx::Vector3dF& base, this may just be my own personal convention, but i usually like to give delegates' methods specific, imperative names (e.g. NotifyObserversAboutAccelerometerReading or perhaps HandleAccelerometerReading/Read) and let observer interfaces use open-ended names like OnAccelerometerRead. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:30: // Files containing the scales of the accelerometers. nit: mention that these are files within kAccelerometerIioBasePath? (maybe also s/Path/BaseName/ in the names to make it more clear that they aren't full paths?) https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:55: const unsigned int kMaxAsciiUintLength = 21; nit: s/unsigned int/size_t/ https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:58: const unsigned int kDelayBetweenReadsMilliseconds = 100; nit: just use int here, maybe also s/Milliseconds/Ms/ (at least, i think that this abbreviation is commonly used) https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:66: &s, kMaxAsciiUintLength)) { nit: can you unwrap this line? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:70: base::TrimString(s, "\r\n", &s); nit: base::TrimWhitespace(s, base::TRIM_ALL, &s)? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:89: BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior( On 2014/03/26 19:06:48, flackr wrote: > Is there a better way to post a non-blocking task for the AccelerometerReader > from the UI thread, we of course don't want it to slow down the UI but putting > it on an IO thread would mean that it would jank while doing heavy operations > right? i think that the blocking pool is the right place for this. in general, i believe that it's usually safe to assume that there will be an available thread there. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:90: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN).get(), if you don't need the SKIP_ON_SHUTDOWN semantics (Initialize() doesn't seem like it's likely to still be running when chrome shuts down, or like it'll take long to run, right?), you can just use BrowserThread::PostBlockingPoolTaskAndReply() here. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:93: base::Unretained(this)), pass weak_ptrs instead to make sure that these tasks won't run after the object has been deleted https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:101: bool AccelerometerReader::Initialize() { nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:106: &device)) { just to check: this is expected for devices that don't have accelerometers, right? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:111: LOG(ERROR) << "Accelerometer trigger does not exist."; nit: include kAccelerometerTriggerPath in the error message https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:120: return false; nit: log an error and include kAccelerometerBaseScalePath in the message https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:124: return false; nit: log an error and include kAccelerometerLidScalePath in the message https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:133: base::strings::SafeSPrintf(accelerometer_index_path, can you just use an std::string in conjunction with base::StringPrintf() instead of a fixed-size char buffer here? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:138: return false; nit: log an error and include the path name in the message https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:140: accelerometer_index_.push_back(index); do bounds-checking here to ensure that a buggy/malicious accelerometer can't trick you into an out-of-bounds read later in ReadAccelerometer() https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:146: void AccelerometerReader::OnInitialized(bool success) { nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:152: void AccelerometerReader::TriggerRead() { nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:160: base::Unretained(this))); same comments here as above https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:163: bool AccelerometerReader::ReadAccelerometer() { nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:165: int bytesWritten = base::WriteFile( s/bytesWritten/bytes_written/ https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:168: LOG(ERROR) << "Accelerometer trigger failure: " << bytesWritten; nit: PLOG(ERROR) https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:174: int bytesRead = base::ReadFile(base::FilePath(kAccelerometerDevicePath), s/bytesRead/bytes_read/ https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:175: data, kTriggerDataLength); just out of curiosity: have you timed this? how long can it block? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:177: LOG(ERROR) << "Read " << bytesRead << " bytes, expected " nit: s/bytes/byte(s)/ https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:182: base_accelerometer_.set_x(*((int16*)data + accelerometer_index_[0])); nit: use reinterpret_cast<int16*> instead of c-style casts https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:190: lid_accelerometer_.Scale(1.0f / accelerometer_lid_scale_); i'm a bit concerned that a future change could add the ability for outside classes to read |base_accelerometer_| and |lid_accelerometer_| directly from this class on the UI thread, in which case it'd be possible to read these while they're being updated on the blocking pool. an alternative approach would be passing the newly-read values back from the blocking pool to the ui thread. i'm not sure if you can do this without packing them into a new struct, though. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:194: void AccelerometerReader::OnDataRead(bool success) { nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:18: class AccelerometerDelegate { maybe move this be a public internal class of AccelerometerReader named Delegate, so it's obvious from the name what it's a delegate for? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:28: AccelerometerReader(AccelerometerDelegate* delegate); nit: explicit https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:29: virtual ~AccelerometerReader(); nit: don't need virtual https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:36: // accelerometer was detected and all configuration paramaters read. nit: s/paramaters/parameters/ https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:48: // If |success|, notifies the delegate_ with the new readings. Triggers nit: s/delegate_/|delegate_|/ (also in other comments in this file -- i think that i've usually seen both arguments and class members get the pipe treatment) https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:66: gfx::Vector3dF base_accelerometer_; nit: base_reading_? base_value_? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:69: gfx::Vector3dF lid_accelerometer_; nit: lid_reading_? lid_value_?
https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... File ash/accelerometer/accelerometer_controller.h (right): https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... ash/accelerometer/accelerometer_controller.h:24: // notifications if an accelerometer was detected. On 2014/03/26 22:26:14, Daniel Erat wrote: > (just wondering; might not be necessary yet): do observers need to be able to > tell whether an accelerometer is present or not? Not yet, I have patches for auto entering touch view and rotation and both of those only do something if they receive events and if not maintain the default behavior (i.e. no screen rotation or touchview). https://codereview.chromium.org/200643005/diff/20001/ash/accelerometer/accele... ash/accelerometer/accelerometer_controller.h:40: virtual void OnAccelerometerRead(const gfx::Vector3dF& base, On 2014/03/26 22:26:14, Daniel Erat wrote: > this may just be my own personal convention, but i usually like to give > delegates' methods specific, imperative names (e.g. > NotifyObserversAboutAccelerometerReading or perhaps > HandleAccelerometerReading/Read) and let observer interfaces use open-ended > names like OnAccelerometerRead. Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:30: // Files containing the scales of the accelerometers. On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: mention that these are files within kAccelerometerIioBasePath? > > (maybe also s/Path/BaseName/ in the names to make it more clear that they aren't > full paths?) Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:55: const unsigned int kMaxAsciiUintLength = 21; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: s/unsigned int/size_t/ Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:58: const unsigned int kDelayBetweenReadsMilliseconds = 100; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: just use int here, maybe also s/Milliseconds/Ms/ (at least, i think that > this abbreviation is commonly used) Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:66: &s, kMaxAsciiUintLength)) { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: can you unwrap this line? Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:70: base::TrimString(s, "\r\n", &s); On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: base::TrimWhitespace(s, base::TRIM_ALL, &s)? Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:90: base::SequencedWorkerPool::SKIP_ON_SHUTDOWN).get(), On 2014/03/26 22:26:14, Daniel Erat wrote: > if you don't need the SKIP_ON_SHUTDOWN semantics (Initialize() doesn't seem like > it's likely to still be running when chrome shuts down, or like it'll take long > to run, right?), you can just use BrowserThread::PostBlockingPoolTaskAndReply() > here. Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:93: base::Unretained(this)), On 2014/03/26 22:26:14, Daniel Erat wrote: > pass weak_ptrs instead to make sure that these tasks won't run after the object > has been deleted Had to avoid return values because weak_ptrs_can_only_bind_to_methods_without_return_values. Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:101: bool AccelerometerReader::Initialize() { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:106: &device)) { On 2014/03/26 22:26:14, Daniel Erat wrote: > just to check: this is expected for devices that don't have accelerometers, > right? Yes. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:111: LOG(ERROR) << "Accelerometer trigger does not exist."; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: include kAccelerometerTriggerPath in the error message Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:120: return false; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: log an error and include kAccelerometerBaseScalePath in the message For here and following uses, errors are already logged (with the path) in ReadFileToUint for both failure cases, is this okay? https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:124: return false; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: log an error and include kAccelerometerLidScalePath in the message See comment above. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:133: base::strings::SafeSPrintf(accelerometer_index_path, On 2014/03/26 22:26:14, Daniel Erat wrote: > can you just use an std::string in conjunction with base::StringPrintf() instead > of a fixed-size char buffer here? Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:138: return false; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: log an error and include the path name in the message See comment above. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:140: accelerometer_index_.push_back(index); On 2014/03/26 22:26:14, Daniel Erat wrote: > do bounds-checking here to ensure that a buggy/malicious accelerometer can't > trick you into an out-of-bounds read later in ReadAccelerometer() Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:146: void AccelerometerReader::OnInitialized(bool success) { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:152: void AccelerometerReader::TriggerRead() { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:160: base::Unretained(this))); On 2014/03/26 22:26:14, Daniel Erat wrote: > same comments here as above Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:163: bool AccelerometerReader::ReadAccelerometer() { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:165: int bytesWritten = base::WriteFile( On 2014/03/26 22:26:14, Daniel Erat wrote: > s/bytesWritten/bytes_written/ Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:168: LOG(ERROR) << "Accelerometer trigger failure: " << bytesWritten; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: PLOG(ERROR) Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:174: int bytesRead = base::ReadFile(base::FilePath(kAccelerometerDevicePath), On 2014/03/26 22:26:14, Daniel Erat wrote: > s/bytesRead/bytes_read/ Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:175: data, kTriggerDataLength); On 2014/03/26 22:26:14, Daniel Erat wrote: > just out of curiosity: have you timed this? how long can it block? I haven't timed it yet, but I can do this. I believe it to be quite fast, given the initialization, reading the first value, and screen rotation can finish before you can see the login screen (i.e. it looks as though the login screen was always correctly oriented). https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:177: LOG(ERROR) << "Read " << bytesRead << " bytes, expected " On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: s/bytes/byte(s)/ Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:182: base_accelerometer_.set_x(*((int16*)data + accelerometer_index_[0])); On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: use reinterpret_cast<int16*> instead of c-style casts Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:190: lid_accelerometer_.Scale(1.0f / accelerometer_lid_scale_); On 2014/03/26 22:26:14, Daniel Erat wrote: > i'm a bit concerned that a future change could add the ability for outside > classes to read |base_accelerometer_| and |lid_accelerometer_| directly from > this class on the UI thread, in which case it'd be possible to read these while > they're being updated on the blocking pool. an alternative approach would be > passing the newly-read values back from the blocking pool to the ui thread. i'm > not sure if you can do this without packing them into a new struct, though. I added a comment to this effect (in the header), and warning others not to read these values directly, and I'd be happy to look into this in a future CL. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:194: void AccelerometerReader::OnDataRead(bool success) { On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:18: class AccelerometerDelegate { On 2014/03/26 22:26:14, Daniel Erat wrote: > maybe move this be a public internal class of AccelerometerReader named > Delegate, so it's obvious from the name what it's a delegate for? Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:28: AccelerometerReader(AccelerometerDelegate* delegate); On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: explicit Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:29: virtual ~AccelerometerReader(); On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: don't need virtual Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:36: // accelerometer was detected and all configuration paramaters read. On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: s/paramaters/parameters/ Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:48: // If |success|, notifies the delegate_ with the new readings. Triggers On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: s/delegate_/|delegate_|/ (also in other comments in this file -- i think > that i've usually seen both arguments and class members get the pipe treatment) Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:66: gfx::Vector3dF base_accelerometer_; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: base_reading_? base_value_? Done. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.h:69: gfx::Vector3dF lid_accelerometer_; On 2014/03/26 22:26:14, Daniel Erat wrote: > nit: lid_reading_? lid_value_? Done.
lgtm https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:93: base::Unretained(this)), On 2014/03/27 15:21:07, flackr wrote: > On 2014/03/26 22:26:14, Daniel Erat wrote: > > pass weak_ptrs instead to make sure that these tasks won't run after the > object > > has been deleted > > Had to avoid return values because > weak_ptrs_can_only_bind_to_methods_without_return_values. Done. ah, that's unfortunate. https://codereview.chromium.org/200643005/diff/20001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:120: return false; On 2014/03/27 15:21:07, flackr wrote: > On 2014/03/26 22:26:14, Daniel Erat wrote: > > nit: log an error and include kAccelerometerBaseScalePath in the message > > For here and following uses, errors are already logged (with the path) in > ReadFileToUint for both failure cases, is this okay? sure, sounds fine. https://codereview.chromium.org/200643005/diff/40001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/40001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:133: CHECK(index < kTriggerDataValues); CHECK_LT() is better (as it'll log both values), but it might be best to log an error here and bail out while leaving has_accelerometer_ set to false.
+danakj for adding ui/gfx to chromeos/DEPS +piman for adding content/public/browser to chromeos/DEPS PTAL, thanks. https://codereview.chromium.org/200643005/diff/40001/chromeos/accelerometer/a... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/40001/chromeos/accelerometer/a... chromeos/accelerometer/accelerometer_reader.cc:133: CHECK(index < kTriggerDataValues); On 2014/03/27 15:37:52, Daniel Erat wrote: > CHECK_LT() is better (as it'll log both values), but it might be best to log an > error here and bail out while leaving has_accelerometer_ set to false. Done. Decided to go with bailing out.
+jam where does chromeos sit in the dependency graph? Is it ok to have it depend on content/, does it not create a cycle?
On 2014/03/27 17:58:53, piman wrote: > +jam > where does chromeos sit in the dependency graph? Is it ok to have it depend on > content/, does it not create a cycle? oh, you're right... i don't think that chromeos/ can depend on content/ or ui/. :-/ a quick fix might be moving AccelerometerReader to ash/system/chromeos.
On 2014/03/27 17:58:53, piman wrote: > +jam > where does chromeos sit in the dependency graph? Is it ok to have it depend on > content/, does it not create a cycle? chromeos sits below content. so this new dependency couldn't work as content depends on src\chromeos, and otherwise there'd be a circular dependency (and also it seems weird that the cros platform layer knows about content, in general). ash, when it creates the AccelerometerReader class, can pass in the SingleThreadTaskRunner that it gets from BrowserThread
Okay, I've merged with master and passed in the TaskRunner I get from ash, which also did not have that DEP yet. PTAL, thanks! Btw, how would I tell that chromeos is below content without following a lot of DEPS files. I tried graphdeps but it produced an 11x11 white png, heh.
On 2014/03/28 13:06:31, flackr wrote: > Okay, I've merged with master and passed in the TaskRunner I get from ash, which > also did not have that DEP yet. PTAL, thanks! lgtm > Btw, how would I tell that chromeos is below content without following a lot of > DEPS files. I tried graphdeps but it produced an 11x11 white png, heh. content/browser/DEPS lists chromeos as a dependency, and content_browser.gypi and content_shell.gypi also pull in targets from chromeos.gyp. i don't know if there's an easier way to tell, though -- having the existing knowledge that chromeos/ is (at least currently) all low-level system-type stuff rather than having anything to do with the UI helps, i guess. :-P
is the design of ash is such that all of it can depend on content? it seems like the deps rules were pretty strict before. conceptually it's not clear to me why ash should depend on content, other than specific places that do it now like content_support and screensaver
On 2014/03/28 17:59:54, jam wrote: > is the design of ash is such that all of it can depend on content? it seems like > the deps rules were pretty strict before. conceptually it's not clear to me why > ash should depend on content, other than specific places that do it now like > content_support and screensaver Okay, so it would be alright to include it from ash/accelerometer? Done in patch set 6.
On 2014/03/28 18:04:56, flackr wrote: > On 2014/03/28 17:59:54, jam wrote: > > is the design of ash is such that all of it can depend on content? it seems > like > > the deps rules were pretty strict before. conceptually it's not clear to me > why > > ash should depend on content, other than specific places that do it now like > > content_support and screensaver > > Okay, so it would be alright to include it from ash/accelerometer? Done in patch > set 6. Indeed, Ben is trying to make ash work with and without content. See ash.gyp:ash_with_content. You'll need to structure things so that both works. I believe that means removing dependency from shell on accelerometercontroller.
On 2014/03/28 18:04:56, flackr wrote: > On 2014/03/28 17:59:54, jam wrote: > > is the design of ash is such that all of it can depend on content? it seems > like > > the deps rules were pretty strict before. conceptually it's not clear to me > why > > ash should depend on content, other than specific places that do it now like > > content_support and screensaver > > Okay, so it would be alright to include it from ash/accelerometer? Done in patch > set 6. why not use dependency injection? i.e. does chrome layer initialize ash somewhere? have it pass the blocking pool object at startup
On 2014/03/28 20:35:10, sky wrote: > On 2014/03/28 18:04:56, flackr wrote: > > On 2014/03/28 17:59:54, jam wrote: > > > is the design of ash is such that all of it can depend on content? it seems > > like > > > the deps rules were pretty strict before. conceptually it's not clear to me > > why > > > ash should depend on content, other than specific places that do it now like > > > content_support and screensaver > > > > Okay, so it would be alright to include it from ash/accelerometer? Done in > patch > > set 6. > > Indeed, Ben is trying to make ash work with and without content. See > ash.gyp:ash_with_content. You'll need to structure things so that both works. I > believe that means removing dependency from shell on accelerometercontroller. Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in order to run blocking tasks from AccelerometerReader in src/chromeos/accelerometer. Perhaps I can add the DEP on content/public/browser from chromeos/accelerometer similar to chromeos/memory?
On 2014/03/28 21:19:25, flackr wrote: > On 2014/03/28 20:35:10, sky wrote: > > On 2014/03/28 18:04:56, flackr wrote: > > > On 2014/03/28 17:59:54, jam wrote: > > > > is the design of ash is such that all of it can depend on content? it > seems > > > like > > > > the deps rules were pretty strict before. conceptually it's not clear to > me > > > why > > > > ash should depend on content, other than specific places that do it now > like > > > > content_support and screensaver > > > > > > Okay, so it would be alright to include it from ash/accelerometer? Done in > > patch > > > set 6. > > > > Indeed, Ben is trying to make ash work with and without content. See > > ash.gyp:ash_with_content. You'll need to structure things so that both works. > I > > believe that means removing dependency from shell on accelerometercontroller. > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in order to > run blocking tasks from AccelerometerReader in src/chromeos/accelerometer. > Perhaps I can add the DEP on content/public/browser from chromeos/accelerometer > similar to chromeos/memory? see comment 8, this is a layering violation. wow, i did not know that chromeos had a deps entry for content, that's wrong
On 2014/03/31 15:58:56, jam wrote: > On 2014/03/28 21:19:25, flackr wrote: > > On 2014/03/28 20:35:10, sky wrote: > > > On 2014/03/28 18:04:56, flackr wrote: > > > > On 2014/03/28 17:59:54, jam wrote: > > > > > is the design of ash is such that all of it can depend on content? it > > seems > > > > like > > > > > the deps rules were pretty strict before. conceptually it's not clear to > > me > > > > why > > > > > ash should depend on content, other than specific places that do it now > > like > > > > > content_support and screensaver > > > > > > > > Okay, so it would be alright to include it from ash/accelerometer? Done in > > > patch > > > > set 6. > > > > > > Indeed, Ben is trying to make ash work with and without content. See > > > ash.gyp:ash_with_content. You'll need to structure things so that both > works. > > I > > > believe that means removing dependency from shell on > accelerometercontroller. > > > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in order > to > > run blocking tasks from AccelerometerReader in src/chromeos/accelerometer. > > Perhaps I can add the DEP on content/public/browser from > chromeos/accelerometer > > similar to chromeos/memory? > > see comment 8, this is a layering violation. > > wow, i did not know that chromeos had a deps entry for content, that's wrong Right, I'm mainly wondering why we don't have a blocking pool task runner available to chromeos and ash which seem like they would both have need for such a thing. That being said, I measured on my device and Initialize takes about 700 microseconds on average (highest I saw in about 10 restarts was just over 1000 microseconds) and ReadAccelerometer takes 500 microseconds on average varying from about 250 to 650 microseconds. Would this be reasonable to do synchronously on the UI thread (see patch set 7)?
On 2014/03/31 22:15:09, flackr wrote: > On 2014/03/31 15:58:56, jam wrote: > > On 2014/03/28 21:19:25, flackr wrote: > > > On 2014/03/28 20:35:10, sky wrote: > > > > On 2014/03/28 18:04:56, flackr wrote: > > > > > On 2014/03/28 17:59:54, jam wrote: > > > > > > is the design of ash is such that all of it can depend on content? it > > > seems > > > > > like > > > > > > the deps rules were pretty strict before. conceptually it's not clear > to > > > me > > > > > why > > > > > > ash should depend on content, other than specific places that do it > now > > > like > > > > > > content_support and screensaver > > > > > > > > > > Okay, so it would be alright to include it from ash/accelerometer? Done > in > > > > patch > > > > > set 6. > > > > > > > > Indeed, Ben is trying to make ash work with and without content. See > > > > ash.gyp:ash_with_content. You'll need to structure things so that both > > works. > > > I > > > > believe that means removing dependency from shell on > > accelerometercontroller. > > > > > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in > order > > to > > > run blocking tasks from AccelerometerReader in src/chromeos/accelerometer. > > > Perhaps I can add the DEP on content/public/browser from > > chromeos/accelerometer > > > similar to chromeos/memory? > > > > see comment 8, this is a layering violation. > > > > wow, i did not know that chromeos had a deps entry for content, that's wrong > > Right, I'm mainly wondering why we don't have a blocking pool task runner > available to chromeos and ash which seem like they would both have need for such > a thing. I agree that would be good. Perhaps the blocking pool object needs to move from a getter in content to one lower down, say in base. It would be good to discuss this with Brett who wrote that code. > > That being said, I measured on my device and Initialize takes about 700 > microseconds on average (highest I saw in about 10 restarts was just over 1000 > microseconds) and ReadAccelerometer takes 500 microseconds on average varying > from about 250 to 650 microseconds. Would this be reasonable to do synchronously > on the UI thread (see patch set 7)? I'll defer to cros people on this.
On 2014/04/01 16:16:35, jam wrote: > On 2014/03/31 22:15:09, flackr wrote: > > On 2014/03/31 15:58:56, jam wrote: > > > On 2014/03/28 21:19:25, flackr wrote: > > > > On 2014/03/28 20:35:10, sky wrote: > > > > > On 2014/03/28 18:04:56, flackr wrote: > > > > > > On 2014/03/28 17:59:54, jam wrote: > > > > > > > is the design of ash is such that all of it can depend on content? > it > > > > seems > > > > > > like > > > > > > > the deps rules were pretty strict before. conceptually it's not > clear > > to > > > > me > > > > > > why > > > > > > > ash should depend on content, other than specific places that do it > > now > > > > like > > > > > > > content_support and screensaver > > > > > > > > > > > > Okay, so it would be alright to include it from ash/accelerometer? > Done > > in > > > > > patch > > > > > > set 6. > > > > > > > > > > Indeed, Ben is trying to make ash work with and without content. See > > > > > ash.gyp:ash_with_content. You'll need to structure things so that both > > > works. > > > > I > > > > > believe that means removing dependency from shell on > > > accelerometercontroller. > > > > > > > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in > > order > > > to > > > > run blocking tasks from AccelerometerReader in src/chromeos/accelerometer. > > > > Perhaps I can add the DEP on content/public/browser from > > > chromeos/accelerometer > > > > similar to chromeos/memory? > > > > > > see comment 8, this is a layering violation. > > > > > > wow, i did not know that chromeos had a deps entry for content, that's wrong > > > > Right, I'm mainly wondering why we don't have a blocking pool task runner > > available to chromeos and ash which seem like they would both have need for > such > > a thing. > > I agree that would be good. Perhaps the blocking pool object needs to move from > a getter in content to one lower down, say in base. It would be good to discuss > this with Brett who wrote that code. > > > > > That being said, I measured on my device and Initialize takes about 700 > > microseconds on average (highest I saw in about 10 restarts was just over 1000 > > microseconds) and ReadAccelerometer takes 500 microseconds on average varying > > from about 250 to 650 microseconds. Would this be reasonable to do > synchronously > > on the UI thread (see patch set 7)? > > I'll defer to cros people on this. if one of the kernel folks guarantees that this will never block, i'm fine with it. :-)
On 2014/04/01 16:25:29, Daniel Erat wrote: > On 2014/04/01 16:16:35, jam wrote: > > On 2014/03/31 22:15:09, flackr wrote: > > > On 2014/03/31 15:58:56, jam wrote: > > > > On 2014/03/28 21:19:25, flackr wrote: > > > > > On 2014/03/28 20:35:10, sky wrote: > > > > > > On 2014/03/28 18:04:56, flackr wrote: > > > > > > > On 2014/03/28 17:59:54, jam wrote: > > > > > > > > is the design of ash is such that all of it can depend on content? > > it > > > > > seems > > > > > > > like > > > > > > > > the deps rules were pretty strict before. conceptually it's not > > clear > > > to > > > > > me > > > > > > > why > > > > > > > > ash should depend on content, other than specific places that do > it > > > now > > > > > like > > > > > > > > content_support and screensaver > > > > > > > > > > > > > > Okay, so it would be alright to include it from ash/accelerometer? > > Done > > > in > > > > > > patch > > > > > > > set 6. > > > > > > > > > > > > Indeed, Ben is trying to make ash work with and without content. See > > > > > > ash.gyp:ash_with_content. You'll need to structure things so that both > > > > works. > > > > > I > > > > > > believe that means removing dependency from shell on > > > > accelerometercontroller. > > > > > > > > > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) in > > > order > > > > to > > > > > run blocking tasks from AccelerometerReader in > src/chromeos/accelerometer. > > > > > Perhaps I can add the DEP on content/public/browser from > > > > chromeos/accelerometer > > > > > similar to chromeos/memory? > > > > > > > > see comment 8, this is a layering violation. > > > > > > > > wow, i did not know that chromeos had a deps entry for content, that's > wrong > > > > > > Right, I'm mainly wondering why we don't have a blocking pool task runner > > > available to chromeos and ash which seem like they would both have need for > > such > > > a thing. > > > > I agree that would be good. Perhaps the blocking pool object needs to move > from > > a getter in content to one lower down, say in base. It would be good to > discuss > > this with Brett who wrote that code. > > > > > > > > That being said, I measured on my device and Initialize takes about 700 > > > microseconds on average (highest I saw in about 10 restarts was just over > 1000 > > > microseconds) and ReadAccelerometer takes 500 microseconds on average > varying > > > from about 250 to 650 microseconds. Would this be reasonable to do > > synchronously > > > on the UI thread (see patch set 7)? > > > > I'll defer to cros people on this. > > if one of the kernel folks guarantees that this will never block, i'm fine with > it. :-) +Alec, can you comment on this? Thanks.
A drive-by query https://codereview.chromium.org/200643005/diff/160001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/160001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:51: const int kDelayBetweenReadsMs = 100; Would it make sense to use a base::FilePathWatcher instead (with a min-interval so the callback doesn't trigger too frequently)?
On 2014/04/01 16:29:49, flackr wrote: > On 2014/04/01 16:25:29, Daniel Erat wrote: > > On 2014/04/01 16:16:35, jam wrote: > > > On 2014/03/31 22:15:09, flackr wrote: > > > > On 2014/03/31 15:58:56, jam wrote: > > > > > On 2014/03/28 21:19:25, flackr wrote: > > > > > > On 2014/03/28 20:35:10, sky wrote: > > > > > > > On 2014/03/28 18:04:56, flackr wrote: > > > > > > > > On 2014/03/28 17:59:54, jam wrote: > > > > > > > > > is the design of ash is such that all of it can depend on > content? > > > it > > > > > > seems > > > > > > > > like > > > > > > > > > the deps rules were pretty strict before. conceptually it's not > > > clear > > > > to > > > > > > me > > > > > > > > why > > > > > > > > > ash should depend on content, other than specific places that do > > it > > > > now > > > > > > like > > > > > > > > > content_support and screensaver > > > > > > > > > > > > > > > > Okay, so it would be alright to include it from ash/accelerometer? > > > Done > > > > in > > > > > > > patch > > > > > > > > set 6. > > > > > > > > > > > > > > Indeed, Ben is trying to make ash work with and without content. See > > > > > > > ash.gyp:ash_with_content. You'll need to structure things so that > both > > > > > works. > > > > > > I > > > > > > > believe that means removing dependency from shell on > > > > > accelerometercontroller. > > > > > > > > > > > > Ultimately, I just need a task runner (i.e. from BlockingThreadPool) > in > > > > order > > > > > to > > > > > > run blocking tasks from AccelerometerReader in > > src/chromeos/accelerometer. > > > > > > Perhaps I can add the DEP on content/public/browser from > > > > > chromeos/accelerometer > > > > > > similar to chromeos/memory? > > > > > > > > > > see comment 8, this is a layering violation. > > > > > > > > > > wow, i did not know that chromeos had a deps entry for content, that's > > wrong > > > > > > > > Right, I'm mainly wondering why we don't have a blocking pool task runner > > > > available to chromeos and ash which seem like they would both have need > for > > > such > > > > a thing. > > > > > > I agree that would be good. Perhaps the blocking pool object needs to move > > from > > > a getter in content to one lower down, say in base. It would be good to > > discuss > > > this with Brett who wrote that code. > > > > > > > > > > > That being said, I measured on my device and Initialize takes about 700 > > > > microseconds on average (highest I saw in about 10 restarts was just over > > 1000 > > > > microseconds) and ReadAccelerometer takes 500 microseconds on average > > varying > > > > from about 250 to 650 microseconds. Would this be reasonable to do > > > synchronously > > > > on the UI thread (see patch set 7)? > > > > > > I'll defer to cros people on this. > > > > if one of the kernel folks guarantees that this will never block, i'm fine > with > > it. :-) > > +Alec, can you comment on this? Thanks. The kernel accelerometer driver is communicating with the EC to read accelerometer data. If there is a problem or a bug with retrieving data, the kernel side will give up and return an error after 250ms. Absolute worst-case it looks like it could take 5 times that long, because under certain circumstances we retry up to 5 times. I can adjust the timeout period and number of retries if desired. Originally, I was leaning towards delaying and retrying in the kernel to guarantee we get a sample, but since Chrome is reading the accelerometer data at a pretty high rate, it might make more sense for the kernel to give up and return an error much faster. The only downside is occasionally Chrome might trigger a data capture and there would be no data available.
https://codereview.chromium.org/200643005/diff/160001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/200643005/diff/160001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:51: const int kDelayBetweenReadsMs = 100; On 2014/04/01 16:33:55, sadrul wrote: > Would it make sense to use a base::FilePathWatcher instead (with a min-interval > so the callback doesn't trigger too frequently)? It shouldn't trigger too frequently as it posts the delayed task only after the read. Data is also not automatically generated, it needs to be triggered by writing to the trigger_now file.
Okay, the potential blocking times are definitely far too long. jam, this is what you were suggesting right? Passing the task runner through from chrome to ash to chromeos? PTAL, thanks.
lgtm
LGTM on the deps/gyp change for ui/gfx/geometry.
The CQ bit was checked by flackr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/200643005/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
cool, this approach seems nice. https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:27: struct ConfigurationData { can you just forward-declare this in the header and define it in the .cc file? (if you additionally wanted to make it private, you could probably make the functions that use it be private static members.) https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:39: typedef base::RefCountedData<char[12]> Reading; can you turn this '12' into a constant in the .cc file?
https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:27: struct ConfigurationData { On 2014/04/02 18:11:57, Daniel Erat wrote: > can you just forward-declare this in the header and define it in the .cc file? > > (if you additionally wanted to make it private, you could probably make the > functions that use it be private static members.) Unfortunately I don't think I can forward the typedef and the RefCountedData template needs the full type. https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:39: typedef base::RefCountedData<char[12]> Reading; On 2014/04/02 18:11:57, Daniel Erat wrote: > can you turn this '12' into a constant in the .cc file? Same for this. I could move the definition of the axes into the .h as well so that this is only defined once or change this to be a vector<char> / vector<int16> to avoid this constant here.
https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.h (right): https://codereview.chromium.org/200643005/diff/260001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.h:39: typedef base::RefCountedData<char[12]> Reading; On 2014/04/02 18:52:55, flackr wrote: > On 2014/04/02 18:11:57, Daniel Erat wrote: > > can you turn this '12' into a constant in the .cc file? > > Same for this. I could move the definition of the axes into the .h as well so > that this is only defined once or change this to be a vector<char> / > vector<int16> to avoid this constant here. Or define the length in this .h and assert that the length matches the number of axes * data size in the .cc.
lgtm (yet again :-) ) w/nits https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... File ash/accelerometer/accelerometer_controller.h (right): https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... ash/accelerometer/accelerometer_controller.h:59: ObserverList<AccelerometerObserver, true> observers_; cool, i'd never seen the 'check_empty' template parameter before! https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... File ash/accelerometer/accelerometer_observer.h (right): https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... ash/accelerometer/accelerometer_observer.h:17: virtual void OnAccelerometerUpdated(const gfx::Vector3dF& base, nit: if these vectors represent the orientation, names like 'base_orientation' and 'lid_orientation' would probably be best (to distinguish from e.g. acceleration) https://codereview.chromium.org/200643005/diff/260001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/200643005/diff/260001/chromeos/chromeos.gyp#n... chromeos/chromeos.gyp:53: 'accelerometer/accelerometer_reader.cc', nit: fix alphabetization
Thanks for the quick reviews! https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... File ash/accelerometer/accelerometer_observer.h (right): https://codereview.chromium.org/200643005/diff/260001/ash/accelerometer/accel... ash/accelerometer/accelerometer_observer.h:17: virtual void OnAccelerometerUpdated(const gfx::Vector3dF& base, On 2014/04/02 20:22:57, Daniel Erat wrote: > nit: if these vectors represent the orientation, names like 'base_orientation' > and 'lid_orientation' would probably be best (to distinguish from e.g. > acceleration) No, they don't represent orientation, they're simply the acceleration (including gravity) present on the base accelerometer and lid accelerometer. https://codereview.chromium.org/200643005/diff/260001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/200643005/diff/260001/chromeos/chromeos.gyp#n... chromeos/chromeos.gyp:53: 'accelerometer/accelerometer_reader.cc', On 2014/04/02 20:22:57, Daniel Erat wrote: > nit: fix alphabetization Done.
The CQ bit was checked by flackr@chromium.org
The CQ bit was unchecked by flackr@chromium.org
The CQ bit was checked by flackr@chromium.org
https://codereview.chromium.org/200643005/diff/280001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/200643005/diff/280001/chromeos/chromeos.gyp#n... chromeos/chromeos.gyp:49: 'accelerometer/accelerometer_reader.cc', still needs to move up a bit more, i think :-)
The CQ bit was unchecked by flackr@chromium.org
https://codereview.chromium.org/200643005/diff/280001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/200643005/diff/280001/chromeos/chromeos.gyp#n... chromeos/chromeos.gyp:49: 'accelerometer/accelerometer_reader.cc', On 2014/04/02 20:44:24, Daniel Erat wrote: > still needs to move up a bit more, i think :-) Oops, I think I'm going bug-eyed. Thanks!
The CQ bit was checked by flackr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/200643005/320001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by flackr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/200643005/340001
Message was sent while issue was closed.
Change committed as 261317 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
