Closed
Bug 700263
Opened 13 years ago
Closed 13 years ago
Add remainingTime support to android backend
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(1 file, 2 obsolete files)
11.92 KB,
patch
|
cjones
:
review+
asa
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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.
Attachment #572430 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 1•13 years ago
|
||
Few changes.
Attachment #572430 -
Attachment is obsolete: true
Attachment #572430 -
Flags: review?(jones.chris.g)
Attachment #572473 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 2•13 years ago
|
||
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 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.
Attachment #572473 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #572473 -
Attachment is obsolete: true
Attachment #572754 -
Flags: review?(jones.chris.g)
Comment on attachment 572754 [details] [diff] [review] Patch v1.2 r=me with level == 1.0 change discussed on IRC.
Attachment #572754 -
Flags: review?(jones.chris.g) → review+
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.
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite?
Whiteboard: [needs review]
Assignee | ||
Comment 8•13 years ago
|
||
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.
Attachment #572754 -
Flags: approval-mozilla-aurora?
Comment 9•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/49278c7d9213
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 10•13 years ago
|
||
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.
Attachment #572754 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•