Closed Bug 1154717 Opened 5 years ago Closed 5 years ago

Fix toLocalTimeISOString()

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- unaffected
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Fix toLocalTimeISOString() (obsolete) — Splinter Review
Bug 1149284 disabled test_TelemetrySession.js since end of May, which dropped under our radar and means we missed test-coverage for some changes.

This fixed toLocalTimeISOString() failures when re-enabling it.
Attachment #8592813 - Flags: review?(dteller)
Status: NEW → ASSIGNED
Blocks: 1154856
Comment on attachment 8592813 [details] [diff] [review]
Fix toLocalTimeISOString()

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +259,5 @@
>      + "T" + padNumber(date.getHours(), 2)
>      + ":" + padNumber(date.getMinutes(), 2)
>      + ":" + padNumber(date.getSeconds(), 2)
>      + "." + date.getMilliseconds()
> +    + sign(tzOffset) + padNumber(Math.abs(Math.floor(tzOffset / 60)), 2)

Are you sure it's `Math.abs(Math.floor(...))` and not `Math.floor(Math.abs(...))`?
Attachment #8592813 - Flags: review?(dteller)
Attachment #8592813 - Attachment is obsolete: true
Attachment #8593397 - Flags: review?(dteller)
Comment on attachment 8593397 [details] [diff] [review]
Fix toLocalTimeISOString()

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

Looks good to me. Any chance you could add a test?
Attachment #8593397 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/fe5714bb1a43
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8593397 [details] [diff] [review]
Fix toLocalTimeISOString()

Approved for Aurora. For approval request see bug 1139460 comment 42. For approval comments see bug 1139460 comment 43.
Attachment #8593397 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.