Replace Date().getTime() calls with SystemClock.uptimeMillis() or System.currentTimeMillis()

RESOLVED FIXED in Firefox 11

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

Trunk
Firefox 12
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 3 obsolete attachments)

1. Date().getTime() is inappropriate for measuring time durations because the "wall clock" can be changed by the user, the cell network, or DST. Google recommends using SystemClock.uptimeMillis() for measuring time durations.

2. new Date().getTime() is also inappropriate for getting the current system time_t because the caller must allocate a new Date object every call. System.currentTimeMillis() will return the same time value directly.
Attachment #588280 - Flags: review?(bugmail.mozilla)
Attachment #588280 - Flags: feedback?(doug.turner)
Fix AwesomeBarTabs.getSectionForTime() date range checks to properly handle exact midnight 00:00:00:0000.

Also remove some redundant range checks and else statements.
Attachment #588282 - Flags: review?(lucasr.at.mozilla)
Attachment #588282 - Flags: feedback?(doug.turner)
Comment on attachment 588280 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls.patch

Review of attachment 588280 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with changes

::: mobile/android/base/GeckoBatteryManager.java
@@ +109,5 @@
>          sRemainingTime = 0.0;
>        } else if (sLevel != previousLevel) {
>          // Estimate remaining time.
> +        if (sLastLevelChange != 0) {
> +          long currentTime = SystemClock.uptimeMillis();

I think for this one you should use SystemClock.elapsedRealtime() because it might span periods of device sleep.

@@ +132,5 @@
>  
>            sLastLevelChange = currentTime;
>          } else {
>            // That's the first time we got an update, we can't do anything.
> +          sLastLevelChange = SystemClock.uptimeMillis();

Ditto.
Attachment #588280 - Flags: review?(bugmail.mozilla) → review+
Attachment #588282 - Flags: review?(lucasr.at.mozilla) → review+
Changed GeckoBatteryManager calls to elapsedRealtime() and rebased patch.

r=kats
Attachment #588280 - Attachment is obsolete: true
Attachment #588280 - Flags: feedback?(doug.turner)
Attachment #588501 - Flags: review+
Rebased patch.

r=lucasr
Attachment #588282 - Attachment is obsolete: true
Attachment #588282 - Flags: feedback?(doug.turner)
Attachment #588502 - Flags: review+
Keywords: checkin-needed
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Keywords: checkin-needed
Rebased patch.

r=kats
Attachment #588501 - Attachment is obsolete: true
Comment on attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

r=kats
Attachment #589244 - Flags: review+
checkin-needed for rebased patches.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: Trivial reduction in memory usage.
Testing completed (on m-c, etc.): Merged to m-c yesterday.
Risk to taking this patch (and alternatives if risky): Unlikely date/time bugs in bookmarks.
Attachment #589244 - Flags: approval-mozilla-aurora?
Comment on attachment 588502 [details] [diff] [review]
bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch

[Approval Request Comment]
Regression caused by (bug #): None
User impact if declined: Bookmarks created at midnight will be miscategorized.
Testing completed (on m-c, etc.): Merged to m-c yesterday.
Risk to taking this patch (and alternatives if risky): Unlikely date/time bugs in bookmarks.
Attachment #588502 - Flags: approval-mozilla-aurora?
I pushed https://hg.mozilla.org/releases/mozilla-aurora/rev/9f53cb953f31 because it had a=dougt in the commit message in the landing queue. But I see now it didn't have approval.
Sorry! I think my patch's "a=dougt" message was leftover from m-c checkin issue, not approval-mozilla-aurora. I will check my other patches for premature a= messages.
Generally you don't need a= for m-c/m-i checkins, unless there's something like a tree closure.
Comment on attachment 588502 [details] [diff] [review]
bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #588502 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #589244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.