Closed
Bug 793418
Opened 12 years ago
Closed 12 years ago
OS X Battery Backend: Report remaining time when charging if the power source supports it
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: reuben, Assigned: reuben)
Details
Attachments
(1 file, 3 obsolete files)
3.21 KB,
patch
|
reuben
:
review+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #663751 -
Flags: review?(justin.lebar+bug)
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
Sure :)
Attachment #663751 -
Attachment is obsolete: true
Attachment #663751 -
Flags: review?(justin.lebar+bug)
Attachment #664074 -
Flags: review?(mounir)
Comment 4•12 years ago
|
||
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-
Assignee | ||
Comment 5•12 years ago
|
||
Nice catch!
Attachment #664074 -
Attachment is obsolete: true
Attachment #664252 -
Flags: review?(mounir)
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
Thanks for the review. Carrying r=mounir.
Attachment #664252 -
Attachment is obsolete: true
Attachment #664458 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 8•12 years ago
|
||
Reuben, do you have commit access level 1? If yes, did that patch went to try?
Assignee | ||
Comment 9•12 years ago
|
||
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.
Comment 10•12 years ago
|
||
You could use trychooser syntax and just run mac builds (both debug and opt) and mochitest-2.
Assignee | ||
Comment 11•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f24dcfb2c2ef
Comment 12•12 years ago
|
||
Comment on attachment 664458 [details] [diff] [review] Report time to charge when available https://hg.mozilla.org/integration/mozilla-inbound/rev/d40d6c7a82fe
Attachment #664458 -
Flags: checkin+
Updated•12 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla18
Comment 13•12 years ago
|
||
Thank you for this patch Reuben! :)
Comment 14•12 years ago
|
||
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
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d40d6c7a82fe
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•