1383457 - Use mozIntl for formatting dates and times in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Frank-Rainer Grahl (:frg)]

+++ This bug was initially created as a clone of Bug #1376167 +++

Followup bug. Jorg did raise a few points in Bug #1376167 and we should use mozIntl for formatting dates, times and timestamp in SeaMonkey.

Comment 1

[Jorg K (CEST = GMT+2)]

It just took me by surprise that FF cookies don't use mozIntl (see bug 1383463), but TB does:
https://hg.mozilla.org/comm-central/rev/a78c4ac9f53c#l3.31

Comment 2

[Jorg K (CEST = GMT+2)]

You need to revisit this as well:
https://hg.mozilla.org/comm-central/rev/a73acae42ace1e85a82818ec51b9690712a0bf57#l1.15
That used to display seconds, but timeStyle: "short" doesn't give you seconds. You should have used "long" here.

Comment 3

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #2)
> You need to revisit this as well:
> https://hg.mozilla.org/comm-central/rev/
> a73acae42ace1e85a82818ec51b9690712a0bf57#l1.15
> That used to display seconds, but timeStyle: "short" doesn't give you
> seconds. You should have used "long" here.

Not sure what I was smoking when I missed that.  I filed a followup patch on the other bug.

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Comment 5

[Bill Gianopoulos [:WG9s]]

THere were other changes I was going to make but it seems others decided to fix things so I am largely confused so has no fix for pageinfo bookmarks or permissions/cookieviewer.

Comment 6

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

fixed pageInfo.js as well

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8892135 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

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

Looks right. What about page info and cookies? I fixed those two for FF (bug 1383463), but not SM.
https://dxr.mozilla.org/comm-central/rev/f593d2d106c15ad9682a35eaa1a8679e7127acd6/suite/common/permissions/cookieViewer.js#170
https://dxr.mozilla.org/comm-central/rev/f593d2d106c15ad9682a35eaa1a8679e7127acd6/suite/browser/pageinfo/pageInfo.js#1314
still need fixing.

