Last Comment Bug 700263 - Add remainingTime support to android backend
: Add remainingTime support to android backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 700261
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-07 04:16 PST by Mounir Lamouri (:mounir)
Modified: 2011-11-15 14:47 PST (History)
5 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (11.67 KB, patch)
2011-11-07 04:16 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (11.89 KB, patch)
2011-11-07 07:59 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.2 (11.92 KB, patch)
2011-11-08 01:14 PST, Mounir Lamouri (:mounir)
cjones.bugs: review+
asa: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-11-07 04:16:48 PST
Created attachment 572430 [details] [diff] [review]
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) 2011-11-07 07:59:06 PST
Created attachment 572473 [details] [diff] [review]
Patch v1.1

Few changes.
Comment 2 Mounir Lamouri (:mounir) 2011-11-07 08:29:29 PST
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 2011-11-07 20:43:31 PST
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) 2011-11-08 01:13:20 PST
(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) 2011-11-08 01:14:02 PST
Created attachment 572754 [details] [diff] [review]
Patch v1.2
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-08 10:48:27 PST
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 2011-11-08 10:51:06 PST
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) 2011-11-09 06:23:16 PST
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] 2011-11-10 03:11:12 PST
https://hg.mozilla.org/mozilla-central/rev/49278c7d9213
Comment 10 Asa Dotzler [:asa] 2011-11-15 14:47:57 PST
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.

Note You need to log in before you can comment on or make changes to this bug.