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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

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]]

Attachment: bug-717841-part-1-replace-Date-getTime-calls.patch

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: 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:[e-mail])]

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]]

Attachment: 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]]

Attachment: bug-717841-part-2-fix-HistoryQuery-day-checks-v1-rebased.patch

Rebased patch.

r=lucasr

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: bug-717841-part-1-replace-Date-getTime-calls-v2-rebased.patch

Rebased patch.

r=kats

Comment 7

[Chris Peterson [:cpeterson]]

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]]

checkin-needed for rebased patches.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46328da0936d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e312a306ed01

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/46328da0936d
https://hg.mozilla.org/mozilla-central/rev/e312a306ed01

Comment 11

[Chris Peterson [:cpeterson]]

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]]

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?)]

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]]

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:[e-mail])]

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]]

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?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f71567515b5