1313659 - Replace nsIScriptableDateFormat in editor/ and chat/

Status: RESOLVED FIXED

(MailNews Core :: Internationalization, defect)

Description

[Jonathan Kew [:jfkthame]]

We'd like to remove nsIScriptableDateFormat (and its supporting platform-specific code) from mozilla-central; see bug 1313625.

There are a bunch of callers in comm-central, but I suspect they could all be fairly easily converted to use Intl.DateTimeFormat, either directly or through the extended options to Date.toLocaleString() etc.

(See also bug 1301655, which is doing this conversion for the Firefox front-end code in m-c.)

Comment 1

[Wayne Mery (:wsmwk)]

any impact to calendar?

Comment 2

[Stefan Sitter [:ssitter]]

Bug 1229684 was filed last year to replace nsIScriptableDateFormat in Lightning.

Comment 3

[[:MakeMyDay]]

Thanks for the heads up - is nsIScriptableDateFormat still intended to be removed for ESR52 already or 53+?

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to [:MakeMyDay] from comment #3)
> Thanks for the heads up - is nsIScriptableDateFormat still intended to be
> removed for ESR52 already or 53+?

No, I don't expect it will be removed from 52. First, I'd like to issue a warning that it is deprecated, and allow some time for people to convert callsites, so I expect it'll take at least a couple of additional release cycles before we're ready to actually kill it.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: editor, v3

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat/im, v3

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8846658 [details] [diff] [review]
editor, v3

I'll take this review, Magnus has too many. I suggest to use toLocaleString(), same comment as in bug 1346549, but let's wait for bug 1332351.

Comment 8

[Jorg K (CEST = GMT+2)]

Bug 1332351 has just landed, so this can go ahead now.

Comment 9

[:aceman]

Comment on attachment 8846658 [details] [diff] [review]
editor, v3

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

