Closed Bug 793418 Opened 13 years ago Closed 13 years ago

OS X Battery Backend: Report remaining time when charging if the power source supports it

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: reuben, Assigned: reuben)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attachment #663751 - Flags: review?(justin.lebar+bug)
Comment on attachment 663751 [details] [diff] [review] Report time to charge when available Review of attachment 663751 [details] [diff] [review]: ----------------------------------------------------------------- Could you move the remaining time check you do earlier in the loop after |charging| so there, you will check what has to be checked instead of checking for the discharging part and then re-checking if we are actually charging.
Sure :)
Attachment #663751 - Attachment is obsolete: true
Attachment #663751 - Flags: review?(justin.lebar+bug)
Attachment #664074 - Flags: review?(mounir)
Comment on attachment 664074 [details] [diff] [review] Report time to charge when available Review of attachment 664074 [details] [diff] [review]: ----------------------------------------------------------------- So, if we are charging but we were not able to get the remaining time value, we might have remainingTime = kDefaultRemaningTime. We should actually have remainingTime = kUnknownRemainingTime unless we are fully charged. Maybe you could add this: if (level != 1.0) { // Default value that will be changed if we happen to find the actual remaining time. remainingTime = kUnknownRemainingTime; } after |if (charging) {|. The patch looks good otherwise but I would prefer to see this change before r+'ing.
Attachment #664074 - Flags: review?(mounir) → review-
Nice catch!
Attachment #664074 - Attachment is obsolete: true
Attachment #664252 - Flags: review?(mounir)
Comment on attachment 664252 [details] [diff] [review] Report time to charge when available Review of attachment 664252 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the small change below. ::: hal/cocoa/CocoaBattery.cpp @@ +260,5 @@ > + if (charging) { > + if (level != 1.0) { > + // Default value that will be changed if we happen to find the actual > + // remaining time. > + remainingTime = kUnknownRemainingTime; nit: Could you actually change this to: remainingTime = level == 1.0 ? kDefaultRemainingTime : kUnknownRemainingTime; Better to be explicit that when level == 1.0, we actually want the default value to be kDefaultRemainingTime.
Attachment #664252 - Flags: review?(mounir) → review+
Thanks for the review. Carrying r=mounir.
Attachment #664252 - Attachment is obsolete: true
Attachment #664458 - Flags: review+
Keywords: checkin-needed
Reuben, do you have commit access level 1? If yes, did that patch went to try?
Yes, no. I ran mochitest-2 locally and things were fine, though. I was under the impression that I should avoid sending low risk patches to try since it's already under heavy load.
You could use trychooser syntax and just run mac builds (both debug and opt) and mochitest-2.
Keywords: checkin-needed
Target Milestone: --- → mozilla18
Thank you for this patch Reuben! :)
Try run for f24dcfb2c2ef is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f24dcfb2c2ef Results (out of 8 total builds): success: 8 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/reuben.morais@gmail.com-f24dcfb2c2ef
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: