Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add remainingTime support to android backend

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Attachment #572430 - Flags: review?(jones.chris.g)
(Assignee)

Comment 1

6 years ago
Created attachment 572473 [details] [diff] [review]
Patch v1.1

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

6 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

6 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

6 years ago
Created attachment 572754 [details] [diff] [review]
Patch v1.2
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

6 years ago
Flags: in-testsuite?
Whiteboard: [needs review]
(Assignee)

Comment 8

6 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?
https://hg.mozilla.org/mozilla-central/rev/49278c7d9213
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Comment 10

6 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-
You need to log in before you can comment on or make changes to this bug.