700263 - Add remainingTime support to android backend

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

The computed remainingTime is far from perfect. There are two big issues:
1. It takes some time to compute the remaining time because we need to know the time between two values changes and the value changes happen every percent.
2. This is assuming battery charging and discharging time is linear which isn't true. We could improve that later by checking the battery type and change the estimated time given that.

Though, that should be very correct for a first version I believe.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Few changes.

Comment 2

[Mounir Lamouri (:mounir)]

There is actually another issue:
3. If Firefox is put in background, we stop listening to the battery events from Android for perf reasons. As a result, we are going to be very bad at computing the remaining time in those situations. We should probably find a solution in a follow-up.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 572473 [details] [diff] [review]
Patch v1.1

>diff --git a/embedding/android/GeckoBatteryManager.java
 b/embedding/android/GeckoBatteryManager.java

>+ + if (sLevel == 1.0) { + sRemainingTime = 0.0;

Same issue here as in the upower backend.

>+ } else if (sLevel != previousLevel) { + // Estimate remaining time.
>+ if (sLastLevelChange.getTime() != 0) { + Date currentTime = new
>Date(); + long dt = (currentTime.getTime() -
>sLastLevelChange.getTime()) / 1000; + + if (sCharging) { + if
>(previousLevel > sLevel) { + Log.e("GeckoBatteryManager", "When
>charging, level should increase!");

This could maybe happen if there's something really sucking power, and
the phone is on a low-wattage charger like an unpowered USB hub.  It's
definitely not an error conditon; maybe info or warning.  Falling back
on "unknown remaining time" sounds fine.

>+ sRemainingTime = kUnknownRemainingTime; + } else { + sRemainingTime
>= Math.round(dt / (sLevel - previousLevel) * (1.0 - sLevel));

Nit: this would be easier for me to read if you had a |dq| variable
defined as |sLevel - previousLevel|.  Then the branch above would be,
|if (dq < 0)|.

>+ } + } else { + if (previousLevel < sLevel) { +
>Log.e("GeckoBatteryManager", "When discharging, level should
>decrease!");

I don't think this should happen, but there may be some exotic cases
like unplugging a charger after which a buffering capacitor discharges
and briefly increasing the battery level.  Again, I would just info or
warn.

>+ sRemainingTime = kUnknownRemainingTime; + } else { + sRemainingTime
>= Math.round(dt / (previousLevel - sLevel) * (sLevel));

(Same thing here, would prefer |-dq|.)

We can get arbitrarily fancy with estimating remaining time, but this
implementation seems OK.  Easy to change in a followup.

r=me except for question about what's supposed to happen when level ==
1.0.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Comment on attachment 572473 [details] [diff] [review] [diff] [details] [review]
> Patch v1.1
> 
> >diff --git a/embedding/android/GeckoBatteryManager.java
>  b/embedding/android/GeckoBatteryManager.java
> 
> >+ + if (sLevel == 1.0) { + sRemainingTime = 0.0;
> 
> Same issue here as in the upower backend.

Same explanation as bug 700262, this is per spec.

> >+ } else if (sLevel != previousLevel) { + // Estimate remaining time.
> >+ if (sLastLevelChange.getTime() != 0) { + Date currentTime = new
> >Date(); + long dt = (currentTime.getTime() -
> >sLastLevelChange.getTime()) / 1000; + + if (sCharging) { + if
> >(previousLevel > sLevel) { + Log.e("GeckoBatteryManager", "When
> >charging, level should increase!");
> 
> This could maybe happen if there's something really sucking power, and
> the phone is on a low-wattage charger like an unpowered USB hub.  It's
> definitely not an error conditon; maybe info or warning.  Falling back
> on "unknown remaining time" sounds fine.

I would except the backend to tell us the battery is discharging, not charging. Though, it might not be the case, thus a warning might be better than an error.

> >+ sRemainingTime = kUnknownRemainingTime; + } else { + sRemainingTime
> >= Math.round(dt / (sLevel - previousLevel) * (1.0 - sLevel));
> 
> Nit: this would be easier for me to read if you had a |dq| variable
> defined as |sLevel - previousLevel|.  Then the branch above would be,
> |if (dq < 0)|.
>
> >+ sRemainingTime = kUnknownRemainingTime; + } else { + sRemainingTime
> >= Math.round(dt / (previousLevel - sLevel) * (sLevel));
> 
> (Same thing here, would prefer |-dq|.)

I'm using dLevel in both expressions. Not really what you asked but close enough I guess.

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.2

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 572754 [details] [diff] [review]
Patch v1.2

r=me with level == 1.0 change discussed on IRC.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Having dLevel be |cur - prev| is confusing, because it's a difference.  You would want to call it absDLevel or something.  I would prefer the previous impl to this one, but an impl where dLevel is cur-prev most of all.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 572754 [details] [diff] [review]
Patch v1.2

This has just landed in mozilla-inbound. Requesting aurora approval for API completeness in Firefox 10. It would be better to ship Firefox 10 with the entire API if we say it is present. The risk is very low given that there is no use of the API for the moment.

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/49278c7d9213

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 572754 [details] [diff] [review]
Patch v1.2

It looks like this feature wasn't ready for 10. Please re-nominate if you have a good justification for trying to get this into 10 rather than just waiting for 11.