Closed Bug 1332351 Opened 7 years ago Closed 7 years ago

Remove Date.prototype.toLocaleFormat uses in comm-central

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 55.0

People

(Reporter: anba, Assigned: aceman)

References

Details

Attachments

(3 files, 4 obsolete files)

Remove Date.prototype.toLocaleFormat uses in chat/
12.27 KB, patch
clokep
: review+
Details | Diff | Splinter Review
fix tests [landed in comment #49]
3.64 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
mail/mailnews v1.3
18.94 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review 
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 comm-central with standardized APIs.
So what's the replacement, can you give an example please.

I can't see it used in C-C:
https://dxr.mozilla.org/comm-central/search?q=Date.prototype.toLocaleFormat&redirect=false
Sorry, you meant:
https://dxr.mozilla.org/comm-central/search?q=toLocaleFormat&redirect=true

That's used quite a bit. Thanks for the heads-up.
(In reply to Jorg K (GMT+1) from comment #1)
> So what's the replacement, can you give an example please.

It depends on the used format strings. Either the normal Date.prototype methods combined with custom formatting functions (bug 1313793, bug 1313794, bug 1313797, bug 1313798, bug 1313799) or formatting through Intl.DateTimeFormat (bug 1313795, bug 1313796). If necessary it's even possible to reimplement toLocaleFormat/strftime in JavaScript itself (https://bugzilla.mozilla.org/attachment.cgi?id=8817559), but hopefully that's not needed here. :-)
(In reply to Jorg K (GMT+1) from comment #2)
> That's used quite a bit. Thanks for the heads-up.

Yes, and it's somewhat unfortunate for me. I was about to request checkin for bug 1299900 when I saw bug 818634, comment 2, so I probably need to wait a bit until toLocaleFormat is also removed from comm-central.
Calendar and Chat people take note, this is coming your way, too.
Flags: needinfo?(makemyday)
Flags: needinfo?(aleth)
How about you land your bug next week after the branch date. Then it causes less stress. We'll get onto it in the next few days.
(In reply to Jorg K (GMT+1) from comment #6)
> How about you land your bug next week after the branch date. Then it causes
> less stress. We'll get onto it in the next few days.

Well, it'd be nice to start reporting a warning for toLocaleFormat in Firefox, but it's also not that important compared to other tasks. So I don't really mind if the warning gets delayed to a different Firefox release in this year.
André, whatever happened to this? I suggested to land bug 1299900 after the branch day to FF 54 in January of 2017. Today is branch day going to FF 55 and nothing has happened.
Flags: needinfo?(andrebargull)
(In reply to Jorg K (GMT+1) from comment #8)
> André, whatever happened to this? I suggested to land bug 1299900 after the
> branch day to FF 54 in January of 2017. Today is branch day going to FF 55
> and nothing has happened.

Ah, okay. There was a misunderstanding on my part, I assumed it'd be easier for you if I wait with bug 1299900 until toLocaleFormat uses in comm-central are removed. I didn't want to put extra pressure on the comm-central maintainers and instead decided to wait a little longer until I can land bug 1299900.
Flags: needinfo?(andrebargull)
Well, the sad story is that there is only one official C-C maintainer (me) and a group of volunteers. Unless you exert some pressure, nothing will change. M-C's most efficient way is to cause compile errors (so we can't build), or withdraw/change (JS) interfaces, so we can build but nothing works. Then suddenly everyone jumps into action. We have technical debt going back a decade and action is only taken when something finally breaks. Anyway, that's the sad story. But you're only proposing to turn up the pressure by should a warning. That won't break anything so far.
(In reply to Jorg K (GMT+1) from comment #10)
> Well, the sad story is that there is only one official C-C maintainer (me)
> and a group of volunteers.

Oh that's sad to hear. :-(

> But you're only proposing to turn up the
> pressure by should a warning. That won't break anything so far.

So it's okay with you when I land bug 1299900 after today's merge?
Oops: you're only proposing to turn up the pressure by *showing* a warning.

Aceman, affected are: Calendar, chat, mail, mailnews: https://dxr.mozilla.org/comm-central/search?q=toLocaleFormat
It doesn't appear to be a huge problem.

(In reply to André Bargull from comment #11)
> So it's okay with you when I land bug 1299900 after today's merge?
As I said: Land it or wait forever ;-(
Flags: needinfo?(acelists)
(In reply to Jorg K (GMT+1) from comment #12)
> (In reply to André Bargull from comment #11)
> > So it's okay with you when I land bug 1299900 after today's merge?
> As I said: Land it or wait forever ;-(

Understood. Thanks for your responses! :-)
The page at https://www.fxsitecompat.com/en-CA/docs/2015/date-prototype-tolocaleformat-will-be-removed/ recommends to use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString but the deprecation message offers Intl.DateTimeFormat . So what is the difference?
Those two are the same. The only difference is that Intl.DateTimeFormat allows you to cache an object with most of the computations done so that formatting is fast.

So, basically, if you have one date to format, use date.toLocaleString. If you have a loop of dates to format, create a datetimeformatter.
Since message styles can use any strftime/toLocaleFormat format they like, I think we're pretty much forced to include the whole JS implementation of toLocaleFormat provided by Andre in comment 3. I stuck it into a module for reuseability. I've checked the output matches the existing toLocaleFormat where it matters for us (log file names), there are slight differences in the verbose date/time strings as e.g. Intl includes commas and timezone info.
Attachment #8844470 - Flags: review?(clokep)
(In reply to aleth [:aleth] from comment #16)
I stuck it into a module for reuseability.

chat/modules/ToLocaleFormat.jsm (new file): Should that sit somewhere else for further use in TB? After all, we have calendar, chat, mail and mailnews affected: https://dxr.mozilla.org/comm-central/search?q=toLocaleFormat
(In reply to Jorg K (GMT+1) from comment #17)
> (In reply to aleth [:aleth] from comment #16)
> I stuck it into a module for reuseability.
> 
> chat/modules/ToLocaleFormat.jsm (new file): Should that sit somewhere else
> for further use in TB? After all, we have calendar, chat, mail and mailnews
> affected: https://dxr.mozilla.org/comm-central/search?q=toLocaleFormat

I don't think there's a location which satisfies this:
* chat/ is used in Instantbird and Thunderbird.
* calendar/ is used in Thunderbird and SeaMonkey.
* mailnews/ is used in Thunderbird and SeaMonkey (and thus Lightning has access).
* mail/ is used in Thunderbird.

I think we've run into this in the past (with OAuth code?) and have multiple copies checked in (which sucks). We should probably just put it in mailnews/ and do some weird path stuff in Instantbird builds files.
Comment on attachment 8844470 [details] [diff] [review]
Remove Date.prototype.toLocaleFormat uses in chat/

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

This looks good regardless.
Attachment #8844470 - Flags: review?(clokep) → review+
(In reply to Jorg K (GMT+1) from comment #17)
> chat/modules/ToLocaleFormat.jsm (new file): Should that sit somewhere else
> for further use in TB? After all, we have calendar, chat, mail and mailnews
> affected: https://dxr.mozilla.org/comm-central/search?q=toLocaleFormat

It's possible the other places where toLocaleFormat is used can use a simple substitution and don't actually need to import the module. I would suggest we hold off on landing this until the other patches here are done, then we'll see.
Flags: needinfo?(aleth)
Whiteboard: [Thunderbird-testfailure: X all]
I've looked at this a bit and we do have "variable" format strings to deal with in mail and mailnews, for example in abCardViewOverlay.js and glodaFacetVis.js.

So the best and fastest way is to do what Aleth has done for chat/. I looked at bug 818634 to see why this interface is removed. However, the replacements I've see aren't really elegant, some examples:
https://hg.mozilla.org/mozilla-central/rev/6e4a31120b53#l1.12
https://hg.mozilla.org/mozilla-central/rev/7d4a5ed8adf1#l4.14

So maybe we do a combined approach: The new module for variable and complicated formats, and direct replacement for something like: let month = end.toLocaleFormat("%B");
For mail/mailnews this is urgent since it's causing a heap of test failures.

This should do it, untested(!!) and Aceman will move Aleth' module to mailnews. Then we can land and chat can pick up up the module.

We might change String() to .toString, right?
Attached patch alternative for mail/mailnews (obsolete) — — Splinter Review 
Sorry, I do not think we need the module. I see it is convenient and means less changes to existing code.
The differences to Jorg's patch are the following:
1.Convert the single placeholder formats (e.g. "%B") to the corresponding format in Date.toLocaleDataString().
2. Formats like "%Y-%m-%d %H:%M:%S" or even "%Y-%m" aren't really in any locale (can you find a language which produces such strings?) so should not be run through a locale function. They are just some hardcoded order of the datetime parts. So we can hardcode them again using the native Date functions.
3. I remove strings like "dateFormatMonthDay=%B %d" which want to format a date and rely on localizers to order the parts (like day an month) in the correct order. It seems to me the .toLocaleDateString() does that automatically.
Try new Date().toLocaleDateString("en-US") and new Date().toLocaleDateString("de-DE") and new Date().toLocaleDateString("sk-SK") . Each version is different, according to locale requirements. Without needing to pass any string the translators prepared. 
4. I think my version also fixes some real localization bugs. E.g. the old %d code produces only the number of the day. However, Date().toLocaleDateString({ day: "numeric"}) produces the number plus also any locale specific characters. E.g. in sk-SK it returns "11." for today, which is more correct. This can be seen e.g. in the gloda results (the date in column under the distribution graphs).

OK, there is one change of behaviour here! The original version took the strings from the .properties files so the string depended on the version of TB you downloaded (so en-US TB on de-DE system produces en-US formats). The new version uses the strings according to the current system locale (ignoring TB locale).

I've tested Intl.DateTimeFormat().resolvedOptions().locale from Aleth's patch and it also returns the system locale, so that should provide the same behaviour as in my patch.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9212c238a40cbb0af0417df594cf97b5a817da57
Flags: needinfo?(acelists)
Attachment #8846238 - Flags: review?(jorgk)
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9212c238a40cbb0af0417df594cf97b5a817da57
Kindly explain the following: How did you manage to get the tree almost green, I don't get it.
There are meant to be failures due to bug 1342745 and bug 1229684. How did they get fixed magically? Are you a magician?

I'm happy with the alternative approach, especially when it comes with a clean-up. However, as I said before, things like
https://hg.mozilla.org/try-comm-central/rev/7a1b5d3eb2c1cef909a1e60ba14ac7bddd980c83#l10.34
https://hg.mozilla.org/try-comm-central/rev/7a1b5d3eb2c1cef909a1e60ba14ac7bddd980c83#l11.14
aren't strictly elegant.

(In reply to :aceman from comment #23)
> OK, there is one change of behaviour here! The original version took the
> strings from the .properties files so the string depended on the version of
> TB you downloaded (so en-US TB on de-DE system produces en-US formats). The
> new version uses the strings according to the current system locale
> (ignoring TB locale).
I don't think so. What makes you say that it uses the system locale? M-C are busily removing all uses of the system locale (as retrieved from the OS) and do everything via the build locale, last bug in the series was bug 1342753 causing bug 1344594.
(In reply to Jorg K (GMT+1) from comment #24)
> There are meant to be failures due to bug 1342745 and bug 1229684. How did
> they get fixed magically?
Failures due to bug 1342745 magically disappeared a few days ago (and nobody noticed, since the tests were orange for a different reason), see bug 1342745 comment #6.

As for the failures from bug 1229684, maybe Aceman included that patch in his push, hard to tell.
Comment on attachment 8846238 [details] [diff] [review]
alternative for mail/mailnews

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

Looking very good, but I have a few questions. f++ in any case. And thanks for the patch!

::: mail/base/content/mailWindowOverlay.js
@@ +1678,5 @@
>  
>        // Convert date to JS date object.
>        let msgDate = new Date(msgHdr.date / 1000);
>        let msgYear = msgDate.getFullYear().toString();
> +      let monthFolderName = msgYear + "-" + String(msgDate.getMonth() + 1);

.toString()?

::: mail/components/activity/content/activity.xml
@@ -129,5 @@
>              canceled: "canceled",
>              failed: "failed",
>              waitingforinput: "waitingForInput",
>              waitingforretry: "waitingForRetry",
> -            yesterday: "yesterday",

Do we still show "yesterday"?

@@ -259,5 @@
> -              dateTime = dts.FormatTime("", dts.timeFormatNoSeconds,
> -                                            end.getHours(), end.getMinutes(),0);
> -            } else if (today - end < kDayInMsecs) {
> -              // activity finished after yesterday started, show yesterday
> -              dateTime = this.text.yesterday;

Yesterday removed here.

@@ +615,5 @@
>            ]]>
>          </getter>
>          <setter>
>            <![CDATA[
> +            this.setAttribute("completionTime", this.makeFriendlyDateAgo(val));

What does the "Ago" stand for?

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +288,5 @@
>        // if the year doesn't exist, display Month DD (ex. January 01)
>        else {
>          date = new Date(saneBirthYear(year), month - 1, day);
> +        dateStr = date.toLocaleDateString(undefined,
> +                      { day: "numeric", month: "long", timeZone: "UTC" });

So here is where you're using the new .toLocaleDateString() instead of getting a localised string 'dateFormatMonthDay'. I'm pretty sure that this will use the build locale.

::: mail/locales/en-US/chrome/messenger/activity.properties
@@ -12,5 @@
>  completed=Completed
>  canceled=Canceled
>  
> -# LOCALIZATION NOTE (yesterday): Displayed time for files finished yesterday
> -yesterday=Yesterday

That's for the activities, so we lose the "yesterday" display?

::: mail/locales/en-US/chrome/messenger/templateUtils.properties
@@ -3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # LOCALIZATION NOTE yesterday: used in various places where we compute
>  # a "friendly" date, e.g. displaying that a message was from yesterday.
>  yesterday=yesterday

Note that here "yesterday" is maintained.

::: mail/test/mozmill/folder-display/test-message-commands.js
@@ +402,5 @@
>    let firstMsgHdrMsgId = firstMsgHdr.messageId;
>    let lastMsgHdrMsgId = lastMsgHdr.messageId;
>    let firstMsgDate = new Date(firstMsgHdr.date / 1000);
>    let firstMsgYear = firstMsgDate.getFullYear().toString();
> +  let firstMonthFolderName = firstMsgYear + "-" + (firstMsgDate.getMonth() + 1);

.toString()?

@@ +407,3 @@
>    let lastMsgDate = new Date(lastMsgHdr.date / 1000);
>    let lastMsgYear = lastMsgDate.getFullYear().toString();
> +  let lastMonthFolderName = lastMsgYear + "-" + (lastMsgDate.getMonth() + 1);

.toString()?

::: mailnews/base/util/templateUtils.js
@@ +77,3 @@
>    } else if (now.getFullYear() == end.getFullYear()) {
>      // activity must have been from some time ago.. show month/day
> +    dateTime = end.toLocaleDateString(undefined, { day: "numeric", month: "long" });

Same comment as above, .toLocaleDateString() instead of string 'monthDate'.
Attachment #8846238 - Flags: review?(jorgk) → feedback+
Comment on attachment 8846184 [details] [diff] [review]
1332351-toLocaleFormat.patch - WIP for mail/mailnews

We have a better solution.
Attachment #8846184 - Attachment is obsolete: true
Aleth, are you sure you need that ToLocaleFormat.jsm module? For mail/ and mailnews/ Aceman was able to eliminate the "variable" formats and use simple substitution and .toLocaleDateString() to cover all 13 call sites. I haven't looked into it to figure out where the variable format comes from in chat/.
Flags: needinfo?(aleth)
(In reply to Jorg K (GMT+1) from comment #24)
> Kindly explain the following: How did you manage to get the tree almost
> green, I don't get it.
> There are meant to be failures due to bug 1342745 and bug 1229684. How did
> they get fixed magically? Are you a magician?

No. I don't know.

> I'm happy with the alternative approach, especially when it comes with a
> clean-up. However, as I said before, things like
> https://hg.mozilla.org/try-comm-central/rev/
> 7a1b5d3eb2c1cef909a1e60ba14ac7bddd980c83#l10.34
> https://hg.mozilla.org/try-comm-central/rev/
> 7a1b5d3eb2c1cef909a1e60ba14ac7bddd980c83#l11.14
> aren't strictly elegant.

I agree they are not. But as I understand it the new toLocaleDateString function is optimized to just give you the right format of date for a particular locale. Without you telling it the requested order of the numbers.
But in these cases the requested format does not correspond to any locale format. It is just a specific string, maybe mandated by some spec. So toLocaleDateString can't give you that. So we need to implement the format ourselves.
 
> (In reply to :aceman from comment #23)
> > OK, there is one change of behaviour here! The original version took the
> > strings from the .properties files so the string depended on the version of
> > TB you downloaded (so en-US TB on de-DE system produces en-US formats). The
> > new version uses the strings according to the current system locale
> > (ignoring TB locale).
> I don't think so. What makes you say that it uses the system locale? M-C are
> busily removing all uses of the system locale (as retrieved from the OS) and
> do everything via the build locale, last bug in the series was bug 1342753
> causing bug 1344594.

Running the TB trunk is in en-US locale. However, all the Date().toLocaleDateString(undefined, ...) calls and Intl.DateTimeFormat().resolvedOptions().locale produced output in my system locale (sk-SK). Have you tried it as I asked?

(In reply to Jorg K (GMT+1) from comment #25)
> As for the failures from bug 1229684, maybe Aceman included that patch in
> his push, hard to tell.

I'm not aware of that.

But I've seen some nsIScriptableDateFormat uses in /mail. If we want to remove it, I could look at that. 1229684 seems to be only for calendar.
(In reply to Jorg K (GMT+1) from comment #27)
> >        // Convert date to JS date object.
> >        let msgDate = new Date(msgHdr.date / 1000);
> >        let msgYear = msgDate.getFullYear().toString();
> > +      let monthFolderName = msgYear + "-" + String(msgDate.getMonth() + 1);
> 
> .toString()?

Yes.
 
> ::: mail/components/activity/content/activity.xml
> @@ -129,5 @@
> >              canceled: "canceled",
> >              failed: "failed",
> >              waitingforinput: "waitingForInput",
> >              waitingforretry: "waitingForRetry",
> > -            yesterday: "yesterday",
> 
> Do we still show "yesterday"?

Yes we do, but do not need this declaration here. It can even be seen in the patch, that the string is still defined in templateUtils.properties and we use it in templateUtils.js. I just tried to merge the 2 implementations of the code in makeFriendlyDateAgo(). I try to use the one from templateUtils. But I haven't run tested it. I haven't yet found how to trigger it in the activity manager.
 
> @@ +615,5 @@
> >            ]]>
> >          </getter>
> >          <setter>
> >            <![CDATA[
> > +            this.setAttribute("completionTime", this.makeFriendlyDateAgo(val));
> 
> What does the "Ago" stand for?

That the 'val' time is in the past. So it means "some time ago'. It is not Age.
 
> ::: mail/components/addrbook/content/abCardViewOverlay.js
> @@ +288,5 @@
> >        // if the year doesn't exist, display Month DD (ex. January 01)
> >        else {
> >          date = new Date(saneBirthYear(year), month - 1, day);
> > +        dateStr = date.toLocaleDateString(undefined,
> > +                      { day: "numeric", month: "long", timeZone: "UTC" });
> 
> So here is where you're using the new .toLocaleDateString() instead of
> getting a localised string 'dateFormatMonthDay'. I'm pretty sure that this
> will use the build locale.

I'm sure, it takes system locale and I have tested it. You can just put a date of birthday (without year!) into an AB contact and you will see it. Please try it.
 
> ::: mail/locales/en-US/chrome/messenger/activity.properties
> > -# LOCALIZATION NOTE (yesterday): Displayed time for files finished yesterday
> > -yesterday=Yesterday
> 
> That's for the activities, so we lose the "yesterday" display?

Yes. The other occurrence still left.
 
> ::: mail/locales/en-US/chrome/messenger/templateUtils.properties
> @@ -3,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  # LOCALIZATION NOTE yesterday: used in various places where we compute
> >  # a "friendly" date, e.g. displaying that a message was from yesterday.
> >  yesterday=yesterday
> 
> Note that here "yesterday" is maintained.

Yes. The code of makeFriendlyDateAgo() in templateUtils.js uses it.
 
> ::: mail/test/mozmill/folder-display/test-message-commands.js
> @@ +402,5 @@
> >    let firstMsgHdrMsgId = firstMsgHdr.messageId;
> >    let lastMsgHdrMsgId = lastMsgHdr.messageId;
> >    let firstMsgDate = new Date(firstMsgHdr.date / 1000);
> >    let firstMsgYear = firstMsgDate.getFullYear().toString();
> > +  let firstMonthFolderName = firstMsgYear + "-" + (firstMsgDate.getMonth() + 1);
> 
> .toString()?

OK.

> ::: mailnews/base/util/templateUtils.js
> @@ +77,3 @@
> >    } else if (now.getFullYear() == end.getFullYear()) {
> >      // activity must have been from some time ago.. show month/day
> > +    dateTime = end.toLocaleDateString(undefined, { day: "numeric", month: "long" });
> 
> Same comment as above, .toLocaleDateString() instead of string 'monthDate'.

Yes. according to my tests, the function produces the correct string automatically, without needing "monthDate" with hints from localizers. Hopefully I am right here. Please test it too on your locales before we blow away the string and all its translations and then need to put it back.
(In reply to :aceman from comment #30)
> Running the TB trunk is in en-US locale. However, all the
> Date().toLocaleDateString(undefined, ...) calls and
> Intl.DateTimeFormat().resolvedOptions().locale produced output in my system
> locale (sk-SK). Have you tried it as I asked?
No, I haven't and I can't see where you asked. Maybe in comment #23 point 3 and 4?

> But I've seen some nsIScriptableDateFormat uses in /mail. If we want to remove it, I could look at that.
Yes, please, but in another bug.

(In reply to :aceman from comment #31)
> I'm sure, it takes system locale and I have tested it. You can just put a
> date of birthday (without year!) into an AB contact and you will see it.
> Please try it.
OK, I've tested it now. I see, for example, "1. März" (1st March). I'm really confused by this, since M-C are making a lot of effort of *not* using the system locale, see bug 1342753. Maybe we should not pass "undefined" but instead use:
https://hg.mozilla.org/try-comm-central/rev/8f3e07e8e3e1e80918aea6e61379c0097a1a9c7a#l3.72

I'm out for some hours now, but from about 15:00 till the end of Sunday I'm at your disposition ;-)
Thanks for asking in bug 1342753 comment #36.

I'm not sure what this is all about, but if you look at
https://hg.mozilla.org/try-comm-central/rev/8f3e07e8e3e1e80918aea6e61379c0097a1a9c7a
you can see both uses: Intl.DateTimeFormat(undefined, ...) and Intl.DateTimeFormat(this.mLocale, ...).

Javi and MMD, what's the intended difference? The first one uses the system locale, and if you want the build locale you need to fetch it first via
+    this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
+                             .getService(Components.interfaces.nsIXULChromeRegistry)
+                             .getSelectedLocale("global", true);

I can't NI MMD here since he's already NI'ed, so let's try Javi.
Flags: needinfo?(foss)
Yes I noticed that and also do not understand why they not use it consistently.
Aceman, could you fix the nits from comment #31, get the build locale as described and then we land this. We can always follow up with a small tweak later.

As for
> I haven't yet found how to trigger it in the activity manager.
Well, the activity manager records many things: received/sent messages, moved/delete messages, etc. So guess you need to do something at 15:55 and then set the clock to 0:05 at 16:05 and then 15:55 was yesterday. Hmm.
OK, I set my time zone to Tokyo now, 23:45. I have stuff in the activity manager. In 15 minutes I will see what happens ;-)
Something wrong with the activity manager with your patch. There aren't any times displayed. Instead I get a little icon (circular arrow) with a tooltip "Undo" where the time display was.
23:50:29 activity-base ERROR Exception constructing activity-event: TypeError: end.getHours is not a function
Something wrong here.
OK, it is now 00:05 in Tokyo and all the events in the activity manager show "Yesterday" in Daily. Easy to trigger. With your patch, the little "undo icon" has disappeared, and I see "yesterday", in lower case. So that part works. Do you trust me or do you want to see a screenshot?
I have tested the yesterday string too and it works (I edit the code to only have one of the branches).

I'll see what is wrong with the current time display.
(In reply to Jorg K (GMT+1) from comment #35)
> Aceman, could you fix the nits from comment #31, get the build locale as
> described and then we land this. We can always follow up with a small tweak
> later.

Can't we leave it to m-c to fix their stuff if they want app locale be the default? The temporary fix is quite ugly for us.
This fixes the time display in activity manager. The makeFriendlyDateAgo takes the time in different object than the code in activity.xml did. But the rest of the formatting seems identical.
Attachment #8846238 - Attachment is obsolete: true
Blocks: 1346549
Comment on attachment 8844470 [details] [diff] [review]
Remove Date.prototype.toLocaleFormat uses in chat/

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

::: chat/modules/ToLocaleFormat.jsm
@@ +80,5 @@
> +    return timeZoneNamePart ? timeZoneNamePart.value : "";
> +  }
> +
> +  let locale =
> +    Intl.DateTimeFormat().resolvedOptions().locale + "-u-ca-gregory-nu-latn";

This returns the OS locale, not the app locale. Do you want to change it per Jorg's comments?

E.g. in calendar they are also using
Components.classes["@mozilla.org/chrome/chrome-registry;1"].getService(Components.interfaces.nsIXULChromeRegistry).getSelectedLocale("global", true);
which returns the app locale.
See https://hg.mozilla.org/try-comm-central/rev/8f3e07e8e3e1e80918aea6e61379c0097a1a9c7a#l3.72 .

Of course, my proposal still is to use "undefined" and let the default in m-c sort this out what they want.
Thanks for the updated patch. The activity manager now works. Still being at Toyko time, I see 2:23 AM. In the thread pane I see 7:02 PM. All en-US dates that follow the build locale.
However, in the address book, for the birthday I get: 1. März (following the system locale "Germany").
That's inconsistent.

BTW, the activity manager uses makeFriendlyDateAgo() and for display with only times, that uses

  let dts = Cc["@mozilla.org/intl/scriptabledateformat;1"]
              .getService(Ci.nsIScriptableDateFormat);
    dateTime = dts.FormatTime("", dts.timeFormatNoSeconds,
                                  end.getHours(), end.getMinutes(),0);
which is also ear-marked for replacement.
Thanks for the test.

Yes, nsIScriptableDateFormat is being converted in bug 1346549. I think you told me to not touch it here :)
OK, I'd like to make some progress with this, so I'm trying the hunks in log4moz.js and imapd.js. Those are the ones that don't rely on .toLocaleDateString() if I use %m instead of %b in imapd.js. %d-%b-%Y really makes no sense since it makes no sense to localise the %b but not the order of the components. Try here:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5975592ff0fcfdc15d1aab1f2ef25982fc8f2173
OK, I messed that up, the IMAP INTERNALDATE is fixed. And it needs to have an English month. So trying again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ca0a3007555bd2bbc6a6999b8c5bab7d9c5bbd0e
Flags: needinfo?(foss)
Yes, seems to have helped. So this part can be landed to fix the tests. The rest we can consider a bit more with respect to what should be the default locale.
Attachment #8846345 - Flags: review?(jorgk)
Comment on attachment 8846345 [details] [diff] [review]
fix tests [landed in comment #49]

Excellent.

https://hg.mozilla.org/comm-central/rev/f59d7941588cc64ce160acf4ee45816c870af97e
Attachment #8846345 - Attachment description: fix tests → fix tests [landed in comment #49]
Attachment #8846345 - Flags: review?(jorgk) → review+
Whiteboard: [Thunderbird-testfailure: X all]
Spreading the message into all related bugs ;-)

(In reply to Zibi Braniecki [:gandalf][:zibi] from bug 1342753 comment #42)
> Bug 1346674 just landed which should give app locale for default locale in
> Intl API.
Thanks for letting us know.
(In reply to Jorg K (GMT+1) from comment #33)
> Thanks for asking in bug 1342753 comment #36.
> 
> I'm not sure what this is all about, but if you look at
> https://hg.mozilla.org/try-comm-central/rev/
> 8f3e07e8e3e1e80918aea6e61379c0097a1a9c7a
> you can see both uses: Intl.DateTimeFormat(undefined, ...) and
> Intl.DateTimeFormat(this.mLocale, ...).
> 
> Javi and MMD, what's the intended difference? The first one uses the system
> locale, and if you want the build locale you need to fetch it first via
> +    this.mLocale =
> Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> +                            
> .getService(Components.interfaces.nsIXULChromeRegistry)
> +                             .getSelectedLocale("global", true);
> 
> I can't NI MMD here since he's already NI'ed, so let's try Javi.

My NeedInfo was cancelled. However, for-the-records: in bug 1229684 comment 41 it is asked to use a different locale in two methods. Locales were causing tests not being passed, so that could be the main reason why it was asked to use.

When these patches were being reviewed, bug 1346674 hadn't landed yet. When it should have been landed, then that lines of code should have been modified again.

It was done this way so tests become green in tree.
Attached patch mail/mailnews v1.2 (obsolete) — — Splinter Review 
I retested this with current trunk and I think the locale language is now taken from the app by default, when I pass in 'undefined' locale. Please try it out too.
Attachment #8846309 - Attachment is obsolete: true
Attachment #8847362 - Flags: review?(jorgk)
Does "" work instead of undefined?
Hmm, everyone is using 'undefined' so far: here, in bug 1229684, in bug 1346549 and in bug 1313659.
Comment on attachment 8847362 [details] [diff] [review]
mail/mailnews v1.2

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

I'm looking at the birthday in the calendar.
Without the patch here:
With 2nd of March I get "März 02", so US order and German wording.
With 2nd March 1999 I get "3/2/1999", so US.
In the editing panel it shows "03/  2".

With the patch:
"March 1", yes, one, no typo, and "3/2/1999".
I tried setting to 3rd of March and the resulting display is "March 2". Strange.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +288,5 @@
>        // if the year doesn't exist, display Month DD (ex. January 01)
>        else {
>          date = new Date(saneBirthYear(year), month - 1, day);
> +        dateStr = date.toLocaleDateString(undefined,
> +                      { day: "numeric", month: "long", timeZone: "UTC" });

Maybe try without the UTC here?
Attachment #8847362 - Flags: review?(jorgk)
(In reply to Magnus Melin from comment #53)
> Does "" work instead of undefined?

Spec at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#Locale_identification_and_negotiation says:
"The locales argument must be either a string holding a BCP 47 language tag, or an array of such language tags. If the locales argument is not provided or is undefined, the runtime's default locale is used."

In my experiments this works:
[], undefined, any number (but that may be a bug)
Doesn't work:
"", null, "bogus string that isn't a locale name"

So which would you like and still keep the spec?
Flags: needinfo?(mkmelin+mozilla)
(In reply to Jorg K (GMT+1) from comment #55)
> With the patch:
> "March 1", yes, one, no typo, and "3/2/1999".
> I tried setting to 3rd of March and the resulting display is "March 2".
> Strange.

Yes, I see it. 

> ::: mail/components/addrbook/content/abCardViewOverlay.js
> @@ +288,5 @@
> >        // if the year doesn't exist, display Month DD (ex. January 01)
> >        else {
> >          date = new Date(saneBirthYear(year), month - 1, day);
> > +        dateStr = date.toLocaleDateString(undefined,
> > +                      { day: "numeric", month: "long", timeZone: "UTC" });
> 
> Maybe try without the UTC here?

Try this with Date.UTC which is also done in the full year branch:
+        date = new Date(Date.UTC(saneBirthYear(year), month - 1, day));
+        dateStr = date.toLocaleDateString(undefined,
+                      { day: "numeric", month: "long", timeZone: "UTC" });
That works. I just saw the existing line:
dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
Can we change the [] to undefined for consistency.
Yeah it seems currently also in m-c they either set a locale or use undefined:
https://dxr.mozilla.org/comm-central/search?q=toLocaleDateString(&redirect=false
(In reply to Jorg K (GMT+1) from comment #29)
> Aleth, are you sure you need that ToLocaleFormat.jsm module? For mail/ and
> mailnews/ Aceman was able to eliminate the "variable" formats and use simple
> substitution and .toLocaleDateString() to cover all 13 call sites. I haven't
> looked into it to figure out where the variable format comes from in chat/.

The formatting module is only really needed for these 2 functions in imThemes.jsm:
   timeOpened: function(aConv, aFormat) {
     let date = new Date(aConv.startDate / 1000);
     if (aFormat)
-      return date.toLocaleFormat(aFormat);
+      return ToLocaleFormat(aFormat, date);

   time: function(aMsg, aFormat) {
     let date = new Date(aMsg.time * 1000);
     if (aFormat)
-      return date.toLocaleFormat(aFormat);
+      return ToLocaleFormat(aFormat, date);
     return date.toLocaleTimeString();
   },

There the caller can request any (maybe even user defined) format string.

At least the timeOpened function is NOT used in base c-c. Maybe there are external themes calling it.
So in that case the ToLocaleFormat module can stay in chat/modules/ToLocaleFormat.jsm , it seems we do not need it elsewhere.
Aceman, can you please update the patch with
  date = new Date(Date.UTC(saneBirthYear(year), month - 1, day));
as discussed in comment #57 and then get this landed with 'undefined'.

We're holding up bug 1313659 and bug 1346549. The chat part has r+ and you found out that the module is needed.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(makemyday)
Flags: needinfo?(aleth)
Assignee: nobody → acelists

        Attachment #8847362 -
        Attachment is obsolete: true

        Attachment #8850224 -
        Flags: review?(jorgk)

    
    

    
  
Comment on attachment 8850224 [details] [diff] [review]
mail/mailnews v1.3

I've seen this so many times, I can recite it in my sleep ;-)

        Attachment #8850224 -
        Flags: review?(jorgk) → review+
Blocks: 1313659

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: