1346549 - Replace nsIScriptableDateFormat in mail/ and mailnews/

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[:aceman]

Bug 1313625 is deprecating nsIScriptableDateFormat. We can use Intl.DateTimeFormat or Date.toLocaleDateString().

Comment 1

[:aceman]

Apparently Aryx is already working on this.

Comment 2

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

Attachment: mail, v3

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8846656 [details] [diff] [review]
mail, v3

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

This will get merge conflicts with bug 1332351, for example in activity.xml where we're busily replacing |.toLocaleFormat("%A")| a few lines further down.

As a general question: In bug 1332351 Aceman is using .toLocaleDateString() instead of Intl.DateTimeFormat.format(). The calendar guys are using Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]. Which one is preferred?

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);

Any reason to have a separate variable 'dtOptions'?

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #3)
> Which one is preferred?
Oh, looks like that if you want to format date and time, you use Intl.DateTimeFormat(), for just date or time you can used .toLocaleDateString() or .toLocaleTimeString(). And you can use the former to prepare a formatter once outside a loop.

Comment 5

[:aceman]

(In reply to Jorg K (GMT+1) from comment #3)
> As a general question: In bug 1332351 Aceman is using .toLocaleDateString()
> instead of Intl.DateTimeFormat.format(). The calendar guys are using
> Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]
> [review]. Which one is preferred?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_toLocaleFormat

Intl.DateTimeFormat is an object that caches something so it preferable to use in tight loops converting many dates. It is probably fine to use it anywhere, I don't know if there are any disadvantages.

I just used Date.toLocaleDateString() as it is using methods on the date object we already have in hand, without having to instantiate a new Intl object and prepare the format elsewhere, e.g. on a separate line like Aryx has done.

