1376167 - Use Intl.DateTimeFormat or similar instead of nsIScriptableDateFormat in SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Frank-Rainer Grahl (:frg)]

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

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

Comment 1

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

This handles all of the .js files affected b this.  I am still trying to figure out what to do with suite/common/console/consoleBindings.xml.

Comment 2

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

Oops a typo.

Comment 3

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

I thought I had fixed both typos there.

Comment 4

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

One more try I think i should just decide I am too tired to work on this today.

Comment 5

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-remove-console.xml-timestamp-formatting.diff

In my opinion, this is the simplest thing to do and possibly good enough.  It just leaves the timestamps when you copy paste entries out of the Console to another app to be in UNIX timestamp format with no conversion.

Comment 6

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-remove-console.xml-timestamp-formatting.patch

Comment 7

[Ian Neal]

Comment on attachment 8886815 [details] [diff] [review]
1376167-avoid-nsISrciptableDateFormat-in-js.patch

>+++ b/suite/common/bookmarks/bookmarksManager.js
>+    const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                   .getService(Components.interfaces.nsIXULChromeRegistry)
>+                   .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+++ b/suite/common/dataman/dataman.js
>+        const dtOptions = { year: "numeric", month: "long", day: "numeric",
>+                        hour: "numeric", minute: "numeric", second: "numeric",
>+                        timeZoneName: "short" };
Nit: Should be lined up with start of year

>+        const dtOptions = { year: "numeric", month: "long", day: "numeric",
>+                        hour: "numeric", minute: "numeric", second: "numeric",
>+                        timeZoneName: "short" };
Nit: Should be lined up with start of year

>+++ b/suite/common/downloads/treeView.js

