The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
5 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)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 588280 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls.patch
Attachment #588280 - Flags: review?(bugmail.mozilla)
Attachment #588280 - Flags: feedback?(doug.turner)
(Assignee)

Comment 2

5 years ago
Created attachment 588282 [details] [diff] [review]
bug-717841-part-2-fix-HistoryQuery-day-checks.patch

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+

Updated

5 years ago
Attachment #588282 - Flags: review?(lucasr.at.mozilla) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 588501 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2.patch

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+
(Assignee)

Comment 5

5 years ago
Created attachment 588502 [details] [diff] [review]
bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch

Rebased patch.

r=lucasr
Attachment #588282 - Attachment is obsolete: true
Attachment #588282 - Flags: feedback?(doug.turner)
Attachment #588502 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 6

5 years ago
Created attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

Rebased patch.

r=kats
Attachment #588501 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Comment on attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

r=kats
Attachment #589244 - Flags: review+
(Assignee)

Comment 8

5 years ago
checkin-needed for rebased patches.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/46328da0936d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e312a306ed01
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/46328da0936d
https://hg.mozilla.org/mozilla-central/rev/e312a306ed01
Target Milestone: --- → Firefox 12

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
tracking-fennec: --- → ?
status-firefox12: --- → fixed
(Assignee)

Comment 11

5 years ago
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?
(Assignee)

Comment 12

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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+

Updated

5 years ago
Attachment #589244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f71567515b5
tracking-fennec: ? → 11+
status-firefox11: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.