(In reply to Jorg K (GMT+1) from comment #4)
> Oh, looks like that if you want to format date and time, you use
> Intl.DateTimeFormat(), for just date or time you can used
> .toLocaleDateString() or .toLocaleTimeString().

Or .toLocaleString() when you need both.

> And you can use the former
> to prepare a formatter once outside a loop.

Yes. But we have no loops in the patch. So it seems usage of Intl. is unnecessarily verbose and format split away from the object we apply it on.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> Yes. But we have no loops in the patch. So it seems usage of Intl. is
> unnecessarily verbose and format split away from the object we apply it on.
I was going to suggest to use .toLocale[Date|Time]String(), but I'm not the reviewer here. Also in bug 1313659 where possible.

Comment 7

[:aceman]

Comment on attachment 8846656 [details] [diff] [review]
mail, v3

Thanks, I'll cancel the request for now. The patch will need update after bug 1332351 anyway and we also wait for Magnus whether to use the locale of 'undefined' or something else.

Comment 8

[Jorg K (CEST = GMT+2)]

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

Comment 9

[:aceman]

Comment on attachment 8846656 [details] [diff] [review]
mail, v3

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

Aryx please update the patch now, it was bitrotted by the other bug.

::: mail/components/activity/content/activity.xml
@@ +238,5 @@
>          <parameter name="time"/>
>          <body>
>            <![CDATA[
> +            const timeFormatter = new Intl.DateTimeFormat(undefined, { hour: 'numeric',
> +                                                                       minute: 'numeric' });

I think this method got removed.

@@ +280,5 @@
>          <body>
>            <![CDATA[
> +            const dateTimeOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                                      hour: 'numeric', minute: 'numeric' };
> +            const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dateTimeOptions);

Ok, formatTimeTip may be run in a loop (for each item in the list) so if you want to use Intl, set it up in the constructor, not in this method.

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);

formatExpiresString may be run in a loop (for each item in a list) so set up the dateTimeFormatter beforehand, e.g. instead of the _ds you remove. Yes, and merge the options into it.

::: mailnews/base/content/dateFormat.js
@@ +24,5 @@
>    try {
> +    const dateFormatter = new Intl.DateTimeFormat(undefined,
> +      { year: '2-digit', month: 'numeric', day: 'numeric' });
> +    var aDate = new Date(1999, 11, 1);
> +    var dateString = dateFormatter.format(aDate);

This doesn't need Intl.
The change probably does not make things worse, but ask myself whether the whole dateFormat.js is needed today. It analyses how the locale formats dates and then produces similar date strings when we ask it ( via convertDateToString), mimicking the locale format.
I see we may need to convert the string back to a JS date/time milliseconds (convertStringToPRTime). But I would like to see a bug about analysing if this is still needed. Maybe we could just store the seconds value at the callers and format them properly via Intl. not via convertDateToString.

::: mailnews/base/util/templateUtils.js
@@ +50,5 @@
>  {
> +  const dateFormatter = new Intl.DateTimeFormat(undefined,
> +    { year: '2-digit', month: 'numeric', day: 'numeric' });
> +  const timeFormatter = new Intl.DateTimeFormat(undefined,
> +    { hour: 'numeric', minute: 'numeric' });

OK, makeFriendlyDateAgo may be called in a loop, but then you need to setup dateFormatter and timeFormatter beforehand (at the initialization of the module, not in this function, to take advantage of Intl's internal caching. You may the want to convert all 4 branches (starting at 'if (end >= today)') to predefined formatters.

Comment 10

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

Attachment: mail, v5

(In reply to Jorg K (GMT+2) from comment #3)
> As a general question: In bug 1332351 Aceman is using .toLocaleDateString()
> instead of Intl.DateTimeFormat.format(). The calendar guys are using
> Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]
> [review]. Which one is preferred?
I switched the code Services.intl.createDateTimeFormat which caches the last used formatter and is preferred on mozilla-central now: https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/components/mozintl/mozIntl.js#91

> ::: mail/components/preferences/cookies.js
> @@ +516,5 @@
> >      if (aExpires) {
> >        var date = new Date(1000 * aExpires);
> > +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> > +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> > +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);
> 
> Any reason to have a separate variable 'dtOptions'?
Inlined them.

(In reply to :aceman from comment #9)
> ::: mailnews/base/content/dateFormat.js
> @@ +24,5 @@
> >    try {
> > +    const dateFormatter = new Intl.DateTimeFormat(undefined,
> > +      { year: '2-digit', month: 'numeric', day: 'numeric' });
> > +    var aDate = new Date(1999, 11, 1);
> > +    var dateString = dateFormatter.format(aDate);
> 
> This doesn't need Intl.
> The change probably does not make things worse, but ask myself whether the
> whole dateFormat.js is needed today. It analyses how the locale formats
> dates and then produces similar date strings when we ask it ( via
> convertDateToString), mimicking the locale format.
> I see we may need to convert the string back to a JS date/time milliseconds
> (convertStringToPRTime). But I would like to see a bug about analysing if
> this is still needed. Maybe we could just store the seconds value at the
> callers and format them properly via Intl. not via convertDateToString.
Filed bug 1356242.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8857963 [details] [diff] [review]
mail, v5

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

::: mail/components/activity/content/activity.xml
@@ +238,5 @@
>          <body>
>            <![CDATA[
> +            const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +              year: 'numeric', month: 'long', day: 'numeric',
> +              hour: 'numeric', minute: 'numeric'

Why don't we just use something like:
https://hg.mozilla.org/mozilla-central/rev/79673e659b31#l9.19
Services.intl.createDateTimeFormat(undefined,
+                             { dateStyle: "long",
+                               timeStyle: "short" });

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +        year: 'numeric', month: 'long', day: 'numeric',
> +        hour: 'numeric', minute: 'numeric', second: 'numeric'

And here. I've I'm not mistaken that goes through mozIMozIntl and even take the OS settings into account. And further down, too.

Comment 12

[:aceman]

Comment on attachment 8857963 [details] [diff] [review]
mail, v5

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

Please do what Jorg says and I found some more nits.

::: mailnews/base/content/dateFormat.js
@@ +22,5 @@
>    gSearchDateLeadingZeros = true;
>  
>    try {
> +    const dateFormatter = Services.intl.createDateTimeFormat(undefined,
> +      { year: '2-digit', month: 'numeric', day: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { dateStyle: "short" }) ?

::: mailnews/base/util/templateUtils.js
@@ +35,5 @@
>    "chrome://messenger/locale/templateUtils.properties"
>  );
>  
> +const _dateFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { year: '2-digit', month: 'numeric', day: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { dateStyle: "short" })

@@ +39,5 @@
> +  { year: '2-digit', month: 'numeric', day: 'numeric' });
> +const _dayMonthFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { month: 'long', day: 'numeric' });
> +const _timeFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { hour: 'numeric', minute: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { timeStyle: "short" })

@@ +83,3 @@
>    } else if (now.getFullYear() == end.getFullYear()) {
>      // activity must have been from some time ago.. show month/day
> +    dateTime = _timeFormatter.format(end);

Did you mean _dayMonthFormatter ?

Comment 13

[Albert Scheiner [:alberts]]

Might this block (fix) bug 253883?

Comment 14

[Jorg K (CEST = GMT+2)]

Friendly take-over, I'm just addressing the nits here and merging my fix from bug 1356403 which is also in mail/.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1346549-replace-scriptable-date-in-mail-mailnews.patch (JK v1).

OK, I addressed the comments, and added a hunk from bug 1356403. I left the original author in place.

Comment 17

[:aceman]

Comment on attachment 8881526 [details] [diff] [review]
1346549-replace-scriptable-date-in-mail-mailnews.patch (JK v1).

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

Thanks.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +284,5 @@
>          // due to time zone/dst discontinuity
>          date = new Date(Date.UTC(year, month - 1, day));
>          date.setUTCFullYear(year); // to handle two-digit years properly
> +        formatter = Services.intl.createDateTimeFormat(undefined,
> +                      { dateStyle: "long" });

Can we keep the 'timeZone: "UTC"' here? I think it it part of the feature. Does createDateTimeFormat() support the property?

Comment 18

[Jorg K (CEST = GMT+2)]

I think it does, at least Javi claims it does ;-)
https://hg.mozilla.org/try-comm-central/rev/4e8d50eb86630a093c0602c6d261e398b9c3273e#l1.59

What's the UTC for? Does it make a difference in the display?

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1346549-replace-scriptable-date-in-mail-mailnews.patch (JK v1b).

Added |timeZone: "UTC"| back in.

Comment 20

[:aceman]

(In reply to Jorg K (GMT+2) from comment #18)
> What's the UTC for? Does it make a difference in the display?
The comments say it is important. Also we create new Date(Date.UTC()) so the dates are not in local time but UTC. Then displaying them in local zone will not show the date we intended, but shifted by some hours due to local timezone.

Comment 21

[:aceman]

See e.g. bug 1139167.

Comment 22

[Jorg K (CEST = GMT+2)]

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

Aceman, thanks for catching this.

Comment 23

[Jorg K (CEST = GMT+2)]

Beta (TB 55):
https://hg.mozilla.org/releases/comm-beta/rev/ea1cb39ab253

Comment 24

[Eyal Rozenberg]

With this deprecation, do I now need a version check to decide which interface to use? Or are the other interfaces all the way back to, say, TB 1.0?

Comment 25

[Eyal Rozenberg]

Also, what about Seamonkey?

Comment 26

[Jorg K (CEST = GMT+2)]

Nothing goes back to TB 1.0. I believe that the interfaces you need to use are available in TB 52 ESR or mozilla52 and the equivalent SM version of 2.49. Nothing before TB 52 is currently supported.