The default bug view has changed. See this FAQ.

Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

ASSIGNED
Assigned to

Status

Calendar
General
P2
major
ASSIGNED
a year ago
15 hours ago

People

(Reporter: m_kato, Assigned: Javi Rueda, NeedInfo)

Tracking

(Blocks: 4 bugs)

Dependency tree / graph

Details

(Whiteboard: [Thunderbird-testfailure: Z Linux32], URL)

Attachments

(3 attachments, 9 obsolete attachments)

9.60 KB, image/png
Details
30.73 KB, image/png
Details
51.02 KB, patch
Javi Rueda
: review?
MakeMyDay
Details | Diff | Splinter Review
Except to Android, ICU is already turned on m-c.  So it means JS Intl API supported on all c-c platform.

So calendar should use Intl API to format date time.
This is getting more important now that m-c is deprecating it.

https://groups.google.com/forum/#!topic/mozilla.dev.platform/ZgjzNNvR_tE
Blocks: 1313625
(Assignee)

Updated

2 months ago
Assignee: nobody → leofigueres

Updated

2 months ago
Blocks: 1325792
Hi Javi,

this bug is getting more and more importance, since it is the reason for some of our mozmill-tests failing on linux32.

is there anything I can provide to help you here?
Blocks: 1329957
Status: NEW → ASSIGNED
Flags: needinfo?(leofigueres)
Priority: -- → P2
(Assignee)

Comment 3

2 months ago
Created attachment 8831771 [details] [diff] [review]
Very draft but working patch

Hi, Markus. This is what I have done until now. It is mostly working, but code is not final.

As can be seen from the date, I had to stop working on it some days ago because of some other stuff I had to do not related with Mozilla.

If code seems good and someone could guide me about changes needed, I would be glad to keep working on it with top priority.

If someone else thinks it is much more important to re-do the work, no problem to take it and remove myself from the asignee field.

Not asking a review as it is working-in-progress.
Flags: needinfo?(leofigueres)
Hi Javi,

Good work so far.

Some suggestions from me:
>@@ -116,18 +116,19 @@
>...
>               topbox.lastChild.remove();
>           }
> 
>-          let formatter = Components.classes["@mozilla.org/intl/scriptabledateformat;1"]
>-                                    .getService(Components.interfaces.nsIScriptableDateFormat);
>+          let timeOptions = { hour: 'numeric', minute: 'numeric' };
>+          let timeFormatter = new Intl.DateTimeFormat(undefined, timeOptions);
>+          let date = new Date();
I would define the date not here already, but when it is needed (inside the "if" block further down).

