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

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?

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.

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.

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

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?

(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.

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.

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.

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

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

Bustage now fixed.

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

Javi, thank you for the patch. I will do the review on the weekend.

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

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.

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.

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.

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.

Try push for the recent patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f5547dd38ebe2d386de3946058da3de0999f3ce1

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 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.

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

(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.

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.

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.

(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

(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.

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.

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.

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.

(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.

(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 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.

(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?

> 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.

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.

(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.

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.

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

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?

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.

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!

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. :)

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.

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.

(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

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

(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.

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

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.

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".

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?

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.

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.

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.

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?

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.

(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 ;-)

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.

(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.

(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

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.

(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 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.

(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.

(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.

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".

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?

(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?

(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 :(

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

(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.

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.

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.

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.

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

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.

(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).

FYI, https://treeherder.mozilla.org/#/jobs?repo=comm-central is 100% green right now on Windows and Mac. Linux doesn't build due to bug 1350011 and according to bug 1350011 comment #10 we won't have Linux builds at all for the foreseeable future (read the last line of that comment).

Javi, can you please still answer the question regarding utc handling from comment 78?

(In reply to [:MakeMyDay] from comment #97) > Javi, can you please still answer the question regarding utc handling from > comment 78? Yes and not. That code was added so tests pass. One of the tests were using UTC so code tried to detect that situation and do the conversion. That if-else block was written with a trial-and-error approach. This is the only explanation I have for that part of the code.

How can we bring this matter to a close? (In reply to Philipp Kewisch [:Fallen] from comment #34 - 2017-03-03) > That said, Javi, if you need help and have questions you can also ping me on > IRC. My nickname is Fallen Maybe that helps.

(In reply to Javi Rueda from comment #98) > Yes and not. That code was added so tests pass. One of the tests were using > UTC so code tried to detect that situation and do the conversion. That > if-else block was written with a trial-and-error approach. This is the only > explanation I have for that part of the code. Thanks. You're effectively making a timezone conversion in your formatter code for utc cases atm. I have attached a xpcshell test that passes without your patch but fails with the same applied to demonstrate this (the used patterns for the expected values may be platform specific for the nsIScriptableDateFormat use case, so you might need to adapt them to make this passing for you locally. The patterns in the comments are to match the future patterns based on your patch. If you adapt your formatter code to make this test passing (see my follow up comment), you will realize that you've been working around a problem you introduced by the way you did the formatting conversion in e.g. calendar/base/content/calendar-multiday-view.xml. If you run TB with your patch, you can see that there is an utc timezone offset in the day view (the day doesn't start at midnight - this gets more obvious since the recent DST transition). If you pass cal.floating() as a timezone argument to cal.jsDateToDateTime in calendar-multiday-view.xml, the day view is diplayed correctly again and finally the testBasicFunctionality mozmill test would pass, provided you replace cal.now() by cal.createDateTime() therein. I haven't checked the other potentially failing mozmill tests with a corrected datetimeformatter, but I would expect you can deal with that in a similar way. If you want to extend the attached test to cover the missing methods from datetimeformatter, feel free to do so, but you can leave it a separate patch. If not, we can add that later.

Comment on attachment 8849944 [details] [diff] [review] Use Intl.DateTimeFormat instead of nsIScriptableDateFormat Review of attachment 8849944 [details] [diff] [review]: ----------------------------------------------------------------- r- for now to get the tz conversion issue fixed. ::: calendar/base/content/calendar-multiday-view.xml @@ +123,5 @@ > } > > + let timeFormatter = > + Components.classes["@mozilla.org/calendar/datetime-formatter;1"] > + .getService(Components.interfaces.calIDateTimeFormatter); you can simply use let timeFormatter = cal.getDateFormatter(); instead here and everywhere else you need the formatter. @@ +150,5 @@ > let durPix = endPix - startPix; > let box; > if (dur == 60) { > + jsTime.setHours(theHour, 0, 0); > + timeString = timeFormatter.formatTime(cal.jsDateToDateTime(jsTime)); pass a timezone here: cal.jsDateToDateTime(jsTime, cal.floating()) - check other uses of jsDateToDateTime in your patch as well. ::: calendar/base/src/calDateTimeFormatter.js @@ +41,5 @@ > + if (!someDate.isDate) { > + dateOptions = this._getOptionsSkippingTimezone(someDate, dateOptions); > + } > + > + return cal.dateTimeToJsDate(someDate).toLocaleDateString(undefined, dateOptions); You can simplify this to let dtOptions = { year: 'numeric', month: '2-digit', day: '2-digit' }; dtOptions = this._addTzOption(aDate, dtOptions); return cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dateOptions); here and accordingly in the other places where you're going to do the new formatting (keep in mind that you have that with and without passing undefined as locale, so not just copy and past this. _addTzOption is explained below. @@ +110,5 @@ > + if (!theDate.timezone.isFloating && !theDate.timezone.isUTC) { > + theOptions.timeZone = theDate.timezone.tzid; > + } > + return theOptions; > + }, You can unify _selectTimezone and _getOptionsSkipping and get rid using timezone service: /** * _addTzOption adds a timezone to apply on Intl.DateTimeFormat formatting operations to the * provided options argument based on the timezone of the date argument * * @param {calIDateTime} aDate The options object to extend * @param {JsObject} aOptions The options object to extend * @return {JsObject} The object containing the formatting options */ _addTzOption: function(aDate, aOptions) { let timezone = aDate.timezone; // we set the tz only if we have a valid tz - otherwise localtime will be used on formatting if (timezone && (timezone.isUTC || timezone.icalComponent)) { aOptions.timeZone = timezone.tzid; } return aOptions; }, With that an the calling code adaption above, the provided datetimeformatter test should pass. When adding a new function, please also add some comments that explain the purpose and the way it works. Regarding naming, a function name should reflect its purpose. Parameters should always be prefixed by an "a" to make it obvious that the repective variable is an argument provided to the function.

Javi, are you still working on this? TB 53 beta has been shipped with this issue unfixed https://www.mozilla.org/en-US/thunderbird/53.0beta/releasenotes/ and the next branch date is in one week, 18th of April (shifted by one day from 17th). So we won't need an upliftable patch for TB 53 any more and therefore this code > + this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Components.interfaces.nsIXULChromeRegistry) > + .getSelectedLocale("global", true); is not needed any more, 'undefined' can now be used everywhere instead of this.mLocale, see comment #78. So my periodic question to the module owner and peer: What is the plan to fix this issue that has now entered its fourth month.

(In reply to Jorg K (GMT+2) from comment #102) > Javi, are you still working on this? > Yes. I have finished with the changes from last review. Today I will be doing try-server runs once I get the UI part working as expected.

(In reply to Jorg K (GMT+2) from comment #102) > So we won't need an upliftable patch for TB 53 any more and therefore this > code > > + this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"] > > + .getService(Components.interfaces.nsIXULChromeRegistry) > > + .getSelectedLocale("global", true); > is not needed any more, 'undefined' can now be used everywhere instead of > this.mLocale, see comment #78. We still need an uplift to beta to let it ride to comm-release for SM before the upcomung merge, regardless whether there will ne another TB beta. That said, Javi, please keep that behaviour in the patch intended for backporting.

Javi, is there anything left to do at your end before uploading an updated patch? Your recent try push looked quite green (without having looked at the code therein).

(In reply to [:MakeMyDay] from comment #105) > Javi, is there anything left to do at your end before uploading an updated > patch? Your recent try push looked quite green (without having looked at the > code therein). My last try-push included both the patch to fix this bug and an edited version of your test patch. I replaced the uncommented data with the commented one on the test so it passes tests. Do you want a merged patch or better to split it into the patch and the test itself?

Personally, I don't have a strong preference on that, but keeping the test separate is the probably better to prevent me from reviewing my own code ;-) If you upload the final patch(es) in the next couple of hours, I can do the final review today.

New patch. It was try-tested in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44881b6e0c0cc51c618cdae78c64ff2589504d6c on Mac and Linux x64

Modified test for calDateTimeFormatter. Code is from :MakeMyDay, I just removed some white spaces and replaced expected values with the ones which actually passes the tests. It was also try-tested in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=44881b6e0c0cc51c618cdae78c64ff2589504d6c on Mac and Linux x64

Thanks, maybe also add a patch without the > + this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Components.interfaces.nsIXULChromeRegistry) > + .getSelectedLocale("global", true); that we can apply to the current trunk.

Comment on attachment 8858789 [details] [diff] [review] datetimeformatter.patch Review of attachment 8858789 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Javi, I think you'Re almost there. Codewise this looks good in general, just one nit to fix which I planted on you (see below) and I was already about to grant the patch. However, two mozmill tests are failing for me locally on Windows. so this is a f+ only atm: cal-recurrence/testWeeklyUntilRecurrence.js cal-recurrence/testWeeklyWithExceptionRecurrence.js At least for the first one passing a floating timezone to jsDateToDateTime makes the test passing. Please make sure you do a try run on all platforms once you fixed that (sparing resources is nice, but when it comes to datetime related changes, you should make sure it is working on all platforms and at best also at different times to make sure you don't run into timezone pitfall - even if the formatting is now the same on all platforms, timezone detection is not and mozmill tests usually use the detected timezone). One more thing: please number your patches if you upload multiple versions of a patch to a bug. Having several patches of the same name is a pita when using the interdiff function on bmo. ::: calendar/base/src/calDateTimeFormatter.js @@ +75,5 @@ > + /** > + * _addTzOption adds a timezone to apply on Intl.DateTimeFormat formatting operations to the > + * provided options argument based on the timezone of the date argument. > + * > + * @param {calIDateTime} aDate The options object to extend. You failed the cut and paste test ;-) Please make the description for aDate "The date object holding tz information"

Comment on attachment 8858791 [details] [diff] [review] datetimeformatter_TEST.patch I tend to not review my own code, so Philipp, can you please take a look at it? It should be straight forward. The test is passing locally for me and on try as mentioned above by Javi. This test intentionally doesn't cover all of the formatting methods available in the formatter atm, but all of those changed with this bug.

Comment on attachment 8858791 [details] [diff] [review] datetimeformatter_TEST.patch Review of attachment 8858791 [details] [diff] [review]: ----------------------------------------------------------------- Code looks fine to me, r=philipp

(In reply to [:MakeMyDay] from comment #111) > Comment on attachment 8858789 [details] [diff] [review] > datetimeformatter.patch > > Review of attachment 8858789 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks Javi, I think you'Re almost there. Codewise this looks good in > general, just one nit to fix which I planted on you (see below) and I was > already about to grant the patch. However, two mozmill tests are failing for > me locally on Windows. so this is a f+ only atm: > > cal-recurrence/testWeeklyUntilRecurrence.js > cal-recurrence/testWeeklyWithExceptionRecurrence.js > I changed testWeeklyUntilRecurrence and *WithExceptionRecurrence in my new patch. Last one was on rotatedRecurrence as the one in cal-recurrence/ wasn't modified by my patch at all. > At least for the first one passing a floating timezone to jsDateToDateTime > makes the test passing. > > Please make sure you do a try run on all platforms once you fixed that > (sparing resources is nice, but when it comes to datetime related changes, > you should make sure it is working on all platforms and at best also at > different times to make sure you don't run into timezone pitfall - even if > the formatting is now the same on all platforms, timezone detection is not > and mozmill tests usually use the detected timezone). > Ok. I have just launched a new try-test: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=da9b4cadc7c49d4f945e06a3661c58a773d3bd63 with latest changes. > One more thing: please number your patches if you upload multiple versions > of a patch to a bug. Having several patches of the same name is a pita when > using the interdiff function on bmo. > Ok. Next one will be numbered. Sorry for the inconvenience. > > You failed the cut and paste test ;-) > > Please make the description for aDate "The date object holding tz > information" Oops :) (In reply to Jorg K (GMT+2) from comment #110) > Thanks, maybe also add a patch without the > > + this.mLocale = Components.classes["@mozilla.org/chrome/chrome-registry;1"] > > + .getService(Components.interfaces.nsIXULChromeRegistry) > > + .getSelectedLocale("global", true); > that we can apply to the current trunk. Yes, I can do it in a few minutes.

This patch uses undefined instead of getting the locale in some of the methods. Asked by JorgK for trunk. Untested but it is the same code from my last try-push for all platforms.

(In reply to Makoto Kato [:m_kato] from comment #0) > 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. My last try-push (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f2c0db0e4f9b2e10efaaff30ab106e48cd9be45a&selectedJob=92400280) is failing on Linux debug (both 32 and 64-bits) but not on opt -at least at the time I am writing this, Mac and Windows wasn't finished yet- with: 12:17:05 INFO - SUMMARY-UNEXPECTED-FAIL | testAnnualRecurrence.js | testAnnualRecurrence.js::testAnnualRecurrence 12:17:05 INFO - EXCEPTION: Expression "{"label":"1950"}" returned null. Anonymous == false 12:17:05 INFO - at: elementslib.js line 486 12:17:05 INFO - reduceLookup elementslib.js:486 15 12:17:05 INFO - Lookup.prototype.getNode elementslib.js:501 10 12:17:05 INFO - MozMillController.prototype.click controller.js:527 17 12:17:05 INFO - goToDate test-calendar-utils.js:258 9 12:17:05 INFO - testAnnualRecurrence testAnnualRecurrence.js:42 5 12:17:05 INFO - Runner.prototype.wrapper frame.js:585 9 12:17:05 INFO - Runner.prototype._runTestModule frame.js:655 9 12:17:05 INFO - Runner.prototype.runTestModule frame.js:701 3 12:17:05 INFO - Runner.prototype.runTestDirectory frame.js:525 7 12:17:05 INFO - runTestDirectory frame.js:707 3 12:17:05 INFO - Bridge.prototype._execFunction server.js:179 10 12:17:05 INFO - Bridge.prototype.execFunction server.js:183 16 12:17:05 INFO - Session.prototype.receive server.js:283 3 12:17:05 INFO - AsyncRead.prototype.onDataAvailable server.js:88 3 Any clue about that?

I thought those tests were flaky in general in a slow debug run and Markus was going to fix them.

This failure is not related to this Bug and I have a patch almost ready for it which will be in Bug 1329957

Comment on attachment 8858789 [details] [diff] [review] datetimeformatter.patch Obsoleted by Patch 2

Comment on attachment 8859534 [details] [diff] [review] Patch 2 - Patch Review of attachment 8859534 [details] [diff] [review]: ----------------------------------------------------------------- Ok, let's give it a go. r+ with the below comment considered. Sorry for the delay in reviewing and thanks for all your dedication and endurance! We'll probably need a follow-up bug for this to consider MozIntl approriately, in case your interesetd... ;-) ::: calendar/test/mozmill/shared-modules/test-calendar-utils.js @@ +708,5 @@ > } > > // startdate > if (data.startdate != undefined && data.startdate.constructor.name == "Date") { > + let startdate = dateFormatter.formatDateShort(cal.jsDateToDateTime(data.startdate)); please add also the cal.floating() as a second argument when caliing jsDateToDateTime here and eslewhere in this file.

In reply to Javi Rueda from comment #114) > > cal-recurrence/testWeeklyUntilRecurrence.js > > cal-recurrence/testWeeklyWithExceptionRecurrence.js > > > > I changed testWeeklyUntilRecurrence and *WithExceptionRecurrence in my new > patch. Last one was on rotatedRecurrence as the one in cal-recurrence/ > wasn't modified by my patch at all. cal-recurrence/testWeeklyWithExceptionRecurrence.js is still failing locally for me with the latest patch. This tests swallows data from test-calendar-utils.js where you didn't add the timezone to the calling code, while it doesn't with the patch not applied: SUMMARY-UNEXPECTED-FAIL | f:\workspace\mozilla\comm-central\obj-i686-pc-mingw32\_tests\mozmill\stage\cal-recurrence\testWeeklyWithExceptionRecurrence.js | testWeeklyWithExceptionRecurrence.js::testWee klyWithExceptionRecurrence EXCEPTION: Timeout exceeded for waitForElementNotPresent Lookup: /id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id( "calendarDisplayDeck")/id("calendar-view-box")/id("view-deck")/id("day-view")/anon({"anonid":"mainbox"})/anon({"anonid":"scrollbox"})/anon({"anonid":"daybox"})/[0]/anon({"anonid":"boxstack"})/anon({"a nonid":"topbox"})/{"flex":"1"}/{"flex":"1"}/{"flex":"1"}/{"tooltip":"itemTooltip","calendar":"mozmill"} at: utils.js line 447 TimeoutError utils.js:447 13 waitFor utils.js:485 11 MozMillController.prototype.waitFor controller.js:687 3 MozMillController.prototype.waitForElementNotPresent controller.js:709 3 testWeeklyWithExceptionRecurrence testWeeklyWithExceptionRecurrence.js:107 5 Runner.prototype.wrapper frame.js:585 9 Runner.prototype._runTestModule frame.js:655 9 Runner.prototype.runTestModule frame.js:701 3 Runner.prototype.runTestFile frame.js:534 3 runTestFile frame.js:713 3 Bridge.prototype._execFunction server.js:179 10 Bridge.prototype.execFunction server.js:183 16 Session.prototype.receive server.js:283 3 AsyncRead.prototype.onDataAvailable server.js:88 3 However, even if you did, the test is still failing. So this is likely revealing a problem with the test design (I hadn't had a second look at it - maybe Markus could, once this landed?).

Comment on attachment 8859198 [details] [diff] [review] Patch using undefined as Locale Review of attachment 8859198 [details] [diff] [review]: ----------------------------------------------------------------- Please just provide a delta patch to the already apporved backport version. Based on what I've seen in the interdiff, now that you're always using undefined as a first argument to feed into toLocaleDateString, you can further consolidate let dtOptions = { ... }; dtOptions = this._addTzOption(aDate, dtOptions); return cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dtOptions); to let dtOptions = { ... }; return this._inTimezone(aDate, dtOptions); everywhere while renaming _addTzOptions to _inTimezone with returning cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dtOptions) instead. r+ with the above cosidered

https://hg.mozilla.org/comm-central/rev/f58639c9f2e31f6de391b64900e6cf184ae15968 (new xpcshell test) https://hg.mozilla.org/comm-central/rev/a07da5ede621fc0ec9b6d6d71e1c3b821b4de8ac (backport version) Since the backport version works on trunk as well and MMD wants a "delta patch" for trunk, I've landed the backport version for now. We follow up with the "delta patch". I've addressed the issue from comment #121 by adding cal.floating() as a second argument.

Time to provide a merged patch with the r+ xpcshell test?

Sorry for changing the Target, Jorg. Maybe you could change it again.

(In reply to Javi Rueda from comment #125) > Time to provide a merged patch with the r+ xpcshell test? Hmm, I don't understand the comment. The xpcshell test has landed, so no need to merge anything. The author of that test is MMD, so that wouldn't lend itself to merging with your patches. All MMD asked for was a delta patch that removes the chrome registry call and uses undefined instead of mLocale. He also suggested some refactoring according to comment #123.

Tones of rejections after trying to apply the patch which uses undefined. I had work to do. Will post a new one over this night. As Jorg made patches to land in comm-central, the merge is not needed.

(In reply to [:MakeMyDay] from comment #123) > everywhere while renaming _addTzOptions to _inTimezone with returning > cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dtOptions) instead. > I have done this in a new try-push. I am running it because in formatTime, toLocaleTimeString() was used instead. I cannot do it in my local machine because I am running a very demanding process in it right now. I think that it will fail, because while I was working in the previous patch it failed as it was getting a full date instead of just the time. (In reply to Javi Rueda from comment #128) > Tones of rejections after trying to apply the patch which uses undefined. I > had work to do. Will post a new one over this night. > My fault.

https://hg.mozilla.org/try-comm-central/rev/eb5c9548dcbcf166c3042a470520e39af72d7e8e let timeOptions = { hour: 'numeric', minute: '2-digit' }; - timeOptions = this._addTzOption(aDate, timeOptions); - return cal.dateTimeToJsDate(aDate).toLocaleTimeString(undefined, timeOptions); + return this._inTimezone(aDate, dtOptions); Did you mean timeOptions here?

(In reply to :aceman from comment #130) > https://hg.mozilla.org/try-comm-central/rev/ > eb5c9548dcbcf166c3042a470520e39af72d7e8e > > let timeOptions = { hour: 'numeric', minute: '2-digit' }; > - timeOptions = this._addTzOption(aDate, timeOptions); > - return cal.dateTimeToJsDate(aDate).toLocaleTimeString(undefined, > timeOptions); > + return this._inTimezone(aDate, dtOptions); > > Did you mean timeOptions here? Yes. Thank you. That was me copy-pasting.

(In reply to [:MakeMyDay] from comment #122) > SUMMARY-UNEXPECTED-FAIL | > f:\workspace\mozilla\comm-central\obj-i686-pc- > mingw32\_tests\mozmill\stage\cal- > recurrence\testWeeklyWithExceptionRecurrence.js | > testWeeklyWithExceptionRecurrence.js::testWee > klyWithExceptionRecurrence > EXCEPTION: Timeout exceeded for waitForElementNotPresent Lookup: > /id("messengerWindow")/id("tabmail-container")/id("tabmail")/ > id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id( > "calendarDisplayDeck")/id("calendar-view-box")/id("view-deck")/id("day- > view")/anon({"anonid":"mainbox"})/anon({"anonid":"scrollbox"})/ > anon({"anonid":"daybox"})/[0]/anon({"anonid":"boxstack"})/anon({"a > nonid":"topbox"})/{"flex":"1"}/{"flex":"1"}/{"flex":"1"}/{"tooltip": > "itemTooltip","calendar":"mozmill"} > at: utils.js line 447 > TimeoutError utils.js:447 13 > waitFor utils.js:485 11 > MozMillController.prototype.waitFor controller.js:687 3 > MozMillController.prototype.waitForElementNotPresent > controller.js:709 3 > testWeeklyWithExceptionRecurrence > testWeeklyWithExceptionRecurrence.js:107 5 > Runner.prototype.wrapper frame.js:585 9 > Runner.prototype._runTestModule frame.js:655 9 > Runner.prototype.runTestModule frame.js:701 3 > Runner.prototype.runTestFile frame.js:534 3 > runTestFile frame.js:713 3 > Bridge.prototype._execFunction server.js:179 10 > Bridge.prototype.execFunction server.js:183 16 > Session.prototype.receive server.js:283 3 > AsyncRead.prototype.onDataAvailable server.js:88 3 > > However, even if you did, the test is still failing. So this is likely > revealing a problem with the test design (I hadn't had a second look at it - > maybe Markus could, once this landed?). This looks most likely like something that will be addressed in bug 1329957. I keep an Eye on it.

(In reply to Javi Rueda from comment #129) > (In reply to [:MakeMyDay] from comment #123) > > everywhere while renaming _addTzOptions to _inTimezone with returning > > cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dtOptions) instead. > > > > I have done this in a new try-push. I am running it because in formatTime, > toLocaleTimeString() was used instead. I cannot do it in my local machine > because I am running a very demanding process in it right now. > > I think that it will fail, because while I was working in the previous patch > it failed as it was getting a full date instead of just the time. > Check out toLocaleString() instead - this should cater both cases. I have filed bug 1360915 to follow up once this patch landed to restore the original OS regional setting aware formatting. Feel free to pick this up.

Comment on attachment 8859534 [details] [diff] [review] Patch 2 - Patch We should still uplift this to comm-beta and leave it to the discretion of the SM people whether they want to uplift this also to comm-release.

(In reply to [:MakeMyDay] from comment #133) > (In reply to Javi Rueda from comment #129) > > (In reply to [:MakeMyDay] from comment #123) > > > everywhere while renaming _addTzOptions to _inTimezone with returning > > > cal.dateTimeToJsDate(aDate).toLocaleDateString(undefined, dtOptions) instead. > > > > > > > I have done this in a new try-push. I am running it because in formatTime, > > toLocaleTimeString() was used instead. I cannot do it in my local machine > > because I am running a very demanding process in it right now. > > > > I think that it will fail, because while I was working in the previous patch > > it failed as it was getting a full date instead of just the time. > > > > Check out toLocaleString() instead - this should cater both cases. I have done used and if-else depending of the aOptions having the .day property. I have just asked for a try-run. I did another one before but there were problems with the build script. And I did another one but using (!aOptions.day) instead of (!aOptions.hasOwnProperty("day")) which seems to be the right way. https://hg.mozilla.org/try-comm-central/rev/341302429e41336dfc088520ed8058f53575b1aa I have to leave but when I come back will make a new patch using toLocaleString() instead and ask for a new try-run.

This patch was previously reviewed and needed some changes. Here I made them. However, I would like to get a new r+ as some words for _inTimezone() has been changed and code had to be changed to get formatTime also working.

Comment on attachment 8859198 [details] [diff] [review] Patch using undefined as Locale Obsoleted by Patch 3

Comment on attachment 8863453 [details] [diff] [review] Patch 3 - Patch for trunk repo Review of attachment 8863453 [details] [diff] [review]: ----------------------------------------------------------------- Thanks again, r+ with the below nit fixed. Are you interested to give bug 1360915 a try to continue the conversion? ::: calendar/base/src/calDateTimeFormatter.js @@ +70,4 @@ > * > * @param {calIDateTime} aDate The date object holding the tz information > + * @param {JsObject} aOptions The options object for formatting. > + * @return {JsObject} The date as a string. Now you return a string and not a JsObject here, so please adjust the type specification.

https://hg.mozilla.org/comm-central/rev/f58639c9f2e31f6de391b64900e6cf184ae15968 (new xpcshell test, comment #124) https://hg.mozilla.org/comm-central/rev/a07da5ede621fc0ec9b6d6d71e1c3b821b4de8ac (backport version, comment #124) https://hg.mozilla.org/comm-central/rev/f704b73921c4e9381e9dc43d59936592ba95a4ff (delta patch for trunk). Landed with nit fixed, of course ;-) Done \o/

Attaching this for reference purposes and if needed for importing into local repositories. Thank you Jorg for doing that final edit and landing this :)

(In reply to [:MakeMyDay] from comment #138) > Comment on attachment 8863453 [details] [diff] [review] > Patch 3 - Patch for trunk repo > > Review of attachment 8863453 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks again, r+ with the below nit fixed. Are you interested to give bug > 1360915 a try to continue the conversion? > I will give a try. But not assigning it to myself yet until I have something reviewable. Thank you for counting with me, :MakeMyDay :D Thank you all of you for your patience with my performance on this bug.

Thanks for working on this bug Javi! I'm certain it will get easier for future bugs, I think this one was particularly hard given all the details that needed to be taken care of. If you are working on the other bug feel free to assign yourself so others know you are. We can always ask how you are doing if someone else is interested in fixing it as well.

Beta (TB 54, Calendar 5.6): https://hg.mozilla.org/releases/comm-beta/rev/eed069066fe25f07fbcb36f8f0824cd077489b18 https://hg.mozilla.org/releases/comm-beta/rev/34d4ccf79e6aec7d962f2cb60db43cfd3112b331