::: suite/common/dataman/dataman.js
@@ +2850,5 @@
>        // See bug 238045 for details.
>        let dtString = "";
>        try {
> +        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +          dateStyle: "long", timeStyle: "long"

Try dateStyle: "full" to see whether it give you a weekday.

::: suite/feeds/src/FeedWriter.js
@@ +280,5 @@
>    __dateFormatter: null,
>    get _dateFormatter() {
>      if (!this.__dateFormatter) {
>        const dtOptions = { timeStyle: "short", dateStyle: "long" };
> +      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);`

Editing error, lose the `

Comment 8

[Jorg K (CEST = GMT+2)]

You've attached the same patch twice, same size.

Comment 9

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #8)
> You've attached the same patch twice, same size.
 oribably accidently did a relaod sorry.

(In reply to Jorg K (GMT+2) from comment #7)
> Comment on attachment 8892135 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> Review of attachment 8892135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks right. What about page info and cookies? I fixed those two for FF (bug
> 1383463), but not SM.
> https://dxr.mozilla.org/comm-central/rev/
> f593d2d106c15ad9682a35eaa1a8679e7127acd6/suite/common/permissions/
> cookieViewer.js#170
> https://dxr.mozilla.org/comm-central/rev/
> f593d2d106c15ad9682a35eaa1a8679e7127acd6/suite/browser/pageinfo/pageInfo.
> js#1314
> still need fixing.
> 
> ::: suite/common/dataman/dataman.js
> @@ +2850,5 @@
> >        // See bug 238045 for details.
> >        let dtString = "";
> >        try {
> > +        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> > +          dateStyle: "long", timeStyle: "long"
> 
> Try dateStyle: "full" to see whether it give you a weekday.
> 
> ::: suite/feeds/src/FeedWriter.js
> @@ +280,5 @@
> >    __dateFormatter: null,
> >    get _dateFormatter() {
> >      if (!this.__dateFormatter) {
> >        const dtOptions = { timeStyle: "short", dateStyle: "long" };
> > +      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);`
> 
> Editing error, lose the `

The ` was a cut and paste error.  I will look at full versus long.  I would love to find something that emulates the way it did before timezone under linux and weekday under windows if full does that great if it does not, then I bet some linux user will complain about why display the weekday.  i will work on it and if it does what I want I will make all of the date longs changed to date full.  otherwise i wonder if to make it work as before is it worth it to test for os and adjust the display accordingly.

Comment 10

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #9)
> (In reply to Jorg K (GMT+2) from comment #8)
> > You've attached the same patch twice, same size.
>  oribably accidently did a relaod sorry.

tried to say i accidentally did a reload I guess.

Comment 11

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Fix pageinfo and remove stray ` character.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8892162 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

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

You're still missing:
https://dxr.mozilla.org/comm-central/rev/f593d2d106c15ad9682a35eaa1a8679e7127acd6/suite/common/permissions/cookieViewer.js#170

::: suite/feeds/src/FeedWriter.js
@@ +280,5 @@
>    __dateFormatter: null,
>    get _dateFormatter() {
>      if (!this.__dateFormatter) {
>        const dtOptions = { timeStyle: "short", dateStyle: "long" };
> +      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);

Why does this line appear in the patch? It's not changed, is it?

Comment 13

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

This includes page info and cookie viewer.

Comment 14

[Jorg K (CEST = GMT+2)]

Wrong patch. It does not include the cookie viewer. Also, there is still a change in the feed writer file.

Comment 15

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #14)
> Wrong patch. It does not include the cookie viewer. Also, there is still a
> change in the feed writer file.

I am sorry.  I was trying to get something done today and obviously I was not ready.  I will have a proper tested patch that addresses everything ready by the end of the week.

Comment 16

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Adds back a line inadvertently removed form the history tree viewer.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8892847 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

This looks great know with one nit and one question:

Nit: There is no change in FeedWriter.js
-      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);
+      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);
so I don't see why that hunk is still in your patch.

Question: Have you tried whether dateStyle: "full" gives you a weekday?

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #17)
> Comment on attachment 8892847 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> This looks great know with one nit and one question:
> 
> Nit: There is no change in FeedWriter.js
> -      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined,
> dtOptions);
> +      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined,
> dtOptions);
> so I don't see why that hunk is still in your patch.


Ah OK I did not understand your question about feedwriter the first time, so I thought it was perhaps on the previous patch iteration which was missing the feedwriter part altogether.  I will compare what I did with what is currently on mozilla-central.

> 
> Question: Have you tried whether dateStyle: "full" gives you a weekday?

dateStyle: "long" gives me a weekday under Windows and NOT under Linux.  This is exactly the behavior under sIScriptableDateFormat.  I have verified that dateStyle: "full: gives a weekday under Linux.  I am just not sure a decision has been made to add Weekday display under Linux.

Comment 19

[Jorg K (CEST = GMT+2)]

In FeedWriter.js you're removing a line that you then add again. The code is already correct from bug 1376167.

Thanks for the "long"/"full" test, my feeling is that the platforms should be consistent. So |weekday: "short"| as previously used didn't always display a weekday?

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #19)
> In FeedWriter.js you're removing a line that you then add again. The code is
> already correct from bug 1376167.
> 
> Thanks for the "long"/"full" test, my feeling is that the platforms should
> be consistent. So |weekday: "short"| as previously used didn't always
> display a weekday?

no, the function i was using previously altered the behavior to display a short weekday under both because it seemed not worth the patch complication to mimic the original behavior.  SO I shortened the weekday as a compromise because of adding the timezone name (which formerly only displayed under Linux) .  I was worried making the string longer might cause formatting issues.

Can you get together with Frank and whoever else you might think should weigh in on the change?  It is obviously a simple edit to the patch to make it say full.  I could add a needinfo, but I don;t know enough about SeaMonkey to have a clue who the right people to ask might be.

Comment 21

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #17)
> Comment on attachment 8892847 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> This looks great know with one nit and one question:
> 
> Nit: There is no change in FeedWriter.js
> -      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined,
> dtOptions);
> +      this.__dateFormatter = Services.intl.createDateTimeFormat(undefined,
> dtOptions);
> so I don't see why that hunk is still in your patch.

Oh I see what you mean now.  Not sure how that got in there.

Comment 22

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #19)
> Thanks for the "long"/"full" test, my feeling is that the platforms should
> be consistent. So |weekday: "short"| as previously used didn't always
> display a weekday?

I like the idea of the platfrom being consistent also, but this still does not yield that because Linux displays the time zone and windows does not.  Let me check if there is also a timeStyle: "full" and if that displays the timezone under both platforms.

Comment 23

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Remove extraneous FeedWriter.js diff and change dateStyle from "long" to "full".

I decided I liked the constancy also.  timeStyle "full" ended up being a bust.

Comment 24

[Bill Gianopoulos [:WG9s]]

For anyone playing along at home, Windows and Linux builds including this patch are available at https://www.wg9s.com/mozilla/seamonkey/

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8892919 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

Perfect.

Comment 26

[Ian Neal]

Comment on attachment 8892919 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

>+++ b/suite/common/dataman/dataman.js
>@@ -974,21 +974,20 @@ var gCookies = {
>     if (aExpires) {
>       let date = new Date(1000 * aExpires);
> 
>       // If a server manages to set a really long-lived cookie, the dateservice
>       // can't cope with it properly, so we'll just return a blank string.
>       // See bug 238045 for details.
>       let expiry = "";
>       try {
>-        const dtOptions = { year: "numeric", month: "long", day: "numeric",
>-                            hour: "numeric", minute: "numeric",
>-                            second: "numeric", timeZoneName: "short",
>-                            weekday: "short" };
>-        expiry =  date.toLocaleString(undefined, dtOptions);
>+        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
>+        dateStyle: "full", timeStyle: "long"
>+      });
Nit: this indentation doesn't match what is in other places. Saying that it might be better to indent the date/time style line as far as two spaces after the = in all places, so like:
+        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
+                                    dateStyle: "full", timeStyle: "long"
+        });




