Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2823583002:
chromeos: Add CPU temperature reader (Closed)
Descriptionchromeos: Add CPU temperature reader
Reads from hwmon in sysfs.
This does the same thing as ReadCPUTempInfo() in
chrome/browser/chromeos/policy/device_status_collector.cc, but it is
available to the rest of Chrome.
BUG=chromium:709102
Review-Url: https://codereview.chromium.org/2823583002
Cr-Commit-Position: refs/heads/master@{#466858}
Committed: https://chromium.googlesource.com/chromium/src/+/745fad9fe0f68a795a191a32cb038d864bfc2dbf
Patch Set 1 #
Total comments: 10
Patch Set 2 : Factor out temp and label reading; Add class and unit test #
Total comments: 9
Patch Set 3 : Rename to cpu_temperature_reader; Move struct into class #Patch Set 4 : Make runnable on main UI thread #
Total comments: 22
Patch Set 5 : Applied derat's feedback #Patch Set 6 : Turn back into a blocking operation #Patch Set 7 : Handle trailing newline in sysfs #
Total comments: 32
Patch Set 8 : Answered derat's feedback #
Total comments: 4
Patch Set 9 : Change hwmon_dir_ to FilePath; Remove empty path check #
Total comments: 2
Patch Set 10 : set_hwmon_dir_for_test takes FilePath arg #
Total comments: 2
Patch Set 11 : one last fix #
Messages
Total messages: 47 (10 generated)
sque@chromium.org changed reviewers: + derat@chromium.org, stevenjb@chromium.org
On 2017/04/14 14:01:44, Simon Que wrote: I am not sure if this needs a unit test, and if so, what kind of test coverage. The code is dependent on the implementation of hwmon sysfs interface which is not testable from Chrome apart from a Chrome OS system.
Moving this code to src/chromeos makes sense, but since we are only moving part of device_status_collector.cc we should consider what API we want to present and how it is going to be used efficiently. In order to test this we will probably need to create a delegate interface for retrieving the files to be read (which can be replaced with a test delegate). That means we should probably put this in a class. We should consider whether there are other pieces from device_status_collector.cc that we would like to move as well. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.cc (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.cc:48: } This would be a bit more readable if some or all of the code below was moved to a local helper function. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:24: std::vector<CPUTemperatureInfo> GetCPUTemperatures(); This function does file operations and there is no indication that it must be called from a slow task runner. It is hard to review this without the context of how it is going to be used. I suspect that you will want this to take a callback instead of returning the result immediately so that the function can handle that implementation detail.
agreed re putting this in a class. you don't really need a delegate interface, though -- just add a hwmon_dir_ member that gets initialized to "/sys/class/hwmon/" by default but can be set to something else with a set_hwmon_dir_for_test() method, and then call that from the test to point it at a temp dir instead of sysfs. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.cc (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.cc:71: result.push_back(info); i think, but am not certain, that you might just be able to do this: result.emplace_back(static_cast<double>(temperature) / 1000, label); https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:5: #ifndef CHROMEOS_SYSTEM_CPU_TEMP_READER_H_ nit: just rename this to cpu_temperature.h if you don't plan to actually introduce a class named CPUTemperatureReader? https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:19: std::string label; how's this going to be used? an opaque string label like this seems hard to interpret.
https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.cc (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.cc:48: } On 2017/04/14 15:44:04, stevenjb wrote: > This would be a bit more readable if some or all of the code below was moved to > a local helper function. > Done. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.cc:71: result.push_back(info); On 2017/04/14 18:53:32, Daniel Erat wrote: > i think, but am not certain, that you might just be able to do this: > > result.emplace_back(static_cast<double>(temperature) / 1000, label); No longer applicable due to refactoring. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:5: #ifndef CHROMEOS_SYSTEM_CPU_TEMP_READER_H_ On 2017/04/14 18:53:32, Daniel Erat wrote: > nit: just rename this to cpu_temperature.h if you don't plan to actually > introduce a class named CPUTemperatureReader? Introducing a class. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:19: std::string label; On 2017/04/14 18:53:32, Daniel Erat wrote: > how's this going to be used? an opaque string label like this seems hard to > interpret. Done. https://codereview.chromium.org/2823583002/diff/1/chromeos/system/cpu_temp_re... chromeos/system/cpu_temp_reader.h:24: std::vector<CPUTemperatureInfo> GetCPUTemperatures(); On 2017/04/14 15:44:04, stevenjb wrote: > This function does file operations and there is no indication that it must be > called from a slow task runner. > > It is hard to review this without the context of how it is going to be used. I > suspect that you will want this to take a callback instead of returning the > result immediately so that the function can handle that implementation detail. I will leave it up to the caller to implement it with a task runner and callback. e.g. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/device_st...
https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:18: struct CPUTemperatureInfo { Make this a member of the class. https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:20: double temp_celsius; blank line https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:33: class CPUTemperatureReader { File name should match the class name. https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:41: // function can choose to call it from a task runner instead. Instead of what? Implemented this way, callers *must* call it from a blocking task runner.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:18: struct CPUTemperatureInfo { On 2017/04/17 16:49:00, stevenjb wrote: > Make this a member of the class. Done. https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:20: double temp_celsius; On 2017/04/17 16:49:00, stevenjb wrote: > blank line Done. https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:33: class CPUTemperatureReader { On 2017/04/17 16:49:00, stevenjb wrote: > File name should match the class name. Done. https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:41: // function can choose to call it from a task runner instead. On 2017/04/17 16:49:00, stevenjb wrote: > Instead of what? Implemented this way, callers *must* call it from a blocking > task runner. Do you have an example of Chrome code that does non-blocking file I/O?
https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... File chromeos/system/cpu_temp_reader.h (right): https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... chromeos/system/cpu_temp_reader.h:41: // function can choose to call it from a task runner instead. On 2017/04/17 17:07:48, Simon Que wrote: > On 2017/04/17 16:49:00, stevenjb wrote: > > Instead of what? Implemented this way, callers *must* call it from a blocking > > task runner. > > Do you have an example of Chrome code that does non-blocking file I/O? I'm not sure what you mean. There is no such thing. The point I am trying to make is that the fact that this method does blocking IO is largely an implementation detail. We should either: a) Allow this to be called form the main/UI thread (and make it async). b) Clarify the comment, i.e. "This is a blocking function that performs file operations. It must be called from a task runner with MayBlock() set."
On 2017/04/17 17:24:40, stevenjb wrote: > https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... > File chromeos/system/cpu_temp_reader.h (right): > > https://codereview.chromium.org/2823583002/diff/20001/chromeos/system/cpu_tem... > chromeos/system/cpu_temp_reader.h:41: // function can choose to call it from a > task runner instead. > On 2017/04/17 17:07:48, Simon Que wrote: > > On 2017/04/17 16:49:00, stevenjb wrote: > > > Instead of what? Implemented this way, callers *must* call it from a > blocking > > > task runner. > > > > Do you have an example of Chrome code that does non-blocking file I/O? > > I'm not sure what you mean. There is no such thing. > > The point I am trying to make is that the fact that this method does blocking IO > is largely an implementation detail. We should either: > > a) Allow this to be called form the main/UI thread (and make it async). > b) Clarify the comment, i.e. > "This is a blocking function that performs file operations. It must be called > from a task runner with MayBlock() set." For option (a), can you point me to an example of such a function?
https://cs.chromium.org/chromium/src/chromeos/dbus/session_manager_client.cc?... On Mon, Apr 17, 2017 at 11:36 AM, <sque@chromium.org> wrote: > On 2017/04/17 17:24:40, stevenjb wrote: > > > https://codereview.chromium.org/2823583002/diff/20001/ > chromeos/system/cpu_temp_reader.h > > File chromeos/system/cpu_temp_reader.h (right): > > > > > https://codereview.chromium.org/2823583002/diff/20001/ > chromeos/system/cpu_temp_reader.h#newcode41 > > chromeos/system/cpu_temp_reader.h:41: // function can choose to call it > from a > > task runner instead. > > On 2017/04/17 17:07:48, Simon Que wrote: > > > On 2017/04/17 16:49:00, stevenjb wrote: > > > > Instead of what? Implemented this way, callers *must* call it from a > > blocking > > > > task runner. > > > > > > Do you have an example of Chrome code that does non-blocking file I/O? > > > > I'm not sure what you mean. There is no such thing. > > > > The point I am trying to make is that the fact that this method does > blocking > IO > > is largely an implementation detail. We should either: > > > > a) Allow this to be called form the main/UI thread (and make it async). > > b) Clarify the comment, i.e. > > "This is a blocking function that performs file operations. It must be > called > > from a task runner with MayBlock() set." > > For option (a), can you point me to an example of such a function? > > https://codereview.chromium.org/2823583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/17 17:47:02, stevenjb wrote: > https://cs.chromium.org/chromium/src/chromeos/dbus/session_manager_client.cc?... I'm still a bit confused here. I assume that GetFileContent() is analogous to GetCPUTemperatures() in my CL. GetFileContent() has the comment "Helper to asynchronously retrieve a file's content." But I don't understand why the comment says anything about it being asynchronous. There's nothing inherently asynchronous about it, right? Does it mean that this function must be called asynchronously? Also, the code link points to the function being run on a task runner with the MayBlock trait. That corresponds to option (b). Do you have any examples of option (a)?
On 2017/04/18 14:31:58, Simon Que wrote: > On 2017/04/17 17:47:02, stevenjb wrote: > > > https://cs.chromium.org/chromium/src/chromeos/dbus/session_manager_client.cc?... > > > I'm still a bit confused here. I assume that GetFileContent() is analogous to > GetCPUTemperatures() in my CL. > > GetFileContent() has the comment "Helper to asynchronously retrieve a file's > content." But I don't understand why the comment says anything about it being > asynchronous. There's nothing inherently asynchronous about it, right? Does it > mean that this function must be called asynchronously? > > Also, the code link points to the function being run on a task runner with the > MayBlock trait. That corresponds to option (b). Do you have any examples of > option (a)? No. RetrieveDevicePolicy is analogous to GetCPUTemperatures(). This is option (a). RetrieveDevicePolicy() can be called from the UI thread. It takes a callback that will also be called on the UI thread with the results once they have been read. In order to make a function asynchronous it needs to take a callback to receive the results. One way or another your code is going to need to get called that way: UI thread: Triggers request for CPU temperatures, calls: // note - this is pseudo code GetTemperaturesOnUIThread() { PostTaskAndReply(TaskTraits.MayBlock(), Bind(GetTemperaturesOnBlockingThread(args...)), Bind(ReceiveTemperaturesOnUIThread, GetWeakPtr())) } So, depending on where we expect your GetCPUTemperatures() method to be called from, we either want to implement it to take a callback to be invoked when the temperatures are ready so that it can be called from the UI thread, or we need to clearly document that it *must* be called from a function running on a blocking thread (i.e. posted as a task with MayBlock().
On 2017/04/18 15:44:01, stevenjb wrote: > On 2017/04/18 14:31:58, Simon Que wrote: > > On 2017/04/17 17:47:02, stevenjb wrote: > > > > > > https://cs.chromium.org/chromium/src/chromeos/dbus/session_manager_client.cc?... > > > > > > I'm still a bit confused here. I assume that GetFileContent() is analogous to > > GetCPUTemperatures() in my CL. > > > > GetFileContent() has the comment "Helper to asynchronously retrieve a file's > > content." But I don't understand why the comment says anything about it being > > asynchronous. There's nothing inherently asynchronous about it, right? Does it > > mean that this function must be called asynchronously? > > > > Also, the code link points to the function being run on a task runner with the > > MayBlock trait. That corresponds to option (b). Do you have any examples of > > option (a)? > > No. RetrieveDevicePolicy is analogous to GetCPUTemperatures(). This is option > (a). RetrieveDevicePolicy() can be called from the UI thread. It takes a > callback that will also be called on the UI thread with the results once they > have been read. > > In order to make a function asynchronous it needs to take a callback to receive > the results. One way or another your code is going to need to get called that > way: > > UI thread: Triggers request for CPU temperatures, calls: > > // note - this is pseudo code > GetTemperaturesOnUIThread() { > PostTaskAndReply(TaskTraits.MayBlock(), > Bind(GetTemperaturesOnBlockingThread(args...)), > Bind(ReceiveTemperaturesOnUIThread, GetWeakPtr())) > } > > So, depending on where we expect your GetCPUTemperatures() method to be called > from, we either want to implement it to take a callback to be invoked when the > temperatures are ready so that it can be called from the UI thread, or we need > to clearly document that it *must* be called from a function running on a > blocking thread (i.e. posted as a task with MayBlock(). Thanks, that makes more sense now. If I were to go with option (a), how would I test it in a unit test? Do I need to create a task runner or something?
Create a callback that stores the results then call your method then call: base::RunLoop().RunUntilIdle(). Then check the results. On Tue, Apr 18, 2017 at 10:14 AM, <sque@chromium.org> wrote: > On 2017/04/18 15:44:01, stevenjb wrote: > > On 2017/04/18 14:31:58, Simon Que wrote: > > > On 2017/04/17 17:47:02, stevenjb wrote: > > > > > > > > > > https://cs.chromium.org/chromium/src/chromeos/dbus/ > session_manager_client.cc?type=cs&q=MayBlock+file:% > 5Esrc/chromeos/+package:%5Echromium$&l=890 > > > > > > > > > I'm still a bit confused here. I assume that GetFileContent() is > analogous > to > > > GetCPUTemperatures() in my CL. > > > > > > GetFileContent() has the comment "Helper to asynchronously retrieve a > file's > > > content." But I don't understand why the comment says anything about it > being > > > asynchronous. There's nothing inherently asynchronous about it, right? > Does > it > > > mean that this function must be called asynchronously? > > > > > > Also, the code link points to the function being run on a task runner > with > the > > > MayBlock trait. That corresponds to option (b). Do you have any > examples of > > > option (a)? > > > > No. RetrieveDevicePolicy is analogous to GetCPUTemperatures(). This is > option > > (a). RetrieveDevicePolicy() can be called from the UI thread. It takes a > > callback that will also be called on the UI thread with the results once > they > > have been read. > > > > In order to make a function asynchronous it needs to take a callback to > receive > > the results. One way or another your code is going to need to get called > that > > way: > > > > UI thread: Triggers request for CPU temperatures, calls: > > > > // note - this is pseudo code > > GetTemperaturesOnUIThread() { > > PostTaskAndReply(TaskTraits.MayBlock(), > > Bind(GetTemperaturesOnBlockingThread(args...)), > > Bind(ReceiveTemperaturesOnUIThread, GetWeakPtr())) > > } > > > > So, depending on where we expect your GetCPUTemperatures() method to be > called > > from, we either want to implement it to take a callback to be invoked > when the > > temperatures are ready so that it can be called from the UI thread, or > we need > > to clearly document that it *must* be called from a function running on a > > blocking thread (i.e. posted as a task with MayBlock(). > > Thanks, that makes more sense now. > > If I were to go with option (a), how would I test it in a unit test? Do I > need > to create a task runner or something? > > https://codereview.chromium.org/2823583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/18 16:35:47, stevenjb wrote: > Create a callback that stores the results then call your method then call: > > base::RunLoop().RunUntilIdle(). > > Then check the results. Done
i didn't look at the tests yet https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:22: const char kDefaultHwmonDir[] = "/sys/class/hwmon/"; nit: use constexpr char for all of these https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:27: using CPUTemperatureInfo = CPUTemperatureReader::CPUTemperatureInfo; move this up outside of the anon namespace (it feels a bit weird to me to put it here) https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:29: bool ReadTemperatureFromPath(const base::FilePath& path, double* temp_celsius) { temp_celsius_out https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:34: *temp_celsius = static_cast<double>(temperature) / 1000; instead of static_cast, you can probably just divide the uint32_t by 1000.0 https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:53: return label; returning an empty string feels a bit strange. would it be better to just return the original path if we can't find anything else? https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:60: // Get directories /sys/class/hwmon/hwmon* nit: add trailing period https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:66: // Get temp*_input files in hwmon*/ and hwmon*/device/ nit: add trailing period https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:41: CPUTemperatureReader(); declare a d'tor here and define it in the .cc file https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:44: // containing a reading from each sensor to |callback|. This function can be nit: change second sentence to "Asynchronously runs |callback| with a reading from each sensor." https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:48: void set_hwmon_dir_for_test(const std::string& dir) { hwmon_dir_ = dir; } nit: inline getters/setters usually come after d'tor and before non-inline methods https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:52: // overriden by set_hwmon_dir_for_test(). nit: don't need second sentence
https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:22: const char kDefaultHwmonDir[] = "/sys/class/hwmon/"; On 2017/04/20 21:23:52, Daniel Erat wrote: > nit: use constexpr char for all of these Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:27: using CPUTemperatureInfo = CPUTemperatureReader::CPUTemperatureInfo; On 2017/04/20 21:23:52, Daniel Erat wrote: > move this up outside of the anon namespace (it feels a bit weird to me to put it > here) Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:29: bool ReadTemperatureFromPath(const base::FilePath& path, double* temp_celsius) { On 2017/04/20 21:23:53, Daniel Erat wrote: > temp_celsius_out Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:34: *temp_celsius = static_cast<double>(temperature) / 1000; On 2017/04/20 21:23:53, Daniel Erat wrote: > instead of static_cast, you can probably just divide the uint32_t by 1000.0 Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:53: return label; On 2017/04/20 21:23:53, Daniel Erat wrote: > returning an empty string feels a bit strange. would it be better to just return > the original path if we can't find anything else? Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:60: // Get directories /sys/class/hwmon/hwmon* On 2017/04/20 21:23:53, Daniel Erat wrote: > nit: add trailing period Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.cc:66: // Get temp*_input files in hwmon*/ and hwmon*/device/ On 2017/04/20 21:23:52, Daniel Erat wrote: > nit: add trailing period Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:41: CPUTemperatureReader(); On 2017/04/20 21:23:53, Daniel Erat wrote: > declare a d'tor here and define it in the .cc file Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:44: // containing a reading from each sensor to |callback|. This function can be On 2017/04/20 21:23:53, Daniel Erat wrote: > nit: change second sentence to "Asynchronously runs |callback| with a reading > from each sensor." Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:48: void set_hwmon_dir_for_test(const std::string& dir) { hwmon_dir_ = dir; } On 2017/04/20 21:23:53, Daniel Erat wrote: > nit: inline getters/setters usually come after d'tor and before non-inline > methods Done. https://codereview.chromium.org/2823583002/diff/80001/chromeos/system/cpu_tem... chromeos/system/cpu_temperature_reader.h:52: // overriden by set_hwmon_dir_for_test(). On 2017/04/20 21:23:53, Daniel Erat wrote: > nit: don't need second sentence Done.
After reviewing where I would call this code elsewhere in Chrome, it makes more sense to make it a blocking function, and let the caller post it to the proper thread. Specifically, I intend to call it from a derived class of SystemInfoProvider: https://cs.chromium.org/chromium/src/extensions/browser/api/system_info/syste... This class is already set up to run on the right thread, and expects the implementation of QueryInfo() to be blocking.
On 2017/04/21 09:32:44, Simon Que wrote: > After reviewing where I would call this code elsewhere in Chrome, it makes more > sense to make it a blocking function, and let the caller post it to the proper > thread. > > Specifically, I intend to call it from a derived class of SystemInfoProvider: > https://cs.chromium.org/chromium/src/extensions/browser/api/system_info/syste... > > This class is already set up to run on the right thread, and expects the > implementation of QueryInfo() to be blocking. that sounds fine to me. just document that it's blocking and add a DCHECK that it's not called on the browser thread, if possible.
On Fri, Apr 21, 2017 at 4:15 PM, <derat@chromium.org> wrote: > On 2017/04/21 09:32:44, Simon Que wrote: > > After reviewing where I would call this code elsewhere in Chrome, it > makes > more > > sense to make it a blocking function, and let the caller post it to the > proper > > thread. > > > > Specifically, I intend to call it from a derived class of > SystemInfoProvider: > > > https://cs.chromium.org/chromium/src/extensions/browser/api/system_info/ > system_info_provider.h?l=64 > > > > This class is already set up to run on the right thread, and expects the > > implementation of QueryInfo() to be blocking. > > that sounds fine to me. just document that it's blocking and add a DCHECK > that > it's not called on the browser thread, if possible. > Got example? I've only used a ThreadChecker to ensure that a function is always called on the same thread. Is that sufficient for this case? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
base::OpenFile already calls ThreadRestrictions::AssertIOAllowed, so I don't think we need to explicitly check that here, I just wanted to make sure the documentation was clear. lgtm
https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:39: bool ReadTemperatureFromPath(const base::FilePath& path, nit: add a comment describing this method https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:47: *temp_celsius_out = temperature / 1000.; nit: add '0' after '.' https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:51: // Get the label describing this temperature. Use the file "temp*_label" if it nit: s/Get/Gets/ https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:73: CPUTemperatureReader::~CPUTemperatureReader() {} nit: i think we prefer "= default;" instead of "{}" now https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:93: std::string label_path = temperature_path.MaybeAsASCII(); it feels confusing how you initialize this local here, use it in a log message, and then later modify it. i'd recommend reading the temperature first (using temperature_path.value() if you need to log) and then putting all the label stuff after that. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:21: struct CPUTemperatureInfo { i think you need to declare a c'tor and d'tor here (and define them in the .cc file with "= default;" since this struct has a non-POD member https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:30: bool operator<(const CPUTemperatureInfo& other) const { move this method above the members; also move the definition (and <utility> include) to the .cc file https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:31: return std::make_pair(temp_celsius, label) < sorting by temperature is a bit surprising. consider comparing label first instead https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:37: nit: delete blank line here https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:43: // containing a reading from each sensor to |callback|. This is a blocking fix comment; there's no callback now. also s/Read/Reads/ in first sentence since that's the form usually used in chrome method comments https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader_unittest.cc (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:23: void SetUp() override { put things in c'tor and d'tor instead of SetUp when possible: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:30: void TearDown() override { dir_.reset(); } why reset this? the d'tor will already destroy it for you https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:45: bool CreateFileWithContents(const base::FilePath& path, you never expect this to fail, right? return void and either ASSERT_EQ or CHECK_EQ within it so the callers don't need to verify that it suceeded https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:52: std::unique_ptr<base::ScopedTempDir> dir_; why does this need to be in a unique_ptr? https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:62: auto subdir_empty = CreateHwmonSubdir("hwmon0"); use explicit type rather than 'auto' since the type isn't obvious from the context; ditto below https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:74: auto cpu_temp_readings = reader_.GetCPUTemperatures(); use explicit types instead of 'auto'
https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:39: bool ReadTemperatureFromPath(const base::FilePath& path, On 2017/04/21 21:01:20, Daniel Erat wrote: > nit: add a comment describing this method Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:47: *temp_celsius_out = temperature / 1000.; On 2017/04/21 21:01:20, Daniel Erat wrote: > nit: add '0' after '.' Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:51: // Get the label describing this temperature. Use the file "temp*_label" if it On 2017/04/21 21:01:20, Daniel Erat wrote: > nit: s/Get/Gets/ Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:73: CPUTemperatureReader::~CPUTemperatureReader() {} On 2017/04/21 21:01:20, Daniel Erat wrote: > nit: i think we prefer "= default;" instead of "{}" now Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:93: std::string label_path = temperature_path.MaybeAsASCII(); On 2017/04/21 21:01:20, Daniel Erat wrote: > it feels confusing how you initialize this local here, use it in a log message, > and then later modify it. i'd recommend reading the temperature first (using > temperature_path.value() if you need to log) and then putting all the label > stuff after that. Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:21: struct CPUTemperatureInfo { On 2017/04/21 21:01:20, Daniel Erat wrote: > i think you need to declare a c'tor and d'tor here (and define them in the .cc > file with "= default;" since this struct has a non-POD member Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:30: bool operator<(const CPUTemperatureInfo& other) const { On 2017/04/21 21:01:20, Daniel Erat wrote: > move this method above the members; also move the definition (and <utility> > include) to the .cc file Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:31: return std::make_pair(temp_celsius, label) < On 2017/04/21 21:01:20, Daniel Erat wrote: > sorting by temperature is a bit surprising. consider comparing label first > instead Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:37: On 2017/04/21 21:01:20, Daniel Erat wrote: > nit: delete blank line here Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:43: // containing a reading from each sensor to |callback|. This is a blocking On 2017/04/21 21:01:20, Daniel Erat wrote: > fix comment; there's no callback now. also s/Read/Reads/ in first sentence since > that's the form usually used in chrome method comments Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader_unittest.cc (right): https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:23: void SetUp() override { On 2017/04/21 21:01:20, Daniel Erat wrote: > put things in c'tor and d'tor instead of SetUp when possible: > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:30: void TearDown() override { dir_.reset(); } On 2017/04/21 21:01:20, Daniel Erat wrote: > why reset this? the d'tor will already destroy it for you Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:45: bool CreateFileWithContents(const base::FilePath& path, On 2017/04/21 21:01:20, Daniel Erat wrote: > you never expect this to fail, right? return void and either ASSERT_EQ or > CHECK_EQ within it so the callers don't need to verify that it suceeded Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:52: std::unique_ptr<base::ScopedTempDir> dir_; On 2017/04/21 21:01:20, Daniel Erat wrote: > why does this need to be in a unique_ptr? Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:62: auto subdir_empty = CreateHwmonSubdir("hwmon0"); On 2017/04/21 21:01:20, Daniel Erat wrote: > use explicit type rather than 'auto' since the type isn't obvious from the > context; ditto below Done. https://codereview.chromium.org/2823583002/diff/140001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader_unittest.cc:74: auto cpu_temp_readings = reader_.GetCPUTemperatures(); On 2017/04/21 21:01:20, Daniel Erat wrote: > use explicit types instead of 'auto' Done.
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2823583002/#ps160001 (title: "Answered derat's feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:107: if (temperature_path.value().empty()) { what are you trying to catch with this check? looking at the implementation of FilePath, i'm not convinced that this can ever be empty (particularly if ReadTemperatureFromPath succeeded using the same path earlier). https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:51: std::string hwmon_dir_; make this be a base::FilePath instead
The CQ bit was unchecked by sque@chromium.org
https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.cc (right): https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.cc:107: if (temperature_path.value().empty()) { On 2017/04/24 20:16:50, Daniel Erat wrote: > what are you trying to catch with this check? looking at the implementation of > FilePath, i'm not convinced that this can ever be empty (particularly if > ReadTemperatureFromPath succeeded using the same path earlier). Done. https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/160001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:51: std::string hwmon_dir_; On 2017/04/24 20:16:50, Daniel Erat wrote: > make this be a base::FilePath instead Done.
https://codereview.chromium.org/2823583002/diff/180001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/180001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:43: void set_hwmon_dir_for_test(const std::string& dir) { this should take a const base::FilePath&. it's better to make interfaces be explicit in what they want, using the language's type system whenever possible.
https://codereview.chromium.org/2823583002/diff/180001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/180001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:43: void set_hwmon_dir_for_test(const std::string& dir) { On 2017/04/24 22:29:43, Daniel Erat wrote: > this should take a const base::FilePath&. it's better to make interfaces be > explicit in what they want, using the language's type system whenever possible. Done.
you need to re-upload
Done. On Mon, Apr 24, 2017 at 6:50 PM, <derat@chromium.org> wrote: > you need to re-upload > > https://codereview.chromium.org/2823583002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2823583002/diff/200001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/200001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:44: hwmon_dir_ = base::FilePath(dir); don't call base::FilePath here now
https://codereview.chromium.org/2823583002/diff/200001/chromeos/system/cpu_te... File chromeos/system/cpu_temperature_reader.h (right): https://codereview.chromium.org/2823583002/diff/200001/chromeos/system/cpu_te... chromeos/system/cpu_temperature_reader.h:44: hwmon_dir_ = base::FilePath(dir); On 2017/04/24 22:59:23, Daniel Erat wrote: > don't call base::FilePath here now Done.
lgtm
The CQ bit was checked by sque@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2823583002/#ps220001 (title: "one last fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1493080446499660,
"parent_rev": "f1269b6b08358849f9fed290de619122e34135b1", "commit_rev":
"745fad9fe0f68a795a191a32cb038d864bfc2dbf"}
Message was sent while issue was closed.
Description was changed from ========== chromeos: Add CPU temperature reader Reads from hwmon in sysfs. This does the same thing as ReadCPUTempInfo() in chrome/browser/chromeos/policy/device_status_collector.cc, but it is available to the rest of Chrome. BUG=chromium:709102 ========== to ========== chromeos: Add CPU temperature reader Reads from hwmon in sysfs. This does the same thing as ReadCPUTempInfo() in chrome/browser/chromeos/policy/device_status_collector.cc, but it is available to the rest of Chrome. BUG=chromium:709102 Review-Url: https://codereview.chromium.org/2823583002 Cr-Commit-Position: refs/heads/master@{#466858} Committed: https://chromium.googlesource.com/chromium/src/+/745fad9fe0f68a795a191a32cb03... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/745fad9fe0f68a795a191a32cb03...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/2836843004/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit(https://goo.gl/kROfz5) identified CL at revision 466858 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/2843433005/ by yhirano@chromium.org. The reason for reverting is: This change causes a build error..
Message was sent while issue was closed.
Patchset #12 (id:240001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
