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

Status: RESOLVED FIXED

Description

[Reuben Morais [:reuben]]

Comment 1

[Reuben Morais [:reuben]]

Created attachment 663751 [details] [diff] [review]
Report time to charge when available

Comment 2

[Mounir Lamouri (:mounir)]

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.

Comment 3

[Reuben Morais [:reuben]]

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

Sure :)

Comment 4

[Mounir Lamouri (: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.

Comment 5

[Reuben Morais [:reuben]]

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

Nice catch!

Comment 6

[Mounir Lamouri (: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.

Comment 7

[Reuben Morais [:reuben]]

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

Thanks for the review. Carrying r=mounir.

Comment 8

[Mounir Lamouri (:mounir)]

Reuben, do you have commit access level 1? If yes, did that patch went to try?

Comment 9

[Reuben Morais [:reuben]]

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

[Mounir Lamouri (:mounir)]

You could use trychooser syntax and just run mac builds (both debug and opt) and mochitest-2.

Comment 11

[Reuben Morais [:reuben]]

https://tbpl.mozilla.org/?tree=Try&rev=f24dcfb2c2ef

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 664458 [details] [diff] [review]
Report time to charge when available

https://hg.mozilla.org/integration/mozilla-inbound/rev/d40d6c7a82fe

Comment 13

[Mounir Lamouri (:mounir)]

Thank you for this patch Reuben! :)

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-f24dcfb2c2ef

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d40d6c7a82fe