>+  __todayFormatter: null,
>+  get _todayFormatter() {
>+    if (!this.__todayFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                     .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+  __dateFormatter: null,
>+  get _dateFormatter() {
>+    if (!this.__dateFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                     .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+++ b/suite/common/history/treeView.js

>+  const MS_PER_MINUTE = 60000;
Nit: indentation needs correcting
>+    const MS_PER_DAY = 86400000;

>+  __todayFormatter: null,
>+  get _todayFormatter() {
>+    if (!this.__todayFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                    .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+  __dateFormatter: null,
>+  get _dateFormatter() {
>+    if (!this.__dateFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                     .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+++ b/suite/common/permissions/cookieViewer.js

>+      const dtOptions = { year: "numeric", month: "long", day: "numeric",
>+                      hour: "numeric", minute: "numeric", second: "numeric",
>+                      timeZoneName: "short" };
Nit: Should be lined up with start of year

>+++ b/suite/common/places/treeView.js
>+  __todayFormatter: null,
>+  get _todayFormatter() {
>+    if (!this.__todayFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                     .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+  get _dateFormatter() {
>+    if (!this.__dateFormatter) {
>+      const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
>+                     .getService(Components.interfaces.nsIXULChromeRegistry)
>+                     .getSelectedLocale("global", true);
Nit: should be lined up with . of .classes

>+++ b/suite/feeds/src/FeedWriter.js

>+  get _dateFormatter() {
>+    if (!this.__dateFormatter) {
>+      const dtOptions = {
>+        timeStyle: "short",
>+        dateStyle: "long"
>+      };
Why aren't these on a single line? i.e.
>+      const dtOptions = { timeStyle: "short", dateStyle: "long" };

r=me with those addressed

Comment 8

[Ian Neal]

Comment on attachment 8886815 [details] [diff] [review]
1376167-avoid-nsISrciptableDateFormat-in-js.patch

Sorry should have been an f+ rather than an r+

Comment 9

[Bill Gianopoulos [:WG9s]]

I know understand my confusion about your complaint about lack of the day of the week.  I was doing my testing under Linux and you were using Windows.
It appears the old code displayed the Day of the week under Windows but not under Linux and the timezone name under Linux but not Windows.
They modified Firefox to display neither, but I changed the SeaMonkey code to mimic the previous Linux behavior.
What do you think I should do here?

Comment 10

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

Addressing Ian's feedback comments.

Comment 11

[Frank-Rainer Grahl (:frg)]

>>
It appears the old code displayed the Day of the week under Windows but not under Linux and the timezone name under Linux but not Windows.
<<

Ahh ok. Maybe stefanh can look at it under OSX.

FRG

Comment 12

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

This re-adds the weekday, yet in short format.  I think adding both the day-of-week and timezone in short format is the best compromise to get this to display what was formerly displayed.

Comment 13

[Frank-Rainer Grahl (:frg)]

I just did a quick check for the latest patch. 

gLocSvc got accidently removed from suite/common/dataman/dataman.js. Needs to be added back for the Data Manager to work.

>  // locally loaded services
> +var gLocSvc = {};
>  XPCOMUtils.defineLazyModuleGetter(gLocSvc, "FormHistory",
>                                    "resource://gre/modules/FormHistory.jsm",
>                                   "FormHistory");

Comment 14

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsISrciptableDateFormat-in-js.patch

Ooops I deleted an extra line by mistake.

Comment 15

[Stefan [:stefanh]]

(In reply to Frank-Rainer Grahl (:frg) from comment #11)
> >>
> It appears the old code displayed the Day of the week under Windows but not
> under Linux and the timezone name under Linux but not Windows.
> <<
> 
> Ahh ok. Maybe stefanh can look at it under OSX.
> 
> FRG

I'm a bit unsure what you wanted me to check, but timestamps in the Error Console (2.48b, sv-SE locale) looks like this:
2017-07-20 22:27:08

Which is basically what I'd expect.

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to Stefan [:stefanh] from comment #15)
> (In reply to Frank-Rainer Grahl (:frg) from comment #11)
> > >>
> > It appears the old code displayed the Day of the week under Windows but not
> > under Linux and the timezone name under Linux but not Windows.
> > <<
> > 
> > Ahh ok. Maybe stefanh can look at it under OSX.
> > 
> > FRG
> 
> I'm a bit unsure what you wanted me to check, but timestamps in the Error
> Console (2.48b, sv-SE locale) looks like this:
> 2017-07-20 22:27:08
> 
> Which is basically what I'd expect.

No the issue is that once nsISriptableDateFormat disappears there is not a good way to format dates anymore via xml.  Do you have any idea as to how to accomplish date formatting once that happens or any idea as to what to do if that is not possible?

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to Bill Gianopoulos [:WG9s] from comment #16)
> (In reply to Stefan [:stefanh] from comment #15)
> > (In reply to Frank-Rainer Grahl (:frg) from comment #11)
> > > >>
> > > It appears the old code displayed the Day of the week under Windows but not
> > > under Linux and the timezone name under Linux but not Windows.
> > > <<
> > > 
> > > Ahh ok. Maybe stefanh can look at it under OSX.
> > > 
> > > FRG
> > 
> > I'm a bit unsure what you wanted me to check, but timestamps in the Error
> > Console (2.48b, sv-SE locale) looks like this:
> > 2017-07-20 22:27:08
> > 
> > Which is basically what I'd expect.
> 
> No the issue is that once nsISriptableDateFormat disappears there is not a
> good way to format dates anymore via xml.  Do you have any idea as to how to
> accomplish date formatting once that happens or any idea as to what to do if
> that is not possible?

(In reply to Stefan [:stefanh] from comment #15)
> (In reply to Frank-Rainer Grahl (:frg) from comment #11)
> > >>
> > It appears the old code displayed the Day of the week under Windows but not
> > under Linux and the timezone name under Linux but not Windows.
> > <<
> > 
> > Ahh ok. Maybe stefanh can look at it under OSX.
> > 
> > FRG
> 
> I'm a bit unsure what you wanted me to check, but timestamps in the Error
> Console (2.48b, sv-SE locale) looks like this:
> 2017-07-20 22:27:08
> 
> Which is basically what I'd expect.

Oh but the other question is what does the date of last modification look like in pageinfo under OSX.

Comment 18

[Bill Gianopoulos [:WG9s]]

ON the pageinfo question it is under Windows it displays the day of the week but not any timezone info and under Linux it displays the timezone but no day of the week.  We were wondering how it displays under OSX to figure out what to do for a unifying the display under all platforms.

Comment 19

[Stefan [:stefanh]]

Well, for me it looks like this:
20 juli 2017 22:51:50

Comment 20

[Bill Gianopoulos [:WG9s]]

(In reply to Stefan [:stefanh] from comment #19)
> Well, for me it looks like this:
> 20 juli 2017 22:51:50

thanks so no timezone nor weekday which is what firefox decided on.  Ok we think we are going for short weekday and short timezone as a compromise between what windows did with long timezone and linux did with short timezone.

Comment 21

[Bill Gianopoulos [:WG9s]]

I meant long weekday for windows is what previously displayed.
so short both takes less pace so should not cause any formatting issues because of field widths.

Comment 22

[Stefan [:stefanh]]

Is there any reason for displaying time zone if the time displayed is local time? Note also that there's probably region-specific formatting in place here.

Comment 23

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #22)
> Note also that there's probably region-specific formatting in place
> here.
Ah, but I guess that's taken care of with Locale negotiation.

Comment 24

[Bill Gianopoulos [:WG9s]]

(In reply to Stefan [:stefanh] from comment #22)
> Is there any reason for displaying time zone if the time displayed is local
> time? Note also that there's probably region-specific formatting in place
> here.

The reason for displaying the timezone was mostly that it always did under Linux.  Also it is a bit ambiguous with no timezone displayed does that mean it is localtime or does that mean it is UTC?  It is also helpful if the info is cut-and-pasted into a bug because other reading the bug do not have to guess as to what your local timezone is.

Myself, I see no benefit in displaying the day of the week, but got pushback about NOT including it because evidently it always did display under Windows.

Comment 25

[Bill Gianopoulos [:WG9s]]

Comment on attachment 8886847 [details] [diff] [review]
1376167-remove-console.xml-timestamp-formatting.patch

[e-mail] thought you might have more insight into how to fix this better.

Comment 26

[Bill Gianopoulos [:WG9s]]

Comment on attachment 8886847 [details] [diff] [review]
1376167-remove-console.xml-timestamp-formatting.patch

This worked well for Firefox where the Error Console is no longer supported.  However, we have this in SeaMonkey consoleBindings.xml.  do you have any idea how to handle this?

Comment 27

[Jonathan Kew [:jfkthame]]

What exactly are the requirements for formatting the timestamp? Presumably you can just call .toLocaleString() on the timestamp, passing a set of appropriate options to get the format desired.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString

Comment 28

[Bill Gianopoulos [:WG9s]]

(In reply to Jonathan Kew (:jfkthame) from comment #27)
> What exactly are the requirements for formatting the timestamp? Presumably
> you can just call .toLocaleString() on the timestamp, passing a set of
> appropriate options to get the format desired.
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Date/toLocaleDateString

The issue is this is NOT JavaScript.  this is xml only  how do I format this now that the api that used to be available to do this no longer exists form an .xml file.

Comment 29

[Jonathan Kew [:jfkthame]]

I don't think I understand your question. AFAICS, what we have here is an XML file that contains fragments of JS; so can't you simply use JS methods such as Date.toLocaleString() within those fragments?

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8886847 [details] [diff] [review]
1376167-remove-console.xml-timestamp-formatting.patch

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

::: suite/common/console/consoleBindings.xml
@@ -227,5 @@
>            row.setAttribute("typetext", this.mStrBundle.getString(typetext));
>            row.setAttribute("type", type);
>            row.setAttribute("msg", aObject.errorMessage);
>            row.setAttribute("category", aObject.category);
> -          row.setAttribute("time", this.properFormatTime(aObject.timeStamp));

I assume this code worked, so the method properFormatTime() was executed. So why don't you just change that method?

@@ -274,5 @@
>            Services.console.reset();
>          ]]></body>
>        </method>
>  
> -      <method name="properFormatTime">

As I said above, instead of removing the method, I'd just change it. Either use toLocaleString() or if this is something that shows somewhere in the UI, you should use mozIntl, for example like here:
https://dxr.mozilla.org/comm-central/rev/d9846d89641fc3c57e25f22ff9fcdeafc2b4e54e/calendar/resources/content/datetimepickers/datetimepickers.xml#566
More examples here:
https://dxr.mozilla.org/comm-central/search?q=formatter.format(&redirect=false

Comment 31

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #30)
> Comment on attachment 8886847 [details] [diff] [review]
> 1376167-remove-console.xml-timestamp-formatting.patch
> 
> Review of attachment 8886847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: suite/common/console/consoleBindings.xml
> @@ -227,5 @@
> >            row.setAttribute("typetext", this.mStrBundle.getString(typetext));
> >            row.setAttribute("type", type);
> >            row.setAttribute("msg", aObject.errorMessage);
> >            row.setAttribute("category", aObject.category);
> > -          row.setAttribute("time", this.properFormatTime(aObject.timeStamp));
> 
> I assume this code worked, so the method properFormatTime() was executed. So
> why don't you just change that method?
> 
> @@ -274,5 @@
> >            Services.console.reset();
> >          ]]></body>
> >        </method>
> >  
> > -      <method name="properFormatTime">
> 
> As I said above, instead of removing the method, I'd just change it. Either
> use toLocaleString() or if this is something that shows somewhere in the UI,
> you should use mozIntl, for example like here:
> https://dxr.mozilla.org/comm-central/rev/
> d9846d89641fc3c57e25f22ff9fcdeafc2b4e54e/calendar/resources/content/
> datetimepickers/datetimepickers.xml#566
> More examples here:
> https://dxr.mozilla.org/comm-central/search?q=formatter.
> format(&redirect=false

I am sorry I should have made it clearer  rather than looking for feedback on the patch what I was really looking for was a way to make the original code here work as desired.  Which seems to be what you answered with so thank you for that.  that is actually what I was lookiing for.

Comment 32

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8886847 [details] [diff] [review]
1376167-remove-console.xml-timestamp-formatting.patch

Clearing r? for this one as a new patch seems to be comming.

Comment 33

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8887543 [details] [diff] [review]
1376167-avoid-nsISrciptableDateFormat-in-js.patch

Looks good. I am unable to test the patch for feedwriter.js. Seems that our livebookmarks implementation is broken.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9cea2329de98
Avoid use of nsIScriptableDateFomat in Seamonkey JavaScript files. r=IanN,frg

Comment 35

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-avoid-nsIScriptableDateFormat-in-xml.patch

Obviously I was making this way to complicated.  Thanks to the input in comment 30, I found what I needed in comm-central/chat/content/imtooltip.xml.
This worked for me in Windows. I will submit it for review as soon as my Linux build completes.  Pending successful testing of course.

Comment 36

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/9cea2329de98ee52acdbfac1a2240d174ccaf280

Since when have we pulsebot working? 

Reopen for part 2

Comment 37

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8889062 [details] [diff] [review]
1376167-avoid-nsIScriptableDateFormat-in-xml.patch

Works fine in Windows. I think it it safe to give r+.

I thought about asking for a change in indention but it would come clear over 80 chars any other way and it is now indented as like most other createDateTimeFormat statements in mail and mailnews. If IanN objects later we can always do a whitspace fix.

Let me know when the Linux compile finished. Will wait with checkin.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887543 [details] [diff] [review]
1376167-avoid-nsISrciptableDateFormat-in-js.patch

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

Sorry to say, but this patch looks pretty wrong to me. Everywhere where TB or FF used toLocaleString() or Intl.DateTimeFormat(), it's been pulled out again and replaced by mozIntl.

Bug 1346549, bug 1313659, bug 1360915, bug 1350679 and more. FF changed all chrome dates to mozIntl in bug 1354445.

P.S.: Pulsebot works on C-C since today.

::: suite/browser/pageinfo/pageInfo.js
@@ +1313,5 @@
>  
> +  const dtOptions = { year: "numeric", month: "long", day: "numeric",
> +                      hour: "numeric", minute: "numeric", second: "numeric",
> +                      timeZoneName: "short", weekday: "short" };
> +  return date.toLocaleString(undefined, dtOptions);

If the date is visible in the UI, you should be using monIntl like in the other patch.

::: suite/common/bookmarks/bookmarksManager.js
@@ +365,5 @@
> +    const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> +                             .getSelectedLocale("global", true);
> +    const dtOptions = { year: 'numeric', month: 'long', day: 'numeric' };
> +    let dateFormatter = new Intl.DateTimeFormat(locale, dtOptions);

I assume that will show on the UI, so Intl.DateTimeFormat() should not be used.

Besides, just pass 'undefined' as locale.

Comment 39

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #38)
> Comment on attachment 8887543 [details] [diff] [review]
> 1376167-avoid-nsISrciptableDateFormat-in-js.patch
> 
> Review of attachment 8887543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry to say, but this patch looks pretty wrong to me. Everywhere where TB
> or FF used toLocaleString() or Intl.DateTimeFormat(), it's been pulled out
> again and replaced by mozIntl.
> 
> Bug 1346549, bug 1313659, bug 1360915, bug 1350679 and more. FF changed all
> chrome dates to mozIntl in bug 1354445.
> 
> P.S.: Pulsebot works on C-C since today.
> 
> ::: suite/browser/pageinfo/pageInfo.js
> @@ +1313,5 @@
> >  
> > +  const dtOptions = { year: "numeric", month: "long", day: "numeric",
> > +                      hour: "numeric", minute: "numeric", second: "numeric",
> > +                      timeZoneName: "short", weekday: "short" };
> > +  return date.toLocaleString(undefined, dtOptions);
> 
> If the date is visible in the UI, you should be using monIntl like in the
> other patch.
> 
> ::: suite/common/bookmarks/bookmarksManager.js
> @@ +365,5 @@
> > +    const locale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> > +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> > +                             .getSelectedLocale("global", true);
> > +    const dtOptions = { year: 'numeric', month: 'long', day: 'numeric' };
> > +    let dateFormatter = new Intl.DateTimeFormat(locale, dtOptions);
> 
> I assume that will show on the UI, so Intl.DateTimeFormat() should not be
> used.
> 
> Besides, just pass 'undefined' as locale.

Well I will start out by saying this is kind of a difference in philosophy.  I went with for code that we essentially copied form mozilla-central originally I would just copy the new code so the code in PageIno.js is essentially the same as what is in  mozilla-central/browser/base/content/pageinfo/pageInfo.js.

The same for the other code .  The one difference I did make, since the new mozilla-central code for times just displayed in the UI is to add back the timezone name (previously displayed under Linux) and the day of the week (previously displayed under Windows).  All of the code I put in the bug for the js files is really just a straight copy from the equivalent place in mozilla-central.  I am not sure that is what I would have done starting out with nothing to copy and I certainly see your point.  Once I saw how much simpler what you said to do for the XML case, I was thinking maybe I should go back and re-visit the JS patch but still back on the is there benefit in keeping it similar to the mozilla-central code that is used in Firefox?

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a73acae42ace
Avoid use of nsIScriptableDateFomat in Seamonkey XML files. r=frg

Comment 41

[Jorg K (CEST = GMT+2)]

Hmm, pageInfo.xul is the XUL file that includes pageInfo.js and I'm really surprised that that hasn't been moved to mozIntl, Zibi, did you forget that? Is that not the panel that says: Modified: 22 July 2017, 21:57:22?

bookmarksManager.js does not exist in M-C, instead they have places. Places and also Feedwriter got moved to mozIntl:
https://hg.mozilla.org/mozilla-central/rev/79673e659b31#l3.2 - Feeds
https://hg.mozilla.org/mozilla-central/rev/79673e659b31#l4.32 - Places
https://hg.mozilla.org/mozilla-central/rev/79673e659b31#l5.27 - Places

You're already using mozIntl for feeds:
https://hg.mozilla.org/comm-central/rev/9cea2329de98ee52acdbfac1a2240d174ccaf280#l9.31

Looks like M-C also forgot cookies:
http://searchfox.org/mozilla-central/source/browser/components/preferences/cookies.js#502

Zibi, why does pageInfo.js and cookies.js not use mozIntl?

Comment 42

[Jorg K (CEST = GMT+2)]

I think you'll have to reopen this or file another bug for the use of mozIntl.

Comment 43

[Frank-Rainer Grahl (:frg)]

I verifixed the console patch on Linux. 

Btw. pulsebot is only doing the short rev so here is the long one:
https://hg.mozilla.org/comm-central/rev/a73acae42ace1e85a82818ec51b9690712a0bf57

Thought about backing it out but it does get rid of nsIScriptableDateFormat and matches Fx. 

Spoke with Bill and lets do a followup bug for clean up. There are still numerous createDateTimeFormat in calendar, mail, mailnews, browser and now suite. I would like to have a better understanding first what should be replaced with what.

Comment 44

[Jorg K (CEST = GMT+2)]

Well, Firefox have forgotten page info and cookies, see bug 1354445 comment #28. I've just tried it.

Yes, you've eliminated nsIScriptableDateFormat and you match FF, which is not a good thing in this case.

let formatter = Services.intl.createDateTimeFormat(...) is the construct to use, that's mozIntl, so I'm glad you can see a lot of usage.

What should be avoided is toLocale[Date|Time]String() on dates/times. There is very little use in C-C, and I'm eliminating the remaining ones from chat/ in bug 1380799. It's OK to format a weekday or single day that way, but not entire times/dates if they're visible in the UI.

Comment 45

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #44)
> Well, Firefox have forgotten page info and cookies, see bug 1354445 comment
> #28. I've just tried it.
> 
> Yes, you've eliminated nsIScriptableDateFormat and you match FF, which is
> not a good thing in this case.
> 
> let formatter = Services.intl.createDateTimeFormat(...) is the construct to
> use, that's mozIntl, so I'm glad you can see a lot of usage.
> 
> What should be avoided is toLocale[Date|Time]String() on dates/times. There
> is very little use in C-C, and I'm eliminating the remaining ones from chat/
> in bug 1380799. It's OK to format a weekday or single day that way, but not
> entire times/dates if they're visible in the UI.

Yes I did see that toLocale etc. was being deprecated in JavaScript.  But I figured if I just copy FF then when they have to fix it for that I would have to do a second change but would be just do another copy now that I see we have Services.intl.createDateTimeFormat, which I did not know about, that would seem to be the way to go.  Perhaps after I fix this for SeaMonkey I will fix it for Firefox also.

Comment 46

[Bill Gianopoulos [:WG9s]]

And silly me was actually specifically watching pageInfo.js to see what they were going to do and that is one they missed, go figure!

Comment 47

[Jorg K (CEST = GMT+2)]

Sorry for criticising your patch, I was under the impression that FF fixed all chrome dates. I tried TB and there cookies do the right thing, but that's because I fixed them (well, in Aryx's name since I finished off his patch). Let's move on to bug 1383457. I've already sent a patch for bug 1383463.

Comment 48

[Bill Gianopoulos [:WG9s]]

(In reply to Jorg K (GMT+2) from comment #47)
> Sorry for criticising your patch, I was under the impression that FF fixed
> all chrome dates. I tried TB and there cookies do the right thing, but
> that's because I fixed them (well, in Aryx's name since I finished off his
> patch). Let's move on to bug 1383457. I've already sent a patch for bug
> 1383463.

IF you don't criticize, how am I going to learn anything?  Way back in my patching career someone asked why I asked for bz to review a patch because he was going to be super-critical.  My response was I picked him because he is a hard marker.  I would rather learn something than get an r+ on a defective patch.

Comment 49

[Bill Gianopoulos [:WG9s]]

Attachment: 1376167-followup.patch

I am not sure how I missed this.  I guess I was so happy it worked.  In this case a timestamp without the seconds is really less than useful.

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889146 [details] [diff] [review]
1376167-followup.patch

Sure, thanks.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e7e88db2d5d
Follow-up: add seconds back to Error Console timestamps. r=jorgk