Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2853863002:
Add parser for new cpu freq file (Closed)
DescriptionAdd parser for new cpu freq file
CPU frequency file exists in [2] for a majority of devices. It can
also exist in [1] for certain devices like chell, cave, caroline,
veyron_speedy, etc. Add a parser to read data from [1] which has
different format from [2].
[1] /sys/devices/system/cpu/cpufreq/all_time_in_state
[2] /sys/devices/system/cpu/cpu*/cpufreq/stats/time_in_state
BUG=548510
Review-Url: https://codereview.chromium.org/2853863002
Cr-Commit-Position: refs/heads/master@{#469503}
Committed: https://chromium.googlesource.com/chromium/src/+/904fd51f0ad7bb153457c7a46d9432883d1fa372
Patch Set 1 #
Total comments: 20
Patch Set 2 : Add unit test for both parsers #Patch Set 3 : Turn sampling period back to 30 (3 is used in test) #
Total comments: 51
Patch Set 4 : Add parser for new cpu freq file #
Total comments: 18
Patch Set 5 : Update time_in_state to be TimeDelta type #Patch Set 6 #
Total comments: 6
Patch Set 7 : Add parser for new cpu freq file #
Total comments: 8
Patch Set 8 : const->constexpr #Patch Set 9 : Move code to SetUp; CHECK->ASSERT_TRUE #
Total comments: 2
Patch Set 10 : Add '// testing::Test:' #
Messages
Total messages: 37 (11 generated)
weidongg@chromium.org changed reviewers: + oshima@chromium.org
derat@chromium.org changed reviewers: + derat@chromium.org
this string-parsing code seems extremely unit-testable. please write tests for it and include them in this change. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:244: bool ReadCpuFreqFromNewFile( "old file" and "new file" aren't very descriptive. :-) please add comments to the tops of both functions describing the differences between the two (i.e. the standard locations of the files and an example of the content of each). https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:248: std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) { please document all of these parameters. freq_samples looks like it's supposed to contain some existing information, for example. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:265: state_count -= 1; consider just using base::TrimWhitespaceASCII on the input string to drop trailing whitespace without needing to walk the count back like this https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:267: if (state_count > 0) { if lines is empty, shouldn't you just return false? https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = IndexInVector(state_name, cpu_freq_state_names); presumably you need to check the index here. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:289: base::StringToInt64(array[cpu + 1], &occupancy_time_centisecond); you need to check the return value of this conversion https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { test for bad input and return false immediately; then you don't need to indent the non-broken case
https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = IndexInVector(state_name, cpu_freq_state_names); On 2017/05/01 19:46:32, Daniel Erat wrote: > presumably you need to check the index here. the index returned is always valid, as the state_name is added to the cpu_freq_state_names if it does not exist. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { On 2017/05/01 19:46:32, Daniel Erat wrote: > test for bad input and return false immediately; then you don't need to indent > the non-broken case Sorry, I don't understand, could you be more specific? Do you mean I need to check format at the beginning and then return false immediately if it's bad format?
https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:247: std::vector<std::string>* cpu_freq_state_names, high-level question about this code: is there any reason that we don't instead pass a std::map<std::string, CpuDataCollector::StateOccupancySample>*? it seems strange to have a separate vector that's just used to map names to indices. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = IndexInVector(state_name, cpu_freq_state_names); On 2017/05/01 21:50:10, weidongg wrote: > On 2017/05/01 19:46:32, Daniel Erat wrote: > > presumably you need to check the index here. > > the index returned is always valid, as the state_name is added to the > cpu_freq_state_names if it does not exist. sorry, i was confused by the name IndexInVector. would you mind renaming it to something like EnsureInVector to make it more obvious that it'll insert the element if it isn't already there? https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { On 2017/05/01 21:50:10, weidongg wrote: > On 2017/05/01 19:46:32, Daniel Erat wrote: > > test for bad input and return false immediately; then you don't need to indent > > the non-broken case > > Sorry, I don't understand, could you be more specific? Do you mean I need to > check format at the beginning and then return false immediately if it's bad > format? i mean this code would be simpler if you did: if (array.size() != size_t(cpu_count) + 1 || base::StringToInt(...)) { LOG(ERROR) << ...; return false; } const std::string state_name = ...;
On 2017/05/01 22:28:39, Daniel Erat wrote: > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > File chrome/browser/chromeos/power/cpu_data_collector.cc (right): > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > chrome/browser/chromeos/power/cpu_data_collector.cc:247: > std::vector<std::string>* cpu_freq_state_names, > high-level question about this code: is there any reason that we don't instead > pass a std::map<std::string, CpuDataCollector::StateOccupancySample>*? it seems > strange to have a separate vector that's just used to map names to indices. > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = > IndexInVector(state_name, cpu_freq_state_names); > On 2017/05/01 21:50:10, weidongg wrote: > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > presumably you need to check the index here. > > > > the index returned is always valid, as the state_name is added to the > > cpu_freq_state_names if it does not exist. > > sorry, i was confused by the name IndexInVector. would you mind renaming it to > something like EnsureInVector to make it more obvious that it'll insert the > element if it isn't already there? > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { > On 2017/05/01 21:50:10, weidongg wrote: > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > test for bad input and return false immediately; then you don't need to > indent > > > the non-broken case > > > > Sorry, I don't understand, could you be more specific? Do you mean I need to > > check format at the beginning and then return false immediately if it's bad > > format? > > i mean this code would be simpler if you did: > > if (array.size() != size_t(cpu_count) + 1 || > base::StringToInt(...)) { > LOG(ERROR) << ...; > return false; > } > > const std::string state_name = ...; These parser functions take in file path as parameter, I am not sure how to make unit tests with these file path. Maybe I should change the parameter file path to the string content of the file, and then make unit tests over these functions? These parser functions are all in anonymous namespace, should I export them all for unit tests?
On 2017/05/02 00:21:56, weidongg wrote: > On 2017/05/01 22:28:39, Daniel Erat wrote: > > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > > File chrome/browser/chromeos/power/cpu_data_collector.cc (right): > > > > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > > chrome/browser/chromeos/power/cpu_data_collector.cc:247: > > std::vector<std::string>* cpu_freq_state_names, > > high-level question about this code: is there any reason that we don't instead > > pass a std::map<std::string, CpuDataCollector::StateOccupancySample>*? it > seems > > strange to have a separate vector that's just used to map names to indices. > > > > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > > chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = > > IndexInVector(state_name, cpu_freq_state_names); > > On 2017/05/01 21:50:10, weidongg wrote: > > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > > presumably you need to check the index here. > > > > > > the index returned is always valid, as the state_name is added to the > > > cpu_freq_state_names if it does not exist. > > > > sorry, i was confused by the name IndexInVector. would you mind renaming it to > > something like EnsureInVector to make it more obvious that it'll insert the > > element if it isn't already there? > > > > > https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... > > chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { > > On 2017/05/01 21:50:10, weidongg wrote: > > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > > test for bad input and return false immediately; then you don't need to > > indent > > > > the non-broken case > > > > > > Sorry, I don't understand, could you be more specific? Do you mean I need to > > > check format at the beginning and then return false immediately if it's bad > > > format? > > > > i mean this code would be simpler if you did: > > > > if (array.size() != size_t(cpu_count) + 1 || > > base::StringToInt(...)) { > > LOG(ERROR) << ...; > > return false; > > } > > > > const std::string state_name = ...; > > These parser functions take in file path as parameter, I am not sure how to make > unit tests with these file path. Maybe I should change the parameter file path > to the string content of the file, and then make unit tests over these > functions? > These parser functions are all in anonymous namespace, should I export them all > for unit tests? you can use base::ScopedTempDir to create a temporary directory for a test and later clean it up, and base::WriteFile to create files. since the file content is short, it'd probably be better (from the perspective of having fast tests) to just change these parser functions to take strings as you suggested, though. yes, the normal approach i take for testing functions in the anon namespace is to make them be public static methods of the class so it's easy for tests to call them.
I think Dan is better reviewer than me. Removing myself.
Description was changed from ========== Add parser for new cpu freq file CPU frequency file exists in [2] for a majority of devices. It can also exist in [1] for certain devices like chell, cave, caroline, veyron_speedy, etc. Add a parser to read data from [1] which has different format from [2]. [1] /sys/devices/system/cpu/cpufreq/all_time_in_state [2] /sys/devices/system/cpu/cpu*/cpufreq/stats/time_in_state BUG=548510 ========== to ========== Add parser for new cpu freq file CPU frequency file exists in [2] for a majority of devices. It can also exist in [1] for certain devices like chell, cave, caroline, veyron_speedy, etc. Add a parser to read data from [1] which has different format from [2]. [1] /sys/devices/system/cpu/cpufreq/all_time_in_state [2] /sys/devices/system/cpu/cpu*/cpufreq/stats/time_in_state BUG=548510 ==========
oshima@chromium.org changed reviewers: - oshima@chromium.org
https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:244: bool ReadCpuFreqFromNewFile( On 2017/05/01 19:46:32, Daniel Erat wrote: > "old file" and "new file" aren't very descriptive. :-) please add comments to > the tops of both functions describing the differences between the two (i.e. the > standard locations of the files and an example of the content of each). Done. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:247: std::vector<std::string>* cpu_freq_state_names, On 2017/05/01 22:28:38, Daniel Erat wrote: > high-level question about this code: is there any reason that we don't instead > pass a std::map<std::string, CpuDataCollector::StateOccupancySample>*? it seems > strange to have a separate vector that's just used to map names to indices. It seems that the author intentionally separated the indice with values. Putting CpuDataCollector::StateOccupancySample in vector, it is easier to pop out them from the vector and push them into the sample queues [1]. [1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/cpu_data_c... https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:248: std::vector<CpuDataCollector::StateOccupancySample>* freq_samples) { On 2017/05/01 19:46:32, Daniel Erat wrote: > please document all of these parameters. freq_samples looks like it's supposed > to contain some existing information, for example. Done. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:265: state_count -= 1; On 2017/05/01 19:46:32, Daniel Erat wrote: > consider just using base::TrimWhitespaceASCII on the input string to drop > trailing whitespace without needing to walk the count back like this Done. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:267: if (state_count > 0) { On 2017/05/01 19:46:32, Daniel Erat wrote: > if lines is empty, shouldn't you just return false? Yes, it is supposed to return false. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:281: size_t index = IndexInVector(state_name, cpu_freq_state_names); On 2017/05/01 22:28:38, Daniel Erat wrote: > On 2017/05/01 21:50:10, weidongg wrote: > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > presumably you need to check the index here. > > > > the index returned is always valid, as the state_name is added to the > > cpu_freq_state_names if it does not exist. > > sorry, i was confused by the name IndexInVector. would you mind renaming it to > something like EnsureInVector to make it more obvious that it'll insert the > element if it isn't already there? Done. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:289: base::StringToInt64(array[cpu + 1], &occupancy_time_centisecond); On 2017/05/01 19:46:32, Daniel Erat wrote: > you need to check the return value of this conversion Done. https://codereview.chromium.org/2853863002/diff/1/chrome/browser/chromeos/pow... chrome/browser/chromeos/power/cpu_data_collector.cc:293: } else { On 2017/05/01 22:28:38, Daniel Erat wrote: > On 2017/05/01 21:50:10, weidongg wrote: > > On 2017/05/01 19:46:32, Daniel Erat wrote: > > > test for bad input and return false immediately; then you don't need to > indent > > > the non-broken case > > > > Sorry, I don't understand, could you be more specific? Do you mean I need to > > check format at the beginning and then return false immediately if it's bad > > format? > > i mean this code would be simpler if you did: > > if (array.size() != size_t(cpu_count) + 1 || > base::StringToInt(...)) { > LOG(ERROR) << ...; > return false; > } > > const std::string state_name = ...; Ok, I see. Yes, that's better.
https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:212: // The last line could end with '\n'. Ignore the last empty string in shorter: "// Remove trailing newlines." https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:231: << "Dropping sample."; include the bad line in the error message so this is easier to debug https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:239: // The occupancy time is in units of centiseconds. nit: remove this comment; the variable name occupancy_time_centisecond already makes the units obvious https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:247: const std::string& path, pass paths as const base::FilePath&, not std::string https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:256: LOG(ERROR) << "Error reading " << path << ". " LOG(ERROR) << "Error reading " << path << "; dropping sample."; https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:261: // such cases. nit: shorten this comment too https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:269: size_t state_count = lines.size(); why not just do this instead of resizing, maintaining a separate count, etc.? // Skip the first line, which contains column names. for (size_t i = 1; i < lines.size(); ++i) { ... https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:279: int freq_in_khz; declare this just before it's used instead of up here: https://google.github.io/styleguide/cppguide.html#Local_Variables https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:284: if (array.size() != size_t(cpu_count) + 1 || use c++ casts, not c casts: https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:287: << "Dropping sample."; include the bad data in the error message https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:302: << "Dropping sample."; include bad data https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:46: std::vector<int64_t> time_in_state; if you're up for changing this, it should be either ms_in_state or (even better) a std::vector<base::TimeDelta>, which is completely unambiguous. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:77: static void SampleCpuIdleData( only the methods that you're actually calling from tests need to be public static methods. everything else should only be declared in the .cc file as before. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:115: static void SampleCpuFreqData( see above https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:125: static void SampleCpuStateAsync( see above https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:17: const char time_in_state_suffix_path_cpu0[] = "cpu0/time_in_state"; please follow the style guide's naming convention for constants, e.g. kTimeInStateSuffixPathCpu0. https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:40: const std::vector<std::string> expected_cpu_freq_state_names = {"20", "60", class-type static variables aren't allowed: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables use arrays instead. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:42: // Expceted time_in_state of sample in |freq_samples| for cpu0. "Expected" https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:45: // Expceted time_in_state of sample in |freq_samples| for cpu1. |freq_samples| is a local var defined it tests; referring to it here doesn't make sense https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:56: CHECK(temp_dir.CreateUniqueTempDir()); put code in c'tor and d'tor instead of SetUp/TearDown: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:65: CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu0)); remove all of these CreateTemporaryFile calls. see the function's documentation -- they're actually creating new temp files, probably under /tmp, and returning the paths. base::WriteFile will create new files at the paths you pass to it. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: void TearDown() override { CHECK(temp_dir.Delete()); } you don't need this call. ScopedTempDir's d'tor already does this. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:82: base::ScopedTempDir temp_dir; please read the style guide section on naming: https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:83: base::FilePath time_in_state_path_cpu0; make all of these const after you initialize them in the c'tor https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:87: DISALLOW_COPY_AND_ASSIGN(CpuDataCollectorTest); put this in the private section
https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:212: // The last line could end with '\n'. Ignore the last empty string in On 2017/05/02 23:41:10, Daniel Erat wrote: > shorter: "// Remove trailing newlines." Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:231: << "Dropping sample."; On 2017/05/02 23:41:09, Daniel Erat wrote: > include the bad line in the error message so this is easier to debug Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:239: // The occupancy time is in units of centiseconds. On 2017/05/02 23:41:10, Daniel Erat wrote: > nit: remove this comment; the variable name occupancy_time_centisecond already > makes the units obvious Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:247: const std::string& path, On 2017/05/02 23:41:09, Daniel Erat wrote: > pass paths as const base::FilePath&, not std::string Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:256: LOG(ERROR) << "Error reading " << path << ". " On 2017/05/02 23:41:10, Daniel Erat wrote: > LOG(ERROR) << "Error reading " << path << "; dropping sample."; Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:261: // such cases. On 2017/05/02 23:41:10, Daniel Erat wrote: > nit: shorten this comment too Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:269: size_t state_count = lines.size(); On 2017/05/02 23:41:10, Daniel Erat wrote: > why not just do this instead of resizing, maintaining a separate count, etc.? > > // Skip the first line, which contains column names. > for (size_t i = 1; i < lines.size(); ++i) { > ... Yes, this is better. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:279: int freq_in_khz; On 2017/05/02 23:41:10, Daniel Erat wrote: > declare this just before it's used instead of up here: > https://google.github.io/styleguide/cppguide.html#Local_Variables Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:284: if (array.size() != size_t(cpu_count) + 1 || On 2017/05/02 23:41:10, Daniel Erat wrote: > use c++ casts, not c casts: > https://google.github.io/styleguide/cppguide.html#Casting Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:287: << "Dropping sample."; On 2017/05/02 23:41:10, Daniel Erat wrote: > include the bad data in the error message Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:302: << "Dropping sample."; On 2017/05/02 23:41:10, Daniel Erat wrote: > include bad data Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:46: std::vector<int64_t> time_in_state; On 2017/05/02 23:41:10, Daniel Erat wrote: > if you're up for changing this, it should be either ms_in_state or (even better) > a std::vector<base::TimeDelta>, which is completely unambiguous. time_in_state here may try to be consistent with the cpu freq file name in linux kernel. https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/cpu_data_c... Maybe change it to time_in_state_milisecond (which is a little bit long)? https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:77: static void SampleCpuIdleData( On 2017/05/02 23:41:10, Daniel Erat wrote: > only the methods that you're actually calling from tests need to be public > static methods. everything else should only be declared in the .cc file as > before. Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:115: static void SampleCpuFreqData( On 2017/05/02 23:41:10, Daniel Erat wrote: > see above Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:125: static void SampleCpuStateAsync( On 2017/05/02 23:41:10, Daniel Erat wrote: > see above Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:17: const char time_in_state_suffix_path_cpu0[] = "cpu0/time_in_state"; On 2017/05/02 23:41:11, Daniel Erat wrote: > please follow the style guide's naming convention for constants, e.g. > kTimeInStateSuffixPathCpu0. > > https://google.github.io/styleguide/cppguide.html#Constant_Names Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:40: const std::vector<std::string> expected_cpu_freq_state_names = {"20", "60", On 2017/05/02 23:41:10, Daniel Erat wrote: > class-type static variables aren't allowed: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > use arrays instead. Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:42: // Expceted time_in_state of sample in |freq_samples| for cpu0. On 2017/05/02 23:41:11, Daniel Erat wrote: > "Expected" Done. I put these expected variables in the test class and use std::vector so that EXPECT_EQ can compare these with the results. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:45: // Expceted time_in_state of sample in |freq_samples| for cpu1. On 2017/05/02 23:41:10, Daniel Erat wrote: > |freq_samples| is a local var defined it tests; referring to it here doesn't > make sense Removed https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:56: CHECK(temp_dir.CreateUniqueTempDir()); On 2017/05/02 23:41:10, Daniel Erat wrote: > put code in c'tor and d'tor instead of SetUp/TearDown: > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:65: CHECK(base::CreateTemporaryFile(&time_in_state_path_cpu0)); On 2017/05/02 23:41:11, Daniel Erat wrote: > remove all of these CreateTemporaryFile calls. see the function's documentation > -- they're actually creating new temp files, probably under /tmp, and returning > the paths. base::WriteFile will create new files at the paths you pass to it. When I removed these CreateTemporaryFile calls and run the test locally, the following checks would fail. It seems that the file is not created by base::WriteFile. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: void TearDown() override { CHECK(temp_dir.Delete()); } On 2017/05/02 23:41:11, Daniel Erat wrote: > you don't need this call. ScopedTempDir's d'tor already does this. Removed https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:82: base::ScopedTempDir temp_dir; On 2017/05/02 23:41:10, Daniel Erat wrote: > please read the style guide section on naming: > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:83: base::FilePath time_in_state_path_cpu0; On 2017/05/02 23:41:11, Daniel Erat wrote: > make all of these const after you initialize them in the c'tor The initialization of these variable should happen after temp_dir_.CreateUniqueTempDir(), so it looks like they could not be marked const. I put these variables in private, and mark the getter function const. https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:87: DISALLOW_COPY_AND_ASSIGN(CpuDataCollectorTest); On 2017/05/02 23:41:10, Daniel Erat wrote: > put this in the private section Done.
https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://codereview.chromium.org/2853863002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.h:46: std::vector<int64_t> time_in_state; On 2017/05/03 21:32:31, weidongg wrote: > On 2017/05/02 23:41:10, Daniel Erat wrote: > > if you're up for changing this, it should be either ms_in_state or (even > better) > > a std::vector<base::TimeDelta>, which is completely unambiguous. > > time_in_state here may try to be consistent with the cpu freq file name in linux > kernel. > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/power/cpu_data_c... > Maybe change it to time_in_state_milisecond (which is a little bit long)? time_in_state_ms would be okay, but using base::TimeDelta is even better. it's also 64 bits and copyable, but it doesn't have any ambiguity about units.
https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:54: // in some kernels such as 3.14.0. nit: would "in 3.14.0 or newer kernels" be more accurate? https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:220: size_t state_count = lines.size(); remove this variable and just use lines.size() in the loop condition https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:265: size_t state_count = lines.size(); remove this variable and just use lines.size() in the loop condition https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:268: for (size_t state = 1; state < state_count; ++state) { nit: line_num instead of state? https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:305: void SampleCpuFreqData( this should still be up in the anonymous namespace, no? https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:330: const std::string time_in_state_path = switch this to base::FilePath so that you aren't converting it multiple times later https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:16: // The suffix path of fake cpu frequency file for cpu0. nit: please add a blank line above each comment to make this easier to read https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:38: } // namespace nit: add a blank line above this one https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:91: base::FilePath all_time_in_state_path_; you can just make all these members protected and remove the accessors
https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:54: // in some kernels such as 3.14.0. On 2017/05/03 22:35:43, Daniel Erat wrote: > nit: would "in 3.14.0 or newer kernels" be more accurate? Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:220: size_t state_count = lines.size(); On 2017/05/03 22:35:43, Daniel Erat wrote: > remove this variable and just use lines.size() in the loop condition Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:265: size_t state_count = lines.size(); On 2017/05/03 22:35:43, Daniel Erat wrote: > remove this variable and just use lines.size() in the loop condition Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:268: for (size_t state = 1; state < state_count; ++state) { On 2017/05/03 22:35:43, Daniel Erat wrote: > nit: line_num instead of state? Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:305: void SampleCpuFreqData( On 2017/05/03 22:35:43, Daniel Erat wrote: > this should still be up in the anonymous namespace, no? Yes, I forgot to put them in the namespace. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector.cc:330: const std::string time_in_state_path = On 2017/05/03 22:35:43, Daniel Erat wrote: > switch this to base::FilePath so that you aren't converting it multiple times > later Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:16: // The suffix path of fake cpu frequency file for cpu0. On 2017/05/03 22:35:43, Daniel Erat wrote: > nit: please add a blank line above each comment to make this easier to read Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:38: } // namespace On 2017/05/03 22:35:43, Daniel Erat wrote: > nit: add a blank line above this one Done. https://codereview.chromium.org/2853863002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:91: base::FilePath all_time_in_state_path_; On 2017/05/03 22:35:43, Daniel Erat wrote: > you can just make all these members protected and remove the accessors Done.
lgtm with comments https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.cc:54: // in some kernels such as 3.14.0 or newer kernels. nit: just change this line to "in 3.14.0 or newer kernels" (delete "some kernels such as") https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.cc:226: const base::FilePath time_in_state_path = base::FilePath( just do: const base::FilePath time_in_state_path( base::StringPrintf...); https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.h:42: // A mapping from a CPU state to time spent in that state in milliseconds. nit: remove "in milliseconds" from this comment now
https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector.cc (right): https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.cc:54: // in some kernels such as 3.14.0 or newer kernels. On 2017/05/03 23:39:35, Daniel Erat wrote: > nit: just change this line to "in 3.14.0 or newer kernels" (delete "some kernels > such as") Done. https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.cc:226: const base::FilePath time_in_state_path = base::FilePath( On 2017/05/03 23:39:35, Daniel Erat wrote: > just do: > > const base::FilePath time_in_state_path( > base::StringPrintf...); Done. https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector.h (right): https://codereview.chromium.org/2853863002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector.h:42: // A mapping from a CPU state to time spent in that state in milliseconds. On 2017/05/03 23:39:35, Daniel Erat wrote: > nit: remove "in milliseconds" from this comment now Done.
weidongg@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@chromium.org: Please review changes in chrome/browser/ui/webui/chromeos/power_ui.cc
power_ui.cc lgtm but I have nits in the test code. https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:20: const char kTimeInStateSuffixPathCpu0[] = "cpu0/time_in_state"; nit: const -> constexpr, here and other places. https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); Any reason why not put all of these in an overidden testing::Test::SetUp()? And CHECK -> ASSERT_TRUE when moving the code.
https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:20: const char kTimeInStateSuffixPathCpu0[] = "cpu0/time_in_state"; On 2017/05/04 20:12:01, xiyuan wrote: > nit: const -> constexpr, here and other places. Done. https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); On 2017/05/04 20:12:01, xiyuan wrote: > Any reason why not put all of these in an overidden testing::Test::SetUp()? And > CHECK -> ASSERT_TRUE when moving the code. There are some constants to be initialized in the c'tor, so may be it's better to put these code here. https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul...
https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); On 2017/05/04 20:32:46, weidongg wrote: > On 2017/05/04 20:12:01, xiyuan wrote: > > Any reason why not put all of these in an overidden testing::Test::SetUp()? > And > > CHECK -> ASSERT_TRUE when moving the code. > > There are some constants to be initialized in the c'tor, so may be it's better > to put these code here. > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... The const member are initialized in the member initializer line. Leaving the code in ctor has no real benefit on that. The code here is too complicated for a ctor, IMHO. Also you have to use CHECK in ctor. CHECK should be used with care as it aborts the process and might have side effects, e.g. generating a core file if we don't config test bots correctly. I would prefer to move the code into Setup and change CHECK to ASSERT_TRUE.
https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); On 2017/05/04 20:40:02, xiyuan wrote: > On 2017/05/04 20:32:46, weidongg wrote: > > On 2017/05/04 20:12:01, xiyuan wrote: > > > Any reason why not put all of these in an overidden testing::Test::SetUp()? > > And > > > CHECK -> ASSERT_TRUE when moving the code. > > > > There are some constants to be initialized in the c'tor, so may be it's better > > to put these code here. > > > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... > > The const member are initialized in the member initializer line. Leaving the > code in ctor has no real benefit on that. > > The code here is too complicated for a ctor, IMHO. Also you have to use CHECK in > ctor. CHECK should be used with care as it aborts the process and might have > side effects, e.g. generating a core file if we don't config test bots > correctly. I would prefer to move the code into Setup and change CHECK to > ASSERT_TRUE. Thanks for explanation. Done.
https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); On 2017/05/04 21:03:45, weidongg wrote: > On 2017/05/04 20:40:02, xiyuan wrote: > > On 2017/05/04 20:32:46, weidongg wrote: > > > On 2017/05/04 20:12:01, xiyuan wrote: > > > > Any reason why not put all of these in an overidden > testing::Test::SetUp()? > > > And > > > > CHECK -> ASSERT_TRUE when moving the code. > > > > > > There are some constants to be initialized in the c'tor, so may be it's > better > > > to put these code here. > > > > > > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... > > > > The const member are initialized in the member initializer line. Leaving the > > code in ctor has no real benefit on that. > > > > The code here is too complicated for a ctor, IMHO. Also you have to use CHECK > in > > ctor. CHECK should be used with care as it aborts the process and might have > > side effects, e.g. generating a core file if we don't config test bots > > correctly. I would prefer to move the code into Setup and change CHECK to > > ASSERT_TRUE. > > Thanks for explanation. Done. not a huge deal either way, but just to give my justification for using c'tor/d'tor rather than SetUp/TearDown: - these commands should never fail. if they do, something is seriously wrong (e.g. the disk is broken) and other tests are going to fail too. - it's pointless and confusing to have two places to put init and de-init code. we often end up with code being split between the c'tor/d'tor and SetUp/TearDown. - as mentioned above, the googletest faq says to use c'tor/d'tor unless you have a good reason not to. - it's easy to make typos (e.g. "Setup"), although hopefully the compiler catches this now with 'override'. so in my opinion, the only reason to use SetUp/TearDown is if you're deriving from another test class that already uses it and you need to do setup that runs after the base class.
https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:79: -1); On 2017/05/04 21:12:46, Daniel Erat wrote: > On 2017/05/04 21:03:45, weidongg wrote: > > On 2017/05/04 20:40:02, xiyuan wrote: > > > On 2017/05/04 20:32:46, weidongg wrote: > > > > On 2017/05/04 20:12:01, xiyuan wrote: > > > > > Any reason why not put all of these in an overidden > > testing::Test::SetUp()? > > > > And > > > > > CHECK -> ASSERT_TRUE when moving the code. > > > > > > > > There are some constants to be initialized in the c'tor, so may be it's > > better > > > > to put these code here. > > > > > > > > > > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... > > > > > > The const member are initialized in the member initializer line. Leaving the > > > code in ctor has no real benefit on that. > > > > > > The code here is too complicated for a ctor, IMHO. Also you have to use > CHECK > > in > > > ctor. CHECK should be used with care as it aborts the process and might have > > > side effects, e.g. generating a core file if we don't config test bots > > > correctly. I would prefer to move the code into Setup and change CHECK to > > > ASSERT_TRUE. > > > > Thanks for explanation. Done. > > not a huge deal either way, but just to give my justification for using > c'tor/d'tor rather than SetUp/TearDown: > > - these commands should never fail. if they do, something is seriously wrong > (e.g. the disk is broken) and other tests are going to fail too. > - it's pointless and confusing to have two places to put init and de-init code. > we often end up with code being split between the c'tor/d'tor and > SetUp/TearDown. > - as mentioned above, the googletest faq says to use c'tor/d'tor unless you have > a good reason not to. > - it's easy to make typos (e.g. "Setup"), although hopefully the compiler > catches this now with 'override'. > > so in my opinion, the only reason to use SetUp/TearDown is if you're deriving > from another test class that already uses it and you need to do setup that runs > after the base class. My preference is to make ctor as simple as possible to avoid strange and hard to find bugs such as calling virtual functions from ctor etc. It is not a big deal for the code here though. That is why it was just a nit. https://codereview.chromium.org/2853863002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:60: void SetUp() override { nit: // testing::Test:
https://codereview.chromium.org/2853863002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/power/cpu_data_collector_unittest.cc (right): https://codereview.chromium.org/2853863002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/power/cpu_data_collector_unittest.cc:60: void SetUp() override { On 2017/05/04 21:25:35, xiyuan wrote: > nit: // testing::Test: Done.
still lgtm Thanks.
The CQ bit was checked by weidongg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org Link to the patchset: https://codereview.chromium.org/2853863002/#ps180001 (title: "Add '// testing::Test:'")
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": 180001, "attempt_start_ts": 1493934720295920,
"parent_rev": "154f59583bb9ef7a52c5a749cb201c1f4d1107d0", "commit_rev":
"9321b18141e9f49e12d72878b32c5fd187166b5c"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493934720295920,
"parent_rev": "8d06f045c53f2f42420e6ca4d57d3c608f529c9c", "commit_rev":
"904fd51f0ad7bb153457c7a46d9432883d1fa372"}
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493934720295920,
"parent_rev": "8d06f045c53f2f42420e6ca4d57d3c608f529c9c", "commit_rev":
"904fd51f0ad7bb153457c7a46d9432883d1fa372"}
Message was sent while issue was closed.
Description was changed from ========== Add parser for new cpu freq file CPU frequency file exists in [2] for a majority of devices. It can also exist in [1] for certain devices like chell, cave, caroline, veyron_speedy, etc. Add a parser to read data from [1] which has different format from [2]. [1] /sys/devices/system/cpu/cpufreq/all_time_in_state [2] /sys/devices/system/cpu/cpu*/cpufreq/stats/time_in_state BUG=548510 ========== to ========== Add parser for new cpu freq file CPU frequency file exists in [2] for a majority of devices. It can also exist in [1] for certain devices like chell, cave, caroline, veyron_speedy, etc. Add a parser to read data from [1] which has different format from [2]. [1] /sys/devices/system/cpu/cpufreq/all_time_in_state [2] /sys/devices/system/cpu/cpu*/cpufreq/stats/time_in_state BUG=548510 Review-Url: https://codereview.chromium.org/2853863002 Cr-Commit-Position: refs/heads/master@{#469503} Committed: https://chromium.googlesource.com/chromium/src/+/904fd51f0ad7bb153457c7a46d94... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/904fd51f0ad7bb153457c7a46d94... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
