Last Comment Bug 717841 - Replace Date().getTime() calls with SystemClock.uptimeMillis() or System.currentTimeMillis()
: Replace Date().getTime() calls with SystemClock.uptimeMillis() or System.curr...
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 12
Assigned To: Chris Peterson [:cpeterson]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-12 18:52 PST by Chris Peterson [:cpeterson]
Modified: 2012-02-01 13:01 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
bug-717841-part-1-replace-Date-getTime-calls.patch (19.22 KB, patch)
2012-01-12 18:55 PST, Chris Peterson [:cpeterson]
bugmail: review+
Details | Diff | Splinter Review
bug-717841-part-2-fix-HistoryQuery-day-checks.patch (1.23 KB, patch)
2012-01-12 18:59 PST, Chris Peterson [:cpeterson]
lucasr.at.mozilla: review+
Details | Diff | Splinter Review
bug-717841-part-1-replace-Date-getTime-calls-v2.patch (19.31 KB, patch)
2012-01-13 12:58 PST, Chris Peterson [:cpeterson]
cpeterson: review+
Details | Diff | Splinter Review
bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch (1.23 KB, patch)
2012-01-13 13:00 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch (19.31 KB, patch)
2012-01-17 11:26 PST, Chris Peterson [:cpeterson]
cpeterson: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-01-12 18:52:31 PST
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.
Comment 1 Chris Peterson [:cpeterson] 2012-01-12 18:55:10 PST
Created attachment 588280 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls.patch
Comment 2 Chris Peterson [:cpeterson] 2012-01-12 18:59:03 PST
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.
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-12 19:35:59 PST
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.
Comment 4 Chris Peterson [:cpeterson] 2012-01-13 12:58:52 PST
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
Comment 5 Chris Peterson [:cpeterson] 2012-01-13 13:00:11 PST
Created attachment 588502 [details] [diff] [review]
bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch

Rebased patch.

r=lucasr
Comment 6 Chris Peterson [:cpeterson] 2012-01-17 11:26:40 PST
Created attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

Rebased patch.

r=kats
Comment 7 Chris Peterson [:cpeterson] 2012-01-17 11:27:18 PST
Comment on attachment 589244 [details] [diff] [review]
bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

r=kats
Comment 8 Chris Peterson [:cpeterson] 2012-01-17 11:34:41 PST
checkin-needed for rebased patches.
Comment 11 Chris Peterson [:cpeterson] 2012-01-19 13:35:40 PST
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.
Comment 12 Chris Peterson [:cpeterson] 2012-01-19 13:36:55 PST
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.
Comment 13 Brad Lassey [:blassey] (use needinfo?) 2012-01-20 08:52:05 PST
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.
Comment 14 Chris Peterson [:cpeterson] 2012-01-20 10:10:01 PST
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.
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-20 10:24:38 PST
Generally you don't need a= for m-c/m-i checkins, unless there's something like a tree closure.
Comment 16 Alex Keybl [:akeybl] 2012-01-20 13:00:55 PST
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.
Comment 17 Brad Lassey [:blassey] (use needinfo?) 2012-01-20 13:49:24 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f71567515b5

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