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

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

Trunk
mozilla18
All
Mac OS X
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 663751 [details] [diff] [review]
Report time to charge when available
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.
(Assignee)

Comment 3

6 years ago
Created attachment 664074 [details] [diff] [review]
Report time to charge when available

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-
(Assignee)

Comment 5

6 years ago
Created attachment 664252 [details] [diff] [review]
Report time to charge when available

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+
(Assignee)

Comment 7

6 years ago
Created attachment 664458 [details] [diff] [review]
Report time to charge when available

Thanks for the review. Carrying r=mounir.
Attachment #664252 - Attachment is obsolete: true
Attachment #664458 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Reuben, do you have commit access level 1? If yes, did that patch went to try?
(Assignee)

Comment 9

6 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.
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! :)

Comment 14

6 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
https://hg.mozilla.org/mozilla-central/rev/d40d6c7a82fe
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.