Perfherder uses toLocaleFormat() which has been removed in Firefox 58

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jmaher, Assigned: wlach)

Tracking

Details

Attachments

(1 attachment)

for example, go here:
https://treeherder.mozilla.org/perf.html#/alerts?id=10314

select the improvement and there are two options "copy summary" and "unlink from bug"- choose "copy summary" and you will see this in the firefox console:
Error: G.toLocaleFormat is not a function
[601]/</</q.prototype.getTextualSummary@https://treeherder.mozilla.org/perf.765571126be0e2904a57.bundle.js:48:67194
[577]/</</A.copyTextToClipboard@https://treeherder.mozilla.org/perf.765571126be0e2904a57.bundle.js:48:7278
fn@https://treeherder.mozilla.org/vendor.51f52799a614eccb6b35.bundle.js line 41 > Function:4:362
Qc@https://treeherder.mozilla.org/vendor.51f52799a614eccb6b35.bundle.js:41:436577
Reporter

Comment 1

2 years ago
oddly the only index I see via github search is:
https://github.com/mozilla/treeherder/blob/7233dfd46f63c8d478c1ddbdebe608023d3d32e5/ui/js/models/perf/alerts.js#L96

but github search was last updated october 3rd, possibly this is out of date.
The toLocaleFormat() call has existed in Perfherder unchanged since it landed in bug 1285368 (July 2016), and there is nothing else of note in the git log of that file recently - so this doesn't appear to be a regression on our side.

The MDN page for it:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleFormat

...led me to bug 818634 which just landed, and has removed toLocaleFormat() in Nightly (Firefox 58).

I'm presuming Perfherder should be using something other than this function to format the dates.

Rob, I don't suppose you'd mind taking a look? :-)
Blocks: 818634
Flags: needinfo?(rwood)
Priority: -- → P1
Summary: perfherder has a regression where we are unable to copy the summary of performance alerts → Perfherder uses toLocaleFormat() which has been removed in Firefox 58
Oh it appears toLocaleFormat() was Firefox only - so I guess the fix for this will also make this feature work for the first time in other browsers too :-)
if your goal is for the user to read and understand the date, use Intl.DateTimeFormat (or date.toLocaleString which is the same). If you need a parsable/computer-readable date, format ISO date with `{$day}-${month}-${year}` etc.
Comment on attachment 8925078 [details] [review]
[treeherder] wlach:1414312 > mozilla:master

Sorry, I missed the conversation here while fixing the problem. :)

I think the short-term fix is to just use toUTCString which is good enough for our purposes and seems to be standard. The purpose of the textual summary is to provide a standard template inside bugzilla reports that is readable by audience who understands english and is aware of utc. More advanced localization stuff is not required.
Flags: needinfo?(rwood)
Attachment #8925078 - Flags: review?(jmaher)
Reporter

Comment 7

2 years ago
Comment on attachment 8925078 [details] [review]
[treeherder] wlach:1414312 > mozilla:master

thanks for the fix
Attachment #8925078 - Flags: review?(jmaher) → review+
Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/117bfd620142e34f12093eb3ae81bff483401224
Bug 1414312 - Fix perfherder textual alert summary copy function on recent Firefoxen (#2910)

It used toLocaleFormat, which was never standard and seems to no longer be defined
in recent Firefox versions. Use toUTCString instead, which seems to be available
everywhere.
I think we're ok here now.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Assignee: nobody → wlachance
You need to log in before you can comment on or make changes to this bug.