>+++ b/calendar/base/src/calDateTimeFormatter.js
>@@ -80,21 +80,20 @@ calDateTimeFormatter.prototype = {
>...
>+        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
>+
>+        return timeFormatter.format(new Date(aDate.year, aDate.month, aDate.day));
>     },
> 
>     formatDateLong: function(aDate) {
>         let longDate;
>         if (this.mUseLongDateService) {
>             longDate = this.mDateService.FormatDate("",
>                                                     nsIScriptableDateFormat.dateFormatLong,
>                                                     aDate.year,
Here at the end, there is another occurrence of ScriptableDateFormat.
I think, this is where you had to stop - but wanted to be sure it is not overlooked.

>+++ b/calendar/base/src/calDateTimeFormatter.js
>@@ -80,21 +80,20 @@ calDateTimeFormatter.prototype = {
>...
>     formatDateShort: function(aDate) {
>-        return this.mDateService.FormatDate("",
>-                                            nsIScriptableDateFormat.dateFormatShort,
>-                                            aDate.year,
>-                                            aDate.month + 1,
>-                                            aDate.day);
>+        let dateOptions = { year: 'numeric', month: 'numeric', day: 'numeric' };
>+        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
>+
>+        return timeFormatter.format(new Date(aDate.year, aDate.month, aDate.day));
>     },
You get a date Object passed into this function, so you could just do:
return timeFormatter.format(aDate);

same for the last part of the patch.
(Assignee)

Comment 5

2 months ago
Created attachment 8832276 [details] [diff] [review]
qrefreshed draft

What I attached and was reviewed by Markus was a patch which wasn't qrefreshed on my local machine, so it didn't included latest changes I made. This one is the one which I have in my machine
(Assignee)

Updated

2 months ago
Attachment #8831771 - Attachment is obsolete: true
(Assignee)

Comment 6

2 months ago
Thank you for reviewing that patch, Markus. I will try to edit the newest patch to add your suggestions.

Tests are also make use of the deprecated interface, so it should also be patched. Would a patch only for tests make treeherder happier?
(Assignee)

Comment 7

2 months ago
(In reply to Markus Adrario [:Taraman] from comment #4)

> I would define the date not here already, but when it is needed (inside the
> "if" block further down).
> 

Ok.

> Here at the end, there is another occurrence of ScriptableDateFormat.
> I think, this is where you had to stop - but wanted to be sure it is not
> overlooked.
> 
Exactly.

> You get a date Object passed into this function, so you could just do:
> return timeFormatter.format(aDate);
> 
> same for the last part of the patch.

No, it is not. aDate is an calIDateTime object, not an JavaScript Date object.

When using directly aDate in formatDateShort, an exception throws:

RangeError: date value is not finite in DateTimeFormat.format()

So the object reterned by the component calDateTime is not JavaScript compatible, at least the way we could use it.



And now, a self-suggestion: remove the try/catch for the calDateTimeFormatter as we are not using the problematic interface it tries to catch.
(In reply to Javi Rueda from comment #6)
> Tests are also make use of the deprecated interface, so it should also be
> patched. Would a patch only for tests make treeherder happier?

I don't think so. At the moment, the tests fail, because the Time-Picker in the Event-Dialog is broken due to the weird return-values of the time-formatter.
So take your time to change all the occureences of the formatter.

(In reply to Javi Rueda from comment #7)
>> You get a date Object passed into this function, so you could just do:
>> return timeFormatter.format(aDate);
>> 
>> same for the last part of the patch.
>
>No, it is not. aDate is an calIDateTime object, not an JavaScript Date object.
Ah, I see.
(Assignee)

Comment 9

a month ago
Created attachment 8834713 [details] [diff] [review]
TESTED patch

This is not a draft patch. I have run mozmill and it passes all of /calendar tests. I will ask for a review as soon as I am able to see who could do it.
(Assignee)

Updated

a month ago
Attachment #8832276 - Attachment is obsolete: true
(Assignee)

Comment 10

a month ago
Comment on attachment 8834713 [details] [diff] [review]
TESTED patch

It seems :MakeMyDay has been reviewing most of the calendar code this patch modifies, so I guess he is the right one.

Hi, :MakeMyDay, could you take a look into this? Thank you.
Attachment #8834713 - Flags: review?(makemyday)
Since I had to start a try run anyway, i decided to put this in:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b98b1d4e7424399c5b8b3524d8788d3ca61dde57

Comment 12

a month ago
Thanks, Markus. Can you please kick off another try run for all plaforms with mozmill and xpcshell tests?
As soon as the Builds are working again on try

Comment 14

a month ago
Bustage now fixed.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4e21c81ccc1bd47eb5859f0c73d7ae73662a01f8

Updated

a month ago
Whiteboard: [Thunderbird-testfailure: Z Linux32]
Created attachment 8835715 [details]
The Day-View on linux32 with patch applied
Created attachment 8835716 [details]
event-dialog on linux32 with patch applied

These two pictures show that the things broken before now work again.

Comment 18

a month ago
Javi, thank you for the patch. I will do the review on the weekend.
(Reporter)

Updated

a month ago
Blocks: 1334798

Comment 19

a month ago
Comment on attachment 8834713 [details] [diff] [review]
TESTED patch

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

Thanks for your work and sorry for the delay in reviewing.

This patch already looks quite good, but it is not exactly restoring the behaviour that was implemented before. For that, see my comments below. With the long format modifications, also the failing xpcshell test test_ltninvitationutils should pass, but you also can clean up the not longer needed test pattern at https://dxr.mozilla.org/comm-central/source/calendar/test/unit/test_ltninvitationutils.js#451 subsequently.

::: calendar/base/content/calendar-multiday-view.xml
@@ +150,5 @@
>                let durPix = endPix - startPix;
>                let box;
>                if (dur == 60) {
> +                  calTime.hour = theHour;
> +                  timeString = timeFormatter.formatTime(calTime);

We have a helper function for the date object conversion: just use cal.jsDateToDateTime(jsDate) to get the calDateTime object. This comment applied to occurences in this patch where you do such a conversion - just make sure you have resource://calendar/modules/calUtils.jsm imported then.

::: calendar/base/src/calDateTimeFormatter.js
@@ +82,5 @@
>  
>      formatDateShort: function(aDate) {
> +        const locale = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +                       .getService(Components.interfaces.nsIXULChromeRegistry)
> +                       .getSelectedLocale("global", true);

This is different from what we had before, when we were relying on the OS locale and not the application locale for datetime formatting.

If I get this correct, passing undefined instead of a locale would trigger the previous behaviour.

However, it's probably a good idea to introduce a new pref to offer such an additional option like you proposed - that might get handy also for running tests in a reliable setup. When doing so, can you move the locale definition out of the function scope as your using the same multiple times here. Also please use Components.classes instead of The Cc shortcut - this is not common in calendar code.

@@ +95,5 @@
>          if (this.mUseLongDateService) {
> +            const locale = Cc["@mozilla.org/chrome/chrome-registry;1"]
> +                           .getService(Components.interfaces.nsIXULChromeRegistry)
> +                           .getSelectedLocale("global", true);
> +            let dateOptions = { weekday: 'short', year: 'numeric', month: 'numeric', day: 'numeric' };

Please use 'long' for month and and weekday
Attachment #8834713 - Flags: feedback+

Comment 20

a month ago
One more thing: we can clean up calDateTimeFormatter from some then obsolete code and also remove some of the strings we used before with this change. But I'm happy to get this done in a separate patch or bug to not hold to get rid of the Linux32 bustage and the test failures first.
(Assignee)

Comment 21

a month ago
This bug could impact the way localized builds show strings. We should remember to add it to the QA tests before release.

I am working in your requests, :MakeMyDay.
For your information, bug 1339650:
> Once we have bug 1329904 and bug 1308329 we can merge them in mozIntl.DateTimeFormat which will format date and time taking into account users preferences set in the OS.

Comment 23

a month ago
Thanks for the heads-up, Aryx. This may help us, but I'm not sure that it will do for all our use cases. If I understood correctly, applying the OS setting will be limited to chrome code and not available in content.

@Javi: as the toolkit implemenation is currently in motion (and probably will be for the next time), we can expect that whatever we implement will need further polishing later on - that said, I would appreciate to get something ready for landing given passing |undefined| as mentioned in comment 19 would be enough to achive this for the moment (and not to wait for the bugs mentioned in comment 22 to land), so that we can resolve the Linux32 bustage first and take care of implementing further improvements based on mozIntl once they get available.
(Assignee)

Comment 24

29 days ago
Created attachment 8839842 [details] [diff] [review]
datetimeformat.patch
(Assignee)

Comment 25

29 days ago
Comment on attachment 8839842 [details] [diff] [review]
datetimeformat.patch

Sorry for attaching the patch alone instead of with this comment, but I wanted to be sure that everything looked as I would like and Splinter Review is the best to do that, instead of colordiff :)

Global comment
--------------
:MakeMyDay asked to remove some XPC-Shell use-cases tests, but I ran ./mach xpcshell-test calendar/test/unit/test_ltninvitationutils.js and every single test passed with no error nor warnings. (macOS platform)

Some notes on the latest changes added into this patch:

calendar-multiday-view.xml
--------------------------
It uses cal.jsDateToDateTime when needed, as required by :MakeMyDay

views.js
--------
This is a preference tab. Code modified by this patch is about the configuration of workday hours.

It uses the calUtils JS Module to create a calDateTime object with current date. This way we can pass it right away to formatTime with the right timezone information. (I wasted almost all day of yesterday until I realized that I wasn't creating a calDateTime object)

calDateTimeFormatter.js
-----------------------
Here we are using the calUtils.jsm again. This allows the patch to be simpler.

datetimepickers.xml
-------------------
This is the widget used to set date or time for events.

Here, the patch is using calUtils.jsm to convert between calDdateTime and JavaScript Date().

testWeekly*.js,test-calendar-utils.js
-------------------------------------
Patch uses calUtils to make conversion between date objects.

testBasicFunctionality.js,testDayView.js,testMonthView.js,testMultiWeekView.js,testWeekView.js
--------------------------------------------------------------------------------
A calDateTime object is created and its fields are filled with integers. No type conversion is needed.

Mozmill tests
-------------
I was unable to run the calendar suite of mozmill tests. They will likely be run before/during the review. Sorry about this inconvenience.
Attachment #8839842 - Flags: review?(makemyday)

Updated

26 days ago
Attachment #8834713 - Attachment is obsolete: true
Attachment #8834713 - Flags: review?(makemyday)

Comment 26

26 days ago
Try push for the recent patch:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f5547dd38ebe2d386de3946058da3de0999f3ce1
(Assignee)

Comment 27

25 days ago
Oh! Oh! I can see some unexpected-fails which were previously fixed. I suppose my patch is not good. I am testing again in my local machine.

Comment 28

24 days ago
Comment on attachment 8839842 [details] [diff] [review]
datetimeformat.patch

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

Just some comments on calDateTimeFormater.js for now, because formatDateLong effectively doesn't use the intl formatting atm anyway, because it's prevented by mUseLongDateService - the debug output from the xpcshell test in the try push shows the fallback date format on all platforms as well as my local build with this patch.

Simply removing the entire fallback code would be appropriate in general, as formatting with intl should work in the same way for all platforms, but this will regress the special treatment for minority languages implemented with bug 1167939 - at least until the capabilities mentioned in comment 22 will become available. That said, I would accept a temporary regression of the minority language use case if a follow-up bug would be filed for that.

::: calendar/base/src/calDateTimeFormatter.js
@@ +79,5 @@
>          return (format == 0 ? this.formatDateLong(aDate) : this.formatDateShort(aDate));
>      },
>  
>      formatDateShort: function(aDate) {
> +        let dateOptions = { year: 'numeric', month: 'numeric', day: 'numeric' };

please use the 2-digit format for month here.

@@ +91,5 @@
>          if (this.mUseLongDateService) {
> +            let dateOptions = { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' };
> +            let dateFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
> +
> +            longDate = timeFormatter.format(cal.dateTimeToJsDate(aDate));

You want to use dateFormatter here instead.

@@ +118,5 @@
>      formatDateWithoutYear: function(aDate) {
> +        let dateOptions = { month: 'short', day: 'numeric' };
> +        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
> +
> +        return timeFormatter.format(cal.dateTimeToJsDate(aDate));

With changing the implementation you can remove all the oter code related to mMonthFirst in this file.

@@ +126,5 @@
>          if (aDate.isDate) {
>              return this.mDateStringBundle.GetStringFromName("AllDay");
>          }
>  
> +        let timeOptions = { hour: 'numeric', minute: 'numeric' };

Use 2-digit for minutes, please.
Attachment #8839842 - Flags: review?(makemyday) → review-

Comment 29

24 days ago
One more thing if you're at it: the string based composition within formatInterval deserves some attention and a conversion to intl formatting.
(Assignee)

Comment 30

24 days ago
(In reply to [:MakeMyDay] from comment #28)
> Comment on attachment 8839842 [details] [diff] [review]
> datetimeformat.patch
> 
> Review of attachment 8839842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some comments on calDateTimeFormater.js for now, because formatDateLong
> effectively doesn't use the intl formatting atm anyway, because it's
> prevented by mUseLongDateService - the debug output from the xpcshell test
> in the try push shows the fallback date format on all platforms as well as
> my local build with this patch.
> 

On this: I tested xpcshell on my machine. I even rebuilt after doing a mach clobber, and all tests for the invitation passed.

> 
> > +            longDate = timeFormatter.format(cal.dateTimeToJsDate(aDate));
> 
> You want to use dateFormatter here instead.

I am very ashamed about this one.

> 
> @@ +118,5 @@
> >      formatDateWithoutYear: function(aDate) {
> > +        let dateOptions = { month: 'short', day: 'numeric' };
> > +        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
> > +
> > +        return timeFormatter.format(cal.dateTimeToJsDate(aDate));
> 
> With changing the implementation you can remove all the oter code related to
> mMonthFirst in this file.

Comment 31

24 days ago
This test can pass locally and fail on automation because of different locales/datetime configuration on the respective machines. Therefor I kicked off the try build and added the debug patch to get the observed value dumped to the test log right before the line with the unexpected failure.

But at the end this is not a test issue but one in the datetimefirmatter as mentioned above. That you didn't run into an error due to the wrong var name when testing is an evidence for it. Commenting out the entire fallback code and fixing the var name issue brought up the expected format on my local machine.

The easiest to check for long date formatting against the defined pattern is the date format preview in preferences - you can do this already without running the xpcshell test.

Comment 32

20 days ago
I am disappointed that this bug still isn't resolved. It's been an issue since Christmas 2016 (bug 1325792), more than two months ago. This is causing test failures on Linux, and I believe loss of functionality, on trunk and Aurora. After branch day next week, the failures will also move to beta, so they will be on *all* branches. Unless it's a severe problem, which this one isn't, we strive to fix it on C-C so that it doesn't even move to Aurora.

Updated

20 days ago
Severity: normal → critical
(Assignee)

Comment 33

20 days ago
Created attachment 8843234 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat
(Assignee)

Updated

20 days ago
Attachment #8839842 - Attachment is obsolete: true
(In reply to Jorg K (GMT+1) from comment #32)
> I am disappointed that this bug still isn't resolved. It's been an issue
> since Christmas 2016 (bug 1325792), more than two months ago. 

Jörg, while I understand that you would like to see this bug fixed as soon as possible, I am sure you understand that issues take time to solve and it would be unfair to pressure a volunteer contributor at this stage. Javi has been very responsive and has been doing a great job working on this issue. In addition, this bug has had some negative side effects from m-c issues that needed to be worked around.

Be assured that everyone involved is doing the most possible under their respective circumstances to ensure this bug is fixed soon. If this is the only bug preventing green test runs then I think it is acceptable to wait a little bit longer. I am happy to step in if this becomes more critical, but at the moment I believe Javi and MakeMyDay have it covered.

That said, Javi, if you need help and have questions you can also ping me on IRC. My nickname is Fallen

Comment 35

20 days ago
(In reply to Philipp Kewisch [:Fallen] from comment #34)
> I am sure you understand that issues take time to solve and it
> would be unfair to pressure a volunteer contributor at this stage.
This issue has not been resolved since 2017-01-17, more than six weeks ago, when Javi assigned himself (no one pressured him). So I need to question the decision to let a volunteer work on a critical bug (already mentioned in comment #2) with minimal assistance. As I said, two branches affected with a third one to come. So maybe it's time for the module owners or peers to step up to the challenge. I believe the red tests reflect a loss of functionality, but I'm happy to stand corrected on that one.

Comment 36

19 days ago
To put things in perspective: it's not affecting any versions going to "real end users". Yes beta soon, but it's beta for a version that will never really ship as we don't do a tb53.
Severity: critical → major
(Assignee)

Updated

19 days ago
Attachment #8843234 - Flags: review?(makemyday)
(Assignee)

Comment 37

19 days ago
If there is anyone who feels more capable of trying to fix this one, please *feel free* to take this without need to explanations. After seeing my error while using a wrong variable maybe it is that I am not the most capable person to fix this particular bug. I will understand it because I will think that the need to make those tests pass are of high priority, although I cannot remember the last time there were no red tests in Treeherder.

If it was critical before I took it, then why it wasn't fixed?

This bug is not causing the full tree to go red, neither.

The last patch I have attached until now removes code for verifying if we must be using our strings for some minority locales. That removal will cause a regression, as previously commented by :MakeMyDay.

It also corrects the variable which I misused. And the numbering format.

With the new code, OS locale will be used, so runing xpcshell in my local machine (es-ES) is useless. I don't have -neither want- access to try-build server, so it will be needed to test there, as I think they are using en-US so tests should pass. :MakeMyDay will do it, for sure, but I just wanted to say why I am not able to correct any problem that I am not able to see.

This also doesn't do anything of the suggested in comment 29.

(In reply to Jorg K (GMT+1) from comment #35)
> (In reply to Philipp Kewisch [:Fallen] from comment #34)
> > I am sure you understand that issues take time to solve and it
> > would be unfair to pressure a volunteer contributor at this stage.
> This issue has not been resolved since 2017-01-17, more than six weeks ago,
> when Javi assigned himself (no one pressured him). So I need to question the
> decision to let a volunteer work on a critical bug (already mentioned in
> comment #2) with minimal assistance. As I said, two branches affected with a
> third one to come. So maybe it's time for the module owners or peers to step
> up to the challenge. I believe the red tests reflect a loss of
> functionality, but I'm happy to stand corrected on that one.

It wasn't critical until you changed its priority today.

Critical bugs are about those which provoke "Crashes, loss of data, severe memory leak" (https://wiki.mozilla.org/BMO/UserGuide/BugFields#Severity). I don't see any of these problems here. A blocker, maybe: "Blocks development and/or testing work"

So, as long as any other person isn't taking this bug, I am going to remain here if there are any other changes needed in my last attachement or more work to be done.

Comment 38

19 days ago
Let's not argue about the details, rather let's get this fixed.

(In reply to Javi Rueda from comment #37)
> I cannot remember the last time there were no red tests in Treeherder
I have a collection. Last time almost everything was green (one red X) was on the 29th of Dec. 2016 before this issue here hit us.

> It wasn't critical until you changed its priority today.
The bug was flagged as "more and more" important in comment #2.

In general, M-C landed the transition to ICU for date/time formats and that caused bug 1325792. Suddenly many tests on C-C tree herder went red. A red test may or may not indicate loss of functionality, in most cases it does, since we don't have those test for fun. Each test guarantees a function and a failing test most likely indicates a failing function although we also have "just failing" tests. It is my job to look at the tree daily and spot bustage and it makes my work really hard having to read through the same failures over and over and over and over again. Also, it is good practise to fix incoming bustage from M-C within a week or so.

> Critical bugs are about those which provoke "Crashes, loss of data, severe
> memory leak" (https://wiki.mozilla.org/BMO/UserGuide/BugFields#Severity). I
> don't see any of these problems here. A blocker, maybe: "Blocks development
> and/or testing work"
Well, these priorities/severities are in order, so a blocker is more severe than a critical bug. Anyway, regardless of the label, and Magnus just turned it down to "major", this needs to be fixed now.

> So, as long as any other person isn't taking this bug, I am going to remain
> here if there are any other changes needed in my last attachment or more
> work to be done.
Sure, thank you. Perhaps we can cut the review cycle short and let the reviewer fix a few nits and push this to the try server.

Once again, I think this needs to be given more attention by the module owner and peers since Javi doesn't have Level 1 access rights.

Comment 39

19 days ago
(In reply to Javi Rueda from comment #37)
> With the new code, OS locale will be used, so runing xpcshell in my local
> machine (es-ES) is useless. I don't have -neither want- access to try-build
> server, so it will be needed to test there, as I think they are using en-US

Debugging on try server is cumbersome. But for the benefit of everybody do consider getting level1 access so you can push to try. https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server - I'd be happy to vouch for you.
(Assignee)

Comment 40

18 days ago
(In reply to Magnus Melin from comment #39)
> Debugging on try server is cumbersome. But for the benefit of everybody do
> consider getting level1 access so you can push to try.
> https://wiki.mozilla.org/ReleaseEngineering/
> TryServer#Getting_access_to_the_Try_Server - I'd be happy to vouch for you.

I have just done it in bug 1344506. Thank you for the support, Magnus.

Comment 41

16 days ago
Comment on attachment 8843234 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

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

Thank you for the updated patch. I triggered a try-push for you [1]. As you can see, there are failures on all tests, but all of this boil down to a timezone issue when converting from js Date to calIDateTime and vice versa. You can see that when looking at the xpcshell test, where I added a debug output just before the failure - if you compare that to the expected values, the difference is exactly the TZ offset from that to America/Los Angeles, which is the timezone on automation.

This is a result of using the helper functions for conversion, as these respect timezones as set (please also note that cal.now() provides a datetime object in default timezone, while cal.createDateTime() doesn't include such a tz conversion). So take care that the conversion is always done consistently.

For the xpcshell test, you can remove the now unneccessary test pattern from test_ltninvitationutils.js in compareInvitationOverlay_test (just make sure you leave two versions in there, one with 24h and onewith 12h format). There is a second test in that test file, which needs a similar treatment (-> parseCounter_test).

For further comments see below.

I set this r- for now to get an updated patch with the comments considered and the tz issue addressed. If you need further support to get there, don't hesitate to ask. And thank you for all your work so far.


[1] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6ca6b025b82c21f16cf9671eb457309865c114b

::: calendar/base/src/calDateTimeFormatter.js
@@ -10,3 @@
>  
>  function calDateTimeFormatter() {
> -    this.wrappedJSObject = this;

don't remove this line.

@@ +28,5 @@
>          return (format == 0 ? this.formatDateLong(aDate) : this.formatDateShort(aDate));
>      },
>  
>      formatDateShort: function(aDate) {
> +        let dateOptions = { year: 'numeric', month: '2-digit', day: 'numeric' };

make the day also 2-digit, please.

@@ +36,4 @@
>      },
>  
>      formatDateLong: function(aDate) {
> +        let dateOptions = { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' };

the same here.

@@ +36,5 @@
>      },
>  
>      formatDateLong: function(aDate) {
> +        let dateOptions = { weekday: 'long', year: 'numeric', month: 'long', day: 'numeric' };
> +        let dateFormatter = new Intl.DateTimeFormat(undefined, dateOptions);

To limit the impact of breaking the locale support until bug 1339650 lands and to prepare for backporting this patch, please make use the current locale is used in formatDateLong and formatDateWithoutYear instead of undefined as you started in your first patch.

Use a member attribut like this.mLocale for this and assign the detected locale in the constructor above.

@@ +43,5 @@
>      },
>  
>      formatDateWithoutYear: function(aDate) {
> +        let dateOptions = { month: 'short', day: 'numeric' };
> +        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);

See comment above.

::: calendar/test/mozmill/shared-modules/test-calendar-utils.js
@@ +707,5 @@
>          menuitem.getNode().setAttribute("checked", data.timezone);
>          dialog.click(menuitem);
>      }
>  
> +    Components.utils.import("resource://calendar/modules/calUtils.jsm");

Do imports like this at the beginning of the file.

::: calendar/test/mozmill/testBasicFunctionality.js
@@ +64,5 @@
> +        Components.classes["@mozilla.org/calendar/datetime;1"]
> +                  .createInstance(Components.interfaces.calIDateTime);
> +    someTime.hour = 9;
> +    someTime.minute = 0;
> +    someTime.second = 0;

you can use let someTime = cal.createDateTime() - respectively cal.now() and someTime.resetTo(...) instead

::: calendar/test/mozmill/views/testDayView.js
@@ +108,5 @@
> +            Components.classes["@mozilla.org/calendar/datetime;1"]
> +                      .createInstance(Components.interfaces.calIDateTime);
> +        someDate.year = 2009;
> +        someDate.month = 1;
> +        someDate.day = 1;

The same here.

::: calendar/test/mozmill/views/testMonthView.js
@@ +119,5 @@
> +            Components.classes["@mozilla.org/calendar/datetime;1"]
> +                      .createInstance(Components.interfaces.calIDateTime);
> +        someDate.year = 2009;
> +        someDate.month = 1;
> +        someDate.day = 1;

and here.

::: calendar/test/mozmill/views/testMultiweekView.js
@@ +118,5 @@
> +            Components.classes["@mozilla.org/calendar/datetime;1"]
> +                      .createInstance(Components.interfaces.calIDateTime);
> +        someDate.year = 2009;
> +        someDate.month = 1;
> +        someDate.day = 1;

here again

::: calendar/test/mozmill/views/testWeekView.js
@@ +115,5 @@
> +            Components.classes["@mozilla.org/calendar/datetime;1"]
> +                      .createInstance(Components.interfaces.calIDateTime);
> +        someDate.year = 2009;
> +        someDate.month = 1;
> +        someDate.day = 1;

And here again.
Attachment #8843234 - Flags: review?(makemyday) → review-
(Assignee)

Comment 42

16 days ago
(In reply to [:MakeMyDay] from comment #41)
> 
> For the xpcshell test, you can remove the now unneccessary test pattern from
> test_ltninvitationutils.js in compareInvitationOverlay_test (just make sure
> you leave two versions in there, one with 24h and onewith 12h format). There
> is a second test in that test file, which needs a similar treatment (->
> parseCounter_test).

Thank you. That was what I was expecting for, as I was unable to see which strings should be removed.

> To limit the impact of breaking the locale support until bug 1339650 lands
> and to prepare for backporting this patch, please make use the current
> locale is used in formatDateLong and formatDateWithoutYear instead of
> undefined as you started in your first patch.
> 
> Use a member attribut like this.mLocale for this and assign the detected
> locale in the constructor above.

That will show app-locale instead of OS locale.

> ::: calendar/test/mozmill/testBasicFunctionality.js
> @@ +64,5 @@
> > +        Components.classes["@mozilla.org/calendar/datetime;1"]
> > +                  .createInstance(Components.interfaces.calIDateTime);
> > +    someTime.hour = 9;
> > +    someTime.minute = 0;
> > +    someTime.second = 0;
> 
> you can use let someTime = cal.createDateTime() - respectively cal.now() and
> someTime.resetTo(...) instead



Like this?
>     // default view is day view which should have 09:00 label and box
>    let someTime = cal.createDateTime();
>    someTime = cal.now();
>    someTime.resetTo(someTime.year, someTime.month, someTime.day, 9, 0, 0);
>    let label = dateFormatter.formatTime(someTime);



Also, from that first comment from the code I have just pasted above, it seems that the test is expecting "09:00" instead of what it is currently shown with the patch applied, "9:00". Shouldn't 2-digit be used in formatTime for hours?

Comment 43

16 days ago
> That will show app-locale instead of OS locale.

Yes, but only for the two mentioned methods, as these are the only ones that contain icu strings atm. All other must remain using undefined. There are other methods in this file which still work with calendar owned strings, but we will replace them in a follow up patch/bug.

> Like this?
> >     // default view is day view which should have 09:00 label and box
> >    let someTime = cal.createDateTime();
> >    someTime = cal.now();
> >    someTime.resetTo(someTime.year, someTime.month, someTime.day, 9, 0, 0);
> >    let label = dateFormatter.formatTime(someTime);

cal.now() already includes cal.createDateTime(), so you can spare that here. See the now() implementation in calendar/base/src/calUtils.js

> Also, from that first comment from the code I have just pasted above, it
> seems that the test is expecting "09:00" instead of what it is currently
> shown with the patch applied, "9:00". Shouldn't 2-digit be used in
> formatTime for hours?

Iirc, this is just a literally mentioning to distinguish it from other times but not formats.
(Assignee)

Comment 44

15 days ago
I managed to run mozmill tests in my local machine. With my latest changes all of them are passing.

Interestingly, tests in calendar/test/mozmill/views/ weren't run, so changes on those files in this patch will not have any effect. Latest try-build by :MakeMyDay didn't run them, neither.
Only the tests in the mozmilltets.list will be run.
The others still need fixing. Your changes there will come to effect when I come around to fix them.

Comment 46

14 days ago
(In reply to Javi Rueda from comment #44)
> I managed to run mozmill tests in my local machine. With my latest changes
> all of them are passing.

This is great. How are things going for the xpcshell tests? As you now have L1 access, can you verify it to working for othere platforms as well by triggering a try push? For parameters to use see my above try push from comment 41.
(Assignee)

Comment 47

14 days ago
Yesterday I managed to get all mozmill and the ltninvitationutils xpchell tests passing.

For the xpcshell, I removed most of the patterns and left only 2: with long weekday, en-us locale date format and both with 24 and 12hour format. I tested it by changing my locale on my local system. I also had to stop using the calDateTimeToJsDate conversion function in formatTime. Instead I just created a new JS Date object using the fields from the calDateTime passed to formatTime. This way no conversion is done.

mozmill tests are also passing.

In a few minutes I will try to push to try server. I will paste the url if it is not done automatically.
(Assignee)

Comment 48

14 days ago
My first commit to try-server:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1dd68fdcdf9ef2e89d108e2848979fabc2831789

Comment 49

13 days ago
Xpcshell looking good (sorry about the orange, that was expected and has since been fixed), but a failure on Mozmill on all platforms. Does testBasicFunctionality.js pass locally?
(Assignee)

Comment 50

12 days ago
I updated the patch and, after testing it better locally I run it in try-server:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d464f151e32e4799dab6484dd72db52392c3c8f1&selectedJob=82936304

Now problems seem to be testAnnualRecurrence, in mozmill. As testBasicFunctionality was the only which failed in try-erver, it was also the one I tested locally and let Mozilla's run the other ones, as Mozilla server could do it faster.

Comment 51

12 days ago
Thanks for the latest try run. I think as far as this bug here is concerned, it's all working.

The orange X (shocking, really) is due to bug 1345832/bug 1332351 which we will be addressing RSN(TM) ;-)
RSN = real soon now.

I think testAnnualRecurrence.js on Linux is covered by bug 1329957, which is still assigned to Markus.

So we're getting there. I assume your patch is ready for review now, isn't it?
(In reply to Jorg K (GMT+1) from comment #51)
> I think testAnnualRecurrence.js on Linux is covered by bug 1329957, which is
> still assigned to Markus.
After looking at the log, I can confirm this!
(Assignee)

Comment 53

12 days ago
Created attachment 8846082 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat
(Assignee)

Updated

12 days ago
Attachment #8843234 - Attachment is obsolete: true
(Assignee)

Comment 54

12 days ago
Created attachment 8846084 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat
(Assignee)

Updated

12 days ago
Attachment #8846082 - Attachment is obsolete: true
(Assignee)

Comment 55

12 days ago
Comment on attachment 8846084 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

This patch tries to solve the failing tests after switching to Intl.DateTimeFormat.

It also adds some Components.utils.imports which wasn't added to some files in my latests patches although they were needed. That didn't cause any trouble, however.

thank you to Jörg and Markus for confirming that testAnnualRecurrence failed test wasn't because of this patch. I took too many cycles to me to fix the timezones problems for testBasicFunctionality and invitations tests failures.

I didn't run testAnnualRecurrence.js test, as I explained in previous comment, so I thought that that test was still failing because of the formatTime changes.

I will be available here this weekend if any change is needed after your review, :MakeMyDay.

:)
Attachment #8846084 - Flags: review?(makemyday)

Comment 56

11 days ago
Comment on attachment 8846084 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

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

::: calendar/base/src/calDateTimeFormatter.js
@@ +11,5 @@
>      this.wrappedJSObject = this;
>      this.mDateStringBundle = Services.strings.createBundle("chrome://calendar/locale/dateFormat.properties");
> +    this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> +                             .getSelectedLocale("global", true);

Apparently this is not the best way to get the locale. Please see bug 1342753 comment 37.

Comment 57

11 days ago
Background: We're doing work in bug 1332351, also switching to .toLocaleDateString(), so we asked how to best get the build/app locale. The answer in bug 1342753 comment #37 was to use |Services.locale.getAppLocale();|.

We have so many bugs changing date/time formats at the moment, also see bug 1346549.
(Assignee)

Comment 58

11 days ago
(In reply to :aceman from comment #56)
> Comment on attachment 8846084 [details] [diff] [review]
> Use Intl.DateTimeFormat instead of nsIScriptableDateFormat
> 
> Review of attachment 8846084 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: calendar/base/src/calDateTimeFormatter.js
> @@ +11,5 @@
> >      this.wrappedJSObject = this;
> >      this.mDateStringBundle = Services.strings.createBundle("chrome://calendar/locale/dateFormat.properties");
> > +    this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> > +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> > +                             .getSelectedLocale("global", true);
> 
> Apparently this is not the best way to get the locale. Please see bug
> 1342753 comment 37.

See comment 41

Comment 59

11 days ago
(In reply to Javi Rueda from comment #58)
> See comment 41
I can't see a reference to how this should be retrieved.
(Assignee)

Comment 60

11 days ago
(In reply to Jorg K (GMT+1) from comment #59)
> (In reply to Javi Rueda from comment #58)
> > See comment 41
> I can't see a reference to how this should be retrieved.

It was an explanation about why it was used the getSelectedLocale instead of the "undefined" path.

Comment 61

11 days ago
Yes, I can see the discussion about not using 'undefined'. But it was suggested to get it via |Services.locale.getAppLocale();|.
(Assignee)

Comment 62

11 days ago
Ok. I can see the point now. Sorry about the misunderstanding.

I have replaced the code so that it is using the Services module, instead. I have also triggered a try-server build with this new code and will attach it if it passes all tests.

Comment 63

11 days ago
I cancelled your try run and started another one instead:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bd4a19bcc972a94bb4071137fef8c888e41f8b0

You should always rebase you patch to the latest C-C before pushing to try. Yesterday, we fixed a lot of test failures, see https://treeherder.mozilla.org/#/jobs?repo=comm-central.

To pick up these fixes, you needed to rebase otherwise your test results will look as bad as before.

Another word on getting the build/app locale. Please read bug 1342753 comment #40. Zibi is telling us that *now* passing 'undefined' will use the system locale, in the future, it will use the build/app locale. Since you seem to be distinguishing between using app locale and system locale, there will/may be further work required when
  .toLocaleDateFormat(undefined, ...) and
  .toLocaleDateFormat(Services.locale.getAppLocale(), ...)
will yield the same result.

Maybe you want to switch to using "en-US" instead of 'undefined' if you already know that you want today's system locale for the tests which is "en-US".

Comment 64

11 days ago
Using |Services.locale.getAppLocale();| caused Xpcshell failures. Maybe the app locale is not available in Xpcshell test, which are after all not running as a full-blown application. Zibi, do you know?
Flags: needinfo?(gandalf)

Comment 65

11 days ago
Another try with the original patch that uses the "chrome registry" to get the locale:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=80cf44311e380ac61dfffe5c2e57f1d47071e536
Just to be sure.

Comment 66

11 days ago
And another one for Win/Mac also using "chrome registry":
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=43accaaac07cb980ac89f8cf483378a3d1d9e624

You can't see the patch since it's the same as in the preceding try run.

Comment 67

11 days ago
Reading tons of comments around this, it's still unclear to me what the final goal is. Yes consistency, but with OS or app? Or differing based on intl.locale.matchOS? 

For Thunderbird using OS locale setting for time/date needs to work out of the box.

Comment 68

11 days ago
This is a little off-topic:

Magnus, thanks for reading up and joining the club of the confused. There are currently may bugs on the way to straighten out date/time display with regards to locale settings.

TB is currently really broken, since the prominent date/time display in the tread pane follows the app locale. This is due to bug 1344594 caused by bug 1342753 which made the C++ generated times use the build/app locale.

But not all dates/times in TB come from C++. We have plenty of them generated in JS, for example the time on the activity manager and the birthday in the address book.

Playing around with the patch from bug 1332351 (see bug 1332351 comment #44), I noticed that the activity manager used app locale formats (en-US) whereas the address book used system formats (de-DE). Of course this situation is not great, since the TB should offer *consistent* app locale times everywhere.

The roadmap to get there is via bug 1325870 (read bug 1342753 comment #40 for details). Once that is done, all defaults in JS code and on the C++ interface will be the app locale.

The next step is to reintroduce respecting OS settings, that's bug 1339650 (read bug 1342753 comment #33 for details).

After bug 1325870 and bug 1339650 are done, you should have consistent JS date/time displays everywhere which take OS setting into account.

But TB will still be broken, since in an ultimate step, those JS formats need to be retrofitted into the C++ interface.

At least that's how I understand it, Zibi can correct me.

Further TB bugs: This one here, bug 1332351 (already mentioned) and bug 1346549.

So is this a little clearer now?

Comment 69

11 days ago
To get to the bottom of those test failures, here's another try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfae20f524cb4f9cb44952af9786b62fcdb4d547

In there I have:

+    let cl = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
+                             .getService(Components.interfaces.nsIXULChromeRegistry)
+                             .getSelectedLocale("global", true);
+    let sl = Services.locale.getAppLocale();
+    dump("cl = "+cl+"\n");
+    dump("sl = "+sl+"\n");

And I'm not using any locale but 'undefined' all the time.

Let's see what results that gives.

Comment 70

10 days ago
(In reply to [:MakeMyDay] from comment #41)
> To limit the impact of breaking the locale support until bug 1339650 lands
> and to prepare for backporting this patch, please make use the current
> locale is used in formatDateLong and formatDateWithoutYear instead of
> undefined as you started in your first patch.
From three try runs the observed behaviour is:

Using
Components.classes["@mozilla.org/chrome/chrome-registry;1"]
          .getService(Components.interfaces.nsIXULChromeRegistry)
          .getSelectedLocale("global", true);
makes the Xpcshell tests pass. This is not the method that should be used to get the app locale.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=80cf44311e380ac61dfffe5c2e57f1d47071e536
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=43accaaac07cb980ac89f8cf483378a3d1d9e624

Using the approved method
Services.locale.getAppLocale();
some Xpcshell tests fail.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bd4a19bcc972a94bb4071137fef8c888e41f8b0
However, reading up in bug 1315520 it seems that they fail during the transition to DST (daylight saving time), which happened exactly in California today. So I guess if I reran the tests now, they would pass.

Just passing 'undefined' also works:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfae20f524cb4f9cb44952af9786b62fcdb4d547

So MMD has a choice of three versions ;-)
Flags: needinfo?(gandalf)

Comment 71

10 days ago
Comment on attachment 8846084 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

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

::: calendar/base/src/calDateTimeFormatter.js
@@ +11,5 @@
>      this.wrappedJSObject = this;
>      this.mDateStringBundle = Services.strings.createBundle("chrome://calendar/locale/dateFormat.properties");
> +    this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> +                             .getSelectedLocale("global", true);

Yes, Services.locale.getAppLocale(); should be used here.

Comment 72

10 days ago
(In reply to Jorg K (GMT+1) from comment #70)
> So I guess if I reran the tests now, they would pass.
I retriggered them and they passed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5bd4a19bcc972a94bb4071137fef8c888e41f8b0
See green Linux x64 opt X next to red and see green OS X 10.10 opt X next to red.

Comment 73

10 days ago
thanks
Created attachment 8846433 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat (using getAppLocale()).
Attachment #8846433 - Flags: review?(makemyday)

Comment 74

9 days ago
(In reply to [:MakeMyDay] from comment #41)
> To limit the impact of breaking the locale support until bug 1339650 lands
> and to prepare for backporting this patch, please make use the current
> locale is used in formatDateLong and formatDateWithoutYear instead of
> undefined as you started in your first patch.
Note that all the patch authors in bug 1346549, bug 1313659 and bug 1332351 seem to have settled on passing 'undefined' to accept the current M-C "default" locale. That this will most likely change from system locale to app/build locale is a different story. Using 'undefined' also passes the test, see comment #70:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfae20f524cb4f9cb44952af9786b62fcdb4d547

Comment 75

9 days ago
Created attachment 8847036 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat (using 'undefined')
Attachment #8847036 - Flags: review?(makemyday)
Heads up, due to inconsistency between mozilla locale negotaiation and ICU I landed bug 1346819 which forces users to chose between getAppLocalesAsLangTags and getAppLocalesAsBCP47.

The former is returning language tag ("ja-JP-mac") and the latter BCP47 locale id ("ja-JP-x-lvariant-mac").
For ICU/Intl use you want ot take the BCP47 locale chain:

```
let locales = Services.locale.getAppLocalesAsBCP47();
Intl.DateTimeFormat(locales);
```

The good news is that you can also skip the locale because bug 1346674 also landed which takes app locale if the parameter is undefined.

Comment 77

8 days ago
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #76)

> The good news is that you can also skip the locale because bug 1346674 also
> landed which takes app locale if the parameter is undefined.

Thanks for the heads up. Will bug 1339650 just derive the locale to use from the system and apply the icu formatting based on that locale or will it make the custom datetime formatting settings of the system available to be applied instead of the ICU settings?

Comment 78

8 days ago
Comment on attachment 8846084 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

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

Thanks for the updated patch. Please check the comments below on the datetimeformatter changes.

::: calendar/base/src/calDateTimeFormatter.js
@@ +11,5 @@
>      this.wrappedJSObject = this;
>      this.mDateStringBundle = Services.strings.createBundle("chrome://calendar/locale/dateFormat.properties");
> +    this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"]
> +                             .getService(Components.interfaces.nsIXULChromeRegistry)
> +                             .getSelectedLocale("global", true);

We would need to to go with this approach for the 53 (and for convenience also 54)  - for 55, we could the workaround for getting the system locale formatting will not work anymore, so we could change all call sites to use undefined (so we need two patches at the end, but you should do the split once everything else is in shape). To get the intended behaviour back, we would need a follow-up bug then.

@@ +35,5 @@
>      formatDateShort: function(aDate) {
> +        let dateOptions = { year: 'numeric', month: '2-digit', day: '2-digit' };
> +        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
> +
> +        return timeFormatter.format(cal.dateTimeToJsDate(aDate));

Please do a check here and in the other places where you apply format, whether aDate.isDate is true - if not, you should treat aDate similar to like you do in formatTime to avoid unwanted effects and intermittent test failures.

@@ +68,5 @@
> +        } else {
> +            let jsDate = new Date(aDate.year, aDate.month, aDate.day,
> +                                  aDate.hour, aDate.minute, 0);
> +            return timeFormatter.format(jsDate);
> +        }

Can you please explain what the purpose of this switch is? You will do a timezone conversion for the output string if tz is utc, but wouldn't do it in any other case.

To simple cut off the timezone information of aDate, which means using floating time, you can do something like this:

if (!aDate.isMutable) {
    aDate = aDate.clone();
}
aDate.timezone = cal.floating();
return timeFormatter.format(cal.dateTimeToJsDate(aDate));

Instead of creating a new Intl.DateTimeFormat object, you could simply use 

return cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, timeOptions)

or toLocaleTimeString, toLocaleString respectively - but I leave that to your descretion.

Updated

8 days ago
Attachment #8847036 - Attachment is obsolete: true
Attachment #8847036 - Flags: review?(makemyday)

Updated

8 days ago
Attachment #8846433 - Attachment is obsolete: true
Attachment #8846433 - Flags: review?(makemyday)

Comment 79

8 days ago
(In reply to [:MakeMyDay] from comment #78)
> We would need to to go with this approach for the 53 (and for convenience
> also 54)  - for 55, we could the workaround for getting the system locale
> formatting will not work anymore, so we could change all call sites to use
> undefined (so we need two patches at the end, but you should do the split
> once everything else is in shape). To get the intended behaviour back, we
> would need a follow-up bug then.
I think the way forward is to prepare a patch that can be landed on C-C trunk, TB 55, since that is where it will land first. Then we can see which variations are required for the branches.

We are considering building TB 53 beta 1 within 24 hours, so this would ship with this regression. Please provide me with a line describing the failure to go into the release notes. Presently I'm not aware of plans for TB 53 beta 2.
Flags: needinfo?(makemyday)
(In reply to Jorg K (GMT+1) from comment #79)
>
> We are considering building TB 53 beta 1 within 24 hours, so this would ship
> with this regression. Please provide me with a line describing the failure
> to go into the release notes. Presently I'm not aware of plans for TB 53
> beta 2.

Only becaues there was no need foreseen for a beta 2. That doesn't mean we can't do one if it's needed. Note, 54 beta is at least 5-6 weeks out.

Comment 81

7 days ago
For one or more recent betas we didn't distribute the build for the OSX. I recommend to do the same now for 53b1 for Linux 32, spin a b2 once the patch got backported and notify the package maintainers of the Linux distributions (and the SM people) so they can do the same.

If you don't want to take that route, you should at least warn the users on that platform to backup their profile before updating and disable Lightning in that version as it wouldn't be usable properly anyway just to avoid any chance of damaging the users calendar data:

"Datetime formatting is not working properly on Linux 32bit platform. This results in party broken calendar views and incorrect event times - for users on this platform it is strictly recommended to backup the user profile before updating and disable Lightning after updating for the remaining lifetime of this version to avoid damages to your calendar data".
Flags: needinfo?(makemyday)

Comment 82

7 days ago
Thanks. That's a lot of effort for very little benefit of having a TB 53 at all. I can only think of one user-facing new feature in TB 53. All other "interesting" bugs have been backported to TB 52. The warning sounds quite serious and doesn't make the product look good, not even for a beta version. We can't guarantee users reading these release notes, and especially since it's Linux, all sorts of problems can be expected with the various package maintainers.
If we can get 53b out and only lose linux then that's a small price with frankly little effort and coordination required. This easily within distros capabilities. And besides, it's a good test of our and others abilities to adjust to prevailing conditions. If there is no dataloss potential, then let's do it.

So question - is there potential for dataloss or damage to user data?
Flags: needinfo?(makemyday)
(In reply to Jorg K (GMT+1) from comment #82)
> Thanks. That's a lot of effort for very little benefit of having a TB 53 at
> all. I can only think of one user-facing new feature in TB 53. 
> All other "interesting" bugs have been backported to TB 52. 

This reduces the value of having a beta to just one requirement and one goal, namely uplifting of bugs. And I keep telling you, and you don't seem to be getting, that betas have multiple objectives.

> The warning sounds quite
> serious and doesn't make the product look good, not even for a beta version.
> We can't guarantee users reading these release notes, and especially since
> it's Linux, all sorts of problems can be expected with the various package
> maintainers.

I don't understand this point. I think you underestimate distros commuincation skills and attention to detail. Plus, all the major ones are subscribers to tb-drivers mailing list. So if distros don't build and ship it, and on our side we don't provide automatic updates for linux (perhaps we can even avoid building it), what precisely is the downside?
(Assignee)

Comment 85

7 days ago
(In reply to [:MakeMyDay] from comment #78)

> @@ +35,5 @@
> >      formatDateShort: function(aDate) {
> > +        let dateOptions = { year: 'numeric', month: '2-digit', day: '2-digit' };
> > +        let timeFormatter = new Intl.DateTimeFormat(undefined, dateOptions);
> > +
> > +        return timeFormatter.format(cal.dateTimeToJsDate(aDate));
> 
> Please do a check here and in the other places where you apply format,
> whether aDate.isDate is true - if not, you should treat aDate similar to
> like you do in formatTime to avoid unwanted effects and intermittent test
> failures.
> 


So, if !aDate.isDate then "float" the timezone on the cloned object which I will use to format the date. But, what to do when aDate.isDate? Convert aDate directly to an JS Date without touching the timezone field?


> To simple cut off the timezone information of aDate, which means using
> floating time, you can do something like this:
> 


That was what I was trying to do.


> if (!aDate.isMutable) {
>     aDate = aDate.clone();
> }
> aDate.timezone = cal.floating();
> return timeFormatter.format(cal.dateTimeToJsDate(aDate));
> 


And that was the reason I was unable to change the timezone field :(

Comment 86

6 days ago
Sorry, the Xpcshell tests look pretty red again due to unrelated causes, however, in your latest try you now have:
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:calendar/test/unit/test_ltninvitationutils.js | compareInvitationOverlay_test - [compareInvitationOverlay_test : 511] (test #3): imipHtml-when-content - false == true
(Assignee)

Comment 87

6 days ago
(In reply to Jorg K (GMT+1) from comment #86)
> Sorry, the Xpcshell tests look pretty red again due to unrelated causes,
> however, in your latest try you now have:
> TEST-UNEXPECTED-FAIL |
> xpcshell-icaljs.ini:calendar/test/unit/test_ltninvitationutils.js |
> compareInvitationOverlay_test - [compareInvitationOverlay_test : 511] (test
> #3): imipHtml-when-content - false == true

Yes. There were many failures in that XPC-shell test on Mac. My code wasn't final. I was just testing it over there. It will likely fail on the other platforms, also.

Comment 88

6 days ago
I backed out the cause of the IMAP failures, so it you rebase now, they will be gone. Still failures due to bug 1346916 and bug 1347687.

Maybe to save resources, only try one platform with opt build until the final test.
FYI, there is also https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/modules/Locale.jsm which abstracts away some of the detail. Maybe this is helpful for determining the right locale?
Ok, I read more of the comments here and it seems my suggestion might not be helpful given this would just get you the OS or app locale depending on preference, but you need the BCP47 locale.

Comment 91

3 days ago
In case you're wondering: I've retriggered the Xpcshell test on the last try run
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1137e7e1d6755114b836b5bae322f93a9ef95733
to see whether the calendar/test/unit/test_gdata_provider.js failure would repeat (since there was a misleading suggestion of bug 1266823). Sorry that this wasn't helpful.
(Assignee)

Comment 92

3 days ago
Thank you, Jörg.

This morning I have refactored a little bit last try run. It was using the same code in many places, so I have created a new function.

I am building it on my local machine and will run ltninvitationutils and mozmill tests and then will push to try-server to run the whole suite in all platforms as I think this is the final code for this patch.
(Assignee)

Comment 93

2 days ago
The failing test for gdata_provider were because in one of them a unknown timezone is used and Intl.DateTimeFormatter throws an exception because of it. I will take it into account in my next try-run by checking the existence of the tzid and, if not, I will use a floating timezone.

I have push a try-run for all platforms (this one doesn't includes the fix from the previous paragraph)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f0e32315cd38d42ad9dc495927207adf18ff4e6c&selectedJob=85114935
(Assignee)

Comment 94

16 hours ago
Created attachment 8849944 [details] [diff] [review]
Use Intl.DateTimeFormat instead of nsIScriptableDateFormat

Failures in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=13ca5e177a9e5ecfd0906dcd340bf5049bdbb72a seems unrelated to this patch or needed to fix other ones. Likely there will be needed some changes, as I am unsure the naming for the new functions in calDateTimeFormatter.js are good enough.
Attachment #8849944 - Flags: review?(makemyday)
(Assignee)

Updated

16 hours ago
Attachment #8846084 - Attachment is obsolete: true
Attachment #8846084 - Flags: review?(makemyday)

Comment 95

15 hours ago
(In reply to Javi Rueda from comment #94)
> Failures in ... seem unrelated to this patch ...
Yes, bug 1346916, awaiting check-in (always check: https://mzl.la/2lrQTHC).
You need to log in before you can comment on or make changes to this bug.