Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
398683006:
Battery Status API: implementation for Mac OS. (Closed)
|
Created:
6 years, 5 months ago by timvolodine Modified:
6 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBattery Status API: implementation for Mac OS.
Implementation of Battery Status API for the Mac platform.
BUG=122593
TEST=http://jsbin.com/battery-status-diagnostics (manual)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286562
Patch Set 1 #Patch Set 2 : fix browsertests #
Total comments: 32
Patch Set 3 : fixed comments #
Total comments: 36
Patch Set 4 : fix Mark's comments #Patch Set 5 : add DCHECK and source transport type check #Patch Set 6 : SInt32 -> SInt64 #
Total comments: 8
Patch Set 7 : fix Robert's comments #
Total comments: 6
Patch Set 8 : comments + rebase #
Messages
Total messages: 26 (0 generated)
+ Mark as mac expert
I think you were confused about how booleans are stored in CFDictionaries. Now that I’ve explained it, some of this code may become simpler. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:30: (CFNumberRef)CFDictionaryGetValue(description, key); CFCast<CFNumberRef>. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:43: (CFStringRef)CFDictionaryGetValue(description, CFCast<CFStringRef>. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:49: CFDictionaryGetValue(description, CFSTR(kIOPSIsChargingKey)) This just tells you if kIOPSIsChargingKey was found in the dictionary, it doesn’t tell you what its value is. Right now, I see | | "FullyCharged" = Yes […] | | "IsCharging" = No By your logic here, both is_charging and is_charged would be true. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:55: // Set charging to false if on battery power and not charging. Why not set status.charging to is_charging? Why haven’t you set status.charging to true if it’s charging? https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:64: if (current_capacity != -1 && max_capacity != -1) && current_capacity <= max_capacity? && max_capacity != 0? https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:65: status.level = (double)current_capacity/(double)max_capacity; Don’t use (C_style)casts, use Cplusplus<style>(casts). These can be static_cast. Actually, only one of them needs a static_cast. But put some spaces around the / operator, so people can see what it’s operating on. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:73: status.chargingTime = (charging_time != -1) WebBatteryStatus.h doesn’t have comments in it to help me understand the meaning of the fields. I might have assumed that chargingTime meant “time already spent charging.” I also don’t know if there’s an agreed-upon standard for the use of infinity, how other platforms deal with this, etc. Where’s the documentation? https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:88: status.dischargingTime = Should you be setting status.dischargingTime to some value always? Same for the other members of status. I only see you setting the values sometimes. What will they contain otherwise? Garbage? https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:98: CFTypeRef blob = IOPSCopyPowerSourcesInfo(); Do you need to release this or does IOPSCopyPowerSourcesList take ownership of it? I suspect that you need to release it. In that case, use ScopedCFTypeRef. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:99: CFArrayRef powerSourcesList = IOPSCopyPowerSourcesList(blob); This one almost definitely needs a ScopedCFTypeRef. In CoreFoundaton-land, things with Copy in their name mean that a new copy is made that you need to take ownership of and dispose of when you’re done with. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:107: CFDictionaryGetValue(description, CFSTR(kIOPSIsPresentKey)) CFDictionaryGetValue doesn’t return kCFBooleanTrue or kCFBooleanFalse, it returns a CFBooleanRef. Use CFBooleanGetValue to get the value of the boolean. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:124: : callback_(callback), notifier_run_loop_(0) {} Use NULL for null CFTypeRef-family values here and on line 179. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:151: virtual ~BatteryStatusObserver() {} [D]CHECK that it’s stopped (by notifier_run_loop_ being NULL. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:156: if (notifier_run_loop_) Might this actually happen in practice? It may be better as a [D]CHECK. Same in StopOnUI, line 173.
https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:30: (CFNumberRef)CFDictionaryGetValue(description, key); On 2014/07/17 14:08:31, Mark Mentovai wrote: > CFCast<CFNumberRef>. Better yet: base::mac::GetValueFromDictionary<T>(dictionary, key). https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:192: BatteryCallback& callback_; Why is this stored by reference?
thanks Mark and Robert for the comments! I've uploaded a new patch that should address all of them, ptal. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:30: (CFNumberRef)CFDictionaryGetValue(description, key); On 2014/07/17 14:08:31, Mark Mentovai wrote: > CFCast<CFNumberRef>. Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:30: (CFNumberRef)CFDictionaryGetValue(description, key); On 2014/07/17 15:16:41, rsesek wrote: > On 2014/07/17 14:08:31, Mark Mentovai wrote: > > CFCast<CFNumberRef>. > > Better yet: base::mac::GetValueFromDictionary<T>(dictionary, key). Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:43: (CFStringRef)CFDictionaryGetValue(description, On 2014/07/17 14:08:31, Mark Mentovai wrote: > CFCast<CFStringRef>. Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:49: CFDictionaryGetValue(description, CFSTR(kIOPSIsChargingKey)) On 2014/07/17 14:08:31, Mark Mentovai wrote: > This just tells you if kIOPSIsChargingKey was found in the dictionary, it > doesn’t tell you what its value is. Right now, I see > > | | "FullyCharged" = Yes > […] > | | "IsCharging" = No > > By your logic here, both is_charging and is_charged would be true. I've changed this to use CFBooleanGetValue, as you suggested in one of the later comments. However the initial code works as well, I suspect that is because CFDictionaryGetValue() returns a pointer which equals to kCFBooleanTrue or kCFBooleanFalse if present. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:55: // Set charging to false if on battery power and not charging. On 2014/07/17 14:08:31, Mark Mentovai wrote: > Why not set status.charging to is_charging? > > Why haven’t you set status.charging to true if it’s charging? yes this is on purpose. I've added a comment explaining the default value. The idea is to set fields only when different from the default value. Also, status.charging has slightly different semantics as compared to is_charging, it is defined as being false only when battery is discharging and true otherwise. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:64: if (current_capacity != -1 && max_capacity != -1) On 2014/07/17 14:08:31, Mark Mentovai wrote: > && current_capacity <= max_capacity? > && max_capacity != 0? Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:65: status.level = (double)current_capacity/(double)max_capacity; On 2014/07/17 14:08:31, Mark Mentovai wrote: > Don’t use (C_style)casts, use Cplusplus<style>(casts). These can be static_cast. > Actually, only one of them needs a static_cast. > > But put some spaces around the / operator, so people can see what it’s operating > on. Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:73: status.chargingTime = (charging_time != -1) On 2014/07/17 14:08:31, Mark Mentovai wrote: > WebBatteryStatus.h doesn’t have comments in it to help me understand the meaning > of the fields. I might have assumed that chargingTime meant “time already spent > charging.” I also don’t know if there’s an agreed-upon standard for the use of > infinity, how other platforms deal with this, etc. Where’s the documentation? The specification is here: https://dvcs.w3.org/hg/dap/raw-file/tip/battery/Overview.html. I'll probably add some comments to WebBatteryStatus.h in a separate patch. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:88: status.dischargingTime = On 2014/07/17 14:08:31, Mark Mentovai wrote: > Should you be setting status.dischargingTime to some value always? > > Same for the other members of status. I only see you setting the values > sometimes. What will they contain otherwise? Garbage? no garbage ;). I've added some comments regarding default values to make it more understandable. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:98: CFTypeRef blob = IOPSCopyPowerSourcesInfo(); On 2014/07/17 14:08:31, Mark Mentovai wrote: > Do you need to release this or does IOPSCopyPowerSourcesList take ownership of > it? I suspect that you need to release it. In that case, use ScopedCFTypeRef. you are right, it needs to be released. added ScopedCFTypeRef. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:99: CFArrayRef powerSourcesList = IOPSCopyPowerSourcesList(blob); On 2014/07/17 14:08:31, Mark Mentovai wrote: > This one almost definitely needs a ScopedCFTypeRef. > > In CoreFoundaton-land, things with Copy in their name mean that a new copy is > made that you need to take ownership of and dispose of when you’re done with. added ScopedCFTypeRef, done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:107: CFDictionaryGetValue(description, CFSTR(kIOPSIsPresentKey)) On 2014/07/17 14:08:31, Mark Mentovai wrote: > CFDictionaryGetValue doesn’t return kCFBooleanTrue or kCFBooleanFalse, it > returns a CFBooleanRef. Use CFBooleanGetValue to get the value of the boolean. Done. (but it looks like it actually returns a pointer which has the value of kCFBoolean{True,False}). https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:124: : callback_(callback), notifier_run_loop_(0) {} On 2014/07/17 14:08:31, Mark Mentovai wrote: > Use NULL for null CFTypeRef-family values here and on line 179. not necessary anymore because I made notifier_run_loop_ ScopedCFTypeRef as well. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:151: virtual ~BatteryStatusObserver() {} On 2014/07/17 14:08:31, Mark Mentovai wrote: > [D]CHECK that it’s stopped (by notifier_run_loop_ being NULL. Done. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:156: if (notifier_run_loop_) On 2014/07/17 14:08:31, Mark Mentovai wrote: > Might this actually happen in practice? It may be better as a [D]CHECK. Same in > StopOnUI, line 173. I don't think we should be that strict, users may invoke multiple start/stop without any issues. Also the if-check in StopOnUI handles the case if we failed to create the notifier_run_loop_ in StartOnUI for some reason. https://codereview.chromium.org/398683006/diff/20001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:192: BatteryCallback& callback_; On 2014/07/17 15:16:41, rsesek wrote: > Why is this stored by reference? technically this is ok because the callback is owned by BatteryStatusService. However I've changed this to value semantics now because it arguably looks better and is also more extendable.
Robert said he might want to take another look at this too. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, This is already in an unnamed namespace, so the static doesn’t help at all. Same for all of the other functions in here. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, Why did you choose CFIndex as the type for the CFNumbers? What’s CFNumberGetType return for the numbers you’re interested in? https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:40: static Boolean GetValueAsBoolean(CFDictionaryRef description, I think that this should return a bool and take a bool as the third argument, which is consistent with the type you’ve used elsewhere. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:51: CFStringRef currentState = Follow the C++ style guide. This should be named current_state. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:55: bool on_battery_power = CFStringCompare(currentState, I don’t think this is going to be too happy if currentState is NULL. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:60: GetValueAsBoolean(description, CFSTR(kIOPSIsChargingKey), true); Is true the best default here? I probably would have chosen false. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:66: if (on_battery_power && !is_charging) Why not avoid the conditional and set this explicitly all the time, which avoid the need for the awkward comment explaining the default: status.charging = !on_battery_power || is_charging; This is clearer and functionally equivalent to what you wrote. In fact, for all of these fields, I think it’d be preferable to set the values explicitly, at least in most cases, rather than relying on the default values. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:113: base::ScopedCFTypeRef<CFArrayRef> powerSourcesList( Naming: power_sources_list. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:119: CFArrayGetValueAtIndex(powerSourcesList, i)); if (!description) continue; can go here, you don’t need to wait until line 125 after you’ve done GetValueAsBoolean(). https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; In theory, there can be more than one battery. If you don’t handle that, you should put a comment here. You may also want to remove the break without handling multiple batteries fully. You can let the loop continue to process all power sources so that if it finds more than one battery, it can DCHECK to alert you (or some future person) that the code needs attention. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:142: // Need thread with UI-type message loop for notifications to run. If you added two more words here, it’d be a complete sentence, which would be easier to understand. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:174: OnBatteryStatusChangedUI((void*)&callback_); Use cplusplus<style>(casts). See the style guide. Same on line 177. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:179: if (notifier_run_loop_) { Do you need this conditional? If IOPSNotificationCreateRunLoopSource() failed, we probably want to crash. If we’re actually seeing it fail in practice for some reason, then you should say something about that in a comment.
P.S. Thanks for the spec link.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, On 2014/07/24 01:32:22, Mark Mentovai wrote: > This is already in an unnamed namespace, so the static doesn’t help at all. Same > for all of the other functions in here. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, On 2014/07/24 01:32:22, Mark Mentovai wrote: > Why did you choose CFIndex as the type for the CFNumbers? What’s CFNumberGetType > return for the numbers you’re interested in? usually it is kCFNumberSInt64Type, but can also be kCFNumberSInt32Type (e.g. when "time to empty" is -1). For practical use the CFIndex (which is a signed long) should be sufficient. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:40: static Boolean GetValueAsBoolean(CFDictionaryRef description, On 2014/07/24 01:32:21, Mark Mentovai wrote: > I think that this should return a bool and take a bool as the third argument, > which is consistent with the type you’ve used elsewhere. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:51: CFStringRef currentState = On 2014/07/24 01:32:22, Mark Mentovai wrote: > Follow the C++ style guide. This should be named current_state. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:55: bool on_battery_power = CFStringCompare(currentState, On 2014/07/24 01:32:22, Mark Mentovai wrote: > I don’t think this is going to be too happy if currentState is NULL. actually kIOPSPowerSourceStateKey is a required key so should not be NULL, but anyway I've made it more robust now. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:60: GetValueAsBoolean(description, CFSTR(kIOPSIsChargingKey), true); On 2014/07/24 01:32:22, Mark Mentovai wrote: > Is true the best default here? I probably would have chosen false. kIOPSIsChargingKey is also required so hopefully no need for default value, but as according to specification I've chosen true to reflect the default value in this case. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:66: if (on_battery_power && !is_charging) On 2014/07/24 01:32:22, Mark Mentovai wrote: > Why not avoid the conditional and set this explicitly all the time, which avoid > the need for the awkward comment explaining the default: > > status.charging = !on_battery_power || is_charging; > > This is clearer and functionally equivalent to what you wrote. > > In fact, for all of these fields, I think it’d be preferable to set the values > explicitly, at least in most cases, rather than relying on the default values. Done. Re other fields: normally speaking the idea is to set the fields only if we can do so reliably. Otherwise we'll have to explicitly specify defaults for each platform-specific implementation. ChromeOS implementation is also done this way. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:113: base::ScopedCFTypeRef<CFArrayRef> powerSourcesList( On 2014/07/24 01:32:22, Mark Mentovai wrote: > Naming: power_sources_list. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:119: CFArrayGetValueAtIndex(powerSourcesList, i)); On 2014/07/24 01:32:21, Mark Mentovai wrote: > if (!description) > continue; > > can go here, you don’t need to wait until line 125 after you’ve done > GetValueAsBoolean(). Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; On 2014/07/24 01:32:22, Mark Mentovai wrote: > In theory, there can be more than one battery. If you don’t handle that, you > should put a comment here. You may also want to remove the break without > handling multiple batteries fully. You can let the loop continue to process all > power sources so that if it finds more than one battery, it can DCHECK to alert > you (or some future person) that the code needs attention. that's right, I've added a comment. I am reluctant to put a DCHECK, because if Apple suddenly comes up with devices with multiple batteries chrome will start crashing (in debug), which sounds a bit extreme. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:142: // Need thread with UI-type message loop for notifications to run. On 2014/07/24 01:32:21, Mark Mentovai wrote: > If you added two more words here, it’d be a complete sentence, which would be > easier to understand. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:174: OnBatteryStatusChangedUI((void*)&callback_); On 2014/07/24 01:32:22, Mark Mentovai wrote: > Use cplusplus<style>(casts). See the style guide. Same on line 177. Done. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:179: if (notifier_run_loop_) { On 2014/07/24 01:32:22, Mark Mentovai wrote: > Do you need this conditional? If IOPSNotificationCreateRunLoopSource() failed, > we probably want to crash. If we’re actually seeing it fail in practice for some > reason, then you should say something about that in a comment. I think we should avoid crashing the browser, instead I've changed the code such that it returns the default values, which corresponds to "cannot provide battery data" in the specification.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, timvolodine wrote: > On 2014/07/24 01:32:22, Mark Mentovai wrote: > > Why did you choose CFIndex as the type for the CFNumbers? What’s > CFNumberGetType > > return for the numbers you’re interested in? > > usually it is kCFNumberSInt64Type, but can also be kCFNumberSInt32Type (e.g. > when "time to empty" is -1). For practical use the CFIndex (which is a signed > long) should be sufficient. long is 32 bits on 32-bit x86. You should probably extract this as kCFNumberSInt64Type, since that’s the largest type you’re seeing it stored as, and that type is always capable of holding values of kCFNumberSInt32Type. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; timvolodine wrote: > that's right, I've added a comment. I am reluctant to put a DCHECK, because if > Apple suddenly comes up with devices with multiple batteries chrome will start > crashing (in debug), which sounds a bit extreme. That’s exactly the point of making it crash: otherwise, chances are nobody would recognize that this code needs to be updated, and it would report incorrect and inappropriate results. The point of doing a DCHECK instead of a CHECK is that only developer builds would be affected, so Chrome won’t start crashing on end user systems.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, On 2014/07/24 16:08:52, Mark Mentovai wrote: > timvolodine wrote: > > On 2014/07/24 01:32:22, Mark Mentovai wrote: > > > Why did you choose CFIndex as the type for the CFNumbers? What’s > > CFNumberGetType > > > return for the numbers you’re interested in? > > > > usually it is kCFNumberSInt64Type, but can also be kCFNumberSInt32Type (e.g. > > when "time to empty" is -1). For practical use the CFIndex (which is a signed > > long) should be sufficient. > > long is 32 bits on 32-bit x86. You should probably extract this as > kCFNumberSInt64Type, since that’s the largest type you’re seeing it stored as, > and that type is always capable of holding values of kCFNumberSInt32Type. yes, my point was that the actual values are small and very unlikely to require a full 64 bit integer. For example capacity is between 0 and 100 and times are in minutes. I assume the conversion would simply fail if they cannot be converted to long. If you feel strongly about this I could introduce a SInt64 with the additional complexity of using Math64.h for all operations on them (to make it work on compilers that don't support long long). https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; On 2014/07/24 16:08:52, Mark Mentovai wrote: > timvolodine wrote: > > that's right, I've added a comment. I am reluctant to put a DCHECK, because if > > Apple suddenly comes up with devices with multiple batteries chrome will start > > crashing (in debug), which sounds a bit extreme. > > That’s exactly the point of making it crash: otherwise, chances are nobody would > recognize that this code needs to be updated, and it would report incorrect and > inappropriate results. > > The point of doing a DCHECK instead of a CHECK is that only developer builds > would be affected, so Chrome won’t start crashing on end user systems. the current specification does not strictly impose multiple battery support, it's a recommendation. probably not worth a DCHECK. We can add this feature later if necessary, I can add a reference to crbug in order to not forget this.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, timvolodine wrote: > yes, my point was that the actual values are small and very unlikely to require > a full 64 bit integer. For example capacity is between 0 and 100 and times are > in minutes. I assume the conversion would simply fail if they cannot be > converted to long. > > If you feel strongly about this I could introduce a SInt64 with the additional > complexity of using Math64.h for all operations on them (to make it work on > compilers that don't support long long). Are you serious? The API gives you a SInt64 already, and this is a platform-specific file used on a platform where we know we have a usable int64_t. So at least for the cases where you convert to double and divide one of these by another, there doesn’t seem to be any problem at all, and I don’t know why you wouldn’t just deal with then numbers that the system gives you. The web-side API doesn’t need to change at all, and the 64-bit quantities wouldn’t leave this file. SInt64 has the same range as CFIndex on x86_64 anyway, but a different range on 32-bit x86. I don’t know why you think this is something that should vary based on CPU architecture. I mean, if you really wanted to clamp the range to something narrower than SInt64, then you should be uniformly using that type (SInt32?) https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; timvolodine wrote: > the current specification does not strictly impose multiple battery support, > it's a recommendation. probably not worth a DCHECK. We can add this feature > later if necessary, I can add a reference to crbug in order to not forget this. This is an area where we know ahead of time that we haven’t accounted for something that doesn’t currently exist but may spring into existence in the future. It’s better to catch these things at runtime when the condition to trigger them begins existing than to have some bug logged in a database where everyone can forget about it. Please add the DCHECK now.
fixed comments, also added transport type check to skip potential attached UPS devices. https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, On 2014/07/24 17:03:46, Mark Mentovai wrote: > timvolodine wrote: > > yes, my point was that the actual values are small and very unlikely to > require > > a full 64 bit integer. For example capacity is between 0 and 100 and times are > > in minutes. I assume the conversion would simply fail if they cannot be > > converted to long. > > > > If you feel strongly about this I could introduce a SInt64 with the additional > > complexity of using Math64.h for all operations on them (to make it work on > > compilers that don't support long long). > > Are you serious? The API gives you a SInt64 already, and this is a > platform-specific file used on a platform where we know we have a usable > int64_t. So at least for the cases where you convert to double and divide one of > these by another, there doesn’t seem to be any problem at all, and I don’t know > why you wouldn’t just deal with then numbers that the system gives you. The > web-side API doesn’t need to change at all, and the 64-bit quantities wouldn’t > leave this file. > > SInt64 has the same range as CFIndex on x86_64 anyway, but a different range on > 32-bit x86. I don’t know why you think this is something that should vary based > on CPU architecture. I mean, if you really wanted to clamp the range to > something narrower than SInt64, then you should be uniformly using that type > (SInt32?) Ok, changed to SInt32. Technically casting a SInt64 to double can lead to loss in precision. Actually I was looking at MacTypes.h (https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h) where SInt64 is defined as a struct for compilers that don't support it natively. Not sure how conversion to double would work in that case. Generally speaking, do you know if we can assume that we have 64bit integer compiler support in chromium? https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:129: break; On 2014/07/24 17:03:46, Mark Mentovai wrote: > timvolodine wrote: > > the current specification does not strictly impose multiple battery support, > > it's a recommendation. probably not worth a DCHECK. We can add this feature > > later if necessary, I can add a reference to crbug in order to not forget > this. > > This is an area where we know ahead of time that we haven’t accounted for > something that doesn’t currently exist but may spring into existence in the > future. It’s better to catch these things at runtime when the condition to > trigger them begins existing than to have some bug logged in a database where > everyone can forget about it. > > Please add the DCHECK now. Done.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, timvolodine wrote: > On 2014/07/24 17:03:46, Mark Mentovai wrote: > > timvolodine wrote: > > > yes, my point was that the actual values are small and very unlikely to > > require > > > a full 64 bit integer. For example capacity is between 0 and 100 and times > are > > > in minutes. I assume the conversion would simply fail if they cannot be > > > converted to long. > > > > > > If you feel strongly about this I could introduce a SInt64 with the > additional > > > complexity of using Math64.h for all operations on them (to make it work on > > > compilers that don't support long long). > > > > Are you serious? The API gives you a SInt64 already, and this is a > > platform-specific file used on a platform where we know we have a usable > > int64_t. So at least for the cases where you convert to double and divide one > of > > these by another, there doesn’t seem to be any problem at all, and I don’t > know > > why you wouldn’t just deal with then numbers that the system gives you. The > > web-side API doesn’t need to change at all, and the 64-bit quantities wouldn’t > > leave this file. > > > > SInt64 has the same range as CFIndex on x86_64 anyway, but a different range > on > > 32-bit x86. I don’t know why you think this is something that should vary > based > > on CPU architecture. I mean, if you really wanted to clamp the range to > > something narrower than SInt64, then you should be uniformly using that type > > (SInt32?) > > Ok, changed to SInt32. > > Technically casting a SInt64 to double can lead to loss in precision. That’d be fine. In this case, a loss of precision at the range’s extremities is preferable to integer truncation to a completely unrelated quantity. > Actually I > was looking at MacTypes.h > (https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h) > where SInt64 is defined as a struct for compilers that don't support it > natively. Not sure how conversion to double would work in that case. Generally > speaking, do you know if we can assume that we have 64bit integer compiler > support in chromium? 100% sure. MacTypes.h has legacy stuff in it going back to the classic Mac OS and the 68k family of CPUs. Compiler support for 64-bit integers is always present in Mac OS X. If you haven’t noticed, we use uint64_t with reckless abandon in Chrome.
https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/40001/content/browser/battery_... content/browser/battery_status/battery_status_manager_mac.cc:27: static CFIndex GetValueAsCFIndex(CFDictionaryRef description, > 100% sure. > > MacTypes.h has legacy stuff in it going back to the classic Mac OS and the 68k > family of CPUs. Compiler support for 64-bit integers is always present in Mac OS > X. > > If you haven’t noticed, we use uint64_t with reckless abandon in Chrome. right, thanks for the clarification and sorry for confusion. I've uploaded an upgraded patch using SInt64.
https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:64: GetValueAsBoolean(description, CFSTR(kIOPSIsChargingKey), true); Why assume charging? https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:79: status.level = current_capacity / static_cast<double>(max_capacity); nit: this body requires braces since the condition is multiple lines https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:137: // FIXME: the case when there are multiple internal sources, e.g. when FIXME -> TODO(who) https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:225: base::ScopedCFTypeRef<CFRunLoopSourceRef> notifier_run_loop_; naming: a run_loop_ indicates an object of type CFRunLoopRef, I'd call this notifier_source_ or notifier_run_loop_source_.
https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:64: GetValueAsBoolean(description, CFSTR(kIOPSIsChargingKey), true); On 2014/07/25 14:48:44, rsesek wrote: > Why assume charging? kIOPSIsChargingKey is a required key so this should normally never happen, but I chose true to be in sync with the spec, which says that the default is charging=true. https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:79: status.level = current_capacity / static_cast<double>(max_capacity); On 2014/07/25 14:48:44, rsesek wrote: > nit: this body requires braces since the condition is multiple lines Done. https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:137: // FIXME: the case when there are multiple internal sources, e.g. when On 2014/07/25 14:48:44, rsesek wrote: > FIXME -> TODO(who) Done. https://codereview.chromium.org/398683006/diff/100001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:225: base::ScopedCFTypeRef<CFRunLoopSourceRef> notifier_run_loop_; On 2014/07/25 14:48:44, rsesek wrote: > naming: a run_loop_ indicates an object of type CFRunLoopRef, I'd call this > notifier_source_ or notifier_run_loop_source_. Done.
LGTM
On 2014/07/25 18:26:39, rsesek wrote: > LGTM thanks for the review! Mark, I assume you are ok with this patch?
https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:88: status.chargingTime = (charging_time != -1) nit: you don't need the parenthesis. https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:102: GetValueAsSInt64(description, CFSTR(kIOPSTimeToEmptyKey), -1); What about using this instead: https://developer.apple.com/library/mac/Documentation/IOKit/Reference/IOPower... It will do the logic of checking whether you are on battery, etc. Basically leaves the entire logic to the platform which seems better IMO.
LGTM
https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:88: status.chargingTime = (charging_time != -1) On 2014/07/25 22:14:51, Mounir Lamouri wrote: > nit: you don't need the parenthesis. Done. https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:102: GetValueAsSInt64(description, CFSTR(kIOPSTimeToEmptyKey), -1); On 2014/07/25 22:14:51, Mounir Lamouri wrote: > What about using this instead: > https://developer.apple.com/library/mac/Documentation/IOKit/Reference/IOPower... > > It will do the logic of checking whether you are on battery, etc. Basically > leaves the entire logic to the platform which seems better IMO. yes, IOPSGetTimeRemainingEstimate provides an aggregated view over all sources, something we could potentially use when implementing multiple batteries in a follow-up.
The CQ bit was checked by timvolodine@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/timvolodine@chromium.org/398683006/140001
https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:102: GetValueAsSInt64(description, CFSTR(kIOPSTimeToEmptyKey), -1); On 2014/07/30 17:40:52, timvolodine wrote: > On 2014/07/25 22:14:51, Mounir Lamouri wrote: > > What about using this instead: > > > https://developer.apple.com/library/mac/Documentation/IOKit/Reference/IOPower... > > > > It will do the logic of checking whether you are on battery, etc. Basically > > leaves the entire logic to the platform which seems better IMO. > > yes, IOPSGetTimeRemainingEstimate provides an aggregated view over all sources, > something we could potentially use when implementing multiple batteries in a > follow-up. Why not doing that change in this CL? It seems fairly simple and future-proof.
Message was sent while issue was closed.
Change committed as 286562
Message was sent while issue was closed.
https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... File content/browser/battery_status/battery_status_manager_mac.cc (right): https://codereview.chromium.org/398683006/diff/120001/content/browser/battery... content/browser/battery_status/battery_status_manager_mac.cc:102: GetValueAsSInt64(description, CFSTR(kIOPSTimeToEmptyKey), -1); On 2014/07/30 18:03:25, Mounir Lamouri wrote: > On 2014/07/30 17:40:52, timvolodine wrote: > > On 2014/07/25 22:14:51, Mounir Lamouri wrote: > > > What about using this instead: > > > > > > https://developer.apple.com/library/mac/Documentation/IOKit/Reference/IOPower... > > > > > > It will do the logic of checking whether you are on battery, etc. Basically > > > leaves the entire logic to the platform which seems better IMO. > > > > yes, IOPSGetTimeRemainingEstimate provides an aggregated view over all > sources, > > something we could potentially use when implementing multiple batteries in a > > follow-up. > > Why not doing that change in this CL? It seems fairly simple and future-proof. According to the documentation this function provides aggregated result over all sources, which would currently be inconsistent with the other fields we compute based on a single power source. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