::: editor/ui/dialogs/content/EdPageProps.js
@@ +49,5 @@
>      // Convert modified string to date (0 = unknown date or January 1, 1970 GMT)
>      if(Date.parse(lastmod))
>      {
>        try {
> +        const dateTimeFormatter = new Intl.DateTimeFormat(undefined, {

We probably do not need the Intl. version here as it is not in a loop.

@@ +51,5 @@
>      {
>        try {
> +        const dateTimeFormatter = new Intl.DateTimeFormat(undefined, {
> +          year: 'numeric', month: 'long', day: 'numeric',
> +          hour: 'numeric', minute: 'numeric'

Why 'numeric' for hour and minute? I'd think minutes:'2-digit' is a must-have when also displaying hours and seconds (you wouldn't like something like 1:2:3 displayed for time, but 1:02:03). The display for 'hour' could be argued about.
Also I'd specify second: '2-digit' too here, if the original dateService.timeFormatSeconds also contained them.

Comment 10

[:aceman]

(In reply to :aceman from comment #9)
> Why 'numeric' for hour and minute? I'd think minutes:'2-digit' is a
> must-have when also displaying hours and seconds (you wouldn't like
> something like 1:2:3 displayed for time, but 1:02:03). The display for
> 'hour' could be argued about.

OK, I was mistaken here. It seems the formatter is clever enough to see that if hours and minutes are shown, minutes and seconds are forced to 2-digit display regardless if you set "numeric" or "2-digit". But it should do no harm to specify 2-digit for them explicitly.

It seems new Date().toLocaleTimeString(undefined, { second: "2-digit"}) produces seconds that are not always 2-digit, so I'm not sure what the specification is.

Comment 11

[Jorg K (CEST = GMT+2)]

I'm happy for Aceman to do the review here, I just wanted to lift some burden off Magnus' shoulders. Aceman did a great job in bug 1332351.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8846660 [details] [diff] [review]
chat/im, v3

Unless these calls are run in a loop, I think you can lose the
  new Intl.DateTimeFormat(undefined, ...
as was previously commented.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8846658 [details] [diff] [review]
editor, v3

Clearing this review for now while waiting for an updated patch. Sorry, first stole it from Magnus, now clearing it ;-(

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: editor, v5

(In reply to :aceman from comment #10)
> OK, I was mistaken here. It seems the formatter is clever enough to see that
> if hours and minutes are shown, minutes and seconds are forced to 2-digit
> display regardless if you set "numeric" or "2-digit". But it should do no
> harm to specify 2-digit for them explicitly.
> 
> It seems new Date().toLocaleTimeString(undefined, { second: "2-digit"})
> produces seconds that are not always 2-digit, so I'm not sure what the
> specification is.

>Services.intl.createDateTimeFormat(undefined, {hour: "2-digit", minute: "2-digit"}).format(new Date(2017,3,7,5,9))
returns "5:09 AM" here, so my impression is that it ignores the formats for time parts at the moment and I don't want to specify formats which don't work as expected.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: chat/im, v5

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #14)
> >Services.intl.createDateTimeFormat(undefined, {hour: "2-digit", minute: "2-digit"}).format(new Date(2017,3,7,5,9))
> returns "5:09 AM" here, so my impression is that it ignores the formats for
> time parts at the moment and I don't want to specify formats which don't
> work as expected.
Do you know the work of Zbigniew Braniecki? He's been reworking Intl. For date/time formatting that obeys the OS setting of the user, he created mozIMozIntl.idl. So those functions should be used where date/times show in chrome. He's just converted the UI of FF in bug 1354445: https://hg.mozilla.org/mozilla-central/rev/79673e659b31.
Hmm, if I see it correctly, Services.intl.createDateTimeFormat() should *already* use mozIMozIntl, right?
You're on Windows? Try setting your "format locale" (not the system locale) to en-US or some other en-* locale and then select a format. When mozIMozIntl is used, that format should appear in the UI.

Comment 17

[:aceman]

Comment on attachment 8857969 [details] [diff] [review]
editor, v5

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

::: editor/ui/dialogs/content/EdPageProps.js
@@ +51,5 @@
>      {
>        try {
> +        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +          year: 'numeric', month: 'long', day: 'numeric',
> +          hour: 'numeric', minute: 'numeric'

This probably isn't used in TB, but I wonder if the "last page modification" date shouldn't be stored in the document in some fixed format mandated by spec, not the user's locale. But I do not see where gDialog.PageModDate.value is used later on so maybe this isn't even used right now.

Comment 18

[Jorg K (CEST = GMT+2)]

Why not simply:
dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
         dateStyle: "long",
         timeStyle: "medium" // gives you seconds.

Comment 19

[:aceman]

Another API for the date formatting? OK, so this one should be used for the app UI as it takes some OS settings into account? And the Date().toLocaleString() should only be used by webpages that can ignore OS settings?

Does it already take OS settings into account or does it need to wait for your your C++ bug 1351427 ?

Comment 20

[Jorg K (CEST = GMT+2)]

Intl.DateTimeFormat, that is .toLocaleString(), is a JS thing in-built thing (ECMA402). But where this stuff shows in chrome, like in the case of the patch, it should follow mozIntl.DateTimeFormat. That wasn't available some weeks ago. It's extensively used in attachment 8857963 [details] [diff] [review].

So I suggest not to land the patch, but to follow my suggestion from comment #18 to bring it inline with other chrome dates (bug 1354445: https://hg.mozilla.org/mozilla-central/rev/79673e659b31), see bug 1356403 for TB.

I'm really sorry, this whole area has been in a state of flux. But FF now uses mozIntl.DateTimeFormat everywhere and with bug 1351427 finally landed, all JS and C++ dates will follow the same scheme.

Further reading:
https://mail.mozilla.org/pipermail/firefox-dev/2017-April/005293.html, quotes:
You can think of it as a regular ECMA402 DateTimeFormat on steroids.

let dtf = mozIntl.createDateTimeFormat(undefined, {
  dateStyle: 'long', // full | long | medium | short
  timeStyle: 'medium // full | long | medium | short
});
dtf.format(now);

Please, use the new API for all new code and when possible, migrate old
code to use it.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Jorg K (GMT+2) from comment #16)
> Do you know the work of Zbigniew Braniecki? He's been reworking Intl.
Yes, I know-
> For date/time formatting that obeys the OS setting of the user, he created
> mozIMozIntl.idl. So those functions should be used where date/times show in
> chrome. He's just converted the UI of FF in bug 1354445:
> https://hg.mozilla.org/mozilla-central/rev/79673e659b31.
> Hmm, if I see it correctly, Services.intl.createDateTimeFormat() should
> *already* use mozIMozIntl, right?
Yes.

(In reply to :aceman from comment #17)
> This probably isn't used in TB, but I wonder if the "last page modification"
> date shouldn't be stored in the document in some fixed format mandated by
> spec, not the user's locale. But I do not see where
> gDialog.PageModDate.value is used later on so maybe this isn't even used
> right now.
According to the source, it's just a formatted datetime display of the last page modification and doesn't get stored like that: https://dxr.mozilla.org/comm-central/search?q=PageModDate&redirect=false

(In reply to Jorg K (GMT+2) from comment #18)
> Why not simply:
> dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
>          dateStyle: "long",
>          timeStyle: "medium" // gives you seconds.
Not all of these do at the moment what they are expected to do, e.g. dateStyle "short" uses 4 digits for the year and "long" and "full" are the same: both show the weekday, but only "full" is supposed to do that: http://www.unicode.org/cldr/charts/30/summary/en.html#1940 We can switch to this format when it properly supports these cldr variables.

Comment 22

[Jorg K (CEST = GMT+2)]

There is a misunderstanding here. mozIntl.DateTimeFormat follows the OS settings. So if you ask for "short", the default on Windows for en-US still has yyyy. Long on Windows does show the weekday by default. The user can of course adjust that and all short dates in the application will follow. So my suggestion as stated in comment #18 still stands. The same goes for bug 1346549. I even filed bug 1356403 to undo the "damage" we did in bug 1332351.

BTW, M-C are moving to remove nsIScriptableDateFormat, so we need to move here unless you want me to land this as a bustage fix ;-)

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8857971 [details] [diff] [review]
chat/im, v5

All these dates look like they show in chrome, so I'd use
Services.intl.createDateTimeFormat(undefined,
                             { dateStyle: "long",
                               timeStyle: "short" });
everywhere.

Zibi, would you mind to advise us here. Can you please comment on both patches.

Comment 24

[Zibi Braniecki [:zbraniecki][:gandalf]]

If you're in chrome, you should use mozIntl version as it'll give you the OS Regional Preferences data sync.

Comment 25

[:aceman]

Comment on attachment 8857969 [details] [diff] [review]
editor, v5

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

Aryx, can you please change it to what Jorg proposes?
 Services.intl.createDateTimeFormat(undefined, { dateStyle: "long", timeStyle: "medium" }); seems to produce the time with seconds for me on Linux.

Comment 26

[Patrick Cloke [:clokep]]

Aryx, I'm a bit confused by this bug -- I was requested review a while ago, but there's been a lot of discussion since then. Is there something I need to review or are there changes that need to be made to the chat patch? Thanks!

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-editor.patch (JK v1)

Another friendly take-over addressing review issues.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-editor.patch (JK v1) [landed in comment #30]

Corrected bug number in checkin comment.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-chat.patch (JK v1).

This should be the final version ready for review.

Basically, if we format full dates/times, we just use Services.intl.createDateTimeFormat() with long or short formats. For dates, the short format has a numeric month; for times, the short format doesn't have seconds.

I left Aryx as original author since he started here, but he submitted the patches *before* that service was available. Looks like he's got other commitments now, so I've just finished of the cut/paste job for him to get the bug off the books, just like bug 1346549.

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8881767 [details] [diff] [review]
1313659-editor.patch (JK v1) [landed in comment #30]

https://hg.mozilla.org/comm-central/rev/e5af1840e19169c3e11f8bb6483f0384f7fac71a

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8881778 [details] [diff] [review]
1313659-chat.patch (JK v1).

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

Looks good by code inspection, thanks. I haven't tested this locally, I assume you have.

::: mail/components/im/content/chat-messenger-overlay.js
@@ -449,5 @@
>          return;
>        }
>      }
>    },
> -  _makeFriendlyDate: function(aDate) {

I'm surprised this isn't used anywhere. If you remove this, you should also remove the "today" and "yesterday" strings from chat.properties.

Comment 32

[Jorg K (CEST = GMT+2)]

I must admit, I haven't tested this since I only tweaked Aryx's patch, see interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8857971&action=interdiff&newid=8881778&headers=1
I assume he tested this.

Anyway, let's get someone more familiar with chat test this, someone who has complained about the badly formatted dates ;-)

Richard, can you help us out here and test the chat patch. Thanks in advance.

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-chat.patch (JK v2).

Removed today and yesterday strings as requested.

Richard, this is the test to take for a spin ;-)

Comment 34

[Jorg K (CEST = GMT+2)]

Oops, this is the *patch* to take for a spin.

Comment 35

[Richard Marti (:Paenglab)]

I see no change to without patch. The timestamps in chat area are still AM/PM. In Twitter the user tooltips show still US format for the "User Since" field. The "Previous Conversations" tree shows the correct date format, but this is already without patch correct.

I only use IRC and Twitter and can test only this two.

Comment 36

[Jorg K (CEST = GMT+2)]

Yes, sadly still more uses of toLocale[Time|Date]String():
https://dxr.mozilla.org/comm-central/search?q=path%3Achat+tolocale&redirect=false

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-chat.patch (JK v3).

Comment 38

[Richard Marti (:Paenglab)]

Comment on attachment 8886321 [details] [diff] [review]
1313659-chat.patch (JK v3).

The dates I can check are correct now. Thank you.

Comment 39

[Jorg K (CEST = GMT+2)]

OK, I'll land this now with the additional hunks
https://bugzilla.mozilla.org/attachment.cgi?oldid=8886276&action=interdiff&newid=8886321&headers=1
to fix the visible dates in the UI. I filed bug 1380799 to cover what's left.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1313659-chat.patch (JK v3b).

White-space changes and used const throughout.

Comment 41

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/e5af1840e19169c3e11f8bb6483f0384f7fac71a - editor/ (comment #30)
https://hg.mozilla.org/comm-central/rev/2bc6c38ccf68d00c629309448634dc7857653c06 - chat/

Comment 42

[Jorg K (CEST = GMT+2)]

Beta (TB 55):
https://hg.mozilla.org/releases/comm-beta/rev/9c39c109d351d6b3eae605e4142f02682a6e479a
https://hg.mozilla.org/releases/comm-beta/rev/402684d6d9195fb4f9e1f43bd850de0259717cdf