Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13638018:
Add PeripheralBatteryObserver (Closed)
|
Created:
7 years, 8 months ago by Yufeng Shen (Slow to review) Modified:
7 years, 8 months ago CC:
chromium-reviews, derat+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd PeripheralBatteryObserver
PeripheralBatteryObserver is an observer of PowerManagerClient. It
monitors battery level of peripheral devices and shows notification
when the device battery is in low level conditions.
BUG=221420
TEST=Manual tested on Link with Apple Magic Mouse/Trackpad.
TBR=sky
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193802
Patch Set 1 #
Total comments: 2
Patch Set 2 : update #Patch Set 3 : Listen on bluetooth device removal to cancel notification #Patch Set 4 : removed unused const #
Total comments: 40
Patch Set 5 : removed image icons; use device bluetooth address as key; addressed comments #Patch Set 6 : update #
Total comments: 36
Patch Set 7 : browser tests added #
Total comments: 24
Patch Set 8 : update #
Total comments: 2
Patch Set 9 : remove expection from mock_dbus_thread_manager #Patch Set 10 : fix header file order #Patch Set 11 : Make PeripheralBatteryNotificationDelegate's dtor private (it has a refcounted base) #Patch Set 12 : make a fake kNotificationOriginUrl to make origin check in notification manager happy #Messages
Total messages: 24 (0 generated)
just nitpicks https://codereview.chromium.org/13638018/diff/1/chrome/app/theme/theme_resour... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/13638018/diff/1/chrome/app/theme/theme_resour... chrome/app/theme/theme_resources.grd:513: </if> CQ doesn't support binary files like png well. I'd recommend you to split your CL: a) adding data and b) user code, and dcommit a). https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:151: if (!notification_manager->CancelById(string_id)) { You do not need to cancel it by yourself. NotificationUIManager will handle the replacement. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:157: Notification notification( Please add my TODO here: TODO(mukai): add SYSTEM priority and use here
https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:28: const int kLowBatteryLevel = 15; add comments describing what these mean. for example, i would've guessed from this variable's name that the battery is considered low when it's <= this level. looking at the code, it appears that you're doing < instead. (i'd recommend switching to <= below, but please add a comment regardless.) https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:29: const int kNotificationInterval = 60; include units in this constant's name, e.g. kNotificationIntervalSec https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:31: class PeripheralBatteryNotificationDelegate : public NotificationDelegate { is this class really necessary? it looks like it doesn't do anything except store an id. it'd be worth asking some notifications people if there's a cleaner way to do this. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:33: delete blank line https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:54: return base::TimeDelta::FromInternalValue( why are you converting from one class's internal value to another class's internal value? TimeDelta represents the difference between two TimeTicks; what you're doing here doesn't make sense. don't use ToInternalValue/FromInternalValue for anything except serialization/deserialization. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:83: const std::string& path, const std::string& name, int level) { one parameter per line https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:91: g_browser_process->notification_ui_manager()->CancelById(path); can you return immediately here? is there any reason to continue, and possibly create a new BatteryInfo, for an unknown level? https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: RemoveBattery(UTF16ToUTF8(device->GetName())); is it guaranteed that the names that you get here will match the names coming from powerd in the battery level signals? https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:127: if (name == it->second.name()) { could you just key the map by device name instead of by path so you don't need to iterate through the whole map here? https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:135: void PeripheralBatteryObserver::PostNotification(BatteryInfo* battery) { this should take a const BatteryInfo& instead of a pointer https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:137: // since last notification showed, avoiding the case the battery level nit: s/case the/case where the/ https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:147: std::string string_text = base::StringPrintf("Battery Low (%d%%)", this needs to be a translatable string resource https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:20: // This observer listens for peripheral device battery status and show nit: s/show/shows/ https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:43: class BatteryInfo { this should probably be a struct instead of a class. i don't see any methods besides setters and getters https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:46: BatteryInfo(const std::string& path, const std::string& name, int level, nit: one parameter per line if they don't all fit on the first line https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:49: : path_(path), name_(name), level_(level), one member per line https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:69: const base::TimeDelta& last_update_timestamp() { a TimeDelta isn't a timestamp. use base::TimeTicks for these instead. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:90: std::map<std::string, BatteryInfo> batteries_; add a comment describing what the key is here
PTAL https://codereview.chromium.org/13638018/diff/1/chrome/app/theme/theme_resour... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/13638018/diff/1/chrome/app/theme/theme_resour... chrome/app/theme/theme_resources.grd:513: </if> On 2013/04/06 01:11:56, Jun Mukai wrote: > CQ doesn't support binary files like png well. I'd recommend you to split your > CL: a) adding data and b) user code, and dcommit a). Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:28: const int kLowBatteryLevel = 15; On 2013/04/06 01:52:34, Daniel Erat wrote: > add comments describing what these mean. for example, i would've guessed from > this variable's name that the battery is considered low when it's <= this level. > looking at the code, it appears that you're doing < instead. (i'd recommend > switching to <= below, but please add a comment regardless.) Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:29: const int kNotificationInterval = 60; On 2013/04/06 01:52:34, Daniel Erat wrote: > include units in this constant's name, e.g. kNotificationIntervalSec Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:31: class PeripheralBatteryNotificationDelegate : public NotificationDelegate { On 2013/04/06 01:52:34, Daniel Erat wrote: > is this class really necessary? it looks like it doesn't do anything except > store an id. it'd be worth asking some notifications people if there's a > cleaner way to do this. offline talk to dewittj@ about making NotificationDelegate's pure virtual functions to be non-virtual with default implementation, and he suggested not to do so since they are going to change the prototype pretty soon and I can come back to fix the implementation later. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:33: On 2013/04/06 01:52:34, Daniel Erat wrote: > delete blank line Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:54: return base::TimeDelta::FromInternalValue( On 2013/04/06 01:52:34, Daniel Erat wrote: > why are you converting from one class's internal value to another class's > internal value? TimeDelta represents the difference between two TimeTicks; what > you're doing here doesn't make sense. don't use > ToInternalValue/FromInternalValue for anything except > serialization/deserialization. Removed. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:83: const std::string& path, const std::string& name, int level) { On 2013/04/06 01:52:34, Daniel Erat wrote: > one parameter per line Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:91: g_browser_process->notification_ui_manager()->CancelById(path); On 2013/04/06 01:52:34, Daniel Erat wrote: > can you return immediately here? is there any reason to continue, and possibly > create a new BatteryInfo, for an unknown level? Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: RemoveBattery(UTF16ToUTF8(device->GetName())); On 2013/04/06 01:52:34, Daniel Erat wrote: > is it guaranteed that the names that you get here will match the names coming > from powerd in the battery level signals? So the name can actually be set by the user so it is not guaranteed to be unique. I changed it to use the bluetooth device's address as the key add/remove battery information. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:127: if (name == it->second.name()) { On 2013/04/06 01:52:34, Daniel Erat wrote: > could you just key the map by device name instead of by path so you don't need > to iterate through the whole map here? Changed to use the device's bluetooth address as the key. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:135: void PeripheralBatteryObserver::PostNotification(BatteryInfo* battery) { On 2013/04/06 01:52:34, Daniel Erat wrote: > this should take a const BatteryInfo& instead of a pointer Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:137: // since last notification showed, avoiding the case the battery level On 2013/04/06 01:52:34, Daniel Erat wrote: > nit: s/case the/case where the/ Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:147: std::string string_text = base::StringPrintf("Battery Low (%d%%)", On 2013/04/06 01:52:34, Daniel Erat wrote: > this needs to be a translatable string resource Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:151: if (!notification_manager->CancelById(string_id)) { On 2013/04/06 01:11:56, Jun Mukai wrote: > You do not need to cancel it by yourself. NotificationUIManager will handle the > replacement. great. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.cc:157: Notification notification( On 2013/04/06 01:11:56, Jun Mukai wrote: > Please add my TODO here: > TODO(mukai): add SYSTEM priority and use here Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:20: // This observer listens for peripheral device battery status and show On 2013/04/06 01:52:34, Daniel Erat wrote: > nit: s/show/shows/ Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:43: class BatteryInfo { On 2013/04/06 01:52:34, Daniel Erat wrote: > this should probably be a struct instead of a class. i don't see any methods > besides setters and getters Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:46: BatteryInfo(const std::string& path, const std::string& name, int level, On 2013/04/06 01:52:34, Daniel Erat wrote: > nit: one parameter per line if they don't all fit on the first line Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:49: : path_(path), name_(name), level_(level), On 2013/04/06 01:52:34, Daniel Erat wrote: > one member per line Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:69: const base::TimeDelta& last_update_timestamp() { On 2013/04/06 01:52:34, Daniel Erat wrote: > a TimeDelta isn't a timestamp. use base::TimeTicks for these instead. Done. https://codereview.chromium.org/13638018/diff/5003/chrome/browser/chromeos/po... chrome/browser/chromeos/power/peripheral_battery_observer.h:90: std::map<std::string, BatteryInfo> batteries_; On 2013/04/06 01:52:34, Daniel Erat wrote: > add a comment describing what the key is here Done.
can you add unit tests for PeripheralBatteryObserver? https://codereview.chromium.org/13638018/diff/22001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/13638018/diff/22001/ash/ash_strings.grd#newco... ash/ash_strings.grd:461: Battery Low (<ph name="percentage">$1<ex>56</ex></ph>%) "Battery low" (with a lowercase L) instead? the screenshot notification text seems to not have each word capitalized, for instance https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:34: const int kLowBatteryLevel = 15; nit: add blank line after this https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:37: const int kNotificationIntervalSec = 60; nit: add blank line after this https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:42: const char kHIDBatteryPathHeader[] = "/sys/class/power_supply/hid-"; instead of "header" and "end", how about "prefix" and "suffix"? https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:96: std::string key; rename this variable to "address" to make it clearer what it contains https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:143: if (!device->IsPaired()) { nit: remove curly braces since the statement is a single line https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:160: int header_size = sizeof(kHIDBatteryPathHeader); indent this line and the following ones two spaces, not four https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:161: int end_size = sizeof(kHIDBatteryPathEnd); this makes me nervous. will it break if someone changes the constants to be char* instead for some reason? it might be safer to use strlen, even if it's slower. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:162: int key_len = path.size() - (header_size - 1) - (end_size - 1); this makes me nervous. return early if key_len <= 0? https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:175: if (batteries_.find(key_lowercase) != batteries_.end()) { if you aren't going to save the iterator to make the call to erase() faster, it's easier to just use batteries_.count(key_lowercase) here https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:29: void InitializeOnBluetoothReady( can this be private instead of public? https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:56: const std::string& name() const { return name_; } you don't need getters and setters now that this is a struct https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:78: std::string name_; make these public and remove the trailing underscores http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:86: std::string ExtractBluetoothAddress(const std::string& path); these methods don't look like they need to be class members. if that's correct, just make them be functions in the anonymous namespace in the .cc file https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:90: // Post a low battery notification with unique id |id|, Return true if the nit: s/Post/Posts/, s/,/./, s/Return/Returns/ https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:91: // noitification is posted, false if not. sp: notification https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:94: void CancelNotification(const std::string& id); please use consistent parameter names instead of 'id', 'key', etc., assuming that these are all the same thing. i'd recommend 'address', since that seems to be what it actually is https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:98: std::map<std::string, BatteryInfo> batteries_; nit: add a blank line after this one to set it apart from the following members and make it more clear that its comment doesn't refer to them
PTAL, thanks. https://codereview.chromium.org/13638018/diff/22001/ash/ash_strings.grd File ash/ash_strings.grd (right): https://codereview.chromium.org/13638018/diff/22001/ash/ash_strings.grd#newco... ash/ash_strings.grd:461: Battery Low (<ph name="percentage">$1<ex>56</ex></ph>%) On 2013/04/09 02:06:18, Daniel Erat wrote: > "Battery low" (with a lowercase L) instead? the screenshot notification text > seems to not have each word capitalized, for instance Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:34: const int kLowBatteryLevel = 15; On 2013/04/09 02:06:18, Daniel Erat wrote: > nit: add blank line after this Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:37: const int kNotificationIntervalSec = 60; On 2013/04/09 02:06:18, Daniel Erat wrote: > nit: add blank line after this Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:42: const char kHIDBatteryPathHeader[] = "/sys/class/power_supply/hid-"; On 2013/04/09 02:06:18, Daniel Erat wrote: > instead of "header" and "end", how about "prefix" and "suffix"? Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:96: std::string key; On 2013/04/09 02:06:18, Daniel Erat wrote: > rename this variable to "address" to make it clearer what it contains Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:143: if (!device->IsPaired()) { On 2013/04/09 02:06:18, Daniel Erat wrote: > nit: remove curly braces since the statement is a single line Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:160: int header_size = sizeof(kHIDBatteryPathHeader); On 2013/04/09 02:06:18, Daniel Erat wrote: > indent this line and the following ones two spaces, not four Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:161: int end_size = sizeof(kHIDBatteryPathEnd); On 2013/04/09 02:06:18, Daniel Erat wrote: > this makes me nervous. will it break if someone changes the constants to be > char* instead for some reason? it might be safer to use strlen, even if it's > slower. Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:162: int key_len = path.size() - (header_size - 1) - (end_size - 1); On 2013/04/09 02:06:18, Daniel Erat wrote: > this makes me nervous. return early if key_len <= 0? Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:175: if (batteries_.find(key_lowercase) != batteries_.end()) { On 2013/04/09 02:06:18, Daniel Erat wrote: > if you aren't going to save the iterator to make the call to erase() faster, > it's easier to just use batteries_.count(key_lowercase) here Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:29: void InitializeOnBluetoothReady( On 2013/04/09 02:06:18, Daniel Erat wrote: > can this be private instead of public? Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:56: const std::string& name() const { return name_; } On 2013/04/09 02:06:18, Daniel Erat wrote: > you don't need getters and setters now that this is a struct Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:78: std::string name_; On 2013/04/09 02:06:18, Daniel Erat wrote: > make these public and remove the trailing underscores > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl... > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:86: std::string ExtractBluetoothAddress(const std::string& path); On 2013/04/09 02:06:18, Daniel Erat wrote: > these methods don't look like they need to be class members. if that's correct, > just make them be functions in the anonymous namespace in the .cc file Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:90: // Post a low battery notification with unique id |id|, Return true if the On 2013/04/09 02:06:18, Daniel Erat wrote: > nit: s/Post/Posts/, s/,/./, s/Return/Returns/ Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:91: // noitification is posted, false if not. On 2013/04/09 02:06:18, Daniel Erat wrote: > sp: notification Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:94: void CancelNotification(const std::string& id); On 2013/04/09 02:06:18, Daniel Erat wrote: > please use consistent parameter names instead of 'id', 'key', etc., assuming > that these are all the same thing. i'd recommend 'address', since that seems to > be what it actually is Done. https://codereview.chromium.org/13638018/diff/22001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:98: std::map<std::string, BatteryInfo> batteries_; On 2013/04/09 02:06:18, Daniel Erat wrote: > nit: add a blank line after this one to set it apart from the following members > and make it more clear that its comment doesn't refer to them Done.
https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:379: peripheral_battery_observer_.reset(); can this be moved to line 748, where the other dbus observers are reset? https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:452: peripheral_battery_observer_.reset(new PeripheralBatteryObserver()); move to line 641? it's better to wait as long as possible before creating these so boot isn't slowed https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: // For HID bluetooh device, device address is used as key to index fix indenting: this should be two spaces beyond the previous line instead of four https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:124: if (address == std::string()) { if (address.empty()) { https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:231: void PeripheralBatteryObserver::CancelNotification(const std::string& id) { rename 'id' to 'address' here to match the header https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:43: void SetClockForTesting(base::SimpleTestTickClock* clock) { nit: move above virtual methods and rename to set_testing_clock() since it's inlined https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:65: // Battery leve within range [0, 100], and -1 for unknown level. s/leve/level/ https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:81: // Record of existing battery infomation. For bluetooh HID device, the key s/bluetooh/bluetooth/ https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:89: base::SimpleTestTickClock* testing_clock_; nit: add a comment saying that this pointer is unowned and may be null https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:45: EXPECT_CALL(*mock_dbus_thread_manager->mock_update_engine_client(), why do you need to mess around with the update engine client here? https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:75: base::SimpleTestTickClock* clock = new base::SimpleTestTickClock(); aren't you leaking this? why can't you create it on the stack instead of the heap?
https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:379: peripheral_battery_observer_.reset(); On 2013/04/10 12:58:21, Daniel Erat wrote: > can this be moved to line 748, where the other dbus observers are reset? Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:452: peripheral_battery_observer_.reset(new PeripheralBatteryObserver()); On 2013/04/10 12:58:21, Daniel Erat wrote: > move to line 641? it's better to wait as long as possible before creating these > so boot isn't slowed Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: // For HID bluetooh device, device address is used as key to index On 2013/04/10 12:58:21, Daniel Erat wrote: > fix indenting: this should be two spaces beyond the previous line instead of > four Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:124: if (address == std::string()) { On 2013/04/10 12:58:21, Daniel Erat wrote: > if (address.empty()) { Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:231: void PeripheralBatteryObserver::CancelNotification(const std::string& id) { On 2013/04/10 12:58:21, Daniel Erat wrote: > rename 'id' to 'address' here to match the header Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.h (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:43: void SetClockForTesting(base::SimpleTestTickClock* clock) { On 2013/04/10 12:58:21, Daniel Erat wrote: > nit: move above virtual methods and rename to set_testing_clock() since it's > inlined Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:65: // Battery leve within range [0, 100], and -1 for unknown level. On 2013/04/10 12:58:21, Daniel Erat wrote: > s/leve/level/ Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:81: // Record of existing battery infomation. For bluetooh HID device, the key On 2013/04/10 12:58:21, Daniel Erat wrote: > s/bluetooh/bluetooth/ Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.h:89: base::SimpleTestTickClock* testing_clock_; On 2013/04/10 12:58:21, Daniel Erat wrote: > nit: add a comment saying that this pointer is unowned and may be null Done. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:45: EXPECT_CALL(*mock_dbus_thread_manager->mock_update_engine_client(), On 2013/04/10 12:58:21, Daniel Erat wrote: > why do you need to mess around with the update engine client here? I think it is because ChromeBrowserMainPartsChromeos has a member AutomaticRebootManager, which will call AutomaticRebootManager::Init() { ... UpdateStatusChanged( DBusThreadManager::Get()->GetUpdateEngineClient()->GetLastStatus()); } and mock_update_engine_client::GetLastStatus is a mock function that does not return Status(), so here we have to return a fake Status(), otherwise the tests will crash. That's why all the CrOS browsertests that use MockDBusThreadManager have to put in this workaround. https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:75: base::SimpleTestTickClock* clock = new base::SimpleTestTickClock(); On 2013/04/10 12:58:21, Daniel Erat wrote: > aren't you leaking this? why can't you create it on the stack instead of the > heap? Done.
https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:45: EXPECT_CALL(*mock_dbus_thread_manager->mock_update_engine_client(), On 2013/04/10 18:13:46, Yufeng Shen wrote: > On 2013/04/10 12:58:21, Daniel Erat wrote: > > why do you need to mess around with the update engine client here? > > I think it is because ChromeBrowserMainPartsChromeos has a member > AutomaticRebootManager, which will call > > AutomaticRebootManager::Init() { > ... > UpdateStatusChanged( > DBusThreadManager::Get()->GetUpdateEngineClient()->GetLastStatus()); > } > > and mock_update_engine_client::GetLastStatus is a mock function that does not > return Status(), so here we have to return a fake Status(), otherwise the tests > will crash. > > That's why all the CrOS browsertests that use MockDBusThreadManager have to put > in this workaround. Do you mind moving this expectation to MockDBusThreadManager so it doesn't need to be copy-and-pasted across a bunch of different tests? Note that there are already other similar expectations that get set up in the thread manager's c'tor. https://codereview.chromium.org/13638018/diff/40001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/40001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: // For HID bluetooh device, device address is used as key to index s/bluetooh/bluetooth/
https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc (right): https://codereview.chromium.org/13638018/diff/29001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer_browsertest.cc:45: EXPECT_CALL(*mock_dbus_thread_manager->mock_update_engine_client(), On 2013/04/10 18:21:35, Daniel Erat wrote: > On 2013/04/10 18:13:46, Yufeng Shen wrote: > > On 2013/04/10 12:58:21, Daniel Erat wrote: > > > why do you need to mess around with the update engine client here? > > > > I think it is because ChromeBrowserMainPartsChromeos has a member > > AutomaticRebootManager, which will call > > > > AutomaticRebootManager::Init() { > > ... > > UpdateStatusChanged( > > DBusThreadManager::Get()->GetUpdateEngineClient()->GetLastStatus()); > > } > > > > and mock_update_engine_client::GetLastStatus is a mock function that does not > > return Status(), so here we have to return a fake Status(), otherwise the > tests > > will crash. > > > > That's why all the CrOS browsertests that use MockDBusThreadManager have to > put > > in this workaround. > > Do you mind moving this expectation to MockDBusThreadManager so it doesn't need > to be copy-and-pasted across a bunch of different tests? Note that there are > already other similar expectations that get set up in the thread manager's > c'tor. https://codereview.chromium.org/14092002/ https://codereview.chromium.org/13638018/diff/40001/chrome/browser/chromeos/p... File chrome/browser/chromeos/power/peripheral_battery_observer.cc (right): https://codereview.chromium.org/13638018/diff/40001/chrome/browser/chromeos/p... chrome/browser/chromeos/power/peripheral_battery_observer.cc:116: // For HID bluetooh device, device address is used as key to index On 2013/04/10 18:21:35, Daniel Erat wrote: > s/bluetooh/bluetooth/ Done.
lgtm
cc sky@ for OWNERS
I'm sounding like a broken record. What do you need me to review?
cc oshima@ for OWNERS of chrome/app/theme/* hope this time I am getting the right person
chrome/app/theme lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/13638018/51001
Presubmit check for 13638018-51001 failed and returned exit status 1. INFO:root:Found 9 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/ash/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/app/theme/PRESUBMIT.py ** Presubmit Messages ** Your #include order seems to be broken. Send mail to marja@chromium.org if this is not the case. chrome/browser/chromeos/power/peripheral_battery_observer.cc:9 ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/chrome_browser_chromeos.gypi Presubmit checks took 1.3s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/13638018/60001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/13638018/71001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/13638018/83001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/13638018/83001
Message was sent while issue was closed.
Change committed as 193802 |
