Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1206733002:
ChromeOs Power Emulation Impl (Closed)
|
Created:
5 years, 6 months ago by mozartalouis Modified:
5 years, 5 months ago CC:
chromium-reviews, sadrul, derat+watch_chromium.org, hashimoto+watch_chromium.org, oshima+watch_chromium.org, kalyank Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOs Power Emulation Impl
BUG=499095
Here is the design doc for those who are wondering exactly what is being done:
https://docs.google.com/a/google.com/document/d/1X84nVB9r7Soe2VT2vB16wpE8TqFQ3J5tbkMDFmgVq-k/edit?usp=sharing
Committed: https://crrev.com/7e81ea890d6df774ad8c16868492f1d6d1ef4672
Cr-Commit-Position: refs/heads/master@{#338947}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Moved impl to exsiting FakePowerManagerClient class #
Total comments: 3
Patch Set 3 : Updated init and includes. Fixed RequestUpdate #
Total comments: 6
Patch Set 4 : added local namespace for constants #
Total comments: 2
Patch Set 5 : Naming #
Total comments: 14
Patch Set 6 : Update methods (IGNORE) #Patch Set 7 : Update To new methods and naming #
Total comments: 18
Patch Set 8 : variable changes, removed stub impl, naming #
Total comments: 6
Patch Set 9 : update #Patch Set 10 : New Impl, Work with UI #
Total comments: 16
Patch Set 11 : Updated Build files to include fake impl on chrome os build #Patch Set 12 : Created unittest for FakePowerManagerClient #
Total comments: 20
Patch Set 13 : Updated test and logic #
Total comments: 8
Patch Set 14 : Updated Unittest #
Total comments: 24
Patch Set 15 : Updated Unittest and others #
Total comments: 4
Patch Set 16 : Updated Unittest and removed power manager restrictions #
Total comments: 5
Patch Set 17 : Failed Unittest #Patch Set 18 : Fixed Unit test bug #
Total comments: 34
Patch Set 19 : Updated Unittest #Patch Set 20 : Updated Unittest #Patch Set 21 : Updated unittest #
Total comments: 11
Patch Set 22 : Updated Deps for broken unittests #
Total comments: 28
Patch Set 23 : Updated everying!!! #
Total comments: 14
Patch Set 24 : Making a better battery emu! #
Total comments: 10
Patch Set 25 : nits and includes #
Messages
Total messages: 127 (37 generated)
mozartalouis@google.com changed reviewers: + jennyz@chromium.org, oshima@chromium.org, stevenjb@chromium.org
Ready for review https://codereview.chromium.org/1206733002/diff/1/ash/system/chromeos/emulato... File ash/system/chromeos/emulator/battery_emulation_controller.cc (right): https://codereview.chromium.org/1206733002/diff/1/ash/system/chromeos/emulato... ash/system/chromeos/emulator/battery_emulation_controller.cc:75: void BatteryEmulationController::DecreaseScreenBrightness(bool allow_off) { Will we be needing Brightness features for the emulator? the power_manager_clients makes these pure virtual so they must be implemented. should I just leave empty? https://codereview.chromium.org/1206733002/diff/1/chromeos/dbus/power_manager... File chromeos/dbus/power_manager_client.cc (left): https://codereview.chromium.org/1206733002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.cc:775: PowerManagerClientStubImpl() Not sure if we should keep this class any longer since the emulator will take car of all things power. maybe keep it for first initialization purposes? https://codereview.chromium.org/1206733002/diff/1/chromeos/dbus/power_manager... chromeos/dbus/power_manager_client.cc:876: if (pause_count_ > 0) { Removed the current implementation of changing the battery life and all others because this would conflict with the emulator. Example. Set the emulator to 100%, then click tray, the tray will show you 90%.
derat@google.com changed reviewers: + derat@google.com
high-level comment (i'm out for the rest of the week): how does this differ from the existing stub implementation? why not just use the stub (maybe after splitting it into a separate file)?
stevenjb@chromium.org changed reviewers: + derat@chromium.org - derat@google.com
We already have a FakePowerManagerClient that we use for tests, we should expand that instead. We should also get rid of PowerManagerClientStubImpl and use FakePowerManagerClient instead in PowerManagerClient::Create. For networking (shill) we make of a command line argument (--shill-stub which should probably be --fake-shill) to set up various test networking environments. See FakeShillManagerClient::ParseCommandLineSwitch.
On 2015/06/24 16:17:11, stevenjb wrote: > We already have a FakePowerManagerClient that we use for tests, we should expand > that instead. > > We should also get rid of PowerManagerClientStubImpl and use > FakePowerManagerClient instead in PowerManagerClient::Create. Thanks, we agreed that this is the right direction, and mozart will work on the next patch. > > For networking (shill) we make of a command line argument (--shill-stub which > should probably be --fake-shill) to set up various test networking environments. > See FakeShillManagerClient::ParseCommandLineSwitch. Thank you for the info. I'm asking interns to write a separate mini design doc for each new device that we emulate, and I'll make sure that this will be covered there.
Moved the impl to the existing FakePowerManagerClient. Ready for review (more feature will be implemented once I get the ok)
On 2015/06/25 01:05:36, mozartalouis wrote: > Moved the impl to the existing FakePowerManagerClient. Ready for review (more > feature will be implemented once I get the ok) mozart, could you please incorporate this to the design doc? We should send the doc sooner than later.
On 2015/06/25 01:08:01, oshima wrote: > On 2015/06/25 01:05:36, mozartalouis wrote: > > Moved the impl to the existing FakePowerManagerClient. Ready for review (more > > feature will be implemented once I get the ok) > > mozart, could you please incorporate this to the design doc? We should send the > doc sooner than later. Here is the updated design doc. comments have been enabled so feel free so we can make the best emulator possible. This will be updated regularly https://docs.google.com/a/google.com/document/d/1X84nVB9r7Soe2VT2vB16wpE8TqFQ...
https://codereview.chromium.org/1206733002/diff/20001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/20001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:19: #include "chromeos/dbus/power_manager/power_supply_properties.pb.h" It doesn't look like we need most of these in the .h file. Forward declare anything you can and move other includes to the .cc file that needs them. https://codereview.chromium.org/1206733002/diff/20001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:109: int hours_to_empty_full_battery = 4 * 60 * 60; // 4 hours. default value. We should use inline initialization for all of these or none.
Ready for Review
https://codereview.chromium.org/1206733002/diff/20001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/20001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:8: #include <string> We should keep <string> since we use it (even though it's only a reference, we don't tend to forward declare std:: elements). https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:18: hours_to_empty_full_battery_(4 * 3600), nit: define this a s a locally namespaced constant: namespace { const int kDefaultBatteryEmptyTime = 4 * 3600; // 4 hours } Also, this be re-named seconds_to_empty_full_battery. https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:188: // Say that the system is charing on AC if charging is enabled. charging https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:100: int hours_to_empty_full_battery_; // 4 hours. default value. nit: Comment should be in the .cc file.
Ready for Review! https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:18: hours_to_empty_full_battery_(4 * 3600), On 2015/06/26 20:42:19, stevenjb wrote: > nit: define this a s a locally namespaced constant: > > namespace { > > const int kDefaultBatteryEmptyTime = 4 * 3600; // 4 hours > > } > > Also, this be re-named seconds_to_empty_full_battery. Done. https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:188: // Say that the system is charing on AC if charging is enabled. On 2015/06/26 20:42:19, stevenjb wrote: > charging Done. https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/40001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:100: int hours_to_empty_full_battery_; // 4 hours. default value. On 2015/06/26 20:42:19, stevenjb wrote: > nit: Comment should be in the .cc file. Done.
https://codereview.chromium.org/1206733002/diff/60001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/60001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:23: hours_to_empty_full_battery_(kDefaultBatteryEmptyTime), This variable name is still confusing. It should either be seconds_to_empty_full_battery_, or it should just be set to 4.
ready for Review https://codereview.chromium.org/1206733002/diff/60001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/60001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:23: hours_to_empty_full_battery_(kDefaultBatteryEmptyTime), On 2015/06/26 21:16:53, stevenjb wrote: > This variable name is still confusing. It should either be > seconds_to_empty_full_battery_, or it should just be set to 4. Sorry i didn't see this one. changing now
lgtm
The CQ bit was checked by mozartalouis@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
not lgtm yet also, this code doesn't compile: ../../chromeos/dbus/power_manager_client.cc:784:error: undefined reference to 'chromeos::FakePowerManagerClient::FakePowerManagerClient()' clang:error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed. https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:14: const int kDefaultBatteryEmptyTime = 4 * 3600; // 4 hours s/Time/Sec/ https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:22: power_source_(0), use an enum value here instead of an integer https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:71: void UpdateBatteryFullTimeToEmpty(int time_in_hours); this should take a base::TimeDelta instead "battery full time to empty" doesn't make sense to me either. what does the "full" mean? should it just be UpdateBatteryTimeToEmpty()? after looking at .cc file: oh, i see; this is either the time-to-full or the time-to-empty depending on UpdateIsBatteryCharging(). how about renaming to something like UpdateBatteryTimeToFullOrEmpty()? https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:72: void UpdateExternalPower(int external_power); this should be the proper enum type instead of an int; a caller wouldn't have any idea what the int is supposed to represent. (even after looking at the implementation, i'm not sure.) https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:78: void UpdatePowerStatus(); this should probably be private rather than public, no? https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:93: // Variables that handle states of the battery and power. high-level question: why have all of these different variables and a bunch of different methods to set them instead of just storing a PowerSupplyProperties protocol buffer and having a single method to set a new protobuf? https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:117: base::WeakPtrFactory<FakePowerManagerClient> weak_ptr_factory_; doesn't look like you actually use this...?
https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:14: const int kDefaultBatteryEmptyTime = 4 * 3600; // 4 hours On 2015/06/27 13:09:12, Daniel Erat wrote: > s/Time/Sec/ I'm not sure what this means https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:22: power_source_(0), On 2015/06/27 13:09:12, Daniel Erat wrote: > use an enum value here instead of an integer Done. https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:71: void UpdateBatteryFullTimeToEmpty(int time_in_hours); On 2015/06/27 13:09:12, Daniel Erat wrote: > this should take a base::TimeDelta instead > > "battery full time to empty" doesn't make sense to me either. what does the > "full" mean? should it just be UpdateBatteryTimeToEmpty()? > > after looking at .cc file: oh, i see; this is either the time-to-full or the > time-to-empty depending on UpdateIsBatteryCharging(). how about renaming to > something like UpdateBatteryTimeToFullOrEmpty()? Done. https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:78: void UpdatePowerStatus(); On 2015/06/27 13:09:12, Daniel Erat wrote: > this should probably be private rather than public, no? Done. https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:93: // Variables that handle states of the battery and power. On 2015/06/27 13:09:12, Daniel Erat wrote: > high-level question: why have all of these different variables and a bunch of > different methods to set them instead of just storing a PowerSupplyProperties > protocol buffer and having a single method to set a new protobuf? Done. https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.h:117: base::WeakPtrFactory<FakePowerManagerClient> weak_ptr_factory_; On 2015/06/27 13:09:12, Daniel Erat wrote: > doesn't look like you actually use this...? Done.
https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/80001/chromeos/dbus/fake_powe... chromeos/dbus/fake_power_manager_client.cc:14: const int kDefaultBatteryEmptyTime = 4 * 3600; // 4 hours On 2015/06/29 18:16:11, mozartalouis wrote: > On 2015/06/27 13:09:12, Daniel Erat wrote: > > s/Time/Sec/ > > I'm not sure what this means change "Time" to "Sec" (s/from/to/ is a sed/vi command to replace text)
Ready for Review
https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:16: const int kDefaultBatteryEmptySec = 6 * 3600; // 6 hours please give this a better name and add a comment describing what it represents. it looks like it's the amount of time that the battery takes to fully charge starting at 0% or fully discharge starting at 100%, right? maybe something like kBatteryLifetimeSec? https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:153: switch (power_source) { why do you need this switch statement? can't you just do: props_.set_external_power(power_source); ? https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:180: int64 remaining_battery_time = std::max( please either rename to "remaining_battery_sec" or use base::TimeDelta. it also seems unlikely that you need int64 here rather than int -- 2 ** 31 seconds is ~68 years, i think: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:181: 1, (int)props_.battery_percent() * kDefaultBatteryEmptySec / 100); use static_cast<> rather than a c-style cast: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:187: power_manager::PowerSupplyProperties_BatteryState_FULL); you should also set battery_time_to_empty_sec and battery_time_to_full_sec to 0 here as well, per the comments in power_supply_properties.proto https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:194: kDefaultBatteryEmptySec - remaining_battery_time)); set battery_time_to_empty_sec to 0 https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:198: props_.set_battery_time_to_empty_sec(remaining_battery_time); set battery_time_to_full_sec to 0 https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:209: void FakePowerManagerClient::UpdateObserver() { maybe rename this to NotifyObservers()? (there can be more than one observer, and they're being notified, not updated.) https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/power_ma... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/power_ma... chromeos/dbus/power_manager_client.cc:1005: return new FakePowerManagerClient; have you tried to compile this for chrome os (rather than linux desktop)? dbus/fake_power_manager_client.cc gets built by the chromeos_test_support_without_gmock target, so i don't see how this could build successfully for chrome os... oshima, can you help get the dependencies right here?
Ready for review https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:16: const int kDefaultBatteryEmptySec = 6 * 3600; // 6 hours On 2015/06/29 19:34:18, Daniel Erat wrote: > please give this a better name and add a comment describing what it represents. > it looks like it's the amount of time that the battery takes to fully charge > starting at 0% or fully discharge starting at 100%, right? maybe something like > kBatteryLifetimeSec? Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:153: switch (power_source) { On 2015/06/29 19:34:18, Daniel Erat wrote: > why do you need this switch statement? can't you just do: > > props_.set_external_power(power_source); > > ? It was there before just in case we wanted to do fool proofing. example, if the power is disconnected, then we should change the charging state to discharging. I wasn't sure if it was necessary so i just left it like this for now. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:180: int64 remaining_battery_time = std::max( On 2015/06/29 19:34:18, Daniel Erat wrote: > please either rename to "remaining_battery_sec" or use base::TimeDelta. > > it also seems unlikely that you need int64 here rather than int -- 2 ** 31 > seconds is ~68 years, i think: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:181: 1, (int)props_.battery_percent() * kDefaultBatteryEmptySec / 100); On 2015/06/29 19:34:18, Daniel Erat wrote: > use static_cast<> rather than a c-style cast: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Casting Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:187: power_manager::PowerSupplyProperties_BatteryState_FULL); On 2015/06/29 19:34:18, Daniel Erat wrote: > you should also set battery_time_to_empty_sec and battery_time_to_full_sec to 0 > here as well, per the comments in power_supply_properties.proto Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:194: kDefaultBatteryEmptySec - remaining_battery_time)); On 2015/06/29 19:34:18, Daniel Erat wrote: > set battery_time_to_empty_sec to 0 Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:198: props_.set_battery_time_to_empty_sec(remaining_battery_time); On 2015/06/29 19:34:18, Daniel Erat wrote: > set battery_time_to_full_sec to 0 Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:209: void FakePowerManagerClient::UpdateObserver() { On 2015/06/29 19:34:18, Daniel Erat wrote: > maybe rename this to NotifyObservers()? (there can be more than one observer, > and they're being notified, not updated.) Done. https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/power_ma... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/120001/chromeos/dbus/power_ma... chromeos/dbus/power_manager_client.cc:1005: return new FakePowerManagerClient; On 2015/06/29 19:34:18, Daniel Erat wrote: > have you tried to compile this for chrome os (rather than linux desktop)? > dbus/fake_power_manager_client.cc gets built by the > chromeos_test_support_without_gmock target, so i don't see how this could build > successfully for chrome os... > > oshima, can you help get the dependencies right here? Done. This should work for chrome OS. compiled to a falco board and it works fine. just forgot the () at the end.
https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:153: switch (power_source) { please change this to the simpler form until making it more complicated is actually necessary. https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:180: int64 remaining_battery_sec = static_cast<int>( nit: change to int unless there's reason to believe you actually need 2**63 https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:84: void NotifyObserver(); NotifyObservers rather than NotifyObserver, please
The CQ bit was checked by derat@chromium.org to run a CQ dry run
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/1206733002/#ps140001 (title: "variable changes, removed stub impl, naming")
(just triggered commit queue dry run, not a real commit)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/140001
New patch for review https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:153: switch (power_source) { On 2015/06/29 20:10:10, Daniel Erat wrote: > please change this to the simpler form until making it more complicated is > actually necessary. Done. https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:180: int64 remaining_battery_sec = static_cast<int>( On 2015/06/29 20:10:10, Daniel Erat wrote: > nit: change to int unless there's reason to believe you actually need 2**63 Done. https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/140001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:84: void NotifyObserver(); On 2015/06/29 20:10:10, Daniel Erat wrote: > NotifyObservers rather than NotifyObserver, please Done.
this still doesn't compile for the reason that i mentioned earlier. see the build failures on the chrome os bots in the dry run that i kicked off: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
This implementation works with changing the battery percentage, not anything else yet, but that is more on the device handler side. you can patch this in and use chrome://device-emulator to test it out. https://codereview.chromium.org/1206733002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/chromeos/emulator/device_emulator.js (right): https://codereview.chromium.org/1206733002/diff/180001/chrome/browser/resourc... chrome/browser/resources/chromeos/emulator/device_emulator.js:27: chrome.send('updateBatteryInfo', [slider.valueAsNumber]); typo preventing slider from working properly https://codereview.chromium.org/1206733002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/chromeos/emulator/device_emulator_message_handler.cc:62: power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED); Not sure if this is the best way to do this, please comment. https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:7: #include <algorithm> will remove. not used. https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:73: power_manager::PowerSupplyProperties::ExternalPower external_power); I believe this make it much simpler to use and will relay more on the device handler class then having separate methods https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... chromeos/dbus/power_manager_client.cc:784: return new FakePowerManagerClient(); @derat @oshima. Not exactly sure why this fails when you do a dry run. this compiles fine on Linux and i even deployed a version to a falco board and there was not problems. What are my next steps?
(stevenjb@, see inline question) please add a unittest file for the fake class to ensure that it works as expected. please also put the ui changes into a separate changelist instead of including them here. https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } it seems like the handling of _FULL is broken here. did you mean to include it in the top condition? https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:174: // checks if the battery is in a discharging state first before setting a nit: capitalize 'Checks' https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:178: power_manager::PowerSupplyProperties_BatteryState_DISCHARGING) multiline "then" sections of if/else statements should have curly brackets: if (battery_state == ...) { ... } else { ... } https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:73: power_manager::PowerSupplyProperties::ExternalPower external_power); On 2015/06/30 02:16:09, mozartalouis wrote: > I believe this make it much simpler to use and will relay more on the device > handler class then having separate methods cool, i agree. https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... chromeos/dbus/power_manager_client.cc:784: return new FakePowerManagerClient(); On 2015/06/30 02:16:09, mozartalouis wrote: > @derat @oshima. Not exactly sure why this fails when you do a dry run. this > compiles fine on Linux and i even deployed a version to a falco board and there > was not problems. What are my next steps? here's the problem: - you're instantiating a FakePowerManagerClient from this file. - fake_power_manager_client.cc is only built for test images (see chromeos/chromeos.gyp). the right fix is probably to build fake_power_manager_client.cc unconditionally. this seems similar to what was done with the stub object before, and what is done with stub/fake objects for other client classes (e.g. update_engine_client.cc). to do this, edit chromeos/chromeos.gyp and move dbus/fake_power_manager_client.* from the chromeos_test_support_without_gmock target into the chromeos_sources list. you should also get rid of the power_manager_proto dependency in chromeos_test_support_without_gmock at that point. stevenjb@, do you agree? there's actually e.g. a FakeUpdateEngineClient class that's only compiled for tests, along with both UpdateEngineClientFakeImpl and UpdateEngineClientStubImpl that are defined in update_engine_client.cc, so i don't know what the overarching plan is for any of this stuff.
https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... File chromeos/dbus/power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/power_ma... chromeos/dbus/power_manager_client.cc:784: return new FakePowerManagerClient(); On 2015/06/30 14:03:40, Daniel Erat wrote: > On 2015/06/30 02:16:09, mozartalouis wrote: > > @derat @oshima. Not exactly sure why this fails when you do a dry run. this > > compiles fine on Linux and i even deployed a version to a falco board and > there > > was not problems. What are my next steps? > > here's the problem: > > - you're instantiating a FakePowerManagerClient from this file. > - fake_power_manager_client.cc is only built for test images (see > chromeos/chromeos.gyp). > > the right fix is probably to build fake_power_manager_client.cc unconditionally. > this seems similar to what was done with the stub object before, and what is > done with stub/fake objects for other client classes (e.g. > update_engine_client.cc). > > to do this, edit chromeos/chromeos.gyp and move dbus/fake_power_manager_client.* > from the chromeos_test_support_without_gmock target into the chromeos_sources > list. you should also get rid of the power_manager_proto dependency in > chromeos_test_support_without_gmock at that point. > > stevenjb@, do you agree? there's actually e.g. a FakeUpdateEngineClient class > that's only compiled for tests, along with both UpdateEngineClientFakeImpl and > UpdateEngineClientStubImpl that are defined in update_engine_client.cc, so i > don't know what the overarching plan is for any of this stuff. Yes, we should be replacing "stub" implementations with "fake" ones and include them unconditionally. At some point we should probably do the same for update_engine_client. I can't think of any advantage to supporting both a "stub" and a "fake". I am surprised this compiled and ran on linux with chromeos=1.
Ready for Review https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/06/30 14:03:40, Daniel Erat wrote: > it seems like the handling of _FULL is broken here. did you mean to include it > in the top condition? No. The reason _FULL is not handled is because it will be set once the battery is at 100% and the battery state is charging. For instance, if the battery is no longer charging, then it should display the time remaining. Let me know if you have any objections :) https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:174: // checks if the battery is in a discharging state first before setting a On 2015/06/30 14:03:40, Daniel Erat wrote: > nit: capitalize 'Checks' Done. https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:178: power_manager::PowerSupplyProperties_BatteryState_DISCHARGING) On 2015/06/30 14:03:40, Daniel Erat wrote: > multiline "then" sections of if/else statements should have curly brackets: > > if (battery_state == > ...) { > ... > } else { > ... > } Done.
https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/06/30 18:29:47, mozartalouis wrote: > On 2015/06/30 14:03:40, Daniel Erat wrote: > > it seems like the handling of _FULL is broken here. did you mean to include it > > in the top condition? > > No. The reason _FULL is not handled is because it will be set once the battery > is at 100% and the battery state is charging. For instance, if the battery is no > longer charging, then it should display the time remaining. Let me know if you > have any objections :) i don't think i follow. if i call: UpdatePowerProperties(100, false, _FULL, _AC); then it looks like this method isn't going to update the battery state in props_ at all. that's contrary to what i would expect. (and as mentioned in the last round, please add a fake_power_manager_client_unittest.cc file and some tests in this change.)
Created Unit test. Ready for Review https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/180001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/06/30 20:35:18, Daniel Erat wrote: > On 2015/06/30 18:29:47, mozartalouis wrote: > > On 2015/06/30 14:03:40, Daniel Erat wrote: > > > it seems like the handling of _FULL is broken here. did you mean to include > it > > > in the top condition? > > > > No. The reason _FULL is not handled is because it will be set once the battery > > is at 100% and the battery state is charging. For instance, if the battery is > no > > longer charging, then it should display the time remaining. Let me know if you > > have any objections :) > > i don't think i follow. if i call: > > UpdatePowerProperties(100, false, _FULL, _AC); > > then it looks like this method isn't going to update the battery state in props_ > at all. that's contrary to what i would expect. > > (and as mentioned in the last round, please add a > fake_power_manager_client_unittest.cc file and some tests in this change.) OH! I understand. Added in next patch.
https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn File chromeos/BUILD.gn (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:66: "dbus/fake_power_manager_client.h", why are you adding these here? the comment above the source list in the .gyp file says that it's automatically pulled in by the .gn file. https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:111: ":power_manager_proto", please remove this now https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:171: 'dbus/fake_power_manager_client.cc', keep this list sorted https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:644: 'power_manager_proto', remove this dependency https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:154: power_manager::PowerSupplyProperties_BatteryState_FULL) { use parentheses around nested || and && clauses: if ((battery_percent == 100 && battery_state == _CHARGING) || https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:154: power_manager::PowerSupplyProperties_BatteryState_FULL) { are your tests working? the _FULL part is broken because you're not doing a comparison. also, please use parentheses around nested || and && clauses: if ((battery_percent == 100 && battery_state == _CHARGING) || battery_state == _FULL) { https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:75: power_manager::PowerSupplyProperties GetPowerSupplyProperties() { this is inlined, so you should just name it props(). also, return a const reference instead of a copy: const power_manager::PowerSupplyProperties& props() const { return props_; } https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:12: FakePowerManagerClient* client = new FakePowerManagerClient(); you're leaking memory here. why don't you just allocate this on the stack? https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:14: ASSERT_TRUE(client != NULL); you don't need to test the output of the 'new' operator. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:24: EXPECT_EQ(power_manager::PowerSupplyProperties_BatteryState_CHARGING, please test more cases, e.g. _FULL. make sure you add a test that would've caught the bug that i just pointed out.
abodenha@chromium.org changed reviewers: + abodenha@chromium.org
This CL should really have had more eyes on it before it went out for full review. Can you have oshima@ or afakhry@ work with you face to face to get this into a better state before continuing the full review with derat@?
On 2015/06/30 22:36:11, Albert Bodenhamer wrote: > This CL should really have had more eyes on it before it went out for full > review. Can you have oshima@ or afakhry@ work with you face to face to get this > into a better state before continuing the full review with derat@? abodenha@ Most of the implementation is finished, it's just that I miss certain things. I will work with afakhry@ to fix the testing before continuing with deret@
On 2015/06/30 22:36:11, Albert Bodenhamer wrote: > This CL should really have had more eyes on it before it went out for full > review. Can you have oshima@ or afakhry@ work with you face to face to get this > into a better state before continuing the full review with derat@? abodenha@ Most of the implementation is finished, it's just that I miss certain things. I will work with afakhry@ to fix the testing before continuing with deret@
Exactly. My goal is to pair you closely with experienced mentors who can work with you to find those things. Nobody is expecting perfection in the first draft, but we need to polish things a bit more before they go out for general review. Thanks On Tue, Jun 30, 2015 at 3:44 PM <mozartalouis@google.com> wrote: > On 2015/06/30 22:36:11, Albert Bodenhamer wrote: > > This CL should really have had more eyes on it before it went out for > full > > review. Can you have oshima@ or afakhry@ work with you face to face to > get > this > > into a better state before continuing the full review with derat@? > > abodenha@ Most of the implementation is finished, it's just that I miss > certain > things. I will work with afakhry@ to fix the testing before continuing > with > deret@ > > https://codereview.chromium.org/1206733002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ready For Review https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn File chromeos/BUILD.gn (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:66: "dbus/fake_power_manager_client.h", On 2015/06/30 22:13:31, Daniel Erat wrote: > why are you adding these here? the comment above the source list in the .gyp > file says that it's automatically pulled in by the .gn file. Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/BUILD.gn#newc... chromeos/BUILD.gn:111: ":power_manager_proto", On 2015/06/30 22:13:31, Daniel Erat wrote: > please remove this now Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp File chromeos/chromeos.gyp (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:171: 'dbus/fake_power_manager_client.cc', On 2015/06/30 22:13:31, Daniel Erat wrote: > keep this list sorted Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/chromeos.gyp#... chromeos/chromeos.gyp:644: 'power_manager_proto', On 2015/06/30 22:13:31, Daniel Erat wrote: > remove this dependency Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:154: power_manager::PowerSupplyProperties_BatteryState_FULL) { On 2015/06/30 22:13:31, Daniel Erat wrote: > use parentheses around nested || and && clauses: > > if ((battery_percent == 100 && > battery_state == _CHARGING) || > Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:154: power_manager::PowerSupplyProperties_BatteryState_FULL) { On 2015/06/30 22:13:31, Daniel Erat wrote: > are your tests working? the _FULL part is broken because you're not doing a > comparison. > > also, please use parentheses around nested || and && clauses: > > if ((battery_percent == 100 && > battery_state == _CHARGING) || > battery_state == _FULL) { Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:75: power_manager::PowerSupplyProperties GetPowerSupplyProperties() { On 2015/06/30 22:13:31, Daniel Erat wrote: > this is inlined, so you should just name it props(). also, return a const > reference instead of a copy: > > const power_manager::PowerSupplyProperties& props() const { > return props_; > } Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:12: FakePowerManagerClient* client = new FakePowerManagerClient(); On 2015/06/30 22:13:31, Daniel Erat wrote: > you're leaking memory here. why don't you just allocate this on the stack? Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:14: ASSERT_TRUE(client != NULL); On 2015/06/30 22:13:31, Daniel Erat wrote: > you don't need to test the output of the 'new' operator. Done. https://codereview.chromium.org/1206733002/diff/220001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:24: EXPECT_EQ(power_manager::PowerSupplyProperties_BatteryState_CHARGING, On 2015/06/30 22:13:31, Daniel Erat wrote: > please test more cases, e.g. _FULL. make sure you add a test that would've > caught the bug that i just pointed out. Done.
afakhry@chromium.org changed reviewers: + afakhry@chromium.org
Just some comments on: fake_power_manager_client_unittest.cc https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: // FakePowerManagerClient::Init() function is created. Nits: - "assign" --> "assigned" - "created" --> "called" - You don't need to say "function" here, the parens "()" in Init() is clear enough https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:16: client().Init(NULL); NULL --> nullptr https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:18: EXPECT_EQ(50, client().props().battery_percent()); Nit: You don't have to use client() here, you're inside the class. You can use the "client_" member directly. https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:37: client().UpdatePowerProperties( I would make things more readable here by wrapping this long and tedious call to UpdatePowerProperties() in a member function of FakePowerManagerClientTest. That way you can also make client() be like: const FakePowerManagerClient& client() const; Since in derived test classes you'll only need to use the const getters.
https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: // FakePowerManagerClient::Init() function is created. On 2015/07/01 16:47:48, afakhry wrote: > Nits: > - "assign" --> "assigned" > - "created" --> "called" > - You don't need to say "function" here, the parens "()" in Init() is clear > enough Done. https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:16: client().Init(NULL); On 2015/07/01 16:47:48, afakhry wrote: > NULL --> nullptr Done. https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:18: EXPECT_EQ(50, client().props().battery_percent()); On 2015/07/01 16:47:48, afakhry wrote: > Nit: You don't have to use client() here, you're inside the class. You can use > the "client_" member directly. Done. https://codereview.chromium.org/1206733002/diff/240001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:37: client().UpdatePowerProperties( On 2015/07/01 16:47:48, afakhry wrote: > I would make things more readable here by wrapping this long and tedious call to > UpdatePowerProperties() in a member function of FakePowerManagerClientTest. That > way you can also make client() be like: > const FakePowerManagerClient& client() const; > Since in derived test classes you'll only need to use the const getters. Done.
Ready for Review
The CQ bit was checked by mozartalouis@google.com to run a CQ dry run
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/1206733002/#ps260001 (title: "Updated Unittest")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: A disapproval has been posted by following reviewers: derat@chromium.org. Please make sure to get positive review before another CQ attempt.
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:69: // Updates the PowerSupplyProperties and NotifyObservers of its changes. "NotifyObservers" -> "notifies observers" https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:76: const power_manager::PowerSupplyProperties& props() { return props_; } please move props() up to where all the other accessors are defined (policy(), is_projecting(), etc.) https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:83: // Tells TrayPower to update its contents to the Updated "Updated" -> "updated" this comment also shouldn't refer to TrayPower, since this class doesn't know anything about it. please just use something like: // Notifies |observers_| that |props_| has been updated. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:92: // The power properties set for for the UI. this class doesn't know anything about the UI. how about: // Power status received from the power manager. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: enum BatteryState { FULL, CHARGING, DISCHARGING }; this will break; someone will update the proto's enums without updating these. why not just use the enums from the .proto file? https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: EXPECT_EQ(50, client_.props().battery_percent()); putting expectations in the base class doesn't make sense; it means that these will run over and over for every test case. it would be better to put these in a separate test case. but, i don't think that these expectations accomplish anything -- they're just testing that the same constants are hardcoded in the implementation and in the tests. please just remove these expectations. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:40: FakePowerManagerClient& client() { return client_; } we don't use non-const references (but see below; this should just be deleted) https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:43: FakePowerManagerClient client_; put this in a "protected:" section; then derived classes can use it without needing to go through an accessor
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:61: } Are these going to be implemented too (in separate CLs)? https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } looks like it's possible to call with impossible arguments and the method silently ignore such case. Can you add CHECK instead to catch such cases? Even better, isn't it possible to derive BatteryState from battery_percent and external power state?
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/07/06 18:47:57, oshima wrote: > Even better, isn't it possible to derive BatteryState from battery_percent and > external power state? no, it isn't: - a defective battery can fail to charge even while line power is connected - some systems can draw more power than a charger can provide (e.g. spring) etc. the ui handles these cases specially, so it should be possible to trigger them using this class. i think it might be better to make this class just directly copy the passed-in values to the proto, though, since that allows developers to test any possible combination of fields that powerd can send to chrome.
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/07/06 19:02:54, Daniel Erat wrote: > On 2015/07/06 18:47:57, oshima wrote: > > Even better, isn't it possible to derive BatteryState from battery_percent and > > external power state? > > no, it isn't: > > - a defective battery can fail to charge even while line power is connected > - some systems can draw more power than a charger can provide (e.g. spring) > etc. > > the ui handles these cases specially, so it should be possible to trigger them > using this class. > > i think it might be better to make this class just directly copy the passed-in > values to the proto, though, since that allows developers to test any possible > combination of fields that powerd can send to chrome. That's fine with me. It'd be nice to have utility factory methods/functions to create proto for such scenarios (faulty battery, lower power charger, etc) to avoid creating impossible state. (it'll be never charging without power source for example).
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:61: } On 2015/07/06 18:47:57, oshima wrote: > Are these going to be implemented too (in separate CLs)? These will be implemented in a separate CL, Although I'm not sure if it will be needed. Do we want brightness features? the only reason they are here is because power_manager_client.h defines them as pure virtual. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/07/06 19:54:59, oshima wrote: > On 2015/07/06 19:02:54, Daniel Erat wrote: > > On 2015/07/06 18:47:57, oshima wrote: > > > Even better, isn't it possible to derive BatteryState from battery_percent > and > > > external power state? > > > > no, it isn't: > > > > - a defective battery can fail to charge even while line power is connected > > - some systems can draw more power than a charger can provide (e.g. spring) > > etc. > > > > the ui handles these cases specially, so it should be possible to trigger them > > using this class. > > > > i think it might be better to make this class just directly copy the passed-in > > values to the proto, though, since that allows developers to test any possible > > combination of fields that powerd can send to chrome. > > That's fine with me. It'd be nice to have utility factory methods/functions to > create proto for such scenarios (faulty battery, lower power charger, etc) to > avoid creating impossible state. (it'll be never charging without power source > for example). If we want those features, we can implement them in another CL possibly? (Advanced Battery Features?) https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:69: // Updates the PowerSupplyProperties and NotifyObservers of its changes. On 2015/07/06 14:28:14, Daniel Erat wrote: > "NotifyObservers" -> "notifies observers" Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:76: const power_manager::PowerSupplyProperties& props() { return props_; } On 2015/07/06 14:28:14, Daniel Erat wrote: > please move props() up to where all the other accessors are defined (policy(), > is_projecting(), etc.) Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:83: // Tells TrayPower to update its contents to the Updated On 2015/07/06 14:28:14, Daniel Erat wrote: > "Updated" -> "updated" > > this comment also shouldn't refer to TrayPower, since this class doesn't know > anything about it. please just use something like: > > // Notifies |observers_| that |props_| has been updated. Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:92: // The power properties set for for the UI. On 2015/07/06 14:28:14, Daniel Erat wrote: > this class doesn't know anything about the UI. how about: > > // Power status received from the power manager. Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: enum BatteryState { FULL, CHARGING, DISCHARGING }; On 2015/07/06 14:28:15, Daniel Erat wrote: > this will break; someone will update the proto's enums without updating these. > why not just use the enums from the .proto file? Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: EXPECT_EQ(50, client_.props().battery_percent()); On 2015/07/06 14:28:14, Daniel Erat wrote: > putting expectations in the base class doesn't make sense; it means that these > will run over and over for every test case. it would be better to put these in a > separate test case. > > but, i don't think that these expectations accomplish anything -- they're just > testing that the same constants are hardcoded in the implementation and in the > tests. please just remove these expectations. Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:40: FakePowerManagerClient& client() { return client_; } On 2015/07/06 14:28:15, Daniel Erat wrote: > we don't use non-const references (but see below; this should just be deleted) Done. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:43: FakePowerManagerClient client_; On 2015/07/06 14:28:15, Daniel Erat wrote: > put this in a "protected:" section; then derived classes can use it without > needing to go through an accessor Done.
https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/07/06 20:49:42, mozartalouis wrote: > On 2015/07/06 19:54:59, oshima wrote: > > On 2015/07/06 19:02:54, Daniel Erat wrote: > > > On 2015/07/06 18:47:57, oshima wrote: > > > > Even better, isn't it possible to derive BatteryState from battery_percent > > and > > > > external power state? > > > > > > no, it isn't: > > > > > > - a defective battery can fail to charge even while line power is connected > > > - some systems can draw more power than a charger can provide (e.g. spring) > > > etc. > > > > > > the ui handles these cases specially, so it should be possible to trigger > them > > > using this class. > > > > > > i think it might be better to make this class just directly copy the > passed-in > > > values to the proto, though, since that allows developers to test any > possible > > > combination of fields that powerd can send to chrome. > > > > That's fine with me. It'd be nice to have utility factory methods/functions to > > create proto for such scenarios (faulty battery, lower power charger, etc) to > > avoid creating impossible state. (it'll be never charging without power source > > for example). > > If we want those features, we can implement them in another CL possibly? > (Advanced Battery Features?) You can add these functions/methods when you integrate with UI
Patchset #15 (id:280001) has been deleted
Ready for review. https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/260001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:172: } On 2015/07/06 21:06:05, oshima wrote: > On 2015/07/06 20:49:42, mozartalouis wrote: > > On 2015/07/06 19:54:59, oshima wrote: > > > On 2015/07/06 19:02:54, Daniel Erat wrote: > > > > On 2015/07/06 18:47:57, oshima wrote: > > > > > Even better, isn't it possible to derive BatteryState from > battery_percent > > > and > > > > > external power state? > > > > > > > > no, it isn't: > > > > > > > > - a defective battery can fail to charge even while line power is > connected > > > > - some systems can draw more power than a charger can provide (e.g. > spring) > > > > etc. > > > > > > > > the ui handles these cases specially, so it should be possible to trigger > > them > > > > using this class. > > > > > > > > i think it might be better to make this class just directly copy the > > passed-in > > > > values to the proto, though, since that allows developers to test any > > possible > > > > combination of fields that powerd can send to chrome. > > > > > > That's fine with me. It'd be nice to have utility factory methods/functions > to > > > create proto for such scenarios (faulty battery, lower power charger, etc) > to > > > avoid creating impossible state. (it'll be never charging without power > source > > > for example). > > > > If we want those features, we can implement them in another CL possibly? > > (Advanced Battery Features?) > > You can add these functions/methods when you integrate with UI Acknowledged.
Ready for Review
https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:151: if (battery_percent == 100 && i'm still not convinced that it makes sense to do a bunch of validation here. this is just going to be used by developers to manually exercise the ui code, so why not let them check that it handles implausible combinations if they want to? that would make this method much simpler: props_.set_battery_percent(battery_percent); props_.set_is_calculating_battery_time(is_calculating_battery_time); props_.set_battery_state(battery_state); props_.set_external_power(external_power); // keep existing time_to_full/empty_sec code... NotifyObservers(); https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:33: EXPECT_EQ(50, client_.props().battery_percent()); as mentioned in the last round of comments, i don't think that these tests make sense. they're just verifying that the constants that are hardcoded here match the constants that are hardcoded in the implementation. that doesn't seem like it buys us anything.
Rewrote the unittests, Ready for review. https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:151: if (battery_percent == 100 && On 2015/07/07 21:30:56, Daniel Erat wrote: > i'm still not convinced that it makes sense to do a bunch of validation here. > this is just going to be used by developers to manually exercise the ui code, so > why not let them check that it handles implausible combinations if they want to? > that would make this method much simpler: > > props_.set_battery_percent(battery_percent); > props_.set_is_calculating_battery_time(is_calculating_battery_time); > props_.set_battery_state(battery_state); > props_.set_external_power(external_power); > // keep existing time_to_full/empty_sec code... > NotifyObservers(); Done. https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/300001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:33: EXPECT_EQ(50, client_.props().battery_percent()); On 2015/07/07 21:30:56, Daniel Erat wrote: > as mentioned in the last round of comments, i don't think that these tests make > sense. they're just verifying that the constants that are hardcoded here match > the constants that are hardcoded in the implementation. that doesn't seem like > it buys us anything. Done.
Patchset #16 (id:320001) has been deleted
https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:35: power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED); do you still need this? https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:98: int num_request_notify_observers_; Please move this to the test observer in unit tests. That way, the test can verify that the observers are really called. https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:13: public FakePowerManagerClient::Observer { Please define a separate Observer class. https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: FakePowerManagerClient client_; you can just create this on stack in the test body and you can eliminate this test class. https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:54: EXPECT_EQ(this->props_.battery_percent(), client_.props().battery_percent()); please test against the actual value set instead.
https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:13: public FakePowerManagerClient::Observer { Please define a separate Observer class. https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: FakePowerManagerClient client_; you can just create this on stack in the test body and you can eliminate this test class. https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:54: EXPECT_EQ(this->props_.battery_percent(), client_.props().battery_percent()); please test against the actual value set instead.
On 2015/07/09 00:15:41, oshima wrote: > https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... > File chromeos/dbus/fake_power_manager_client_unittest.cc (right): > > https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... > chromeos/dbus/fake_power_manager_client_unittest.cc:13: public > FakePowerManagerClient::Observer { > Please define a separate Observer class. > > https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... > chromeos/dbus/fake_power_manager_client_unittest.cc:23: FakePowerManagerClient > client_; > you can just create this on stack in the test body and you can eliminate this > test class. > > https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... > chromeos/dbus/fake_power_manager_client_unittest.cc:54: > EXPECT_EQ(this->props_.battery_percent(), client_.props().battery_percent()); > please test against the actual value set instead. Not sure why this test is failing.
Ready for review https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:35: power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED); On 2015/07/09 00:15:39, oshima wrote: > do you still need this? We need it for when chrome starts on the Linux environment. it sets the default values
https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:144: bool is_calculating_battery_time, i'm wondering if this method should also take the time to full/empty instead of calculating it itself. if a developer wants to try to test time-based low-battery notifications, that seems like it'd be useful to have -- otherwise, they'd need to fiddle around with battery_percent until they get the desired time. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:156: if (battery_percent == 100 && to match powerd, this code should just be: switch (battery_state) { case _FULL: // set both to 0 break; case _CHARGING: // update _to_full and set _to_empty to 0 break; case _DISCHARGING: // update _to_empty and set _to_full to 0 break; } using a switch statement will catch the case where a new enum value is added later without this code being updated. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (left): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:11: #include "base/macros.h" why did this get deleted? DISALLOW_COPY_AND_ASSIGN is still being used. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:21: class CHROMEOS_EXPORT FakePowerManagerClient : public PowerManagerClient { you need to #include <chromeos/chromeos_export.h> https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:26: const base::ObserverList<Observer>& observers() { return observers_; } this is an internal implementation detail and probably shouldn't be exposed https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:73: int battery_percent, the proto uses a double for this; this method should too. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:13: TestObserver() : num_request_notify_observers_(0) {} add an empty virtual d'tor: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance "Make your destructor virtual if necessary. If your class has virtual methods, its destructor should be virtual." (i think that clang tests for this when building) https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: power_manager::PowerSupplyProperties& props() { return props_; } both the return value and this method itself should be const: const power_manager::PowerSupplyProperties& props() const { return props_; } https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:16: const int num_request_notify_observers() { this method should be const. the copy-by-value return value shouldn't be: int num_request_notify_observers() const { https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:27: int num_request_notify_observers_; this should probably be num_power_changed_ instead https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:53: TestObserver test_obeserver; this variable is misspelled throughout the function https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:54: FakePowerManagerClient client_; no trailing underscore here; those are just to denote class members https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:58: EXPECT_TRUE(client_.observers().HasObserver(&test_obeserver)); just call client.HasObserver() https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:60: // Test if NotifyObservers sends the corrent values to ||. sp: correct did you mean |observer| instead of ||? https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:77: EXPECT_FALSE(client_.observers().HasObserver(&test_obeserver)); client.HasObserver()
https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/340001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:35: power_manager::PowerSupplyProperties_ExternalPower_DISCONNECTED); On 2015/07/09 01:56:15, mozartalouis wrote: > On 2015/07/09 00:15:39, oshima wrote: > > do you still need this? > > We need it for when chrome starts on the Linux environment. it sets the default > values If this is necessary, please call this in your test as well. That way, the test can verify in a real scenario. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:144: bool is_calculating_battery_time, On 2015/07/09 15:41:47, Daniel Erat wrote: > i'm wondering if this method should also take the time to full/empty instead of > calculating it itself. if a developer wants to try to test time-based > low-battery notifications, that seems like it'd be useful to have -- otherwise, > they'd need to fiddle around with battery_percent until they get the desired > time. If that's the case, I'd suggest to just pass proto data instead, and define utility functions that copys & updates the proto. That way, a developer can have full control, while obvious cases will be simple. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:28: const power_manager::PowerSupplyProperties& props() { return props_; } const method https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:51: can you also test if RequestStatusUpdate() works correctly?
https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:144: bool is_calculating_battery_time, On 2015/07/09 17:10:15, oshima wrote: > On 2015/07/09 15:41:47, Daniel Erat wrote: > > i'm wondering if this method should also take the time to full/empty instead > of > > calculating it itself. if a developer wants to try to test time-based > > low-battery notifications, that seems like it'd be useful to have -- > otherwise, > > they'd need to fiddle around with battery_percent until they get the desired > > time. > > If that's the case, I'd suggest to just pass proto data instead, and > define utility functions that copys & updates the proto. > > That way, a developer can have full control, while obvious cases will be simple. just passing the proto into this class seems fine to me. and i noticed that the design doc also shows time-to-empty/full as being directly controllable.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
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/1206733002/#ps440001 (title: "Updated unittest")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/440001
looks good. just a few more nits. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:41: NotifyObservers(); I'm not sure if these are necessary/useful. I'll leave it to derat@. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:151: props_.Clear(); you don't need to call Clear(). The following line will overwrite everything. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:74: void UpdatePowerProperties(const power_manager::PowerSupplyProperties& proto); nit: proto refers to Protocol Buffer, which is a way to to define data structure. props (properties, power_props etc) would be better name. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:17: int num_power_changed() const { return num_power_changed_; } new line https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:33: void SetTestProperties(power_manager::PowerSupplyProperties& props) { out parameter should be pointer per style guide. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:76: power_manager::PowerSupplyProperties props; move this to line 82 just before it's first used.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
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/1206733002/#ps460001 (title: "Updated Deps for other tests")
The CQ bit was unchecked by afakhry@chromium.org
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
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/1206733002/#ps480001 (title: "Updated Unittest crashes")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
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/1206733002/#ps500001 (title: "Updated Build files")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
Patchset #24 (id:500001) has been deleted
Patchset #24 (id:520001) has been deleted
Patchset #23 (id:480001) has been deleted
Patchset #22 (id:460001) has been deleted
Patchset #22 (id:540001) has been deleted
Ready for full review https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:156: if (battery_percent == 100 && On 2015/07/09 15:41:47, Daniel Erat wrote: > to match powerd, this code should just be: > > switch (battery_state) { > case _FULL: > // set both to 0 > break; > case _CHARGING: > // update _to_full and set _to_empty to 0 > break; > case _DISCHARGING: > // update _to_empty and set _to_full to 0 > break; > } > > using a switch statement will catch the case where a new enum value is added > later without this code being updated. Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:21: class CHROMEOS_EXPORT FakePowerManagerClient : public PowerManagerClient { On 2015/07/09 15:41:47, Daniel Erat wrote: > you need to #include <chromeos/chromeos_export.h> Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:26: const base::ObserverList<Observer>& observers() { return observers_; } On 2015/07/09 15:41:47, Daniel Erat wrote: > this is an internal implementation detail and probably shouldn't be exposed Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:28: const power_manager::PowerSupplyProperties& props() { return props_; } On 2015/07/09 17:10:15, oshima wrote: > const method Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:73: int battery_percent, On 2015/07/09 15:41:47, Daniel Erat wrote: > the proto uses a double for this; this method should too. Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:13: TestObserver() : num_request_notify_observers_(0) {} On 2015/07/09 15:41:48, Daniel Erat wrote: > add an empty virtual d'tor: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inheritance > > "Make your destructor virtual if necessary. If your class has virtual methods, > its destructor should be virtual." > > (i think that clang tests for this when building) Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:15: power_manager::PowerSupplyProperties& props() { return props_; } On 2015/07/09 15:41:48, Daniel Erat wrote: > both the return value and this method itself should be const: > > const power_manager::PowerSupplyProperties& props() const { return props_; } Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:16: const int num_request_notify_observers() { On 2015/07/09 15:41:48, Daniel Erat wrote: > this method should be const. the copy-by-value return value shouldn't be: > > int num_request_notify_observers() const { Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:27: int num_request_notify_observers_; On 2015/07/09 15:41:47, Daniel Erat wrote: > this should probably be num_power_changed_ instead Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:51: On 2015/07/09 17:10:16, oshima wrote: > can you also test if RequestStatusUpdate() works correctly? Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:53: TestObserver test_obeserver; On 2015/07/09 15:41:47, Daniel Erat wrote: > this variable is misspelled throughout the function Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:54: FakePowerManagerClient client_; On 2015/07/09 15:41:48, Daniel Erat wrote: > no trailing underscore here; those are just to denote class members Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:58: EXPECT_TRUE(client_.observers().HasObserver(&test_obeserver)); On 2015/07/09 15:41:48, Daniel Erat wrote: > just call client.HasObserver() Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:60: // Test if NotifyObservers sends the corrent values to ||. On 2015/07/09 15:41:47, Daniel Erat wrote: > sp: correct > > did you mean |observer| instead of ||? Done. https://codereview.chromium.org/1206733002/diff/380001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:77: EXPECT_FALSE(client_.observers().HasObserver(&test_obeserver)); On 2015/07/09 15:41:47, Daniel Erat wrote: > client.HasObserver() Done. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:151: props_.Clear(); On 2015/07/10 22:39:40, oshima wrote: > you don't need to call Clear(). The following line will overwrite everything. Done. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:74: void UpdatePowerProperties(const power_manager::PowerSupplyProperties& proto); On 2015/07/10 22:39:41, oshima wrote: > nit: proto refers to Protocol Buffer, which is a way to to define data > structure. props (properties, power_props etc) would be better name. Done. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:17: int num_power_changed() const { return num_power_changed_; } On 2015/07/10 22:39:41, oshima wrote: > new line Done. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:33: void SetTestProperties(power_manager::PowerSupplyProperties& props) { On 2015/07/10 22:39:41, oshima wrote: > out parameter should be pointer per style guide. Done. https://codereview.chromium.org/1206733002/diff/440001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:76: power_manager::PowerSupplyProperties props; On 2015/07/10 22:39:41, oshima wrote: > move this to line 82 just before it's first used. Done.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/560001
https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (left): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:67: keep new line https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:84: POWER_LOG(USER) << "Requested status update"; do you need this log? https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:87: weak_ptr_factory_.GetWeakPtr())); Can you add comments why this has to be done in posted task? I probably know why but explanation why you needed this would make code review much smoother. Also there is new API for this. Please use ThreadTaskRunnerHandle instead. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:12: #include "base/memory/scoped_ptr.h" do you need this? https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:105: base::MessageLoopForUI message_loop; can you use base::RunLoop instead?
https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:16: const int kDefaultBatteryLifeSpanSec = 6 * 3600; // 6 hours you don't really need this anymore https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:34: int remaining_battery_sec = 50 * kDefaultBatteryLifeSpanSec / 100; why compute this? seems like you can just do something like: props_.set_battery_time_to_empty_sec(1800); below. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:84: POWER_LOG(USER) << "Requested status update"; On 2015/07/15 18:46:50, oshima (OO until July 15th) wrote: > do you need this log? i don't think so either. these exist to debug what's going on on a real system; it doesn't seem to make sense here. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:57: EXPECT_EQ(85, client.props().battery_percent()); there are a bunch of constants (85, _DISCHARGING, etc.) hardcoded in multiple places the source now. doing e.g. EXPECT_EQ(props.battery_percent(), client.props().battery_percent()); EXPECT_EQ(props.battery_state(), client.props().battery_state()); etc. would be better. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:100: // Test if RequestStatusUpdate() will propergate the data to the observer. sp: propagate https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:104: // called as a result of RequestStatusUpdate(). i don't think i understand why you need to post a task in the fake class instead of just notifying observers synchronously. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:118: // Check when values are changed, the correct values are propergated to the sp: propagated https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:119: // Obserever sp: observer
https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:57: EXPECT_EQ(85, client.props().battery_percent()); On 2015/07/15 19:05:47, Daniel Erat wrote: > there are a bunch of constants (85, _DISCHARGING, etc.) hardcoded in multiple > places the source now. doing e.g. > > EXPECT_EQ(props.battery_percent(), client.props().battery_percent()); > EXPECT_EQ(props.battery_state(), client.props().battery_state()); > etc. > > would be better. I prefer tests to use constants or actual value rather than computed values. If they get corrupted somehow, the above code won't be able to catch such error. I'm fine to define constants for percent though.
https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:57: EXPECT_EQ(85, client.props().battery_percent()); On 2015/07/15 19:59:01, oshima (OO until July 15th) wrote: > On 2015/07/15 19:05:47, Daniel Erat wrote: > > there are a bunch of constants (85, _DISCHARGING, etc.) hardcoded in multiple > > places the source now. doing e.g. > > > > EXPECT_EQ(props.battery_percent(), client.props().battery_percent()); > > EXPECT_EQ(props.battery_state(), client.props().battery_state()); > > etc. > > > > would be better. > > I prefer tests to use constants or actual value rather than computed values. > If they get corrupted somehow, the above code won't be able to catch such error. > > I'm fine to define constants for percent though. constants are fine if they're also shared by SetTestProperties(); i was just saying not to duplicate magic numbers and enum values all over the file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ready for review https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:16: const int kDefaultBatteryLifeSpanSec = 6 * 3600; // 6 hours On 2015/07/15 19:05:46, Daniel Erat wrote: > you don't really need this anymore Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:34: int remaining_battery_sec = 50 * kDefaultBatteryLifeSpanSec / 100; On 2015/07/15 19:05:46, Daniel Erat wrote: > why compute this? seems like you can just do something like: > > props_.set_battery_time_to_empty_sec(1800); > > below. Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:84: POWER_LOG(USER) << "Requested status update"; On 2015/07/15 19:05:46, Daniel Erat wrote: > On 2015/07/15 18:46:50, oshima (OO until July 15th) wrote: > > do you need this log? > > i don't think so either. these exist to debug what's going on on a real system; > it doesn't seem to make sense here. Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:84: POWER_LOG(USER) << "Requested status update"; On 2015/07/15 18:46:50, oshima (OO until July 15th) wrote: > do you need this log? No. it was for my own debugging purposed. forgot to remove https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:87: weak_ptr_factory_.GetWeakPtr())); On 2015/07/15 18:46:50, oshima wrote: > Can you add comments why this has to be done in posted task? > > I probably know why but explanation why you needed this would make code review > much smoother. > > Also there is new API for this. Please use > > ThreadTaskRunnerHandle > > instead. > Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:12: #include "base/memory/scoped_ptr.h" On 2015/07/15 18:46:50, oshima (OO until July 15th) wrote: > do you need this? needed for the |weak_ptr_factory_| https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:57: EXPECT_EQ(85, client.props().battery_percent()); On 2015/07/15 20:02:07, Daniel Erat wrote: > On 2015/07/15 19:59:01, oshima (OO until July 15th) wrote: > > On 2015/07/15 19:05:47, Daniel Erat wrote: > > > there are a bunch of constants (85, _DISCHARGING, etc.) hardcoded in > multiple > > > places the source now. doing e.g. > > > > > > EXPECT_EQ(props.battery_percent(), client.props().battery_percent()); > > > EXPECT_EQ(props.battery_state(), client.props().battery_state()); > > > etc. > > > > > > would be better. > > > > I prefer tests to use constants or actual value rather than computed values. > > If they get corrupted somehow, the above code won't be able to catch such > error. > > > > I'm fine to define constants for percent though. > > constants are fine if they're also shared by SetTestProperties(); i was just > saying not to duplicate magic numbers and enum values all over the file. Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:100: // Test if RequestStatusUpdate() will propergate the data to the observer. On 2015/07/15 19:05:46, Daniel Erat wrote: > sp: propagate Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:104: // called as a result of RequestStatusUpdate(). On 2015/07/15 19:05:47, Daniel Erat wrote: > i don't think i understand why you need to post a task in the fake class instead > of just notifying observers synchronously. From the PowerStatusViewTest.Basic the test fails: [ RUN ] PowerStatusViewTest.Basic ../../ash/system/chromeos/power/power_status_view_unittest.cc:60: Failure Value of: IsPercentageVisible() Actual: true Expected: false this happens because the test expects the notification is called asynchronously, which is the same on the real device. I added a comment in the RequestStatusUpdate(). Please let me know if this can be synchronous and ill update the test code. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:105: base::MessageLoopForUI message_loop; On 2015/07/15 18:46:50, oshima wrote: > can you use base::RunLoop instead? Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:118: // Check when values are changed, the correct values are propergated to the On 2015/07/15 19:05:47, Daniel Erat wrote: > sp: propagated Done. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:119: // Obserever On 2015/07/15 19:05:47, Daniel Erat wrote: > sp: observer Done.
lgtm my bits with nits, but please address derat@'s comments and have his lgtm before landing. https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/560001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:12: #include "base/memory/scoped_ptr.h" On 2015/07/15 21:26:38, mozartalouis wrote: > On 2015/07/15 18:46:50, oshima (OO until July 15th) wrote: > > do you need this? > needed for the |weak_ptr_factory_| Acknowledged. https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:38: NotifyObservers(); question to derat@. What is the expected behavior here? Do we expect to get notification after init? Looks like UI always request update, so we may not need this? https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:17: const double kDefaultBatteryPercentChanged = 70; I think following name would be better kInitialBatteryPercent kUpdatedBatteryPercent
https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:38: NotifyObservers(); On 2015/07/15 21:46:23, oshima wrote: > question to derat@. What is the expected behavior here? Do we expect to get > notification after init? Looks like UI always request update, so we may not need > this? > powerd emits the status when requested, and also at least every 30 seconds. https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:17: const double kDefaultBatteryPercentChanged = 70; On 2015/07/15 21:46:23, oshima wrote: > I think following name would be better > > kInitialBatteryPercent > kUpdatedBatteryPercent agreed https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:64: EXPECT_EQ(power_manager::PowerSupplyProperties_BatteryState_DISCHARGING, make this be a constant at the top of the file too https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:66: EXPECT_EQ(power_manager::PowerSupplyProperties_ExternalPower_USB, and this https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:113: base::RunLoop().RunUntilIdle(); thanks for the explanation. running the message loop here sounds correct, since the fake's implementation of RequestStatusUpdate() is trying to mimic what the real implementation does.
https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:38: NotifyObservers(); On 2015/07/15 22:02:39, Daniel Erat wrote: > On 2015/07/15 21:46:23, oshima wrote: > > question to derat@. What is the expected behavior here? Do we expect to get > > notification after init? Looks like UI always request update, so we may not > need > > this? > > > > powerd emits the status when requested, and also at least every 30 seconds. Should we just remove it since chrome doesn't get this immediately? https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:17: const double kDefaultBatteryPercentChanged = 70; On 2015/07/15 21:46:23, oshima wrote: > I think following name would be better > > kInitialBatteryPercent > kUpdatedBatteryPercent Done. https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:113: base::RunLoop().RunUntilIdle(); On 2015/07/15 22:02:39, Daniel Erat wrote: > thanks for the explanation. running the message loop here sounds correct, since > the fake's implementation of RequestStatusUpdate() is trying to mimic what the > real implementation does. Acknowledged.
https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:38: NotifyObservers(); On 2015/07/15 22:10:30, mozartalouis wrote: > On 2015/07/15 22:02:39, Daniel Erat wrote: > > On 2015/07/15 21:46:23, oshima wrote: > > > question to derat@. What is the expected behavior here? Do we expect to get > > > notification after init? Looks like UI always request update, so we may not > > need > > > this? > > > > > > > powerd emits the status when requested, and also at least every 30 seconds. > > Should we just remove it since chrome doesn't get this immediately? yeah, it'd be best to match the real class's behavior in this regard. if ash already requests the status from the client at startup, then you can just rely on that happening.
Ready for review. hope its the last! https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:38: NotifyObservers(); On 2015/07/15 22:18:50, Daniel Erat wrote: > On 2015/07/15 22:10:30, mozartalouis wrote: > > On 2015/07/15 22:02:39, Daniel Erat wrote: > > > On 2015/07/15 21:46:23, oshima wrote: > > > > question to derat@. What is the expected behavior here? Do we expect to > get > > > > notification after init? Looks like UI always request update, so we may > not > > > need > > > > this? > > > > > > > > > > powerd emits the status when requested, and also at least every 30 seconds. > > > > Should we just remove it since chrome doesn't get this immediately? > > yeah, it'd be best to match the real class's behavior in this regard. if ash > already requests the status from the client at startup, then you can just rely > on that happening. Done. https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:64: EXPECT_EQ(power_manager::PowerSupplyProperties_BatteryState_DISCHARGING, On 2015/07/15 22:02:39, Daniel Erat wrote: > make this be a constant at the top of the file too Done. https://codereview.chromium.org/1206733002/diff/580001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:66: EXPECT_EQ(power_manager::PowerSupplyProperties_ExternalPower_USB, On 2015/07/15 22:02:39, Daniel Erat wrote: > and this Done.
https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:12: #include "components/device_event_log/device_event_log.h" think you can delete this https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:12: #include "base/memory/scoped_ptr.h" oshima mentioned this before, and you said that it's needed for weak_ptr_factory_. did you meant to include base/memory/weak_ptr.h here instead? https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:19: kBatteryStateDischarging = nit: kInitialBatteryState https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:21: const power_manager::PowerSupplyProperties_ExternalPower kExternalPowerUsb = nit: kInitialExternalPower https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: const power_manager::PowerSupplyProperties_ExternalPower kExternalPowerAc = you can delete this; i meant that you you should use constants for values that are set in SetTestProperties() and then checked elsewhere.
ready for review https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.cc (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.cc:12: #include "components/device_event_log/device_event_log.h" On 2015/07/15 22:39:05, Daniel Erat wrote: > think you can delete this Done. https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client.h (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client.h:12: #include "base/memory/scoped_ptr.h" On 2015/07/15 22:39:05, Daniel Erat wrote: > oshima mentioned this before, and you said that it's needed for > weak_ptr_factory_. did you meant to include base/memory/weak_ptr.h here instead? I had just taken a quick look at the previous implementation and i think i might have looked at it wrong. you are correct. Changed. https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... File chromeos/dbus/fake_power_manager_client_unittest.cc (right): https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:19: kBatteryStateDischarging = On 2015/07/15 22:39:06, Daniel Erat wrote: > nit: kInitialBatteryState Done. https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:21: const power_manager::PowerSupplyProperties_ExternalPower kExternalPowerUsb = On 2015/07/15 22:39:06, Daniel Erat wrote: > nit: kInitialExternalPower Done. https://codereview.chromium.org/1206733002/diff/600001/chromeos/dbus/fake_pow... chromeos/dbus/fake_power_manager_client_unittest.cc:23: const power_manager::PowerSupplyProperties_ExternalPower kExternalPowerAc = On 2015/07/15 22:39:06, Daniel Erat wrote: > you can delete this; i meant that you you should use constants for values that > are set in SetTestProperties() and then checked elsewhere. Done.
lgtm!
The CQ bit was checked by mozartalouis@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, oshima@chromium.org Link to the patchset: https://codereview.chromium.org/1206733002/#ps620001 (title: "nits and includes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1206733002/620001
Message was sent while issue was closed.
Committed patchset #25 (id:620001)
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/7e81ea890d6df774ad8c16868492f1d6d1ef4672 Cr-Commit-Position: refs/heads/master@{#338947} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