>+++ b/suite/common/downloads/treeView.js
>+      const dtOptions = { timeStyle "short" };
Nit: seems to be missing a :
>+      this.__todayFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);

Almost there f+

Comment 27

[Bill Gianopoulos [:WG9s]]

(In reply to Ian Neal from comment #26)
> Comment on attachment 8892919 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> >+++ b/suite/common/dataman/dataman.js
> >@@ -974,21 +974,20 @@ var gCookies = {
> >     if (aExpires) {
> >       let date = new Date(1000 * aExpires);
> > 
> >       // If a server manages to set a really long-lived cookie, the dateservice
> >       // can't cope with it properly, so we'll just return a blank string.
> >       // See bug 238045 for details.
> >       let expiry = "";
> >       try {
> >-        const dtOptions = { year: "numeric", month: "long", day: "numeric",
> >-                            hour: "numeric", minute: "numeric",
> >-                            second: "numeric", timeZoneName: "short",
> >-                            weekday: "short" };
> >-        expiry =  date.toLocaleString(undefined, dtOptions);
> >+        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> >+        dateStyle: "full", timeStyle: "long"
> >+      });
> Nit: this indentation doesn't match what is in other places. Saying that it
> might be better to indent the date/time style line as far as two spaces
> after the = in all places, so like:
> +        const dateTimeFormatter =
> Services.intl.createDateTimeFormat(undefined, {
> +                                    dateStyle: "full", timeStyle: "long"
> +        });
>

Indentation often seems kind of problematic the indentation done elsewhere results in so much indentation that if you try to say withing 80 characters the code becomes hard to follwo.  I will try to come up with either something that follows the norm or prsents a better compromise if I cannon.t

> 
> 
> 
> >+++ b/suite/common/downloads/treeView.js
> >+      const dtOptions = { timeStyle "short" };
> Nit: seems to be missing a :
> >+      this.__todayFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);
> 
> Almost there f+

I will fix this.  I will also check all of the other things based on the tree view patch. they were all supposed to be the same so a screwup here one would think was either a typo or should be in more places.

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to Ian Neal from comment #26)
> Comment on attachment 8892919 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> >+++ b/suite/common/dataman/dataman.js
> >@@ -974,21 +974,20 @@ var gCookies = {
> >     if (aExpires) {
> >       let date = new Date(1000 * aExpires);
> > 
> >       // If a server manages to set a really long-lived cookie, the dateservice
> >       // can't cope with it properly, so we'll just return a blank string.
> >       // See bug 238045 for details.
> >       let expiry = "";
> >       try {
> >-        const dtOptions = { year: "numeric", month: "long", day: "numeric",
> >-                            hour: "numeric", minute: "numeric",
> >-                            second: "numeric", timeZoneName: "short",
> >-                            weekday: "short" };
> >-        expiry =  date.toLocaleString(undefined, dtOptions);
> >+        const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> >+        dateStyle: "full", timeStyle: "long"
> >+      });
> Nit: this indentation doesn't match what is in other places. Saying that it
> might be better to indent the date/time style line as far as two spaces
> after the = in all places, so like:
> +        const dateTimeFormatter =
> Services.intl.createDateTimeFormat(undefined, {
> +                                    dateStyle: "full", timeStyle: "long"
> +        });
> 


> >+++ b/suite/common/downloads/treeView.js
> >+      const dtOptions = { timeStyle "short" };
> Nit: seems to be missing a :
> >+      this.__todayFormatter = Services.intl.createDateTimeFormat(undefined, dtOptions);
> 
> Almost there f+

Isn;t this covered in get _todayFormatter ?

Comment 29

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Fixes indentation. Not sure the other issuet is valid.

Comment 30

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Fixes indentation issues form :iann.

Comment 31

[Bill Gianopoulos [:WG9s]]

Needinfo on comment 28

Comment 32

[Jorg K (CEST = GMT+2)]

Did you attach the previous patch again? Interdiff shows no differences. Also the patch size is exactly the same, 12.04 KB.

Also, it would be helpful to use a version count in the description field (not the file name) so interdiff works better.

Comment 33

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

OK I think this is actually the correct patch.

Comment 34

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #32)
> Did you attach the previous patch again? Interdiff shows no differences.
> Also the patch size is exactly the same, 12.04 KB.
> 
> Also, it would be helpful to use a version count in the description field
> (not the file name) so interdiff works better.

I know.  I have done that in the past.  it is just people coming in out of left field who have not really looked at previous patch is interdiff useful in that case?

Comment 35

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #32)
> Did you attach the previous patch again? Interdiff shows no differences.
> Also the patch size is exactly the same, 12.04 KB.
> 
> Also, it would be helpful to use a version count in the description field
> (not the file name) so interdiff works better.

OK so the real latest patch is only spacing changes.  no code change. I am not at all sure an interdiff would be helpful.

Comment 36

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #32)
> Did you attach the previous patch again? Interdiff shows no differences.
> Also the patch size is exactly the same, 12.04 KB.
> 
> Also, it would be helpful to use a version count in the description field
> (not the file name) so interdiff works better.

SO you want an interdiff on an indentation change requested by :iann??? Perhaps this is a reason why we have issues getting people to contribute.

Comment 37

[Jorg K (CEST = GMT+2)]

No offence intended but I do you interdiff a lot including for indentation changes. Using interdiff also revealed that you attached the same patches various times.

So instead of questions other people's working workflows, you might improve your own:
If you look at the obsolete attachments by size you can see that you made four needless submissions. Numbering patches is common practise, naming patches .patch or .diff so the system recognises them as patches and they don't get attached as text files is also common practise. It is also common practise, if you work with Mercurial queues, to do a 'qref' before attaching the patch, so hunks removing and adding the same line will not occur.

No offence intended, I've just tried to help you along with the patches, and as you said in bug 1376167 comment #48:
If you don't criticize, how am I going to learn anything?

The reason why we have issues getting people to contribute is manifold. Here are some contributing factors:
- You need decent hardware to do it, a compile can take an hour or more and
  downloading gigabytes of repositories isn't so much fun either.
- The code base it very complicated and there is a very steep learning curve and many
  Mozilla technologies to learn, like XPCOM, XUL, etc.
- It is hard to find where things happen in the code, although DXR helps.
- It is hard to do anything non-trivial.
- It is hard to get a review, sometimes it feels like playing "postal/correspondence" chess.
- Yes, coding standards are a pain, and white-space issues as well.

I'm not sure which one is the worst factor of them, but I guess someone requesting version numbers to do interdiff isn't ;-)

Comment 38

[Bill Gianopoulos [:WG9s]]

Was kind of an I had already figured out i had updated the patch on a system other than the one I uploaded from so I knew it was wrong  I was in the process of fixing so this kind of just seemed a bit harsh.

Comment 39

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #38)
> Was kind of an I had already figured out i had updated the patch on a system
> other than the one I uploaded from so I knew it was wrong  I was in the
> process of fixing so this kind of just seemed a bit harsh.

But then if I knew I had screwed up, I probably should just not have commented.

Comment 40

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #37)
> No offence intended but I do you interdiff a lot including for indentation
> changes. Using interdiff also revealed that you attached the same patches
> various times.
> 
> So instead of questions other people's working workflows, you might improve
> your own:
> If you look at the obsolete attachments by size you can see that you made
> four needless submissions. Numbering patches is common practise, naming
> patches .patch or .diff so the system recognises them as patches and they
> don't get attached as text files is also common practise. It is also common
> practise, if you work with Mercurial queues, to do a 'qref' before attaching
> the patch, so hunks removing and adding the same line will not occur.
> 
> No offence intended, I've just tried to help you along with the patches, and
> as you said in bug 1376167 comment #48:
> If you don't criticize, how am I going to learn anything?
> 
> The reason why we have issues getting people to contribute is manifold. Here
> are some contributing factors:
> - You need decent hardware to do it, a compile can take an hour or more and
>   downloading gigabytes of repositories isn't so much fun either.
> - The code base it very complicated and there is a very steep learning curve
> and many
>   Mozilla technologies to learn, like XPCOM, XUL, etc.
> - It is hard to find where things happen in the code, although DXR helps.
> - It is hard to do anything non-trivial.
> - It is hard to get a review, sometimes it feels like playing
> "postal/correspondence" chess.
> - Yes, coding standards are a pain, and white-space issues as well.
> 
> I'm not sure which one is the worst factor of them, but I guess someone
> requesting version numbers to do interdiff isn't ;-)

Sometimes those of us who are new feel pressured to get things posted before we think they are ready and make mistakes.  Please keep that in mind.  I think I  feel more pressure than there actually is and I should really make sure my patches have at least one day of being included in my automated builds before I attach them to a bug. .

Comment 41

[Bill Gianopoulos [:WG9s]]

BTW.  I did attach an interdiff to bug 1378075 because i thought it would be helpful.

Comment 42

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

One more change :iann found a typo.

Comment 43

[Jorg K (CEST = GMT+2)]

(In reply to Bill Gianopoulos [:WG9s] from comment #40)
> Sometimes those of us who are new feel pressured to get things posted before
> we think they are ready and make mistakes.
Haste make waste, as they say. I make mistakes all the time, but I'm usually quick to fix them since no one is watching anyway. Sorry for the pressure, I've just suffered through date/time formatting issues since Christmas 2016, yes, busted on Christmas day, and I had to fix it, so I'd really like to bring things to a close. They have become my pet hate.

Comment 44

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #43)
> (In reply to Bill Gianopoulos [:WG9s] from comment #40)
> > Sometimes those of us who are new feel pressured to get things posted before
> > we think they are ready and make mistakes.
> Haste make waste, as they say. I make mistakes all the time, but I'm usually
> quick to fix them since no one is watching anyway. Sorry for the pressure,
> I've just suffered through date/time formatting issues since Christmas 2016,
> yes, busted on Christmas day, and I had to fix it, so I'd really like to
> bring things to a close. They have become my pet hate.

And I overreacted.  I am sorry.  We are all striving towards the same goal here.  we need to work together.  (sounds like an ole Canned Heat song, you will find a lot of things remind me of old songs BTW)

Comment 45

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8893051 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

I know you might hate me now but 2 things:

> chrome://communicator/content/permissions/cookieViewer.xul

The cookie exiry data is not shown. Its shows correct in Data Manager. I wonder why. The code looks the same. 

> suite/mailnews/addrbook/abCardViewOverlay.js

has

> dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
>  dateStr = date.toLocaleFormat(gAddressBookBundle.getString("dateFormatMonthDay"));

I didn't check the Thunderbird code yet but could you look at it why it is different from ours here. 

f+ for the moment. If the expiry date not shown is caused by a different bug I will set r+.

Comment 46

[Bill Gianopoulos [:WG9s]]

(In reply to Frank-Rainer Grahl (:frg) from comment #45)
> Comment on attachment 8893051 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> I know you might hate me now but 2 things:
> 
> > chrome://communicator/content/permissions/cookieViewer.xul
> 
> The cookie exiry data is not shown. Its shows correct in Data Manager. I
> wonder why. The code looks the same. 
Can you tell me how to look at ccokies using the cookieviewer?  this is the only part of the code I did not test.  every obvious menu choice I could find led to the data manager.

Comment 47

[Frank-Rainer Grahl (:frg)]

> Can you tell me how to look at ccokies using the cookieviewer? 

Just put the chrome:// link into your location bar. It is no longer linked to ui code but still a great way to blow away cookies and get an overview. I have it in my bookmarks bar.

I suggest testing on comm-beta right now until Gecko 57 stabilizes and our 2.54 code too.

Comment 48

[Bill Gianopoulos [:WG9s]]

(In reply to Frank-Rainer Grahl (:frg) from comment #47)
> > Can you tell me how to look at ccokies using the cookieviewer? 
> 
> Just put the chrome:// link into your location bar. It is no longer linked
> to ui code but still a great way to blow away cookies and get an overview. I
> have it in my bookmarks bar.
> 
> I suggest testing on comm-beta right now until Gecko 57 stabilizes and our
> 2.54 code too.

To benefit the completely thick people  (like me), culd you post the exact URL to paste to the URLbar?

Comment 49

[Frank-Rainer Grahl (:frg)]

> could you post the exact URL to paste 

You already quoted it in comment 46: 

> chrome://communicator/content/permissions/cookieViewer.xul

:)

Comment 50

[Bill Gianopoulos [:WG9s]]

(In reply to Frank-Rainer Grahl (:frg) from comment #45)
> Comment on attachment 8893051 [details] [diff] [review]
> 1383457-use-mozintl-to-format-date-and-times
> 
> I know you might hate me now but 2 things:
> 
> > chrome://communicator/content/permissions/cookieViewer.xul
> 
> The cookie exiry data is not shown. Its shows correct in Data Manager. I
> wonder why. The code looks the same. 

I fixed the cookieViewer by adding this:

Components.utils.import("resource://gre/modules/Services.jsm");

Makes me ask are some of these other things expected to work with chrome url's?  If so perhaps this is needed elsewhere as well.

Comment 51

[Bill Gianopoulos [:WG9s]]

(In reply to Frank-Rainer Grahl (:frg) from comment #45)
> > suite/mailnews/addrbook/abCardViewOverlay.js
> 
> has
> 
> > dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
> >  dateStr = date.toLocaleFormat(gAddressBookBundle.getString("dateFormatMonthDay"));
> 
> I didn't check the Thunderbird code yet but could you look at it why it is
> different from ours here. 
> 
Asking for info on this.

Comment 52

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

Fixes cookieViewer issue.

Comment 53

[Jorg K (CEST = GMT+2)]

SM:
With year:
dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
That needs to be replaced with mozIntl like in TB.

Without year:
dateStr = date.toLocaleFormat(gAddressBookBundle.getString("dateFormatMonthDay"));
Hmm, there you have another string to translate, besides, in an en-US version you'll use the en-US string and you have no chance to set the OS regional setting. I'd lose this.

TB:
With year:
formatter = Services.intl.createDateTimeFormat(undefined, { dateStyle: "long", timeZone: "UTC" });
dateStr = formatter.format(date);
That's correct.

Without year:
formatter = Services.intl.createDateTimeFormat(undefined, { month: "long", day: "numeric", timeZone: "UTC" });
dateStr = formatter.format(date);
That is suboptimal since mozIntl can only format full dates. So if you're using en-US with en-GB in the OS, you still get
month / day, which is terrible. SM's string solution doesn't fix that, it only makes it worse.

Comment 54

[Bill Gianopoulos [:WG9s]]

OK I will do what you did in TB with year.  Thanks for the input.

Comment 55

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

And this one fixes your second issue.

Comment 56

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

That patch did not seem to be correct.  I will ask for review after this one actually builds and seems to work.

Comment 57

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #53)
> SM:
> With year:
> dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
> That needs to be replaced with mozIntl like in TB.
> 
> Without year:
> dateStr =
> date.toLocaleFormat(gAddressBookBundle.getString("dateFormatMonthDay"));
> Hmm, there you have another string to translate, besides, in an en-US
> version you'll use the en-US string and you have no chance to set the OS
> regional setting. I'd lose this.
> 
> TB:
> With year:
> formatter = Services.intl.createDateTimeFormat(undefined, { dateStyle:
> "long", timeZone: "UTC" });
> dateStr = formatter.format(date);
> That's correct.
> 
> Without year:
> formatter = Services.intl.createDateTimeFormat(undefined, { month: "long",
> day: "numeric", timeZone: "UTC" });
> dateStr = formatter.format(date);
> That is suboptimal since mozIntl can only format full dates. So if you're
> using en-US with en-GB in the OS, you still get
> month / day, which is terrible. SM's string solution doesn't fix that, it
> only makes it worse.

After I actually get a patch that builds I will ask for you to review the suite/mailnews portion of the patch.

Comment 58

[Bill Gianopoulos [:WG9s]]

Comment on attachment 8894366 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

Looking for general review from frg and the suite/mailnews part from jorgk

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 8894366 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

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

I have nothing to say in suite/, but it looks good to me - and I've seen this patch a felt million times ;-)

r=jorgk with the issue below fixed.

::: suite/mailnews/addrbook/abCardViewOverlay.js
@@ -259,5 @@
> -        // toLocaleFormat() seems to have a different implementation than
> -        // toLocaleDateString() and returns correct results even when not
> -        // passing UTC times; besides, it would not support an options
> -        // parameter for {timeZone: "UTC"} anyway.
> -        dateStr = date.toLocaleFormat(gAddressBookBundle.getString("dateFormatMonthDay"));

Please remove the string from suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties.

Comment 60

[Bill Gianopoulos [:WG9s]]

Attachment: 1383457-use-mozintl-to-format-date-and-times

This addresses jorgk's review comments.

Comment 61

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8894469 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

This looks and works fine. Thanks.

Lets take it to beta too.

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: inconsistent locale display
Testing completed (on m-c, etc.): c-b
Risk to taking this patch (and alternatives if risky): none
String changes made by this patch: removes dateFormatMonthDay in /suite/locales/en-US/chrome/mailnews/addressbook/addressBook.properties

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f917e4b331ab
Use mozIntl for formatting dates and times in SeaMonkey. r=frg

Comment 63

[Ian Neal]

Comment on attachment 8894469 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

a=me even though there is a string change it is only a removal

Comment 64

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-beta/rev/ee9edfba616b548d8c41e8ac06ad496fe198c745

> even though there is a string change it is only a removal

I think after aurora got removed l10n changes in the beta tree are now more common anyway. We probably need to figure out the current policy the first time we want to add or change a string.

Comment 65

[Bill Gianopoulos [:WG9s]]

(In reply to Frank-Rainer Grahl (:frg) from comment #64)
> https://hg.mozilla.org/releases/comm-beta/rev/
> ee9edfba616b548d8c41e8ac06ad496fe198c745
> 
> > even though there is a string change it is only a removal
> 
> I think after aurora got removed l10n changes in the beta tree are now more
> common anyway. We probably need to figure out the current policy the first
> time we want to add or change a string.

IF it helps we could certainly land on beta sans the string removal.  It would just leave an unused string there.

Comment 66

[Frank-Rainer Grahl (:frg)]

> IF it helps we could certainly land on beta sans the string removal.

Removals are usually not critical.