1313799 - Remove Date.prototype.toLocaleFormat uses in devtools/client/netmonitor/har

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[André Bargull [:anba]]

Date.prototype.toLocaleFormat is a non-standard API and we plan to warn when it's used (bug 1299900) and eventually want to remove it completely (bug 818634). As a first step we need to replace all Date.prototype.toLocaleFormat uses in Firefox with standardized APIs.

Date.prototype.toLocaleFormat is used in devtools/client/netmonitor/har/har-utils.js. Unfortunately har-utils.js uses a dynamic format string, so I had to write a custom strftime function to provide the different formatting options. 

The Intl object isn't supported on all platforms (bug 1215247), that means toLocaleFormat is still used for Android. Nevertheless let's try to switch to the standardized APIs for those platforms which provide Intl support.

Comment 1

[André Bargull [:anba]]

Attachment: devtools_client_network_har.patch

Comment 2

[André Bargull [:anba]]

Attachment: devtools_client_network_har.patch

Updated patch to cover more format specifiers and to improve compatibility with toLocaleFormat().

Comment 3

[Tim Nguyen :ntim]

As far as I know, har-utils.js only runs on Desktop (since it's a client file), I would only worry about compat with Mobile if the code was running on the actor side (which is not the case here).

It should be safe to Intl without any polyfill.

Comment 4

[André Bargull [:anba]]

Sounds great! I'll update the patch to remove the non-Intl fallback paths.

Comment 5

[Tim Nguyen :ntim]

Wait, since we only need to generate a file name with the date, I feel like reimplementing toLocaleString() is a bit over-engineered for that purpose, maybe, something a bit more simple could do the job ?

Comment 6

[André Bargull [:anba]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #5)
> Wait, since we only need to generate a file name with the date, I feel like
> reimplementing toLocaleString() is a bit over-engineered for that purpose,
> maybe, something a bit more simple could do the job ?

Sure, if it's okay to modify the "devtools.netmonitor.har.defaultFileName" preference [1] to remove the date format string part. 

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/devtools/client/netmonitor/har/har-exporter.js#76

Comment 7

[André Bargull [:anba]]

Attachment: bug1313799.patch

Updated patch to remove the Intl fallback code paths per comment #5.

Comment 8

[André Bargull [:anba]]

Any thoughts on comment #6?

Comment 9

[Tim Nguyen :ntim]

(In reply to André Bargull from comment #8)
> Any thoughts on comment #6?

Just asked Honza on IRC, and he was ok with only supporting the formatting string %date in the pref.

Where %date is date.toLocaleString/toString() (whatever you think is the most appropriate).

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Yes, agree with comment #9

(please ping me again as soon as I should review the patch)

Thanks!

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8817559 [details] [diff] [review]
bug1313799.patch

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

Just removing the review request from my queue for now.

Honza

Comment 12

[André Bargull [:anba]]

(In reply to Tim Nguyen :ntim from comment #9)
> Just asked Honza on IRC, and he was ok with only supporting the formatting
> string %date in the pref.

Great!

> Where %date is date.toLocaleString/toString() (whatever you think is the
> most appropriate).

I think it's best to use a custom formatter, because both, toString and toLocaleString, have some drawbacks when used in file names (e.g. when sorting the files lexicographically).

Comment 13

[André Bargull [:anba]]

Attachment: bug1313799.patch

New patch which removes toLocaleFormat() and only allows %date to output the current date/time.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8827856 [details] [diff] [review]
bug1313799.patch

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

Looks good to me, thanks for working on this!

R+m, assuming try is green.

Honza

Comment 15

[André Bargull [:anba]]

(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> Looks good to me, thanks for working on this!
> 
> R+m, assuming try is green.

Thanks for the review!

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2252edfd306556c076fe4a210929b2b0747dd7dd

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3f3764b1d5
Remove Date.prototype.toLocaleFormat uses in devtools/client/netmonitor/har. r=Honza

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc3f3764b1d5

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Will ride the train