Closed Bug 1383457 Opened 2 years ago Closed 2 years ago

Use mozIntl for formatting dates and times in SeaMonkey

Categories

(SeaMonkey :: General, enhancement)

enhancement
Not set

Tracking

(seamonkey2.53 fixed, seamonkey2.54 fixed)

RESOLVED FIXED
seamonkey2.54
Tracking Status
seamonkey2.53 --- fixed
seamonkey2.54 --- fixed

People

(Reporter: frg, Assigned: wgianopoulos)

References

Details

Attachments

(1 file, 13 obsolete files)

16.30 KB, patch
frg
: review+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
+++ 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.
Flags: needinfo?(frgrahl)
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
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.
(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.
Assignee: nobody → wgianopoulos
Status: NEW → ASSIGNED
Attachment #8892135 - Flags: review?(frgrahl)
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.
fixed pageInfo.js as well
Attachment #8892135 - Attachment is obsolete: true
Attachment #8892135 - Flags: review?(frgrahl)
Attachment #8892143 - Flags: review?(frgrahl)
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 `
Attachment #8892135 - Attachment is obsolete: false
Attachment #8892135 - Flags: feedback+
You've attached the same patch twice, same size.
(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.
Attachment #8892135 - Attachment is obsolete: true
(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.
Fix pageinfo and remove stray ` character.
Attachment #8892143 - Attachment is obsolete: true
Attachment #8892143 - Flags: review?(frgrahl)
Attachment #8892162 - Attachment is patch: true
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?
This includes page info and cookie viewer.
Attachment #8892162 - Attachment is obsolete: true
Attachment #8892169 - Flags: review?(frgrahl)
Wrong patch. It does not include the cookie viewer. Also, there is still a change in the feed writer file.
(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.
Adds back a line inadvertently removed form the history tree viewer.
Attachment #8892169 - Attachment is obsolete: true
Attachment #8892169 - Flags: review?(frgrahl)
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?
(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.
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?
(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.
(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.
(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.
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.
Attachment #8892847 - Attachment is obsolete: true
Attachment #8892919 - Flags: review?(frgrahl)
For anyone playing along at home, Windows and Linux builds including this patch are available at https://www.wg9s.com/mozilla/seamonkey/
Comment on attachment 8892919 [details] [diff] [review]
1383457-use-mozintl-to-format-date-and-times

Perfect.
Attachment #8892919 - Flags: feedback+
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+
Attachment #8892919 - Flags: feedback+
(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.
(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 ?
Fixes indentation. Not sure the other issuet is valid.
Attachment #8892919 - Attachment is obsolete: true
Attachment #8892919 - Flags: review?(frgrahl)
Attachment #8893023 - Flags: review?(frgrahl)
Attachment #8893023 - Flags: feedback?(iann_bugzilla)
Attachment #8893023 - Attachment is patch: true
Fixes indentation issues form :iann.
Attachment #8893023 - Attachment is obsolete: true
Attachment #8893023 - Flags: review?(frgrahl)
Attachment #8893023 - Flags: feedback?(iann_bugzilla)
Attachment #8893026 - Flags: review?(frgrahl)
Needinfo on comment 28
Flags: needinfo?(frgrahl) → needinfo?(iann_bugzilla)
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 I think this is actually the correct patch.
Attachment #8893026 - Attachment is obsolete: true
Attachment #8893026 - Flags: review?(frgrahl)
Attachment #8893032 - Flags: review?(frgrahl)
Attachment #8893032 - Flags: feedback?(iann_bugzilla)
(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?
(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.
(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.
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 ;-)
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.
(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.
(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. .
BTW.  I did attach an interdiff to bug 1378075 because i thought it would be helpful.
One more change :iann found a typo.
Attachment #8893051 - Flags: review?(frgrahl)
Attachment #8893032 - Attachment is obsolete: true
Attachment #8893032 - Flags: review?(frgrahl)
Attachment #8893032 - Flags: feedback?(iann_bugzilla)
(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.
(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 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+.
Attachment #8893051 - Flags: review?(frgrahl) → feedback+
(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.
> 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.
(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?
> could you post the exact URL to paste 

You already quoted it in comment 46: 

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

:)
(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.
(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.
Flags: needinfo?(jorgk)
Fixes cookieViewer issue.
Attachment #8893051 - Attachment is obsolete: true
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.
Flags: needinfo?(jorgk)
OK I will do what you did in TB with year.  Thanks for the input.
And this one fixes your second issue.
Attachment #8894359 - Attachment is obsolete: true
Attachment #8894363 - Flags: review?(frgrahl)
That patch did not seem to be correct.  I will ask for review after this one actually builds and seems to work.
Attachment #8894363 - Attachment is obsolete: true
Attachment #8894363 - Flags: review?(frgrahl)
(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 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
Attachment #8894366 - Flags: review?(jorgk)
Attachment #8894366 - Flags: review?(frgrahl)
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.
Attachment #8894366 - Flags: review?(jorgk) → review+
This addresses jorgk's review comments.
Attachment #8894366 - Attachment is obsolete: true
Attachment #8894366 - Flags: review?(frgrahl)
Attachment #8894469 - Flags: review?(frgrahl)
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
Flags: needinfo?(iann_bugzilla)
Attachment #8894469 - Flags: review?(frgrahl)
Attachment #8894469 - Flags: review+
Attachment #8894469 - Flags: approval-comm-beta?
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/f917e4b331ab
Use mozIntl for formatting dates and times in SeaMonkey. r=frg
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.54
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
Attachment #8894469 - Flags: approval-comm-beta? → approval-comm-beta+
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.
(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.
> IF it helps we could certainly land on beta sans the string removal.

Removals are usually not critical.
You need to log in before you can comment on or make changes to this bug.