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)
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•13 years ago
|
||
Attachment #663751 -
Flags: review?(justin.lebar+bug)
Comment 2•13 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•13 years ago
|
||
Sure :)
Attachment #663751 -
Attachment is obsolete: true
Attachment #663751 -
Flags: review?(justin.lebar+bug)
Attachment #664074 -
Flags: review?(mounir)
Comment 4•13 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•13 years ago
|
||
Nice catch!
Attachment #664074 -
Attachment is obsolete: true
Attachment #664252 -
Flags: review?(mounir)
Comment 6•13 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•13 years ago
|
||
Thanks for the review. Carrying r=mounir.
Attachment #664252 -
Attachment is obsolete: true
Attachment #664458 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 8•13 years ago
|
||
Reuben, do you have commit access level 1? If yes, did that patch went to try?
Assignee | ||
Comment 9•13 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•13 years ago
|
||
You could use trychooser syntax and just run mac builds (both debug and opt) and mochitest-2.
Assignee | ||
Comment 11•13 years ago
|
||
Comment 12•13 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•13 years ago
|
Keywords: checkin-needed
Target Milestone: --- → mozilla18
Comment 13•13 years ago
|
||
Thank you for this patch Reuben! :)
Comment 14•13 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•13 years ago
|
||
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.
Description
•